[PATCH] D64656: Ensure placeholder instruction for cleanup is created
oydale created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. A placeholder instruction for use in generation of cleanup code for an initializer list would not be emitted if the base class contained a non-trivial destructor and the class contains no fields of its own. This would be the case when using CTAD to deduce the template arguments for a struct with an overloaded call operator, e.g. template struct ctad : Ts... {}; template ctad(Ts...)->ctad; and this class was initialized with a list of lambdas capturing by copy, e.g. ctad c {[s](short){}, [s](long){}}; In a release build the bug would manifest itself as a crash in the SROA pass, however, in a debug build the following assert in CGCleanup.cpp would fail: assert(dominatingIP && "no existing variable and no dominating IP!"); By ensuring that a placeholder instruction is emitted even if there's no fields in the class, neither the assert nor the crash is reproducible. See https://bugs.llvm.org/show_bug.cgi?id=40771 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D64656 Files: clang/lib/CodeGen/CGExprAgg.cpp Index: clang/lib/CodeGen/CGExprAgg.cpp === --- clang/lib/CodeGen/CGExprAgg.cpp +++ clang/lib/CodeGen/CGExprAgg.cpp @@ -1618,6 +1618,14 @@ GEP->eraseFromParent(); } + // Base class has a destructor and there's no fields in the class itself, + // create a placeholder here since we did not create one earlier. + if (!cleanupDominator && !cleanups.empty()) +cleanupDominator = CGF.Builder.CreateAlignedLoad( +CGF.Int8Ty, +llvm::Constant::getNullValue(CGF.Int8PtrTy), +CharUnits::One()); // placeholder + // Deactivate all the partial cleanups in reverse order, which // generally means popping them. for (unsigned i = cleanups.size(); i != 0; --i) Index: clang/lib/CodeGen/CGExprAgg.cpp === --- clang/lib/CodeGen/CGExprAgg.cpp +++ clang/lib/CodeGen/CGExprAgg.cpp @@ -1618,6 +1618,14 @@ GEP->eraseFromParent(); } + // Base class has a destructor and there's no fields in the class itself, + // create a placeholder here since we did not create one earlier. + if (!cleanupDominator && !cleanups.empty()) +cleanupDominator = CGF.Builder.CreateAlignedLoad( +CGF.Int8Ty, +llvm::Constant::getNullValue(CGF.Int8PtrTy), +CharUnits::One()); // placeholder + // Deactivate all the partial cleanups in reverse order, which // generally means popping them. for (unsigned i = cleanups.size(); i != 0; --i) ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64656: Ensure placeholder instruction for cleanup is created
oydale updated this revision to Diff 209584. oydale added a comment. Added regression test based on code in bug report Moved logic to a lambda Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/ https://reviews.llvm.org/D64656 Files: clang/lib/CodeGen/CGExprAgg.cpp clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp === --- /dev/null +++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp @@ -0,0 +1,25 @@ +// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O0 %s +// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O0 %s +// CHECK no crash + +struct outer +{ +struct inner +{ +~inner() {} +}; + +outer() : member() {} +outer(const outer&) : member() {} + +inner member; +}; + +template struct ctad : Ts... {}; +template ctad(Ts...) -> ctad; + +void crash() +{ +outer s; +ctad c { [s] (int) {}, [s] (short) {} }; +} Index: clang/lib/CodeGen/CGExprAgg.cpp === --- clang/lib/CodeGen/CGExprAgg.cpp +++ clang/lib/CodeGen/CGExprAgg.cpp @@ -1495,6 +1495,13 @@ // initializers throws an exception. SmallVector cleanups; llvm::Instruction *cleanupDominator = nullptr; + auto addCleanup = [&](const EHScopeStack::stable_iterator &cleanup) { +cleanups.push_back(cleanup); +if (!cleanupDominator) // create placeholder once needed + cleanupDominator = CGF.Builder.CreateAlignedLoad( + CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy), + CharUnits::One()); + }; unsigned curInitIndex = 0; @@ -1519,7 +1526,7 @@ if (QualType::DestructionKind dtorKind = Base.getType().isDestructedType()) { CGF.pushDestroy(dtorKind, V, Base.getType()); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); } } } @@ -1596,15 +1603,9 @@ = field->getType().isDestructedType()) { assert(LV.isSimple()); if (CGF.needsEHCleanup(dtorKind)) { -if (!cleanupDominator) - cleanupDominator = CGF.Builder.CreateAlignedLoad( - CGF.Int8Ty, - llvm::Constant::getNullValue(CGF.Int8PtrTy), - CharUnits::One()); // placeholder - CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(), CGF.getDestroyer(dtorKind), false); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); pushedCleanup = true; } } @@ -1620,6 +1621,8 @@ // Deactivate all the partial cleanups in reverse order, which // generally means popping them. + assert((cleanupDominator || cleanups.empty()) && + "Missing cleanupDominator before deactivating cleanup blocks"); for (unsigned i = cleanups.size(); i != 0; --i) CGF.DeactivateCleanupBlock(cleanups[i-1], cleanupDominator); Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp === --- /dev/null +++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp @@ -0,0 +1,25 @@ +// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O0 %s +// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O0 %s +// CHECK no crash + +struct outer +{ +struct inner +{ +~inner() {} +}; + +outer() : member() {} +outer(const outer&) : member() {} + +inner member; +}; + +template struct ctad : Ts... {}; +template ctad(Ts...) -> ctad; + +void crash() +{ +outer s; +ctad c { [s] (int) {}, [s] (short) {} }; +} Index: clang/lib/CodeGen/CGExprAgg.cpp === --- clang/lib/CodeGen/CGExprAgg.cpp +++ clang/lib/CodeGen/CGExprAgg.cpp @@ -1495,6 +1495,13 @@ // initializers throws an exception. SmallVector cleanups; llvm::Instruction *cleanupDominator = nullptr; + auto addCleanup = [&](const EHScopeStack::stable_iterator &cleanup) { +cleanups.push_back(cleanup); +if (!cleanupDominator) // create placeholder once needed + cleanupDominator = CGF.Builder.CreateAlignedLoad( + CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy), + CharUnits::One()); + }; unsigned curInitIndex = 0; @@ -1519,7 +1526,7 @@ if (QualType::DestructionKind dtorKind = Base.getType().isDestructedType()) { CGF.pushDestroy(dtorKind, V, Base.getType()); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); } } } @@ -1596,15 +1603,9 @@ = field->getType().isDestructedType()) { assert(LV.isSimple()); if (CGF.needsEHCleanup(
[PATCH] D64656: Ensure placeholder instruction for cleanup is created
oydale updated this revision to Diff 209585. oydale added a comment. Reduce test case to a single execution with -O1 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/ https://reviews.llvm.org/D64656 Files: clang/lib/CodeGen/CGExprAgg.cpp clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp === --- /dev/null +++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp @@ -0,0 +1,24 @@ +// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s +// CHECK no crash + +struct outer +{ +struct inner +{ +~inner() {} +}; + +outer() : member() {} +outer(const outer&) : member() {} + +inner member; +}; + +template struct ctad : Ts... {}; +template ctad(Ts...) -> ctad; + +void crash() +{ +outer s; +ctad c { [s] (int) {}, [s] (short) {} }; +} Index: clang/lib/CodeGen/CGExprAgg.cpp === --- clang/lib/CodeGen/CGExprAgg.cpp +++ clang/lib/CodeGen/CGExprAgg.cpp @@ -1495,6 +1495,13 @@ // initializers throws an exception. SmallVector cleanups; llvm::Instruction *cleanupDominator = nullptr; + auto addCleanup = [&](const EHScopeStack::stable_iterator &cleanup) { +cleanups.push_back(cleanup); +if (!cleanupDominator) // create placeholder once needed + cleanupDominator = CGF.Builder.CreateAlignedLoad( + CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy), + CharUnits::One()); + }; unsigned curInitIndex = 0; @@ -1519,7 +1526,7 @@ if (QualType::DestructionKind dtorKind = Base.getType().isDestructedType()) { CGF.pushDestroy(dtorKind, V, Base.getType()); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); } } } @@ -1596,15 +1603,9 @@ = field->getType().isDestructedType()) { assert(LV.isSimple()); if (CGF.needsEHCleanup(dtorKind)) { -if (!cleanupDominator) - cleanupDominator = CGF.Builder.CreateAlignedLoad( - CGF.Int8Ty, - llvm::Constant::getNullValue(CGF.Int8PtrTy), - CharUnits::One()); // placeholder - CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(), CGF.getDestroyer(dtorKind), false); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); pushedCleanup = true; } } @@ -1620,6 +1621,8 @@ // Deactivate all the partial cleanups in reverse order, which // generally means popping them. + assert((cleanupDominator || cleanups.empty()) && + "Missing cleanupDominator before deactivating cleanup blocks"); for (unsigned i = cleanups.size(); i != 0; --i) CGF.DeactivateCleanupBlock(cleanups[i-1], cleanupDominator); Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp === --- /dev/null +++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp @@ -0,0 +1,24 @@ +// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s +// CHECK no crash + +struct outer +{ +struct inner +{ +~inner() {} +}; + +outer() : member() {} +outer(const outer&) : member() {} + +inner member; +}; + +template struct ctad : Ts... {}; +template ctad(Ts...) -> ctad; + +void crash() +{ +outer s; +ctad c { [s] (int) {}, [s] (short) {} }; +} Index: clang/lib/CodeGen/CGExprAgg.cpp === --- clang/lib/CodeGen/CGExprAgg.cpp +++ clang/lib/CodeGen/CGExprAgg.cpp @@ -1495,6 +1495,13 @@ // initializers throws an exception. SmallVector cleanups; llvm::Instruction *cleanupDominator = nullptr; + auto addCleanup = [&](const EHScopeStack::stable_iterator &cleanup) { +cleanups.push_back(cleanup); +if (!cleanupDominator) // create placeholder once needed + cleanupDominator = CGF.Builder.CreateAlignedLoad( + CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy), + CharUnits::One()); + }; unsigned curInitIndex = 0; @@ -1519,7 +1526,7 @@ if (QualType::DestructionKind dtorKind = Base.getType().isDestructedType()) { CGF.pushDestroy(dtorKind, V, Base.getType()); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); } } } @@ -1596,15 +1603,9 @@ = field->getType().isDestructedType()) { assert(LV.isSimple()); if (CGF.needsEHCleanup(dtorKind)) { -if (!cleanupDominator) - cleanupDominator = CGF.Builder.CreateAlignedLoad( - CGF.Int8Ty, - llvm::Constant::getNullValue(CGF.Int
[PATCH] D64656: Ensure placeholder instruction for cleanup is created
oydale marked 2 inline comments as done. oydale added inline comments. Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:1 +// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s +// CHECK no crash lebedev.ri wrote: > lebedev.ri wrote: > > Why do you need `-O1`? > > > Also, you only check $? Don't you want to check the actual IR? > > I wanted the test case to cover both a debug build and a release build of clang, hence the use of -O1. For a release build without asserts, the issue is only visible when building with -O1 or higher. For a debug build the optimization level doesn't matter because it hits an assert (the one mentioned in the commit message) earlier in the compile process. Using clang++ as provided by my distro and on godbolt the following works fine: ``` clang++ --std=c++17 -c crash.cpp ``` The following crashes: ``` clang++ --std=c++17 -c -O1 crash.cpp ``` I do not have sufficient knowledge of what the IR is expected to look like in this case to write a test for it. Further, all I have done is to ensure that a placeholder instruction is created when it's needed. This placeholder instruction is erased at the end of the function where it was created, so the generated IR should already be covered by other tests. Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:2 +// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s +// CHECK no crash + lebedev.ri wrote: > Either add missing `:` or please write better comment :) I'm not sure what else that comment would contain. I want the test to verify that clang does not crash given the provided input. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/ https://reviews.llvm.org/D64656 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64656: Ensure placeholder instruction for cleanup is created
oydale marked an inline comment as done. oydale added inline comments. Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:2 +// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s +// CHECK no crash + rjmccall wrote: > oydale wrote: > > lebedev.ri wrote: > > > Either add missing `:` or please write better comment :) > > I'm not sure what else that comment would contain. I want the test to > > verify that clang does not crash given the provided input. > We have a testing tool called FileCheck which is used to pattern-match > compiler output, and it uses a lot of lines that look like `CHECK: blah`, so > the all-caps CHECK makes people think that it's supposed to be a FileCheck > line. Please just write the comment normally, like "PR4771: check that this > doesn't crash." > > Was this crash specific to optimization being enabled? I'll change it to a normal-looking comment. Yes, for a release build the crash does not happen unless -O1 or greater is specified. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/ https://reviews.llvm.org/D64656 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64656: Ensure placeholder instruction for cleanup is created
oydale updated this revision to Diff 209627. oydale added a comment. Minimized test case further, removed misleading CHECK from comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/ https://reviews.llvm.org/D64656 Files: clang/lib/CodeGen/CGExprAgg.cpp clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp === --- /dev/null +++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp @@ -0,0 +1,10 @@ +// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions %s +// PR40771: check that this input does not crash or assert + +struct Q { Q(); }; +struct R { R(Q); ~R(); }; +struct S { S(Q); ~S(); }; +struct T : R, S {}; + +Q q; +T t { R{q}, S{q} }; Index: clang/lib/CodeGen/CGExprAgg.cpp === --- clang/lib/CodeGen/CGExprAgg.cpp +++ clang/lib/CodeGen/CGExprAgg.cpp @@ -1495,6 +1495,13 @@ // initializers throws an exception. SmallVector cleanups; llvm::Instruction *cleanupDominator = nullptr; + auto addCleanup = [&](const EHScopeStack::stable_iterator &cleanup) { +cleanups.push_back(cleanup); +if (!cleanupDominator) // create placeholder once needed + cleanupDominator = CGF.Builder.CreateAlignedLoad( + CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy), + CharUnits::One()); + }; unsigned curInitIndex = 0; @@ -1519,7 +1526,7 @@ if (QualType::DestructionKind dtorKind = Base.getType().isDestructedType()) { CGF.pushDestroy(dtorKind, V, Base.getType()); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); } } } @@ -1596,15 +1603,9 @@ = field->getType().isDestructedType()) { assert(LV.isSimple()); if (CGF.needsEHCleanup(dtorKind)) { -if (!cleanupDominator) - cleanupDominator = CGF.Builder.CreateAlignedLoad( - CGF.Int8Ty, - llvm::Constant::getNullValue(CGF.Int8PtrTy), - CharUnits::One()); // placeholder - CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(), CGF.getDestroyer(dtorKind), false); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); pushedCleanup = true; } } @@ -1620,6 +1621,8 @@ // Deactivate all the partial cleanups in reverse order, which // generally means popping them. + assert((cleanupDominator || cleanups.empty()) && + "Missing cleanupDominator before deactivating cleanup blocks"); for (unsigned i = cleanups.size(); i != 0; --i) CGF.DeactivateCleanupBlock(cleanups[i-1], cleanupDominator); Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp === --- /dev/null +++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp @@ -0,0 +1,10 @@ +// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions %s +// PR40771: check that this input does not crash or assert + +struct Q { Q(); }; +struct R { R(Q); ~R(); }; +struct S { S(Q); ~S(); }; +struct T : R, S {}; + +Q q; +T t { R{q}, S{q} }; Index: clang/lib/CodeGen/CGExprAgg.cpp === --- clang/lib/CodeGen/CGExprAgg.cpp +++ clang/lib/CodeGen/CGExprAgg.cpp @@ -1495,6 +1495,13 @@ // initializers throws an exception. SmallVector cleanups; llvm::Instruction *cleanupDominator = nullptr; + auto addCleanup = [&](const EHScopeStack::stable_iterator &cleanup) { +cleanups.push_back(cleanup); +if (!cleanupDominator) // create placeholder once needed + cleanupDominator = CGF.Builder.CreateAlignedLoad( + CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy), + CharUnits::One()); + }; unsigned curInitIndex = 0; @@ -1519,7 +1526,7 @@ if (QualType::DestructionKind dtorKind = Base.getType().isDestructedType()) { CGF.pushDestroy(dtorKind, V, Base.getType()); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); } } } @@ -1596,15 +1603,9 @@ = field->getType().isDestructedType()) { assert(LV.isSimple()); if (CGF.needsEHCleanup(dtorKind)) { -if (!cleanupDominator) - cleanupDominator = CGF.Builder.CreateAlignedLoad( - CGF.Int8Ty, - llvm::Constant::getNullValue(CGF.Int8PtrTy), - CharUnits::One()); // placeholder - CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(), CGF.getDestroyer(dtorKind), false); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_b
[PATCH] D64656: Ensure placeholder instruction for cleanup is created
oydale updated this revision to Diff 209683. oydale added a comment. Replaced -emit-obj with -emit-llvm-only in test case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/ https://reviews.llvm.org/D64656 Files: clang/lib/CodeGen/CGExprAgg.cpp clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp === --- /dev/null +++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp @@ -0,0 +1,10 @@ +// RUN: %clang_cc1 -emit-llvm-only --std=c++17 -fcxx-exceptions -fexceptions %s +// PR40771: check that this input does not crash or assert + +struct Q { Q(); }; +struct R { R(Q); ~R(); }; +struct S { S(Q); ~S(); }; +struct T : R, S {}; + +Q q; +T t { R{q}, S{q} }; Index: clang/lib/CodeGen/CGExprAgg.cpp === --- clang/lib/CodeGen/CGExprAgg.cpp +++ clang/lib/CodeGen/CGExprAgg.cpp @@ -1495,6 +1495,13 @@ // initializers throws an exception. SmallVector cleanups; llvm::Instruction *cleanupDominator = nullptr; + auto addCleanup = [&](const EHScopeStack::stable_iterator &cleanup) { +cleanups.push_back(cleanup); +if (!cleanupDominator) // create placeholder once needed + cleanupDominator = CGF.Builder.CreateAlignedLoad( + CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy), + CharUnits::One()); + }; unsigned curInitIndex = 0; @@ -1519,7 +1526,7 @@ if (QualType::DestructionKind dtorKind = Base.getType().isDestructedType()) { CGF.pushDestroy(dtorKind, V, Base.getType()); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); } } } @@ -1596,15 +1603,9 @@ = field->getType().isDestructedType()) { assert(LV.isSimple()); if (CGF.needsEHCleanup(dtorKind)) { -if (!cleanupDominator) - cleanupDominator = CGF.Builder.CreateAlignedLoad( - CGF.Int8Ty, - llvm::Constant::getNullValue(CGF.Int8PtrTy), - CharUnits::One()); // placeholder - CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(), CGF.getDestroyer(dtorKind), false); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); pushedCleanup = true; } } @@ -1620,6 +1621,8 @@ // Deactivate all the partial cleanups in reverse order, which // generally means popping them. + assert((cleanupDominator || cleanups.empty()) && + "Missing cleanupDominator before deactivating cleanup blocks"); for (unsigned i = cleanups.size(); i != 0; --i) CGF.DeactivateCleanupBlock(cleanups[i-1], cleanupDominator); Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp === --- /dev/null +++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp @@ -0,0 +1,10 @@ +// RUN: %clang_cc1 -emit-llvm-only --std=c++17 -fcxx-exceptions -fexceptions %s +// PR40771: check that this input does not crash or assert + +struct Q { Q(); }; +struct R { R(Q); ~R(); }; +struct S { S(Q); ~S(); }; +struct T : R, S {}; + +Q q; +T t { R{q}, S{q} }; Index: clang/lib/CodeGen/CGExprAgg.cpp === --- clang/lib/CodeGen/CGExprAgg.cpp +++ clang/lib/CodeGen/CGExprAgg.cpp @@ -1495,6 +1495,13 @@ // initializers throws an exception. SmallVector cleanups; llvm::Instruction *cleanupDominator = nullptr; + auto addCleanup = [&](const EHScopeStack::stable_iterator &cleanup) { +cleanups.push_back(cleanup); +if (!cleanupDominator) // create placeholder once needed + cleanupDominator = CGF.Builder.CreateAlignedLoad( + CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy), + CharUnits::One()); + }; unsigned curInitIndex = 0; @@ -1519,7 +1526,7 @@ if (QualType::DestructionKind dtorKind = Base.getType().isDestructedType()) { CGF.pushDestroy(dtorKind, V, Base.getType()); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); } } } @@ -1596,15 +1603,9 @@ = field->getType().isDestructedType()) { assert(LV.isSimple()); if (CGF.needsEHCleanup(dtorKind)) { -if (!cleanupDominator) - cleanupDominator = CGF.Builder.CreateAlignedLoad( - CGF.Int8Ty, - llvm::Constant::getNullValue(CGF.Int8PtrTy), - CharUnits::One()); // placeholder - CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(), CGF.getDestroyer(dtorKind), false); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_beg
[PATCH] D64656: Ensure placeholder instruction for cleanup is created
oydale updated this revision to Diff 209690. oydale added a comment. Initial attempt at IR output checks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/ https://reviews.llvm.org/D64656 Files: clang/lib/CodeGen/CGExprAgg.cpp clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp === --- /dev/null +++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp @@ -0,0 +1,51 @@ +// RUN: %clang_cc1 -emit-llvm --std=c++17 -fcxx-exceptions -fexceptions \ +// RUN: %s -o - | FileCheck %s + +struct Q { Q(); }; +struct R { R(Q); ~R(); }; +struct S { S(Q); ~S(); }; +struct T : R, S {}; + +Q q; +T t { R{q}, S{q} }; + +// CHECK-LABEL: define internal void @__cxx_global_var_init.1() {{.*}} { +// CHECK-LABEL: entry: +// CHECK: %cleanup.isactive = alloca i1, align 1 +// CHECK: call void @_ZN1RC1E1Q(%struct.R* %ref.tmp) +// CHECK: store i1 true, i1* %cleanup.isactive, align 1 +// CHECK: invoke void @_ZN1SC1E1Q(%struct.S* %ref.tmp1) +// CHECK: to label %invoke.cont unwind label %lpad + +// CHECK-LABEL: invoke.cont: +// CHECK: store i1 false, i1* %cleanup.isactive, align 1 +// CHECK: call void @_ZN1SD1Ev(%struct.S* +// CHECK: call void @_ZN1RD1Ev(%struct.R* +// CHECK: %0 = call i32 @__cxa_atexit( +// CHECK: ret void + +// CHECK-LABEL: lpad: +// CHECK: %1 = landingpad { i8*, i32 } +// CHECK:cleanup +// CHECK: %2 = extractvalue { i8*, i32 } %1, 0 +// CHECK: store i8* %2, i8** %exn.slot, align 8 +// CHECK: %3 = extractvalue { i8*, i32 } %1, 1 +// CHECK: store i32 %3, i32* %ehselector.slot, align 4 +// CHECK: %cleanup.is_active = load i1, i1* %cleanup.isactive, align 1 +// CHECK: br i1 %cleanup.is_active, label %cleanup.action, label %cleanup.done + +// CHECK-LABEL: cleanup.action: +// CHECK: call void @_ZN1RD1Ev(%struct.R* +// CHECK: br label %cleanup.done + +// CHECK-LABEL: cleanup.done: +// CHECK: call void @_ZN1RD1Ev(%struct.R* %ref.tmp) +// CHECK: br label %eh.resume + +// CHECK-LABEL: eh.resume: +// CHECK: %exn = load i8*, i8** %exn.slot, align 8 +// CHECK: %sel = load i32, i32* %ehselector.slot, align 4 +// CHECK: %lpad.val = insertvalue { i8*, i32 } undef, i8* %exn, 0 +// CHECK: %lpad.val3 = insertvalue { i8*, i32 } %lpad.val, i32 %sel, 1 +// CHECK: resume { i8*, i32 } %lpad.val3 +// CHECK: } Index: clang/lib/CodeGen/CGExprAgg.cpp === --- clang/lib/CodeGen/CGExprAgg.cpp +++ clang/lib/CodeGen/CGExprAgg.cpp @@ -1495,6 +1495,13 @@ // initializers throws an exception. SmallVector cleanups; llvm::Instruction *cleanupDominator = nullptr; + auto addCleanup = [&](const EHScopeStack::stable_iterator &cleanup) { +cleanups.push_back(cleanup); +if (!cleanupDominator) // create placeholder once needed + cleanupDominator = CGF.Builder.CreateAlignedLoad( + CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy), + CharUnits::One()); + }; unsigned curInitIndex = 0; @@ -1519,7 +1526,7 @@ if (QualType::DestructionKind dtorKind = Base.getType().isDestructedType()) { CGF.pushDestroy(dtorKind, V, Base.getType()); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); } } } @@ -1596,15 +1603,9 @@ = field->getType().isDestructedType()) { assert(LV.isSimple()); if (CGF.needsEHCleanup(dtorKind)) { -if (!cleanupDominator) - cleanupDominator = CGF.Builder.CreateAlignedLoad( - CGF.Int8Ty, - llvm::Constant::getNullValue(CGF.Int8PtrTy), - CharUnits::One()); // placeholder - CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(), CGF.getDestroyer(dtorKind), false); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); pushedCleanup = true; } } @@ -1620,6 +1621,8 @@ // Deactivate all the partial cleanups in reverse order, which // generally means popping them. + assert((cleanupDominator || cleanups.empty()) && + "Missing cleanupDominator before deactivating cleanup blocks"); for (unsigned i = cleanups.size(); i != 0; --i) CGF.DeactivateCleanupBlock(cleanups[i-1], cleanupDominator); ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64656: Ensure placeholder instruction for cleanup is created
oydale added a comment. I have added my initial attempt at verifying the cleanup code generated in this scenario, it's probably going to require modifications. The original IR can be found at https://reviews.llvm.org/P8156. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/ https://reviews.llvm.org/D64656 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64656: Ensure placeholder instruction for cleanup is created
oydale added a comment. My understanding of the issue is that clang emits incorrect IR. Without my fix and when disabling the assertion mentioned in the commit message by commenting it out, llvm-lit gives the following output when executed against the minimal test case in the current version of the commit: + /home/maestro/llvm/llvm-project/build/bin/clang -cc1 -internal-isystem /home/maestro/llvm/llvm-project/build/lib/clang/9.0.0/include -nostdsysteminc -emit-obj --std=c++17 -fcxx-exceptions -fexceptions /home/maestro/llvm/llvm-project/clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp Instruction referencing instruction not embedded in a basic block! %cleanup.isactive = alloca i1, align 1 store i1 true, i1* %cleanup.isactive, align 1 in function __cxx_global_var_init.1 fatal error: error in backend: Broken function found, compilation aborted! This is what makes me assume that the IR output is incorrect. Without my fix and with the assertion still commented out, disabling the LLVM verification, and enabling optimizations with -O1, the debug build of clang yields a similar crash as what can be observed in release builds such as on godbolt: ~/llvm/llvm-project/build/bin/clang-9 -cc1 -triple x86_64-pc-linux-gnu -emit-obj -disable-free -disable-llvm-verifier -std=c++17 -fexceptions -O1 -fcxx-exceptions minimal.cpp Stack dump: 0.Program arguments: /home/maestro/llvm/llvm-project/build/bin/clang-9 -cc1 -triple x86_64-pc-linux-gnu -emit-obj -disable-free -disable-llvm-verifier -std=c++17 -fexceptions -O1 -fcxx-exceptions minimal.cpp 1. parser at end of file 2.Per-function optimization 3.Running pass 'SROA' on function '@__cxx_global_var_init.1' #0 0x7f2309f7b7a7 llvm::sys::PrintStackTrace(llvm::raw_ostream&) /home/maestro/llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:494:22 #1 0x7f2309f7b83a PrintStackTraceSignalHandler(void*) /home/maestro/llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:555:1 #2 0x7f2309f79834 llvm::sys::RunSignalHandlers() /home/maestro/llvm/llvm-project/llvm/lib/Support/Signals.cpp:68:20 #3 0x7f2309f7b1fd SignalHandler(int) /home/maestro/llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:357:1 #4 0x7f230929e4d0 __restore_rt (/usr/lib/libpthread.so.0+0x124d0) #5 0x7f230be28fda llvm::PointerIntPair*, 1u, unsigned int, llvm::PointerLikeTypeTraits*>, llvm::PointerIntPairInfo*, 1u, llvm::PointerLikeTypeTraits*> > >::setPointer(llvm::ilist_node_base*) /home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/PointerIntPair.h:63:32 #6 0x7f230be267cb llvm::ilist_node_base::setPrev(llvm::ilist_node_base*) /home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist_node_base.h:40:75 #7 0x7f230be3748b llvm::ilist_base::removeImpl(llvm::ilist_node_base&) /home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist_base.h:34:5 #8 0x7f230be37725 void llvm::ilist_base::remove > >(llvm::ilist_node_impl >&) /home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist_base.h:80:64 #9 0x7f230be37166 llvm::simple_ilist::remove(llvm::Instruction&) /home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/simple_ilist.h:183:77 #10 0x7f230be36aa2 llvm::iplist_impl, llvm::SymbolTableListTraits >::remove(llvm::ilist_iterator, false, false>&) /home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist.h:253:12 #11 0x7f230be35bf1 llvm::iplist_impl, llvm::SymbolTableListTraits >::erase(llvm::ilist_iterator, false, false>) /home/maestro/llvm/llvm-project/llvm/include/llvm/ADT/ilist.h:266:5 #12 0x7f230bf7bb5d llvm::Instruction::eraseFromParent() /home/maestro/llvm/llvm-project/llvm/lib/IR/Instruction.cpp:69:1 #13 0x7f230a611a47 llvm::SROA::deleteDeadInstructions(llvm::SmallPtrSetImpl&) /home/maestro/llvm/llvm-project/llvm/lib/Transforms/Scalar/SROA.cpp:4520:13 #14 0x7f230a611df1 llvm::SROA::runImpl(llvm::Function&, llvm::DominatorTree&, llvm::AssumptionCache&) /home/maestro/llvm/llvm-project/llvm/lib/Transforms/Scalar/SROA.cpp:4564:15 #15 0x7f230a62979e llvm::sroa::SROALegacyPass::runOnFunction(llvm::Function&) /home/maestro/llvm/llvm-project/llvm/lib/Transforms/Scalar/SROA.cpp:4620:31 #16 0x7f230bfc820a llvm::FPPassManager::runOnFunction(llvm::Function&) /home/maestro/llvm/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1648:20 #17 0x7f230bfc7e2c llvm::legacy::FunctionPassManagerImpl::run(llvm::Function&) /home/maestro/llvm/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1585:13 #18 0x7f230bfc7a14 llvm::legacy::FunctionPassManager::run(llvm::Function&) /home/maestro/llvm/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1511:1 #19 0x7f2307ff5d67 (anonymous namespace)::EmitAssemblyHelper::EmitAssembly(clang::BackendAction, std::unique_ptr >) /home/maestro/llvm/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:885:25 #20 0x7f2307ffa457 clang::EmitBackendOutput(clang::DiagnosticsEng
[PATCH] D64656: Ensure placeholder instruction for cleanup is created
oydale updated this revision to Diff 209701. oydale added a comment. Changed test to use CHECK-NEXT, -discard-value-names, and pattern matching Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/ https://reviews.llvm.org/D64656 Files: clang/lib/CodeGen/CGExprAgg.cpp clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp Index: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp === --- /dev/null +++ clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp @@ -0,0 +1,56 @@ +// RUN: %clang_cc1 -emit-llvm --std=c++17 -fcxx-exceptions -fexceptions \ +// RUN: -discard-value-names %s -o - | FileCheck %s + +struct Q { Q(); }; +struct R { R(Q); ~R(); }; +struct S { S(Q); ~S(); }; +struct T : R, S {}; + +Q q; +T t { R{q}, S{q} }; + +// CHECK-LABEL: define internal void @__cxx_global_var_init.1() {{.*}} { +// CHECK-NEXT: [[TMP_R:%[a-z0-9.]+]] = alloca %struct.R, align 1 +// CHECK-NEXT: [[TMP_Q1:%[a-z0-9.]+]] = alloca %struct.Q, align 1 +// CHECK-NEXT: [[TMP_S:%[a-z0-9.]+]] = alloca %struct.S, align 1 +// CHECK-NEXT: [[TMP_Q2:%[a-z0-9.]+]] = alloca %struct.Q, align 1 +// CHECK-NEXT: [[XPT:%[a-z0-9.]+]] = alloca i8* +// CHECK-NEXT: [[SLOT:%[a-z0-9.]+]] = alloca i32 +// CHECK-NEXT: [[ACTIVE:%[a-z0-9.]+]] = alloca i1, align 1 +// CHECK-NEXT: call void @_ZN1RC1E1Q(%struct.R* [[TMP_R]]) +// CHECK-NEXT: store i1 true, i1* [[ACTIVE]], align 1 +// CHECK-NEXT: invoke void @_ZN1SC1E1Q(%struct.S* [[TMP_S]]) +// CHECK-NEXT: to label %[[L1:[a-z0-9.]+]] unwind label %[[L2:[a-z0-9.]+]] +// CHECK-EMPTY: +// CHECK-NEXT: [[L1]]: +// CHECK-NEXT: store i1 false, i1* [[ACTIVE]], align 1 +// CHECK-NEXT: call void @_ZN1SD1Ev(%struct.S* +// CHECK-NEXT: call void @_ZN1RD1Ev(%struct.R* +// CHECK-NEXT: [[EXIT:%[a-z0-9.]+]] = call i32 @__cxa_atexit( +// CHECK-NEXT: ret void +// CHECK-EMPTY: +// CHECK-NEXT: [[L2]]: +// CHECK-NEXT: [[LP:%[a-z0-9.]+]] = landingpad { i8*, i32 } +// CHECK-NEXT: cleanup +// CHECK-NEXT: [[X1:%[a-z0-9.]+]] = extractvalue { i8*, i32 } [[LP]], 0 +// CHECK-NEXT: store i8* [[X1]], i8** [[XPT]], align 8 +// CHECK-NEXT: [[X2:%[a-z0-9.]+]] = extractvalue { i8*, i32 } [[LP]], 1 +// CHECK-NEXT: store i32 [[X2]], i32* [[SLOT]], align 4 +// CHECK-NEXT: [[IS_ACT:%[a-z0-9.]+]] = load i1, i1* [[ACTIVE]], align 1 +// CHECK-NEXT: br i1 [[IS_ACT]], label %[[L3:[a-z0-9.]+]], label %[[L4:[a-z0-9.]+]] +// CHECK-EMPTY: +// CHECK-NEXT: [[L3]]: +// CHECK-NEXT: call void @_ZN1RD1Ev(%struct.R* +// CHECK-NEXT: br label %[[L4]] +// CHECK-EMPTY: +// CHECK-NEXT: [[L4]]: +// CHECK-NEXT: call void @_ZN1RD1Ev(%struct.R* [[TMP_R]]) +// CHECK-NEXT: br label %[[L5:[a-z0-9.]+]] +// CHECK-EMPTY: +// CHECK-NEXT: [[L5]]: +// CHECK-NEXT: [[EXN:%[a-z0-9.]+]] = load i8*, i8** [[XPT]], align 8 +// CHECK-NEXT: [[SEL:%[a-z0-9.]+]] = load i32, i32* [[SLOT]], align 4 +// CHECK-NEXT: [[LV1:%[a-z0-9.]+]] = insertvalue { i8*, i32 } undef, i8* [[EXN]], 0 +// CHECK-NEXT: [[LV2:%[a-z0-9.]+]] = insertvalue { i8*, i32 } [[LV1]], i32 [[SEL]], 1 +// CHECK-NEXT: resume { i8*, i32 } [[LV2]] +// CHECK-NEXT: } Index: clang/lib/CodeGen/CGExprAgg.cpp === --- clang/lib/CodeGen/CGExprAgg.cpp +++ clang/lib/CodeGen/CGExprAgg.cpp @@ -1495,6 +1495,13 @@ // initializers throws an exception. SmallVector cleanups; llvm::Instruction *cleanupDominator = nullptr; + auto addCleanup = [&](const EHScopeStack::stable_iterator &cleanup) { +cleanups.push_back(cleanup); +if (!cleanupDominator) // create placeholder once needed + cleanupDominator = CGF.Builder.CreateAlignedLoad( + CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy), + CharUnits::One()); + }; unsigned curInitIndex = 0; @@ -1519,7 +1526,7 @@ if (QualType::DestructionKind dtorKind = Base.getType().isDestructedType()) { CGF.pushDestroy(dtorKind, V, Base.getType()); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); } } } @@ -1596,15 +1603,9 @@ = field->getType().isDestructedType()) { assert(LV.isSimple()); if (CGF.needsEHCleanup(dtorKind)) { -if (!cleanupDominator) - cleanupDominator = CGF.Builder.CreateAlignedLoad( - CGF.Int8Ty, - llvm::Constant::getNullValue(CGF.Int8PtrTy), - CharUnits::One()); // placeholder - CGF.pushDestroy(EHCleanup, LV.getAddress(), field->getType(), CGF.getDestroyer(dtorKind), false); -cleanups.push_back(CGF.EHStack.stable_begin()); +addCleanup(CGF.EHStack.stable_begin()); pushedCleanup = true; } } @@ -1620,6 +1621,8 @@ // Deactivate all the partial cleanups in reverse order, which // generally means popping them. + assert((cleanupDominator || cleanups.empty()) && +
[PATCH] D64656: Ensure placeholder instruction for cleanup is created
oydale accepted this revision. oydale added a comment. Feel free to commit this patch on my behalf. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/ https://reviews.llvm.org/D64656 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64656: Ensure placeholder instruction for cleanup is created
oydale added a comment. @rsmith, @rjmccall, I don't have access to commit this myself, could one of you commit it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/ https://reviews.llvm.org/D64656 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D64656: Ensure placeholder instruction for cleanup is created
oydale abandoned this revision. oydale added a comment. compnerd added a triple to the test case and re-committed the patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/ https://reviews.llvm.org/D64656 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits