[clang] [clang-tools-extra] [llvm] [codegen] Emit cleanups for lifetime-extended temporaries when stmt-expr has control-flow (PR #80698)
Utkarsh Saxena via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 5 08:03:19 PST 2024
https://github.com/usx95 created https://github.com/llvm/llvm-project/pull/80698
Partially solves https://github.com/llvm/llvm-project/issues/63818 for control flow in braced aggregate initialisation.
The reason why we miss cleanups for temporaries in braced aggregate init is that the lifetime of these temporaries is extended, and their cleanup is scheduled (added to EHStack) after the full expression.
When such expressions have control flow (through `return` or `co_await`), we only emit the `EHStack` cleanups which does not contain the `LifetimeExtendedCleanupStack` cleanups.
In this PR, we forcefully emit `LifetimeExtendedCleanupStack` if they are present while emitting a branch.
(This still does not work for control flow in array-like list initialization expressions.)
>From 28d90711ff8e4924135d4bd4e5f252d96ac41b93 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 23 Jan 2024 20:18:25 +0000
Subject: [PATCH 1/2] [misc-coroutine-hostile-raii] Use getOperand instead of
getCommonExpr
---
.../misc/CoroutineHostileRAIICheck.cpp | 2 +-
.../checkers/misc/coroutine-hostile-raii.cpp | 34 ++++++++++++++++++-
2 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
index a0e8700b0522b..360335b86c641 100644
--- a/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/CoroutineHostileRAIICheck.cpp
@@ -56,7 +56,7 @@ AST_MATCHER_P(Stmt, forEachPrevStmt, ast_matchers::internal::Matcher<Stmt>,
// Matches the expression awaited by the `co_await`.
AST_MATCHER_P(CoawaitExpr, awaitable, ast_matchers::internal::Matcher<Expr>,
InnerMatcher) {
- if (Expr *E = Node.getCommonExpr())
+ if (Expr *E = Node.getOperand())
return InnerMatcher.matches(*E, Finder, Builder);
return false;
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
index 55a7e4b8f2954..c23c355dac1b2 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/coroutine-hostile-raii.cpp
@@ -1,7 +1,7 @@
// RUN: %check_clang_tidy -std=c++20 %s misc-coroutine-hostile-raii %t \
// RUN: -config="{CheckOptions: {\
// RUN: misc-coroutine-hostile-raii.RAIITypesList: 'my::Mutex; ::my::other::Mutex', \
-// RUN: misc-coroutine-hostile-raii.AllowedAwaitablesList: 'safe::awaitable; ::my::other::awaitable' \
+// RUN: misc-coroutine-hostile-raii.AllowedAwaitablesList: 'safe::awaitable; ::transformable::awaitable' \
// RUN: }}"
namespace std {
@@ -136,6 +136,9 @@ ReturnObject scopedLockableTest() {
absl::Mutex no_warning_5;
}
+// ================================================================================
+// Safe awaitable
+// ================================================================================
namespace safe {
struct awaitable {
bool await_ready() noexcept { return false; }
@@ -150,6 +153,32 @@ ReturnObject RAIISafeSuspendTest() {
co_await other{};
}
+// ================================================================================
+// Safe transformable awaitable
+// ================================================================================
+struct transformable { struct awaitable{}; };
+using alias_transformable_awaitable = transformable::awaitable;
+struct UseTransformAwaitable {
+ struct promise_type {
+ UseTransformAwaitable get_return_object() { return {}; }
+ std::suspend_always initial_suspend() { return {}; }
+ std::suspend_always final_suspend() noexcept { return {}; }
+ void unhandled_exception() {}
+ std::suspend_always await_transform(transformable::awaitable) { return {}; }
+ };
+};
+
+auto retAwaitable() { return transformable::awaitable{}; }
+UseTransformAwaitable RAIISafeSuspendTest2() {
+ absl::Mutex a;
+ co_await retAwaitable();
+ co_await transformable::awaitable{};
+ co_await alias_transformable_awaitable{};
+}
+
+// ================================================================================
+// Lambdas
+// ================================================================================
void lambda() {
absl::Mutex no_warning;
auto lambda = []() -> ReturnObject {
@@ -164,6 +193,9 @@ void lambda() {
absl::Mutex no_warning_2;
}
+// ================================================================================
+// Denylisted RAII
+// ================================================================================
template<class T>
ReturnObject raii_in_template(){
T a;
>From 442809ce6047f8f83c9f3f54b17182c18535bb03 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Mon, 5 Feb 2024 15:52:56 +0000
Subject: [PATCH 2/2] [codegen] Emit cleanups for lifetime-extended temporaries
when statment-expression has control-flow
---
clang/lib/CodeGen/CGCleanup.cpp | 31 +++++--
clang/lib/CodeGen/CodeGenFunction.h | 4 +
.../return-in-stmt-expr-cleanup.cpp | 37 +++++++++
.../coro-suspend-in-agg-init.cpp | 82 +++++++++++++++++++
4 files changed, 145 insertions(+), 9 deletions(-)
create mode 100644 clang/test/CodeGenCXX/return-in-stmt-expr-cleanup.cpp
create mode 100644 clang/test/CodeGenCoroutines/coro-suspend-in-agg-init.cpp
diff --git a/clang/lib/CodeGen/CGCleanup.cpp b/clang/lib/CodeGen/CGCleanup.cpp
index f87caf050eeaa..da0528b271aa3 100644
--- a/clang/lib/CodeGen/CGCleanup.cpp
+++ b/clang/lib/CodeGen/CGCleanup.cpp
@@ -488,16 +488,11 @@ void CodeGenFunction::PopCleanupBlocks(
}
}
-/// Pops cleanup blocks until the given savepoint is reached, then add the
-/// cleanups from the given savepoint in the lifetime-extended cleanups stack.
-void CodeGenFunction::PopCleanupBlocks(
- EHScopeStack::stable_iterator Old, size_t OldLifetimeExtendedSize,
- std::initializer_list<llvm::Value **> ValuesToReload) {
- PopCleanupBlocks(Old, ValuesToReload);
-
- // Move our deferred cleanups onto the EH stack.
+/// Adds deferred lifetime-extended cleanups onto the EH stack.
+void CodeGenFunction::AddLifetimeExtendedCleanups(size_t OldLifetimeExtendedSize) {
for (size_t I = OldLifetimeExtendedSize,
- E = LifetimeExtendedCleanupStack.size(); I != E; /**/) {
+ E = LifetimeExtendedCleanupStack.size();
+ I != E;) {
// Alignment should be guaranteed by the vptrs in the individual cleanups.
assert((I % alignof(LifetimeExtendedCleanupHeader) == 0) &&
"misaligned cleanup stack entry");
@@ -519,6 +514,17 @@ void CodeGenFunction::PopCleanupBlocks(
I += sizeof(ActiveFlag);
}
}
+}
+
+/// Pops cleanup blocks until the given savepoint is reached, then add the
+/// cleanups from the given savepoint in the lifetime-extended cleanups stack.
+void CodeGenFunction::PopCleanupBlocks(
+ EHScopeStack::stable_iterator Old, size_t OldLifetimeExtendedSize,
+ std::initializer_list<llvm::Value **> ValuesToReload) {
+ PopCleanupBlocks(Old, ValuesToReload);
+
+ // Move our deferred cleanups onto the EH stack.
+ AddLifetimeExtendedCleanups(OldLifetimeExtendedSize);
LifetimeExtendedCleanupStack.resize(OldLifetimeExtendedSize);
}
@@ -1102,6 +1108,13 @@ void CodeGenFunction::EmitBranchThroughCleanup(JumpDest Dest) {
if (!HaveInsertPoint())
return;
+ // If we have lifetime-extended (LE) cleanups, then we must be emitting a
+ // branch within an expression. Emit all the LE cleanups by adding them to the
+ // EHStack. Do not remove them from lifetime-extended stack, they need to be
+ // emitted again after the expression completes.
+ RunCleanupsScope LifetimeExtendedCleanups(*this);
+ AddLifetimeExtendedCleanups(0);
+
// Create the branch.
llvm::BranchInst *BI = Builder.CreateBr(Dest.getBlock());
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 143ad64e8816b..ac55e84034bc6 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -1143,6 +1143,10 @@ class CodeGenFunction : public CodeGenTypeCache {
PopCleanupBlocks(EHScopeStack::stable_iterator OldCleanupStackSize,
std::initializer_list<llvm::Value **> ValuesToReload = {});
+ /// Adds lifetime-extended cleanups from the given position to the stack.
+ /// (does not remove the cleanups from lifetime extended stack).
+ void AddLifetimeExtendedCleanups(size_t OldLifetimeExtendedSize);
+
/// Takes the old cleanup stack size and emits the cleanup blocks
/// that have been added, then adds all lifetime-extended cleanups from
/// the given position to the stack.
diff --git a/clang/test/CodeGenCXX/return-in-stmt-expr-cleanup.cpp b/clang/test/CodeGenCXX/return-in-stmt-expr-cleanup.cpp
new file mode 100644
index 0000000000000..214becd81e61a
--- /dev/null
+++ b/clang/test/CodeGenCXX/return-in-stmt-expr-cleanup.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+// Context: GH63818
+struct Printy {
+ ~Printy() { }
+};
+
+struct Printies {
+ const Printy &a;
+ const Printy &b;
+ ~Printies() {}
+};
+
+bool foo();
+
+void bar() {
+ Printies p2{
+ // CHECK: store ptr %ref.tmp
+ Printy(),
+ ({
+ if(foo()) {
+ // CHECK-LABEL: if.then:
+ // CHECK-NEXT: call void @_ZN6PrintyD1Ev
+ // CHECK-NEXT: br label %return
+ return;
+ }
+ // CHECK-LABEL: if.end:
+ // CHECK-NEXT: store ptr %ref.tmp1
+ Printy();
+ })};
+ // CHECK-NEXT: call void @_ZN8PrintiesD1Ev
+ // CHECK-NEXT: call void @_ZN6PrintyD1Ev
+ // CHECK-NEXT: call void @_ZN6PrintyD1Ev
+ // CHECK-NEXT: br label %return
+ return;
+}
+
diff --git a/clang/test/CodeGenCoroutines/coro-suspend-in-agg-init.cpp b/clang/test/CodeGenCoroutines/coro-suspend-in-agg-init.cpp
new file mode 100644
index 0000000000000..362e212b7fba3
--- /dev/null
+++ b/clang/test/CodeGenCoroutines/coro-suspend-in-agg-init.cpp
@@ -0,0 +1,82 @@
+// RUN: %clang_cc1 --std=c++20 -triple x86_64-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+// Context: GH63818
+
+#include "Inputs/coroutine.h"
+
+struct coroutine {
+ struct promise_type;
+ std::coroutine_handle<promise_type> handle;
+};
+
+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 Printy { ~Printy(); };
+
+struct Printies {
+ const Printy &a;
+ const Printy &b;
+ const Printy &c;
+};
+
+struct Awaiter : std::suspend_always {
+ Printy await_resume() { return {}; }
+};
+
+// CHECK: define dso_local ptr @_Z5test1v()
+coroutine test1() {
+ // CHECK-NOT: @_ZN6PrintyD1Ev
+ Printies p1{
+ Printy(),
+ co_await Awaiter{},
+ // CHECK: await.cleanup:
+ // CHECK-NEXT: call void @_ZN6PrintyD1Ev
+ // CHECK-NEXT: br label %cleanup{{.*}}.from.await.cleanup
+ // CHECK-NOT: @_ZN6PrintyD1Ev
+
+ co_await Awaiter{}
+ // CHECK: await2.cleanup:
+ // CHECK-NEXT: call void @_ZN6PrintyD1Ev
+ // CHECK-NEXT: call void @_ZN6PrintyD1Ev
+ // CHECK-NEXT: br label %cleanup{{.*}}.from.await2.cleanup
+ // CHECK-NOT: @_ZN6PrintyD1Ev
+ };
+
+ // CHECK-COUNT-3: call void @_ZN6PrintyD1Ev
+ // CHECK-NEXT: br label
+
+ // CHECK-NOT: @_ZN6PrintyD1Ev
+
+ // CHECK: unreachable:
+}
+
+void bar(const Printy& a, const Printy& b);
+
+// CHECK: define dso_local ptr @_Z5test2v()
+coroutine test2() {
+ // CHECK-NOT: @_ZN6PrintyD1Ev
+ bar(
+ Printy(),
+ co_await Awaiter{}
+ // CHECK: await.cleanup:
+ // CHECK-NEXT: br label %cleanup{{.*}}.from.await.cleanup
+ // CHECK-NOT: @_ZN6PrintyD1Ev
+ );
+ // CHECK: await.ready:
+ // CHECK: call void @_ZN6PrintyD1Ev
+ // CHECK-NOT: @_ZN6PrintyD1Ev
+
+ // CHECK: cleanup{{.*}}:
+ // CHECK: call void @_ZN6PrintyD1Ev
+ // CHECK-NOT: @_ZN6PrintyD1Ev
+
+ // CHECK: unreachable:
+}
More information about the llvm-commits
mailing list