Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CIR][CIRGen] Handle NYI in CIRGenModule::tryEmitBaseDestructorAsAlias #1180

Merged
merged 2 commits into from
Dec 12, 2024

Conversation

ChuanqiXu9
Copy link
Member

This removes some NYI in CIRGenModule::tryEmitBaseDestructorAsAlias and similar to #1179, use assert(false) to tell devs to add test.

It is slightly verbose due to the difference between LLVM and CIR's type system. LLVM's pointer are opaque types while CIR's pointer are typed. So we need to handle these pointers when transforming the generated cir.

// lowering
// RUN: %clang_cc1 -std=c++17 -triple x86_64-unknown-linux-gnu -O1 \
// RUN: -mconstructor-aliases -fclangir -emit-cir %s -o %t.cir \
// RUN: -clangir-disable-passes -o %t.cir
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without -clangir-disable-passes, we're going to trigger a memory safe internal issue. Let's file an issue for this after we land this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this needs to be tested under -O1? If you can test this under -O0 then you don't need to pass in -clangir-disable-passes.

If for some reason you require -O1, then we usually tackle those right away, specially if they can reproduce consistently - cases we usually workaround are when something is either blocking a rebase or the issue is intermittent. One possible fix is to pinpoint the pass causing the crash, disable the specific transformation from -O1 and file an issue with a small repro (this testcase) so that someone can work on fixing that pass before we re-enable it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The O1 part is surprising. I forgot removing it somehow. We still need -clangir-disable-passes after removing -O1. The crash log is:

clang: /home/chuanqi.xcq/clangir/llvm/include/llvm/ADT/ilist_iterator.h:168: reference llvm::ilist_iterator<llvm::ilist_detail::node_options<mlir::Block, true, false, void, false, void>, false, false>::operator*() const [OptionsT = llvm::ilist_detail::node_options<mlir::Block, true, false, void, false, void>, IsReverse = false, IsConst = false]: Assertion `!NodePtr->isKnownSentinel()' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.	Program arguments: /disk2/workspace.xuchuanqi/clangir/build/bin/clang -cc1 -internal-isystem /disk2/workspace.xuchuanqi/clangir/build/lib/clang/20/include -nostdsysteminc -std=c++17 -triple x86_64-unknown-linux-gnu -mconstructor-aliases -fclangir -emit-cir /disk2/workspace.xuchuanqi/clangir/clang/test/CIR/CodeGen/dtor-alias.cpp -o /disk2/workspace.xuchuanqi/clangir/build/tools/clang/test/CIR/CodeGen/Output/dtor-alias.cpp.tmp.cir -o /disk2/workspace.xuchuanqi/clangir/build/tools/clang/test/CIR/CodeGen/Output/dtor-alias.cpp.tmp.cir
1.	<eof> parser at end of file
 #0 0x000000000243d698 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/disk2/workspace.xuchuanqi/clangir/build/bin/clang+0x243d698)
 #1 0x000000000243b18e llvm::sys::RunSignalHandlers() (/disk2/workspace.xuchuanqi/clangir/build/bin/clang+0x243b18e)
 #2 0x000000000243de7d SignalHandler(int) Signals.cpp:0:0
 #3 0x00007f55d31ae9d0 __restore_rt sigaction.c:0:0
 #4 0x00007f55d29d9f35 raise ../sysdeps/unix/sysv/linux/raise.c:51:1
 #5 0x00007f55d29c38d7 abort abort.c:81:7
 #6 0x00007f55d29c37a7 loadmsgcat.c:509:8
 #7 0x00007f55d29c37a7 _nl_load_domain.cold loadmsgcat.c:970:34
 #8 0x00007f55d29d2536 (/lib64/libc.so.6+0x34536)
 #9 0x00000000047cfe8e (anonymous namespace)::LoweringPreparePass::lowerGlobalOp(cir::GlobalOp) LoweringPrepare.cpp:0:0
#10 0x00000000047caefc (anonymous namespace)::LoweringPreparePass::runOnOperation() LoweringPrepare.cpp:0:0
#11 0x0000000004ef30e6 mlir::detail::OpToOpPassAdaptor::run(mlir::Pass*, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int) (/disk2/workspace.xuchuanqi/clangir/build/bin/clang+0x4ef30e6)
#12 0x0000000004ef3922 mlir::detail::OpToOpPassAdaptor::runPipeline(mlir::OpPassManager&, mlir::Operation*, mlir::AnalysisManager, bool, unsigned int, mlir::PassInstrumentor*, mlir::PassInstrumentation::PipelineParentInfo const*) (/disk2/workspace.xuchuanqi/clangir/build/bin/clang+0x4ef3922)
#13 0x0000000004ef5ece mlir::PassManager::run(mlir::Operation*) (/disk2/workspace.xuchuanqi/clangir/build/bin/clang+0x4ef5ece)
#14 0x0000000003842a1f cir::runCIRToCIRPasses(mlir::ModuleOp, mlir::MLIRContext*, clang::ASTContext&, bool, bool, llvm::StringRef, bool, llvm::StringRef, bool, llvm::StringRef, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>&, bool, bool, bool, bool, bool) (/disk2/workspace.xuchuanqi/clangir/build/bin/clang+0x3842a1f)

From the assertion, we can see it crashes due to we accessing the invalid iterators in LoweringPreparePass. We can file an issue for it then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, can you file it once this lands? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Landed! Issue time :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracked here: #1234

@bcardosolopes bcardosolopes changed the title [CIR] [CodeGen] Handle NYI in CIRGenModule::tryEmitBaseDestructorAsAlias [CIR][CIRGen] Handle NYI in CIRGenModule::tryEmitBaseDestructorAsAlias Dec 2, 2024
// lowering
// RUN: %clang_cc1 -std=c++17 -triple x86_64-unknown-linux-gnu -O1 \
// RUN: -mconstructor-aliases -fclangir -emit-cir %s -o %t.cir \
// RUN: -clangir-disable-passes -o %t.cir
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this needs to be tested under -O1? If you can test this under -O0 then you don't need to pass in -clangir-disable-passes.

If for some reason you require -O1, then we usually tackle those right away, specially if they can reproduce consistently - cases we usually workaround are when something is either blocking a rebase or the issue is intermittent. One possible fix is to pinpoint the pass causing the crash, disable the specific transformation from -O1 and file an issue with a small repro (this testcase) so that someone can work on fixing that pass before we re-enable it.

@ChuanqiXu9 ChuanqiXu9 force-pushed the tryEmitBaseDestructorAsAlias branch from 0bd11c7 to 69a5f2c Compare December 4, 2024 05:43
@bcardosolopes bcardosolopes merged commit 34b3af2 into llvm:main Dec 12, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants