[codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] #85398
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
usx95
changed the title
llvmbot
added
clang
|
@llvm/pr-subscribers-coroutines @llvm/pr-subscribers-clang-codegen Author: Utkarsh Saxena (usx95) ChangesPartially addresses #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. Examples of such deferred cleanups include:
In this PR, we change some of the 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 TODO: Add more tests here. I will change some more Full diff: https://github.com/llvm/llvm-project/pull/85398.diff 3 Files Affected:
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; };
+// }
|
|
@llvm/pr-subscribers-clang Author: Utkarsh Saxena (usx95) ChangesPartially addresses #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. Examples of such deferred cleanups include:
In this PR, we change some of the 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 TODO: Add more tests here. I will change some more Full diff: https://github.com/llvm/llvm-project/pull/85398.diff 3 Files Affected:
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; };
+// }
|
llvmbot
added
the
clang:frontend
…ferred deactivations
usx95
changed the title
|
Friendly ping for review: @jyknight @zygoloid @efriedma-quic |
|
Ping for review: @AaronBallman @jyknight @efriedma-quic @zygoloid |
|
I’m the wrong person to ping if it’s anything codegen-related; I’ll leave reviewing this to people more familiar w/ that part of Clang. |
There was a problem hiding this comment.
Hopefully @rjmccall or @efriedma-quic can take a look at the codegen bits. I did have some questions about other kinds of flow control statements we might want to cover.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
usx95
removed request for
rafaelauler,
ayermolo,
dcci,
JDevlieghere,
tbaederr,
Endilll,
nikic and
a team
|
I think this commit is causing build failure when building clangd in debug mode with clang built in release mode. Dropping this commit and running above clang build command makes it pass. |
|
@usx95 can you repro? |
usx95
added a commit
to usx95/llvm-project
that referenced
this pull request
Apr 16, 2024
…sions [take-2] (llvm#85398)" This reverts commit 89ba7e1.
usx95
added a commit
that referenced
this pull request
Apr 29, 2024
) Latest diff: https://github.com/llvm/llvm-project/pull/89154/files/f1ab4c2677394bbfc985d9680d5eecd7b2e6a882..adf9bc902baddb156c83ce0f8ec03c142e806d45 We address two additional bugs here: ### Problem 1: Deactivated normal cleanup still runs, leading to double-free Consider the following: ```cpp struct A { }; struct B { B(const A&); }; struct S { A a; B b; }; int AcceptS(S s); void Accept2(int x, int y); void Test() { Accept2(AcceptS({.a = A{}, .b = A{}}), ({ return; 0; })); } ``` We add cleanups as follows: 1. push dtor for field `S::a` 2. push dtor for temp `A{}` (used by ` B(const A&)` in `.b = A{}`) 3. push dtor for field `S::b` 4. Deactivate 3 `S::b`-> This pops the cleanup. 5. Deactivate 1 `S::a` -> Does not pop the cleanup as *2* is top. Should create _active flag_!! 6. push dtor for `~S()`. 7. ... It is important to deactivate **5** using active flags. Without the active flags, the `return` will fallthrough it and would run both `~S()` and dtor `S::a` leading to **double free** of `~A()`. In this patch, we unconditionally emit active flags while deactivating normal cleanups. These flags are deleted later by the `AllocaTracker` if the cleanup is not emitted. ### Problem 2: Missing cleanup for conditional lifetime extension We push 2 cleanups for lifetime-extended cleanup. The first cleanup is useful if we exit from the middle of the expression (stmt-expr/coro suspensions). This is deactivated after full-expr, and a new cleanup is pushed, extending the lifetime of the temporaries (to the scope of the reference being initialized). If this lifetime extension happens to be conditional, then we use active flags to remember whether the branch was taken and if the object was initialized. Previously, we used a **single** active flag, which was used by both cleanups. This is wrong because the first cleanup will be forced to deactivate after the full-expr and therefore this **active** flag will always be **inactive**. The dtor for the lifetime extended entity would not run as it always sees an **inactive** flag. In this patch, we solve this using two separate active flags for both cleanups. Both of them are activated if the conditional branch is taken, but only one of them is deactivated after the full-expr. --- Fixes #63818 Fixes #88478 --- Previous PR logs: 1. #85398 2. #88670 3. #88751 4. #88884
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #63818 for control flow out of an expressions.
Background
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
EHCleanupduring 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 the lambda capture list is destroyed by the lambda object.In this PR
In this PR, we change some of the
EHCleanupscleanups toNormalAndEHCleanupsto make sure these are emitted when we see a branch inside an expression (through statement expressions or coroutine suspensions).These are supposed to be deactivated after full expression and destroyed later as part of the destructor of the aggregate or array being constructed. To simplify deactivating cleanups, we add two utilities as well:
DeferredDeactivationCleanupStack: A stack to remember cleanups with deferred deactivation.CleanupDeactivationScope: RAII for deactivating cleanups added to the above stack.Deactivating normal cleanups
These were previously
EHCleanupsand notNormaland deactivation of requiredNormalcleanups had some bugs. These specifically include deactivatingNormalcleanups which are not the top ofEHStacksource1, 2. This has not been part of our test suite (maybe it was never required before statement expressions). In this PR, we also fix the emission of required-deactivated-normal cleanups.Test ideas
I have tried out printf-style ctor-dtor calls in the following cases manually and verified that the correct dtors are invoked: https://godbolt.org/z/jMYTK7Kxe
I would be open to ideas for better tests than just lit-test for LLVM IR. We might benefit from some integration tests which can run the program and assert