[clang-tools-extra] [clang] [llvm] [codegen] Emit cleanups for lifetime-extended temporaries when an expr contains control-flow (PR #80698)
Utkarsh Saxena via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 6 06:27:56 PST 2024
https://github.com/usx95 updated https://github.com/llvm/llvm-project/pull/80698
>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/4] [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/4] [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:
+}
>From 41258086773829db7f17afa9df192d6ca56b2bfa Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Mon, 5 Feb 2024 16:52:25 +0000
Subject: [PATCH 3/4] format
---
clang/lib/CodeGen/CGCleanup.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang/lib/CodeGen/CGCleanup.cpp b/clang/lib/CodeGen/CGCleanup.cpp
index da0528b271aa3..39c65c0b07c6f 100644
--- a/clang/lib/CodeGen/CGCleanup.cpp
+++ b/clang/lib/CodeGen/CGCleanup.cpp
@@ -489,7 +489,8 @@ void CodeGenFunction::PopCleanupBlocks(
}
/// Adds deferred lifetime-extended cleanups onto the EH stack.
-void CodeGenFunction::AddLifetimeExtendedCleanups(size_t OldLifetimeExtendedSize) {
+void CodeGenFunction::AddLifetimeExtendedCleanups(
+ size_t OldLifetimeExtendedSize) {
for (size_t I = OldLifetimeExtendedSize,
E = LifetimeExtendedCleanupStack.size();
I != E;) {
>From 35437116444d9ac2827634456a449c19394434d7 Mon Sep 17 00:00:00 2001
From: Utkarsh Saxena <usx at google.com>
Date: Tue, 6 Feb 2024 14:27:40 +0000
Subject: [PATCH 4/4] Emit not-all but only necessary lifetime-extended
cleanups for each branch
---
clang/lib/CodeGen/CGCleanup.cpp | 3 +-
clang/lib/CodeGen/CGStmt.cpp | 2 +-
clang/lib/CodeGen/CodeGenFunction.h | 17 ++++-
.../control-flow-in-stmt-expr-cleanup.cpp | 68 +++++++++++++++++++
.../return-in-stmt-expr-cleanup.cpp | 37 ----------
5 files changed, 85 insertions(+), 42 deletions(-)
create mode 100644 clang/test/CodeGenCXX/control-flow-in-stmt-expr-cleanup.cpp
delete mode 100644 clang/test/CodeGenCXX/return-in-stmt-expr-cleanup.cpp
diff --git a/clang/lib/CodeGen/CGCleanup.cpp b/clang/lib/CodeGen/CGCleanup.cpp
index 39c65c0b07c6f..a5a49917d108f 100644
--- a/clang/lib/CodeGen/CGCleanup.cpp
+++ b/clang/lib/CodeGen/CGCleanup.cpp
@@ -1114,7 +1114,8 @@ void CodeGenFunction::EmitBranchThroughCleanup(JumpDest Dest) {
// EHStack. Do not remove them from lifetime-extended stack, they need to be
// emitted again after the expression completes.
RunCleanupsScope LifetimeExtendedCleanups(*this);
- AddLifetimeExtendedCleanups(0);
+ if(Dest.isValid())
+ AddLifetimeExtendedCleanups(Dest.getLifetimeExtendedDepth());
// Create the branch.
llvm::BranchInst *BI = Builder.CreateBr(Dest.getBlock());
diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index beff0ad9da270..8d7e2616c168c 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -628,7 +628,7 @@ CodeGenFunction::getJumpDestForLabel(const LabelDecl *D) {
// Create, but don't insert, the new block.
Dest = JumpDest(createBasicBlock(D->getName()),
- EHScopeStack::stable_iterator::invalid(),
+ EHScopeStack::stable_iterator::invalid(), 0,
NextCleanupDestIndex++);
return Dest;
}
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index ac55e84034bc6..78bc45dca2936 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -238,15 +238,20 @@ class CodeGenFunction : public CodeGenTypeCache {
/// A jump destination is an abstract label, branching to which may
/// require a jump out through normal cleanups.
struct JumpDest {
- JumpDest() : Block(nullptr), Index(0) {}
+ JumpDest() : Block(nullptr), LifetimeExtendedDepth(0), Index(0) {}
JumpDest(llvm::BasicBlock *Block, EHScopeStack::stable_iterator Depth,
- unsigned Index)
- : Block(Block), ScopeDepth(Depth), Index(Index) {}
+ unsigned LifetimeExtendedScopeDepth, unsigned Index)
+ : Block(Block), ScopeDepth(Depth),
+ LifetimeExtendedDepth(LifetimeExtendedScopeDepth), Index(Index) {
+ }
bool isValid() const { return Block != nullptr; }
llvm::BasicBlock *getBlock() const { return Block; }
EHScopeStack::stable_iterator getScopeDepth() const { return ScopeDepth; }
unsigned getDestIndex() const { return Index; }
+ unsigned getLifetimeExtendedDepth() const {
+ return LifetimeExtendedDepth;
+ }
// This should be used cautiously.
void setScopeDepth(EHScopeStack::stable_iterator depth) {
@@ -256,6 +261,11 @@ class CodeGenFunction : public CodeGenTypeCache {
private:
llvm::BasicBlock *Block;
EHScopeStack::stable_iterator ScopeDepth;
+
+ // Size of the lifetime-extended cleanup stack in destination scope.
+ // This can only occur when nested stmt-expr's contains branches.
+ // This is useful to emit only the necessary lifeitme-extended cleanups.
+ unsigned LifetimeExtendedDepth;
unsigned Index;
};
@@ -1163,6 +1173,7 @@ class CodeGenFunction : public CodeGenTypeCache {
JumpDest getJumpDestInCurrentScope(llvm::BasicBlock *Target) {
return JumpDest(Target,
EHStack.getInnermostNormalCleanup(),
+ LifetimeExtendedCleanupStack.size(),
NextCleanupDestIndex++);
}
diff --git a/clang/test/CodeGenCXX/control-flow-in-stmt-expr-cleanup.cpp b/clang/test/CodeGenCXX/control-flow-in-stmt-expr-cleanup.cpp
new file mode 100644
index 0000000000000..a53b0fcf04092
--- /dev/null
+++ b/clang/test/CodeGenCXX/control-flow-in-stmt-expr-cleanup.cpp
@@ -0,0 +1,68 @@
+// 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() {
+// CHECK: define dso_local void @_Z3barv()
+// CHECK-NOT: call void @_ZN6PrintyD1Ev
+ Printies p2{
+ Printy(),
+ ({
+ if(foo()) {
+ // CHECK: if.then:
+ // CHECK-NEXT: call void @_ZN6PrintyD1Ev
+ // CHECK-NEXT: br label %return
+ // CHECK-NOT: call void @_ZN6PrintyD1Ev
+ return;
+ }
+ Printy();
+ })};
+ // CHECK: if.end:
+ // CHECK: call void @_ZN8PrintiesD1Ev
+ // CHECK: call void @_ZN6PrintyD1Ev
+ // CHECK: call void @_ZN6PrintyD1Ev
+ // CHECK-NEXT: br label %return
+ return;
+ // CHECK-NOT: call void @_ZN6PrintyD1Ev
+}
+
+
+void test_break() {
+// CHECK: define dso_local void @_Z10test_breakv()
+// CHECK-NOT: call void @_ZN6PrintyD1Ev
+ Printies p2{Printy(), ({
+ for (;;) {
+ Printies p3{Printy(), ({
+ if(foo()) {
+ break;
+ // CHECK: if.then:
+ // CHECK-NEXT: call void @_ZN6PrintyD1Ev
+ // CHECK-NEXT: br label %for.end
+ // CHECK-NOT: call void @_ZN6PrintyD1Ev
+ }
+ Printy();
+ })};
+ // CHECK: if.end:
+ // CHECK: call void @_ZN6PrintyD1Ev
+ // CHECK: call void @_ZN6PrintyD1Ev
+ // CHECK-NOT: call void @_ZN6PrintyD1Ev
+ }
+ Printy();
+ })};
+ // CHECK: for.end:
+ // CHECK-COUNT-2: call void @_ZN6PrintyD1Ev
+ // CHECK-NEXT: ret void
+ // CHECK-NOT: call void @_ZN6PrintyD1Ev
+}
+
diff --git a/clang/test/CodeGenCXX/return-in-stmt-expr-cleanup.cpp b/clang/test/CodeGenCXX/return-in-stmt-expr-cleanup.cpp
deleted file mode 100644
index 214becd81e61a..0000000000000
--- a/clang/test/CodeGenCXX/return-in-stmt-expr-cleanup.cpp
+++ /dev/null
@@ -1,37 +0,0 @@
-// 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;
-}
-
More information about the cfe-commits
mailing list