[clang] [codegen] Emit cleanups for branch in stmt-expr and coro suspensions [take-2] (PR #85398)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 15 08:36:36 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Utkarsh Saxena (usx95)
<details>
<summary>Changes</summary>
Partially addresses https://github.com/llvm/llvm-project/issues/63818 for control flow in expressions.
A control flow could happen in the middle of an expression due to stmt-expr and coroutine suspensions.
Due to branch-in-expr, we miss running cleanups for the temporaries constructed in the expression before the branch.
Previously these cleanups were only added as EHOnlyCleanup during the expression and as normal expression after the full expression.
Examples of such deferred cleanups include:
`ParenList/InitList`: Cleanups for fields are performed by the destructor of the object being constructed.
`Array init`: Cleanup for elements of an array is included in the array cleanup.
`Lifetime-extended temporaries`: reference-binding temporaries in braced-init are lifetime extended to the parent scope.
`Lambda capture init`: init in lambda capture list are destroyed by the lambda object.
In this PR, we change some of the `EHOnly` cleanups to `NormalAndEHCleanups` to make sure these are emitted when we see a branch inside an expression (through statement expressions or coroutine suspensions).
These are supposed deactivated after the full expression and destroyed later as part of the destructor of the aggregate /array being constructed.
Since these were previously `EHOnly` cleanups and not `Normal`, **deactivation** of **required** `Normal` but used cleanups had some bugs and was never tested (maybe it was never required before statement expressions). In this PR, we also fix the emission of required-deactivated-normal cleanups.
TODO: Add more tests here.
I will change some more `EHCleanups` to `Normal` in separate changes later. Array initialization and lifetime extension cleanups are intentionally left out as they need more attention but are very similar.
---
Full diff: https://github.com/llvm/llvm-project/pull/85398.diff
3 Files Affected:
- (modified) clang/lib/CodeGen/CGCleanup.cpp (+5-2)
- (modified) clang/lib/CodeGen/CGExprAgg.cpp (+7-6)
- (added) clang/test/CodeGenCoroutines/control-flow-in-expr-cleanup.cpp (+172)
``````````diff
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;
}
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; };
+// }
``````````
</details>
https://github.com/llvm/llvm-project/pull/85398
More information about the cfe-commits
mailing list