[clang] 5a46123 - Fix missing dtor in function calls accepting trivial ABI structs (#88751)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 16 02:01:07 PDT 2024
Author: Utkarsh Saxena
Date: 2024-04-16T11:01:03+02:00
New Revision: 5a46123ddf62900d3dc73330f699c73038645198
URL: https://github.com/llvm/llvm-project/commit/5a46123ddf62900d3dc73330f699c73038645198
DIFF: https://github.com/llvm/llvm-project/commit/5a46123ddf62900d3dc73330f699c73038645198.diff
LOG: Fix missing dtor in function calls accepting trivial ABI structs (#88751)
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.
We missed emitting CPP scope ends because we remove the `fallthrough`
(clears the insertion point) before deactivating normal cleanups. This
is to make the emitted "normal" cleanup code unreachable. But we still
need to emit CPP scope ends in the original basic block even for a
deactivated normal cleanup.
(This worked correctly before we did not remove `fallthrough` for
`EHCleanup`s).
Added:
Modified:
clang/lib/CodeGen/CGCall.cpp
clang/lib/CodeGen/CGCleanup.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/test/CodeGenCXX/control-flow-in-stmt-expr.cpp
Removed:
################################################################################
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 95deee8bb1f1f2..0a51b0e4121c33 100644
--- a/clang/test/CodeGenCXX/control-flow-in-stmt-expr.cpp
+++ b/clang/test/CodeGenCXX/control-flow-in-stmt-expr.cpp
@@ -391,3 +391,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;
+ }));
+}
More information about the cfe-commits
mailing list