https://github.com/usx95 updated https://github.com/llvm/llvm-project/pull/85398
>From 0654f624b3b60026398e8b0168623a85e3129630 Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena <u...@google.com> Date: Fri, 15 Mar 2024 13:19:36 +0000 Subject: [PATCH 1/3] [codegen] Emit cleanups for branch in stmt-expr and coro suspensions --- clang/lib/CodeGen/CGCleanup.cpp | 7 +++++-- clang/lib/CodeGen/CGExprAgg.cpp | 13 +++++++------ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/clang/lib/CodeGen/CGCleanup.cpp b/clang/lib/CodeGen/CGCleanup.cpp index f87caf050eeaa7..c22d4da0fefc3c 100644 --- a/clang/lib/CodeGen/CGCleanup.cpp +++ b/clang/lib/CodeGen/CGCleanup.cpp @@ -694,7 +694,8 @@ void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough) { // - whether there's a fallthrough llvm::BasicBlock *FallthroughSource = Builder.GetInsertBlock(); - bool HasFallthrough = (FallthroughSource != nullptr && IsActive); + bool HasFallthrough = + FallthroughSource != nullptr && (IsActive || HasExistingBranches); // Branch-through fall-throughs leave the insertion point set to the // end of the last cleanup, which points to the current scope. The @@ -719,7 +720,9 @@ void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough) { // If we have a prebranched fallthrough into an inactive normal // cleanup, rewrite it so that it leads to the appropriate place. - if (Scope.isNormalCleanup() && HasPrebranchedFallthrough && !IsActive) { + if (Scope.isNormalCleanup() && HasPrebranchedFallthrough && + !RequiresNormalCleanup) { + assert(!IsActive); llvm::BasicBlock *prebranchDest; // If the prebranch is semantically branching through the next diff --git a/clang/lib/CodeGen/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp index 5190b22bcc1622..7e62599089b8df 100644 --- a/clang/lib/CodeGen/CGExprAgg.cpp +++ b/clang/lib/CodeGen/CGExprAgg.cpp @@ -15,6 +15,7 @@ #include "CodeGenFunction.h" #include "CodeGenModule.h" #include "ConstantEmitter.h" +#include "EHScopeStack.h" #include "TargetInfo.h" #include "clang/AST/ASTContext.h" #include "clang/AST/Attr.h" @@ -1389,15 +1390,15 @@ AggExprEmitter::VisitLambdaExpr(LambdaExpr *E) { if (QualType::DestructionKind DtorKind = CurField->getType().isDestructedType()) { assert(LV.isSimple()); - if (CGF.needsEHCleanup(DtorKind)) { + if (DtorKind) { if (!CleanupDominator) CleanupDominator = CGF.Builder.CreateAlignedLoad( CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy), CharUnits::One()); // placeholder - CGF.pushDestroy(EHCleanup, LV.getAddress(CGF), CurField->getType(), - CGF.getDestroyer(DtorKind), false); + CGF.pushDestroy(NormalAndEHCleanup, LV.getAddress(CGF), + CurField->getType(), CGF.getDestroyer(DtorKind), false); Cleanups.push_back(CGF.EHStack.stable_begin()); } } @@ -1806,9 +1807,9 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr( if (QualType::DestructionKind dtorKind = field->getType().isDestructedType()) { assert(LV.isSimple()); - if (CGF.needsEHCleanup(dtorKind)) { - CGF.pushDestroy(EHCleanup, LV.getAddress(CGF), field->getType(), - CGF.getDestroyer(dtorKind), false); + if (dtorKind) { + CGF.pushDestroy(NormalAndEHCleanup, LV.getAddress(CGF), + field->getType(), CGF.getDestroyer(dtorKind), false); addCleanup(CGF.EHStack.stable_begin()); pushedCleanup = true; } >From 1b7c160b030f3a765d40723bbbf9a7daa4a1e090 Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena <u...@google.com> Date: Fri, 15 Mar 2024 15:22:07 +0000 Subject: [PATCH 2/3] add tests --- .../control-flow-in-expr-cleanup.cpp | 172 ++++++++++++++++++ 1 file changed, 172 insertions(+) create mode 100644 clang/test/CodeGenCoroutines/control-flow-in-expr-cleanup.cpp diff --git a/clang/test/CodeGenCoroutines/control-flow-in-expr-cleanup.cpp b/clang/test/CodeGenCoroutines/control-flow-in-expr-cleanup.cpp new file mode 100644 index 00000000000000..d703532b5a10b9 --- /dev/null +++ b/clang/test/CodeGenCoroutines/control-flow-in-expr-cleanup.cpp @@ -0,0 +1,172 @@ +// RUN: %clang_cc1 --std=c++20 -triple x86_64-linux-gnu -emit-llvm %s -o - | FileCheck %s + +#include "Inputs/coroutine.h" + +struct Printy { + Printy(const char *name) : name(name) {} + ~Printy() {} + const char *name; +}; + +struct coroutine { + struct promise_type; + std::coroutine_handle<promise_type> handle; + ~coroutine() { + if (handle) handle.destroy(); + } +}; + +struct coroutine::promise_type { + coroutine get_return_object() { + return {std::coroutine_handle<promise_type>::from_promise(*this)}; + } + std::suspend_never initial_suspend() noexcept { return {}; } + std::suspend_always final_suspend() noexcept { return {}; } + void return_void() {} + void unhandled_exception() {} +}; + +struct Awaiter : std::suspend_always { + Printy await_resume() { return {"awaited"}; } +}; + +int foo() { return 2; } + +struct PrintiesCopy { + Printy a; + Printy b; + Printy c; +}; + +void ParenInit() { + // CHECK: define dso_local void @_Z9ParenInitv() + // CHECK: [[CLEANUP_DEST:%.+]] = alloca i32, align 4 + PrintiesCopy ps(Printy("a"), + // CHECK: call void @_ZN6PrintyC1EPKc + ({ + if (foo()) return; + // CHECK: if.then: + // CHECK-NEXT: store i32 1, ptr [[CLEANUP_DEST]], align 4 + // CHECK-NEXT: br label %cleanup + Printy("b"); + // CHECK: if.end: + // CHECK-NEXT: call void @_ZN6PrintyC1EPKc + }), + ({ + if (foo()) return; + // CHECK: if.then{{.*}}: + // CHECK-NEXT: store i32 1, ptr [[CLEANUP_DEST]], align 4 + // CHECK-NEXT: call void @_ZN6PrintyD1Ev + // CHECK-NEXT: br label %cleanup + Printy("c"); + // CHECK: if.end{{.*}}: + // CHECK-NEXT: call void @_ZN6PrintyC1EPKc + // CHECK-NEXT: call void @_ZN12PrintiesCopyD1Ev + // CHECK-NEXT: br label %return + })); + // CHECK: cleanup: + // CHECK-NEXT: call void @_ZN6PrintyD1Ev + // CHECK-NEXT: br label %return +} + +coroutine ParenInitCoro() { + // CHECK: define dso_local void @_Z13ParenInitCorov + // CHECK: [[ACTIVE1:%.+]] = alloca i1, align 1 + // CHECK: [[ACTIVE2:%.+]] = alloca i1, align 1 + PrintiesCopy ps(Printy("a"), Printy("b"), + // CHECK: call void @_ZN6PrintyC1EPKc + // CHECK-NEXT: store i1 true, ptr [[ACTIVE1]].reload.addr, align 1 + // CHECK-NEXT: store i1 true, ptr [[ACTIVE2]].reload.addr, align 1 + // CHECK: call void @_ZN6PrintyC1EPKc + co_await Awaiter{} + + // CHECK: await.cleanup: + // CHECK-NEXT: br label %[[CLEANUP:.+]].from.await.cleanup + + // CHECK: [[CLEANUP]].from.await.cleanup: + // CHECK: br label %[[CLEANUP]] + + // CHECK: await.ready: + // CHECK: store i1 false, ptr [[ACTIVE1]].reload.addr, align 1 + // CHECK-NEXT: store i1 false, ptr [[ACTIVE2]].reload.addr, align 1 + // CHECK-NEXT: br label %[[CLEANUP]].from.await.ready + + // CHECK: [[CLEANUP]].from.await.ready: + // CHECK: br label %[[CLEANUP]] + + // CHECK: [[CLEANUP]]: + // CHECK: [[IS_ACTIVE1:%.+]] = load i1, ptr [[ACTIVE1]].reload.addr, align 1 + // CHECK-NEXT: br i1 [[IS_ACTIVE1]], label %[[ACTION1:.+]], label %[[DONE1:.+]] + + // CHECK: [[ACTION1]]: + // CHECK: call void @_ZN6PrintyD1Ev + // CHECK: br label %[[DONE1]] + + // CHECK: [[DONE1]]: + // CHECK: [[IS_ACTIVE2:%.+]] = load i1, ptr [[ACTIVE2]].reload.addr, align 1 + // CHECK-NEXT: br i1 [[IS_ACTIVE2]], label %[[ACTION2:.+]], label %[[DONE2:.+]] + + // CHECK: [[ACTION2]]: + // CHECK: call void @_ZN6PrintyD1Ev + // CHECK: br label %[[DONE2]] + ); +} + +// TODO: Add more assertions after preliminary review. +// struct S { +// Printy arr1[2]; +// Printy arr2[2]; +// Printy p; +// }; + +// void ArraySubobjects() { +// S s{{Printy("a"), Printy("b")}, +// {Printy("a"), ({ +// if (foo() == 1) { +// return; +// } +// Printy("b"); +// })}, +// ({ +// if (foo() == 2) { +// return; +// } +// Printy("b"); +// })}; +// } + +// coroutine ArraySubobjectsCoro() { +// S s{{Printy("a"), Printy("b")}, +// {Printy("a"), co_await Awaiter{}}, +// co_await Awaiter{}}; +// } + +// struct A { +// Printy a; +// }; +// struct B : A { +// Printy b; +// Printy c; +// }; + +// void BaseClassCtors() { +// auto S = B({Printy("a")}, Printy("b"), ({ +// return; +// Printy("c"); +// })); +// } + +// coroutine BaseClassCtorsCoro() { +// auto S = B({Printy("a")}, Printy("b"), co_await Awaiter{}); +// } + +// void LambdaInit() { +// auto S = [a = Printy("a"), b = ({ +// return; +// Printy("b"); +// })]() { return a; }; +// } + +// coroutine LambdaInitCoro() { +// auto S = [a = Printy("a"), b = co_await Awaiter{}]() { return a; }; +// } >From b5c5fe685df999668890aab0209427bea6eca2f3 Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena <u...@google.com> Date: Mon, 18 Mar 2024 12:23:57 +0000 Subject: [PATCH 3/3] Handle array init cleanups and lifetime extended dtors --- clang/include/clang/AST/Expr.h | 2 ++ clang/lib/AST/Expr.cpp | 17 +++++++++++++++++ clang/lib/CodeGen/CGCleanup.cpp | 8 ++++++++ clang/lib/CodeGen/CGDecl.cpp | 22 ++++++++++++++-------- clang/lib/CodeGen/CGExprAgg.cpp | 3 ++- clang/lib/CodeGen/CGExprCXX.cpp | 2 +- clang/lib/CodeGen/CodeGenFunction.h | 12 +++++++++++- 7 files changed, 55 insertions(+), 11 deletions(-) diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h index 446bec4081e869..26d0d589cd2550 100644 --- a/clang/include/clang/AST/Expr.h +++ b/clang/include/clang/AST/Expr.h @@ -246,6 +246,8 @@ class Expr : public ValueStmt { return static_cast<bool>(getDependence() & ExprDependence::Error); } + bool containsControlFlow() const; + /// getExprLoc - Return the preferred location for the arrow when diagnosing /// a problem with a generic expression. SourceLocation getExprLoc() const LLVM_READONLY; diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp index b4de2155adcebd..40cf06847528b8 100644 --- a/clang/lib/AST/Expr.cpp +++ b/clang/lib/AST/Expr.cpp @@ -24,6 +24,7 @@ #include "clang/AST/IgnoreExpr.h" #include "clang/AST/Mangle.h" #include "clang/AST/RecordLayout.h" +#include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/StmtVisitor.h" #include "clang/Basic/Builtins.h" #include "clang/Basic/CharInfo.h" @@ -2503,6 +2504,22 @@ Stmt *BlockExpr::getBody() { // Generic Expression Routines //===----------------------------------------------------------------------===// +bool Expr::containsControlFlow() const { + struct BranchDetector : public RecursiveASTVisitor<BranchDetector> { + bool HasBranch = false; + bool activate() { + HasBranch = true; + return false; + } + bool VisitCoawaitExpr(CoawaitExpr *) { return activate(); } + bool VisitCoyieldExpr(CoyieldExpr *) { return activate(); } + bool VisitStmtExpr(StmtExpr *) { return activate(); } + }; + BranchDetector detector; + detector.TraverseStmt(const_cast<Expr *>(this)); + return detector.HasBranch; +} + bool Expr::isReadIfDiscardedInCPlusPlus11() const { // In C++11, discarded-value expressions of a certain form are special, // according to [expr]p10: diff --git a/clang/lib/CodeGen/CGCleanup.cpp b/clang/lib/CodeGen/CGCleanup.cpp index c22d4da0fefc3c..5e51f263930848 100644 --- a/clang/lib/CodeGen/CGCleanup.cpp +++ b/clang/lib/CodeGen/CGCleanup.cpp @@ -492,7 +492,15 @@ void CodeGenFunction::PopCleanupBlocks( /// cleanups from the given savepoint in the lifetime-extended cleanups stack. void CodeGenFunction::PopCleanupBlocks( EHScopeStack::stable_iterator Old, size_t OldLifetimeExtendedSize, + size_t OldDeactivateAfterFullExprStackSize, std::initializer_list<llvm::Value **> ValuesToReload) { + for (size_t I = DeactivateAfterFullExprStack.size(); + I > OldDeactivateAfterFullExprStackSize; I--) { + DeactivateCleanupBlock(DeactivateAfterFullExprStack[I - 1].Cleanup, + DeactivateAfterFullExprStack[I - 1].DominatingIP); + DeactivateAfterFullExprStack[I - 1].DominatingIP->eraseFromParent(); + } + DeactivateAfterFullExprStack.resize(OldDeactivateAfterFullExprStackSize); PopCleanupBlocks(Old, ValuesToReload); // Move our deferred cleanups onto the EH stack. diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp index bbe14ef4c17244..24760727d966aa 100644 --- a/clang/lib/CodeGen/CGDecl.cpp +++ b/clang/lib/CodeGen/CGDecl.cpp @@ -19,6 +19,7 @@ #include "CodeGenFunction.h" #include "CodeGenModule.h" #include "ConstantEmitter.h" +#include "EHScopeStack.h" #include "PatternInit.h" #include "TargetInfo.h" #include "clang/AST/ASTContext.h" @@ -2204,10 +2205,16 @@ void CodeGenFunction::pushLifetimeExtendedDestroy(CleanupKind cleanupKind, // Push an EH-only cleanup for the object now. // FIXME: When popping normal cleanups, we need to keep this EH cleanup // around in case a temporary's destructor throws an exception. - if (cleanupKind & EHCleanup) - EHStack.pushCleanup<DestroyObject>( - static_cast<CleanupKind>(cleanupKind & ~NormalCleanup), addr, type, - destroyer, useEHCleanupForArray); + if (cleanupKind) { + // Placeholder dominating IP for this cleanup. + llvm::Instruction *CleanupDominator = Builder.CreateAlignedLoad( + Int8Ty, llvm::Constant::getNullValue(Int8PtrTy), CharUnits::One()); + EHStack.pushCleanup<DestroyObject>(static_cast<CleanupKind>(cleanupKind), + addr, type, destroyer, + useEHCleanupForArray); + DeactivateAfterFullExprStack.push_back( + {EHStack.stable_begin(), CleanupDominator}); + } return pushCleanupAfterFullExprWithActiveFlag<DestroyObject>( cleanupKind, Address::invalid(), addr, type, destroyer, useEHCleanupForArray); @@ -2437,10 +2444,9 @@ void CodeGenFunction::pushIrregularPartialArrayCleanup(llvm::Value *arrayBegin, QualType elementType, CharUnits elementAlign, Destroyer *destroyer) { - pushFullExprCleanup<IrregularPartialArrayDestroy>(EHCleanup, - arrayBegin, arrayEndPointer, - elementType, elementAlign, - destroyer); + pushFullExprCleanup<IrregularPartialArrayDestroy>( + NormalAndEHCleanup, arrayBegin, arrayEndPointer, elementType, + elementAlign, destroyer); } /// pushRegularPartialArrayCleanup - Push an EH cleanup to destroy diff --git a/clang/lib/CodeGen/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp index 7e62599089b8df..8b502596a215c2 100644 --- a/clang/lib/CodeGen/CGExprAgg.cpp +++ b/clang/lib/CodeGen/CGExprAgg.cpp @@ -558,7 +558,8 @@ void AggExprEmitter::EmitArrayInit(Address DestPtr, llvm::ArrayType *AType, Address endOfInit = Address::invalid(); EHScopeStack::stable_iterator cleanup; llvm::Instruction *cleanupDominator = nullptr; - if (CGF.needsEHCleanup(dtorKind)) { + if (CGF.needsEHCleanup(dtorKind) || + (dtorKind && ExprToVisit->containsControlFlow())) { // In principle we could tell the cleanup where we are more // directly, but the control flow can get so varied here that it // would actually be quite complex. Therefore we go through an diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp index 2adbef6d55122c..fd413dcfc236c6 100644 --- a/clang/lib/CodeGen/CGExprCXX.cpp +++ b/clang/lib/CodeGen/CGExprCXX.cpp @@ -1103,7 +1103,7 @@ void CodeGenFunction::EmitNewArrayInitializer( } // Enter a partial-destruction Cleanup if necessary. - if (needsEHCleanup(DtorKind)) { + if (needsEHCleanup(DtorKind) || (DtorKind && E->containsControlFlow())) { // In principle we could tell the Cleanup where we are more // directly, but the control flow can get so varied here that it // would actually be quite complex. Therefore we go through an diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h index 6c825a302913df..8f2d09ba017738 100644 --- a/clang/lib/CodeGen/CodeGenFunction.h +++ b/clang/lib/CodeGen/CodeGenFunction.h @@ -648,6 +648,12 @@ class CodeGenFunction : public CodeGenTypeCache { EHScopeStack EHStack; llvm::SmallVector<char, 256> LifetimeExtendedCleanupStack; + struct DeactivateAfterFullExprCleanup { + EHScopeStack::stable_iterator Cleanup; + llvm::Instruction *DominatingIP; + }; + llvm::SmallVector<DeactivateAfterFullExprCleanup> + DeactivateAfterFullExprStack; llvm::SmallVector<const JumpDest *, 2> SEHTryEpilogueStack; llvm::Instruction *CurrentFuncletPad = nullptr; @@ -904,6 +910,7 @@ class CodeGenFunction : public CodeGenTypeCache { class RunCleanupsScope { EHScopeStack::stable_iterator CleanupStackDepth, OldCleanupScopeDepth; size_t LifetimeExtendedCleanupStackSize; + size_t DeactivateAfterFullExprStackSize; bool OldDidCallStackSave; protected: bool PerformCleanup; @@ -923,6 +930,8 @@ class CodeGenFunction : public CodeGenTypeCache { CleanupStackDepth = CGF.EHStack.stable_begin(); LifetimeExtendedCleanupStackSize = CGF.LifetimeExtendedCleanupStack.size(); + DeactivateAfterFullExprStackSize = + CGF.DeactivateAfterFullExprStack.size(); OldDidCallStackSave = CGF.DidCallStackSave; CGF.DidCallStackSave = false; OldCleanupScopeDepth = CGF.CurrentCleanupScopeDepth; @@ -950,7 +959,7 @@ class CodeGenFunction : public CodeGenTypeCache { assert(PerformCleanup && "Already forced cleanup"); CGF.DidCallStackSave = OldDidCallStackSave; CGF.PopCleanupBlocks(CleanupStackDepth, LifetimeExtendedCleanupStackSize, - ValuesToReload); + DeactivateAfterFullExprStackSize, ValuesToReload); PerformCleanup = false; CGF.CurrentCleanupScopeDepth = OldCleanupScopeDepth; } @@ -1172,6 +1181,7 @@ class CodeGenFunction : public CodeGenTypeCache { void PopCleanupBlocks(EHScopeStack::stable_iterator OldCleanupStackSize, size_t OldLifetimeExtendedStackSize, + size_t OldDeactivateAfterFullExprStackSize, std::initializer_list<llvm::Value **> ValuesToReload = {}); void ResolveBranchFixups(llvm::BasicBlock *Target); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits