[clang] Fix missing dtor in function calls accepting trivial ABI structs (PR #88751)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 15 08:31:26 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-codegen
Author: Utkarsh Saxena (usx95)
<details>
<summary>Changes</summary>
Fixes https://github.com/llvm/llvm-project/issues/88478
Promoting the `EHCleanup` to `NormalAndEHCleanup` in `EmitCallArgs` surfaced another bug with deactivation of normal cleanups. Here we missed emitting CPP scope ends for deactivated normal cleanups. This patch also fixes that bug.
---
Full diff: https://github.com/llvm/llvm-project/pull/88751.diff
4 Files Affected:
- (modified) clang/lib/CodeGen/CGCall.cpp (+7-6)
- (modified) clang/lib/CodeGen/CGCleanup.cpp (+23-14)
- (modified) clang/lib/CodeGen/CodeGenFunction.h (+2-1)
- (modified) clang/test/CodeGenCXX/control-flow-in-stmt-expr.cpp (+16)
``````````diff
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 7a0bc6fa77b889..0c860a3ccbd2f0 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -4694,11 +4694,11 @@ void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E,
AggValueSlot Slot = args.isUsingInAlloca()
? createPlaceholderSlot(*this, type) : CreateAggTemp(type, "agg.tmp");
- bool DestroyedInCallee = true, NeedsEHCleanup = true;
+ bool DestroyedInCallee = true, NeedsCleanup = true;
if (const auto *RD = type->getAsCXXRecordDecl())
DestroyedInCallee = RD->hasNonTrivialDestructor();
else
- NeedsEHCleanup = needsEHCleanup(type.isDestructedType());
+ NeedsCleanup = type.isDestructedType();
if (DestroyedInCallee)
Slot.setExternallyDestructed();
@@ -4707,14 +4707,15 @@ void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E,
RValue RV = Slot.asRValue();
args.add(RV, type);
- if (DestroyedInCallee && NeedsEHCleanup) {
+ if (DestroyedInCallee && NeedsCleanup) {
// Create a no-op GEP between the placeholder and the cleanup so we can
// RAUW it successfully. It also serves as a marker of the first
// instruction where the cleanup is active.
- pushFullExprCleanup<DestroyUnpassedArg>(EHCleanup, Slot.getAddress(),
- type);
+ pushFullExprCleanup<DestroyUnpassedArg>(NormalAndEHCleanup,
+ Slot.getAddress(), type);
// This unreachable is a temporary marker which will be removed later.
- llvm::Instruction *IsActive = Builder.CreateUnreachable();
+ llvm::Instruction *IsActive =
+ Builder.CreateFlagLoad(llvm::Constant::getNullValue(Int8PtrTy));
args.addArgCleanupDeactivation(EHStack.stable_begin(), IsActive);
}
return;
diff --git a/clang/lib/CodeGen/CGCleanup.cpp b/clang/lib/CodeGen/CGCleanup.cpp
index 5bf48bc22a5495..8683f19d9da28e 100644
--- a/clang/lib/CodeGen/CGCleanup.cpp
+++ b/clang/lib/CodeGen/CGCleanup.cpp
@@ -634,12 +634,19 @@ static void destroyOptimisticNormalEntry(CodeGenFunction &CGF,
/// Pops a cleanup block. If the block includes a normal cleanup, the
/// current insertion point is threaded through the cleanup, as are
/// any branch fixups on the cleanup.
-void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough) {
+void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough,
+ bool ForDeactivation) {
assert(!EHStack.empty() && "cleanup stack is empty!");
assert(isa<EHCleanupScope>(*EHStack.begin()) && "top not a cleanup!");
EHCleanupScope &Scope = cast<EHCleanupScope>(*EHStack.begin());
assert(Scope.getFixupDepth() <= EHStack.getNumBranchFixups());
+ // If we are deactivating a normal cleanup, we need to pretend that the
+ // fallthrough is unreachable. We restore this IP before returning.
+ CGBuilderTy::InsertPoint NormalDeactivateOrigIP;
+ if (ForDeactivation && (Scope.isNormalCleanup() || !getLangOpts().EHAsynch)) {
+ NormalDeactivateOrigIP = Builder.saveAndClearIP();
+ }
// Remember activation information.
bool IsActive = Scope.isActive();
Address NormalActiveFlag =
@@ -729,6 +736,8 @@ void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough) {
EHStack.popCleanup(); // safe because there are no fixups
assert(EHStack.getNumBranchFixups() == 0 ||
EHStack.hasNormalCleanups());
+ if (NormalDeactivateOrigIP.isSet())
+ Builder.restoreIP(NormalDeactivateOrigIP);
return;
}
@@ -765,9 +774,16 @@ void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough) {
if (!RequiresNormalCleanup) {
// Mark CPP scope end for passed-by-value Arg temp
// per Windows ABI which is "normally" Cleanup in callee
- if (IsEHa && getInvokeDest() && Builder.GetInsertBlock()) {
- if (Personality.isMSVCXXPersonality())
+ if (IsEHa && getInvokeDest()) {
+ // If we are deactivating a normal cleanup then we don't have a
+ // fallthrough. Restore original IP to emit CPP scope ends in the correct
+ // block.
+ if (NormalDeactivateOrigIP.isSet())
+ Builder.restoreIP(NormalDeactivateOrigIP);
+ if (Personality.isMSVCXXPersonality() && Builder.GetInsertBlock())
EmitSehCppScopeEnd();
+ if (NormalDeactivateOrigIP.isSet())
+ NormalDeactivateOrigIP = Builder.saveAndClearIP();
}
destroyOptimisticNormalEntry(*this, Scope);
Scope.MarkEmitted();
@@ -992,6 +1008,8 @@ void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough) {
}
}
+ if (NormalDeactivateOrigIP.isSet())
+ Builder.restoreIP(NormalDeactivateOrigIP);
assert(EHStack.hasNormalCleanups() || EHStack.getNumBranchFixups() == 0);
// Emit the EH cleanup if required.
@@ -1281,17 +1299,8 @@ void CodeGenFunction::DeactivateCleanupBlock(EHScopeStack::stable_iterator C,
// to the current RunCleanupsScope.
if (C == EHStack.stable_begin() &&
CurrentCleanupScopeDepth.strictlyEncloses(C)) {
- // Per comment below, checking EHAsynch is not really necessary
- // it's there to assure zero-impact w/o EHAsynch option
- if (!Scope.isNormalCleanup() && getLangOpts().EHAsynch) {
- PopCleanupBlock();
- } else {
- // If it's a normal cleanup, we need to pretend that the
- // fallthrough is unreachable.
- CGBuilderTy::InsertPoint SavedIP = Builder.saveAndClearIP();
- PopCleanupBlock();
- Builder.restoreIP(SavedIP);
- }
+ PopCleanupBlock(/*FallthroughIsBranchThrough=*/false,
+ /*ForDeactivation=*/true);
return;
}
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index c49e9fd00c8d3e..d99188671f1f60 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -957,7 +957,8 @@ class CodeGenFunction : public CodeGenTypeCache {
/// PopCleanupBlock - Will pop the cleanup entry on the stack and
/// process all branch fixups.
- void PopCleanupBlock(bool FallThroughIsBranchThrough = false);
+ void PopCleanupBlock(bool FallThroughIsBranchThrough = false,
+ bool ForDeactivation = false);
/// DeactivateCleanupBlock - Deactivates the given cleanup block.
/// The block cannot be reactivated. Pops it if it's the top of the
diff --git a/clang/test/CodeGenCXX/control-flow-in-stmt-expr.cpp b/clang/test/CodeGenCXX/control-flow-in-stmt-expr.cpp
index ffde1bd6a724d8..e2ff4f395d9b2a 100644
--- a/clang/test/CodeGenCXX/control-flow-in-stmt-expr.cpp
+++ b/clang/test/CodeGenCXX/control-flow-in-stmt-expr.cpp
@@ -362,3 +362,19 @@ void ArrayInitWithContinue() {
})};
}
}
+
+struct [[clang::trivial_abi]] HasTrivialABI {
+ HasTrivialABI();
+ ~HasTrivialABI();
+};
+void AcceptTrivialABI(HasTrivialABI, int);
+void TrivialABI() {
+ // CHECK-LABEL: define dso_local void @_Z10TrivialABIv()
+ AcceptTrivialABI(HasTrivialABI(), ({
+ if (foo()) return;
+ // CHECK: if.then:
+ // CHECK-NEXT: call void @_ZN13HasTrivialABID1Ev
+ // CHECK-NEXT: br label %return
+ 0;
+ }));
+}
``````````
</details>
https://github.com/llvm/llvm-project/pull/88751
More information about the cfe-commits
mailing list