[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
usx95 wrote: [ayermolo](https://github.com/ayermolo) Thanks for the reproducer. I was able to reproduce it. Sent out fix https://github.com/llvm/llvm-project/pull/88670 https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
ayermolo wrote: @usx95 can you repro? Also is there ETA on a fix, and if not can you revert this? https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
ayermolo wrote: I think this commit is causing build failure when building clangd in debug mode with clang built in release mode. ``` Instruction does not dominate all uses! %K1104 = getelementptr inbounds %"struct.llvm::json::Object::KV", ptr %arrayinit.begin1100, i32 0, i32 0, !dbg !93928 call void @_ZN4llvm4json9ObjectKeyD2Ev(ptr noundef nonnull align 8 dereferenceable(24) %K1104) #21, !dbg !93937 fatal error: error in backend: Broken module found, compilation aborted! PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script. Stack dump: 0. Program arguments: /home/ayermolo/local/llvm-build-upstream-release/bin/clang++ -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/tools/extra/clangd -I/home/ayermolo/local/upstream-llvm/llvm-project/clang-tools-extra/clangd -I/home/ayermolo/local/upstream-llvm/llvm-project/clang-tools-extra/clangd/../include-cleaner/include -Itools/clang/tools/extra/clangd/../clang-tidy -I/home/ayermolo/local/upstream-llvm/llvm-project/clang/include -Itools/clang/include -Iinclude -I/home/ayermolo/local/upstream-llvm/llvm-project/llvm/include -I/home/ayermolo/local/upstream-llvm/llvm-project/clang-tools-extra/pseudo/lib/../include -isystem/home/ayermolo/local/llvm-build-upstream-llvm/build/lib/clang/19/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -fno-common -Woverloaded-virtual -Wno-nested-anon-types -g -std=c++17 -MD -MT /home/ayermolo/local/llvm-build-upstream-debug/tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ClangdLSPServer.cpp.o -MF /home/ayermolo/local/llvm-build-upstream-debug/tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ClangdLSPServer.cpp.o.d -o /home/ayermolo/local/llvm-build-upstream-debug/tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ClangdLSPServer.cpp.o -c /home/ayermolo/local/upstream-llvm/llvm-project/clang-tools-extra/clangd/ClangdLSPServer.cpp ``` Dropping this commit and running above clang build command makes it pass. build command for easier reading: ``` COMP=/home/ayermolo/local/llvm-build-upstream-release/bin $COMP/clang++ -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS \ -Itools/clang/tools/extra/clangd \ -I/home/ayermolo/local/upstream-llvm/llvm-project/clang-tools-extra/clangd \ -I/home/ayermolo/local/upstream-llvm/llvm-project/clang-tools-extra/clangd/../include-cleaner/include \ -Itools/clang/tools/extra/clangd/../clang-tidy \ -I/home/ayermolo/local/upstream-llvm/llvm-project/clang/include \ -Itools/clang/include \ -Iinclude \ -I/home/ayermolo/local/upstream-llvm/llvm-project/llvm/include \ -I/home/ayermolo/local/upstream-llvm/llvm-project/clang-tools-extra/pseudo/lib/../include \ -isystem/home/ayermolo/local/llvm-build-upstream-llvm/build/lib/clang/19/include \ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra \ -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi \ -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override \ -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -fno-common -Woverloaded-virtual \ -Wno-nested-anon-types -g -std=c++17 -MD \ -MT /home/ayermolo/local/llvm-build-upstream-debug/tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ClangdLSPServer.cpp.o \ -MF /home/ayermolo/local/llvm-build-upstream-debug/tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ClangdLSPServer.cpp.o.d \ -o /home/ayermolo/local/llvm-build-upstream-debug/tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ClangdLSPServer.cpp.o \ -c /home/ayermolo/local/upstream-llvm/llvm-project/clang-tools-extra/clangd/ClangdLSPServer.cpp ``` https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
https://github.com/usx95 closed https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
https://github.com/efriedma-quic approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
https://github.com/usx95 edited https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
@@ -1105,19 +1105,24 @@ void CodeGenFunction::EmitNewArrayInitializer(
}
// Enter a partial-destruction Cleanup if necessary.
-if (needsEHCleanup(DtorKind)) {
+if (DtorKind) {
+ AllocaTrackerRAII AllocaTracker(*this);
usx95 wrote:
Unfortunately, that is not the case. We do create more allocas in conditional
branches in `pushFullExprCleanup`, for eg: create `cond-cleanup.save` allocas
in `DominatingLLVMValue::save`
([src](https://github.com/llvm/llvm-project/blob/e93b5f5a4776ffea12d03652559dfdf8d421184c/clang/lib/CodeGen/CodeGenFunction.h#L5184)).
https://github.com/llvm/llvm-project/pull/85398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
@@ -266,6 +269,54 @@ class alignas(8) EHCleanupScope : public EHScope {
};
mutable struct ExtInfo *ExtInfo;
+ /// Erases auxillary allocas and their usages for an unused cleanup.
+ /// Cleanups should mark these allocas as 'used' if the cleanup is
+ /// emitted, otherwise these instructions would be erased.
+ struct AuxillaryAllocas {
+SmallVector AuxAllocas;
+bool used = false;
+
+// Records a potentially unused instruction to be erased later.
+void Add(llvm::AllocaInst *Alloca) { AuxAllocas.push_back(Alloca); }
+
+// Mark all recorded instructions as used. These will not be erased later.
+void MarkUsed() {
+ used = true;
+ AuxAllocas.clear();
+}
+
+~AuxillaryAllocas() {
+ if (used)
+return;
+ llvm::SetVector Uses;
+ for (auto *Inst : llvm::reverse(AuxAllocas))
+CollectUses(Inst, Uses);
+ // Delete uses in the reverse order of insertion.
+ for (auto *I : llvm::reverse(Uses))
+I->eraseFromParent();
+}
+
+ private:
+void CollectUses(llvm::Instruction *I,
+ llvm::SetVector ) {
+ if (!I || !Uses.insert(I))
+return;
+ for (auto *User : I->materialized_users()) {
usx95 wrote:
Done.
https://github.com/llvm/llvm-project/pull/85398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
@@ -266,6 +269,54 @@ class alignas(8) EHCleanupScope : public EHScope {
};
mutable struct ExtInfo *ExtInfo;
+ /// Erases auxillary allocas and their usages for an unused cleanup.
+ /// Cleanups should mark these allocas as 'used' if the cleanup is
+ /// emitted, otherwise these instructions would be erased.
+ struct AuxillaryAllocas {
+SmallVector AuxAllocas;
+bool used = false;
+
+// Records a potentially unused instruction to be erased later.
+void Add(llvm::AllocaInst *Alloca) { AuxAllocas.push_back(Alloca); }
+
+// Mark all recorded instructions as used. These will not be erased later.
+void MarkUsed() {
+ used = true;
+ AuxAllocas.clear();
+}
+
+~AuxillaryAllocas() {
+ if (used)
+return;
+ llvm::SetVector Uses;
+ for (auto *Inst : llvm::reverse(AuxAllocas))
+CollectUses(Inst, Uses);
+ // Delete uses in the reverse order of insertion.
+ for (auto *I : llvm::reverse(Uses))
+I->eraseFromParent();
+}
+
+ private:
+void CollectUses(llvm::Instruction *I,
+ llvm::SetVector ) {
+ if (!I || !Uses.insert(I))
+return;
+ for (auto *User : I->materialized_users()) {
efriedma-quic wrote:
Just "users()" (materialization isn't relevant here), and you can just `cast<>`
to Instruction (all users of an Instruction are Instructions).
https://github.com/llvm/llvm-project/pull/85398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
@@ -1105,19 +1105,24 @@ void CodeGenFunction::EmitNewArrayInitializer(
}
// Enter a partial-destruction Cleanup if necessary.
-if (needsEHCleanup(DtorKind)) {
+if (DtorKind) {
+ AllocaTrackerRAII AllocaTracker(*this);
efriedma-quic wrote:
"AllocaTracker" seems like overkill; CreateTempAlloca has an out parameter to
access the created alloca, and I don't think you're creating any other allocas.
https://github.com/llvm/llvm-project/pull/85398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
https://github.com/usx95 edited https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
usx95 wrote: Thanks @efriedma-quic . I was not aware that we have such edges in Codegen which could help in deleting these instructions transitively. Switched to capturing only auxillary `allocas` and deleting all transitive uses. PTAL. https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
@@ -2503,6 +2506,29 @@ Stmt *BlockExpr::getBody() {
// Generic Expression Routines
//===--===//
+bool Expr::mayBranchOut() const {
usx95 wrote:
This is now no more needed. We switched to deleting instructions related to
unemitted cleanups.
https://github.com/llvm/llvm-project/pull/85398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
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
Date: Fri, 15 Mar 2024 13:19:36 +
Subject: [PATCH 1/8] [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
Date: Fri, 15 Mar 2024 15:22:07 +
Subject: [PATCH 2/8] 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 00..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 handle;
+ ~coroutine() {
+if (handle) handle.destroy();
+ }
+};
+
+struct coroutine::promise_type {
+ coroutine get_return_object() {
+return {std::coroutine_handle::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 :
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
efriedma-quic wrote: > > Instead of Expr::mayBranchOut, I'd prefer to just unconditionally create > > the alloca, then delete it later if it turns out we didn't actually need to > > emit the branch. > > I had earlier tried tracking instructions auxiliary to a particular cleanup > in #83224 > ([src](https://github.com/llvm/llvm-project/pull/83224/files#diff-9cdaea6a793ed2892bfcd6b431e933a49ebb25caa2bd1d630cd1ca823281092aR263-R286)). > This gets ugly very quickly and adds quite some complexity to the cleanup > addition and emission code. For example, more instructions could be added if > the cleanup is conditional. You could simplify that code significantly, I think: instead of adding every instruction to the list, just add the alloca, then recursively delete all instructions that use the alloca. > As far as compile-time is concerned, this involves revisiting the complete > expression exactly one more time. You can end up visiting more than once if an array init contains another array init. https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
usx95 wrote: > Instead of Expr::mayBranchOut, I'd prefer to just unconditionally create the > alloca, then delete it later if it turns out we didn't actually need to emit > the branch. I had earlier tried tracking instructions auxiliary to a particular cleanup in #83224 ([src](https://github.com/llvm/llvm-project/pull/83224/files#diff-9cdaea6a793ed2892bfcd6b431e933a49ebb25caa2bd1d630cd1ca823281092aR263-R286)). This gets ugly very quickly and adds quite some complexity to the cleanup addition and emission code. For example, more instructions could be added if the cleanup is conditional. > Trying to explicitly compute whether there's a branch out seems both > difficult, and potentially costly for compile-time. Computing this 100% accurately is indeed difficult. But if we allow false-positives, as in the current approach, it gets clearer and simpler. As far as compile-time is concerned, this involves revisiting the complete expression exactly one more time. We could, in principle, cache this per expression to keep this linear. https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
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
Date: Fri, 15 Mar 2024 13:19:36 +
Subject: [PATCH 1/7] [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
Date: Fri, 15 Mar 2024 15:22:07 +
Subject: [PATCH 2/7] 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 00..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 handle;
+ ~coroutine() {
+if (handle) handle.destroy();
+ }
+};
+
+struct coroutine::promise_type {
+ coroutine get_return_object() {
+return {std::coroutine_handle::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 :
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
efriedma-quic wrote: If we do keep mayBranchOut(), "asm goto" should also be added to the list of expressions that can branch out. (I think JumpDiagnostics currently forbids actually branching out in cases that would require a cleanup, but better to be defensive here.) https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
efriedma-quic wrote: Instead of Expr::mayBranchOut, I'd prefer to just unconditionally create the alloca, then delete it later if it turns out we didn't actually need to emit the branch. Trying to explicitly compute whether there's a branch out seems both difficult, and potentially costly for compile-time. I like the unified approach here for eh and non-eh cleanups. https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
@@ -2503,6 +2506,29 @@ Stmt *BlockExpr::getBody() {
// Generic Expression Routines
//===--===//
+bool Expr::mayBranchOut() const {
zygoloid wrote:
Computing this seems a little expensive in general. I wonder if we could track
a bit on `FunctionDecl` that indicates whether it contains any branches out of
statement expressions, and skip calling this if the enclosing function is not a
coroutine and does not contain branches out of statement expressions.
https://github.com/llvm/llvm-project/pull/85398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
usx95 wrote: > Perhaps we want this test to live in CodeGen rather than CodeGenCoroutines > given that it covers so many non-coroutine test cases as well? Makes sense. I split the tests into coroutine and stmt-expr (didn't want to depend on "Inputs/coroutine.h" from CodegenCXX). https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
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
Date: Fri, 15 Mar 2024 13:19:36 +
Subject: [PATCH 1/6] [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
Date: Fri, 15 Mar 2024 15:22:07 +
Subject: [PATCH 2/6] 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 00..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 handle;
+ ~coroutine() {
+if (handle) handle.destroy();
+ }
+};
+
+struct coroutine::promise_type {
+ coroutine get_return_object() {
+return {std::coroutine_handle::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 :
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
https://github.com/usx95 edited https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
https://github.com/AaronBallman commented: I didn't look too hard at the codegen bits, but the Expr bits look good to me. Perhaps we want this test to live in CodeGen rather than CodeGenCoroutines given that it covers so many non-coroutine test cases as well? https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
github-actions[bot] wrote: :white_check_mark: With the latest revision this PR passed the Python code formatter. https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
github-actions[bot] wrote: :white_check_mark: With the latest revision this PR passed the C/C++ code formatter. https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
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
Date: Fri, 15 Mar 2024 13:19:36 +
Subject: [PATCH 1/5] [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
Date: Fri, 15 Mar 2024 15:22:07 +
Subject: [PATCH 2/5] 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 00..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 handle;
+ ~coroutine() {
+if (handle) handle.destroy();
+ }
+};
+
+struct coroutine::promise_type {
+ coroutine get_return_object() {
+return {std::coroutine_handle::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 :
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
https://github.com/usx95 edited https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
@@ -2503,6 +2505,26 @@ Stmt *BlockExpr::getBody() {
// Generic Expression Routines
//===--===//
+bool Expr::mayBranchOut() const {
+ struct BranchDetector : public RecursiveASTVisitor {
+bool HasBranch = false;
+bool activate() {
+ HasBranch = true;
+ return false;
+}
+// Coroutine suspensions.
+bool VisitCoawaitExpr(CoawaitExpr *) { return activate(); }
+bool VisitCoyieldExpr(CoyieldExpr *) { return activate(); }
usx95 wrote:
Done. Added tests.
https://github.com/llvm/llvm-project/pull/85398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
@@ -2503,6 +2505,26 @@ Stmt *BlockExpr::getBody() {
// Generic Expression Routines
//===--===//
+bool Expr::mayBranchOut() const {
+ struct BranchDetector : public RecursiveASTVisitor {
+bool HasBranch = false;
+bool activate() {
+ HasBranch = true;
+ return false;
+}
+// Coroutine suspensions.
+bool VisitCoawaitExpr(CoawaitExpr *) { return activate(); }
+bool VisitCoyieldExpr(CoyieldExpr *) { return activate(); }
+// Control flow in stmt-expressions.
+bool VisitBreakStmt(BreakStmt *) { return activate(); }
usx95 wrote:
Yes. Added a test case for it as well.
https://github.com/llvm/llvm-project/pull/85398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
@@ -2503,6 +2505,26 @@ Stmt *BlockExpr::getBody() {
// Generic Expression Routines
//===--===//
+bool Expr::mayBranchOut() const {
+ struct BranchDetector : public RecursiveASTVisitor {
+bool HasBranch = false;
+bool activate() {
+ HasBranch = true;
+ return false;
+}
+// Coroutine suspensions.
+bool VisitCoawaitExpr(CoawaitExpr *) { return activate(); }
+bool VisitCoyieldExpr(CoyieldExpr *) { return activate(); }
+// Control flow in stmt-expressions.
+bool VisitBreakStmt(BreakStmt *) { return activate(); }
+bool VisitReturnStmt(ReturnStmt *) { return activate(); }
+bool VisitGotoStmt(GotoStmt *) { return activate(); }
+ };
usx95 wrote:
I think it should be fine to stick to non-exceptional control flow out of this
expression. `throw` or, for that matter, any call to a throwing function would
be branching but that is already dealt with exceptions.
https://github.com/llvm/llvm-project/pull/85398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
@@ -2503,6 +2505,26 @@ Stmt *BlockExpr::getBody() {
// Generic Expression Routines
//===--===//
+bool Expr::mayBranchOut() const {
+ struct BranchDetector : public RecursiveASTVisitor {
+bool HasBranch = false;
+bool activate() {
+ HasBranch = true;
+ return false;
+}
+// Coroutine suspensions.
+bool VisitCoawaitExpr(CoawaitExpr *) { return activate(); }
+bool VisitCoyieldExpr(CoyieldExpr *) { return activate(); }
+// Control flow in stmt-expressions.
+bool VisitBreakStmt(BreakStmt *) { return activate(); }
+bool VisitReturnStmt(ReturnStmt *) { return activate(); }
+bool VisitGotoStmt(GotoStmt *) { return activate(); }
usx95 wrote:
Added.
https://github.com/llvm/llvm-project/pull/85398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
@@ -2503,6 +2505,26 @@ Stmt *BlockExpr::getBody() {
// Generic Expression Routines
//===--===//
+bool Expr::mayBranchOut() const {
+ struct BranchDetector : public RecursiveASTVisitor {
+bool HasBranch = false;
+bool activate() {
+ HasBranch = true;
+ return false;
+}
+// Coroutine suspensions.
+bool VisitCoawaitExpr(CoawaitExpr *) { return activate(); }
+bool VisitCoyieldExpr(CoyieldExpr *) { return activate(); }
AaronBallman wrote:
Should this also handle `co_return` statements?
https://github.com/llvm/llvm-project/pull/85398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
@@ -2503,6 +2505,26 @@ Stmt *BlockExpr::getBody() {
// Generic Expression Routines
//===--===//
+bool Expr::mayBranchOut() const {
+ struct BranchDetector : public RecursiveASTVisitor {
+bool HasBranch = false;
+bool activate() {
+ HasBranch = true;
+ return false;
+}
+// Coroutine suspensions.
+bool VisitCoawaitExpr(CoawaitExpr *) { return activate(); }
+bool VisitCoyieldExpr(CoyieldExpr *) { return activate(); }
+// Control flow in stmt-expressions.
+bool VisitBreakStmt(BreakStmt *) { return activate(); }
+bool VisitReturnStmt(ReturnStmt *) { return activate(); }
+bool VisitGotoStmt(GotoStmt *) { return activate(); }
AaronBallman wrote:
Should this also handle indirect goto statements?
https://github.com/llvm/llvm-project/pull/85398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
@@ -2503,6 +2505,26 @@ Stmt *BlockExpr::getBody() {
// Generic Expression Routines
//===--===//
+bool Expr::mayBranchOut() const {
+ struct BranchDetector : public RecursiveASTVisitor {
+bool HasBranch = false;
+bool activate() {
+ HasBranch = true;
+ return false;
+}
+// Coroutine suspensions.
+bool VisitCoawaitExpr(CoawaitExpr *) { return activate(); }
+bool VisitCoyieldExpr(CoyieldExpr *) { return activate(); }
+// Control flow in stmt-expressions.
+bool VisitBreakStmt(BreakStmt *) { return activate(); }
AaronBallman wrote:
Should this also handle continue statements?
https://github.com/llvm/llvm-project/pull/85398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
@@ -2503,6 +2505,26 @@ Stmt *BlockExpr::getBody() {
// Generic Expression Routines
//===--===//
+bool Expr::mayBranchOut() const {
+ struct BranchDetector : public RecursiveASTVisitor {
+bool HasBranch = false;
+bool activate() {
+ HasBranch = true;
+ return false;
+}
+// Coroutine suspensions.
+bool VisitCoawaitExpr(CoawaitExpr *) { return activate(); }
+bool VisitCoyieldExpr(CoyieldExpr *) { return activate(); }
+// Control flow in stmt-expressions.
+bool VisitBreakStmt(BreakStmt *) { return activate(); }
+bool VisitReturnStmt(ReturnStmt *) { return activate(); }
+bool VisitGotoStmt(GotoStmt *) { return activate(); }
+ };
AaronBallman wrote:
Should we consider `throw` to be a kind of branching control flow as well?
https://github.com/llvm/llvm-project/pull/85398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
https://github.com/AaronBallman edited https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
https://github.com/AaronBallman commented: 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. https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
Sirraide wrote: 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. https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
usx95 wrote: Ping for review: @AaronBallman @jyknight @efriedma-quic @zygoloid Could you please suggest other reviewers if you do not have the bandwidth for the same? https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
https://github.com/usx95 edited https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
https://github.com/usx95 edited https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
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
Date: Fri, 15 Mar 2024 13:19:36 +
Subject: [PATCH 1/4] [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
Date: Fri, 15 Mar 2024 15:22:07 +
Subject: [PATCH 2/4] 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 00..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 handle;
+ ~coroutine() {
+if (handle) handle.destroy();
+ }
+};
+
+struct coroutine::promise_type {
+ coroutine get_return_object() {
+return {std::coroutine_handle::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 :
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
usx95 wrote: Friendly ping for review: @jyknight @zygoloid @efriedma-quic https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
https://github.com/usx95 edited https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [codegen] Emit missing cleanups for stmt-expr and coro suspensions [take-2] (PR #85398)
https://github.com/usx95 edited https://github.com/llvm/llvm-project/pull/85398 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
