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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions clang/include/clang/CIR/Dialect/IR/CIROps.td
Original file line number Diff line number Diff line change
Expand Up @@ -3531,6 +3531,16 @@ class CIR_CallOp<string mnemonic, list<Trait> extra_traits = []> :

bool isIndirect() { return !getCallee(); }
mlir::Value getIndirectCall();

void setArg(unsigned index, mlir::Value value) {
if (!isIndirect()) {
setOperand(index, value);
return;
}

// For indirect call, the operand list is shifted by one.
setOperand(index + 1, value);
}
}];

let hasCustomAssemblyFormat = 1;
Expand Down
12 changes: 8 additions & 4 deletions clang/lib/CIR/CodeGen/CIRGenCXX.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,8 @@ bool CIRGenModule::tryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D) {
// members with attribute "AlwaysInline" and expect no reference to
// be generated. It is desirable to reenable this optimisation after
// corresponding LLVM changes.
llvm_unreachable("NYI");
addReplacement(MangledName, Aliasee);
return false;
}

// If we have a weak, non-discardable alias (weak, weak_odr), like an
Expand All @@ -154,7 +155,8 @@ bool CIRGenModule::tryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D) {
// symbol reference from another TU. The other TU must also mark the
// referenced symbol as weak, which we cannot rely on.
if (cir::isWeakForLinker(Linkage) && getTriple().isOSBinFormatCOFF()) {
llvm_unreachable("NYI");
llvm_unreachable("please sent a PR with a test and remove this.\n");
return true;
}

// If we don't have a definition for the destructor yet or the definition
Expand All @@ -168,8 +170,10 @@ bool CIRGenModule::tryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D) {
// different COMDATs in different TUs. Another option would be to
// output the alias both for weak_odr and linkonce_odr, but that
// requires explicit comdat support in the IL.
if (cir::isWeakForLinker(TargetLinkage))
llvm_unreachable("NYI");
if (cir::isWeakForLinker(TargetLinkage)) {
llvm_unreachable("please sent a PR with a test and remove this.\n");
return true;
}

// Create the alias with no name.
emitAliasForGlobal(MangledName, Entry, AliasDecl, Aliasee, Linkage);
Expand Down
37 changes: 37 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3193,6 +3193,39 @@ void CIRGenModule::addReplacement(StringRef Name, mlir::Operation *Op) {
Replacements[Name] = Op;
}

void CIRGenModule::replacePointerTypeArgs(cir::FuncOp OldF, cir::FuncOp NewF) {
auto optionalUseRange = OldF.getSymbolUses(theModule);
if (!optionalUseRange)
return;

for (auto U : *optionalUseRange) {
// CallTryOp only shows up after FlattenCFG.
auto Call = mlir::dyn_cast<cir::CallOp>(U.getUser());
if (!Call)
continue;

auto ArgOps = Call.getArgOps();
auto FuncArgTypes = NewF.getFunctionType().getInputs();
for (unsigned I = 0; I < FuncArgTypes.size(); I++) {
if (ArgOps[I].getType() == FuncArgTypes[I])
continue;

auto argPointerTy = mlir::dyn_cast<cir::PointerType>(ArgOps[I].getType());
auto funcArgPointerTy = mlir::dyn_cast<cir::PointerType>(FuncArgTypes[I]);

// If we can't solve it, leave it for the verifier to bail out.
if (!argPointerTy || !funcArgPointerTy)
continue;

mlir::OpBuilder::InsertionGuard guard(builder);
builder.setInsertionPoint(Call);
auto castedArg =
builder.createBitcast(Call.getLoc(), ArgOps[I], funcArgPointerTy);
Call.setArg(I, castedArg);
}
}
}

void CIRGenModule::applyReplacements() {
for (auto &I : Replacements) {
StringRef MangledName = I.first();
Expand All @@ -3205,6 +3238,10 @@ void CIRGenModule::applyReplacements() {
auto NewF = dyn_cast<cir::FuncOp>(Replacement);
assert(NewF && "not implemented");

// LLVM has opaque pointer but CIR not. So we may have to handle these
// different pointer types when performing replacement.
replacePointerTypeArgs(OldF, NewF);

// Replace old with new, but keep the old order.
if (OldF.replaceAllSymbolUses(NewF.getSymNameAttr(), theModule).failed())
llvm_unreachable("internal error, cannot RAUW symbol");
Expand Down
5 changes: 5 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenModule.h
Original file line number Diff line number Diff line change
Expand Up @@ -871,6 +871,11 @@ class CIRGenModule : public CIRGenTypeCache {
/// Call replaceAllUsesWith on all pairs in Replacements.
void applyReplacements();

/// A helper function to replace all uses of OldF to NewF that replace
/// the type of pointer arguments. This is not needed to tradtional
/// pipeline since LLVM has opaque pointers but CIR not.
void replacePointerTypeArgs(cir::FuncOp OldF, cir::FuncOp NewF);

void setNonAliasAttributes(GlobalDecl GD, mlir::Operation *GV);
/// Map source language used to a CIR attribute.
cir::SourceLanguage getCIRSourceLanguage();
Expand Down
18 changes: 18 additions & 0 deletions clang/test/CIR/CodeGen/dtor-alias.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// FIXME: Remove of -clangir-disable-passes may trigger a memory safe bug in CIR internally during
// lowering
// RUN: %clang_cc1 -std=c++17 -triple x86_64-unknown-linux-gnu \
// 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

// RUN: FileCheck %s --input-file=%t.cir

namespace {
struct A {
~A() {}
};

struct B : public A {};
}

B x;

// CHECK: cir.call @_ZN12_GLOBAL__N_11AD2Ev({{.*}}) : (!cir.ptr<!ty_28anonymous_namespace293A3AA>) -> ()
Loading