[clang] [codegen] Emit cleanups for branch in stmt-expr and coro suspensions [take-2] (PR #85398)
Utkarsh Saxena via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 18 05:24:14 PDT 2024
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 <usx at 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 <usx at 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 <usx at 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);
More information about the cfe-commits
mailing list