[clang] Revert "[codegen] Emit missing cleanups for stmt-expr and coro suspensions" and related commits (PR #88884)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 16 06:30:29 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-coroutines
Author: Utkarsh Saxena (usx95)
<details>
<summary>Changes</summary>
The original change caused widespread breakages in msan/ubsan tests and causes `use-after-free`. Most likely we are adding more cleanups than necessary.
---
Patch is 58.70 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/88884.diff
11 Files Affected:
- (modified) clang/lib/CodeGen/CGCall.cpp (+6-7)
- (modified) clang/lib/CodeGen/CGCleanup.cpp (+16-33)
- (modified) clang/lib/CodeGen/CGCleanup.h (+1-56)
- (modified) clang/lib/CodeGen/CGDecl.cpp (+19-42)
- (modified) clang/lib/CodeGen/CGExpr.cpp (+3-9)
- (modified) clang/lib/CodeGen/CGExprAgg.cpp (+61-26)
- (modified) clang/lib/CodeGen/CGExprCXX.cpp (+19-19)
- (modified) clang/lib/CodeGen/CodeGenFunction.cpp (-6)
- (modified) clang/lib/CodeGen/CodeGenFunction.h (+3-96)
- (removed) clang/test/CodeGenCXX/control-flow-in-stmt-expr.cpp (-409)
- (removed) clang/test/CodeGenCoroutines/coro-suspend-cleanups.cpp (-93)
``````````diff
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 0c860a3ccbd2f0..7a0bc6fa77b889 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, NeedsCleanup = true;
+ bool DestroyedInCallee = true, NeedsEHCleanup = true;
if (const auto *RD = type->getAsCXXRecordDecl())
DestroyedInCallee = RD->hasNonTrivialDestructor();
else
- NeedsCleanup = type.isDestructedType();
+ NeedsEHCleanup = needsEHCleanup(type.isDestructedType());
if (DestroyedInCallee)
Slot.setExternallyDestructed();
@@ -4707,15 +4707,14 @@ void CodeGenFunction::EmitCallArg(CallArgList &args, const Expr *E,
RValue RV = Slot.asRValue();
args.add(RV, type);
- if (DestroyedInCallee && NeedsCleanup) {
+ if (DestroyedInCallee && NeedsEHCleanup) {
// 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>(NormalAndEHCleanup,
- Slot.getAddress(), type);
+ pushFullExprCleanup<DestroyUnpassedArg>(EHCleanup, Slot.getAddress(),
+ type);
// This unreachable is a temporary marker which will be removed later.
- llvm::Instruction *IsActive =
- Builder.CreateFlagLoad(llvm::Constant::getNullValue(Int8PtrTy));
+ llvm::Instruction *IsActive = Builder.CreateUnreachable();
args.addArgCleanupDeactivation(EHStack.stable_begin(), IsActive);
}
return;
diff --git a/clang/lib/CodeGen/CGCleanup.cpp b/clang/lib/CodeGen/CGCleanup.cpp
index 8683f19d9da28e..e6f8e6873004f2 100644
--- a/clang/lib/CodeGen/CGCleanup.cpp
+++ b/clang/lib/CodeGen/CGCleanup.cpp
@@ -634,19 +634,12 @@ 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,
- bool ForDeactivation) {
+void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough) {
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 =
@@ -674,8 +667,7 @@ void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough,
// - whether there's a fallthrough
llvm::BasicBlock *FallthroughSource = Builder.GetInsertBlock();
- bool HasFallthrough =
- FallthroughSource != nullptr && (IsActive || HasExistingBranches);
+ bool HasFallthrough = (FallthroughSource != nullptr && IsActive);
// Branch-through fall-throughs leave the insertion point set to the
// end of the last cleanup, which points to the current scope. The
@@ -700,11 +692,7 @@ 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 &&
- !RequiresNormalCleanup) {
- // FIXME: Come up with a program which would need forwarding prebranched
- // fallthrough and add tests. Otherwise delete this and assert against it.
- assert(!IsActive);
+ if (Scope.isNormalCleanup() && HasPrebranchedFallthrough && !IsActive) {
llvm::BasicBlock *prebranchDest;
// If the prebranch is semantically branching through the next
@@ -736,8 +724,6 @@ 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;
}
@@ -774,19 +760,11 @@ 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()) {
- // 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())
+ if (IsEHa && getInvokeDest() && Builder.GetInsertBlock()) {
+ if (Personality.isMSVCXXPersonality())
EmitSehCppScopeEnd();
- if (NormalDeactivateOrigIP.isSet())
- NormalDeactivateOrigIP = Builder.saveAndClearIP();
}
destroyOptimisticNormalEntry(*this, Scope);
- Scope.MarkEmitted();
EHStack.popCleanup();
} else {
// If we have a fallthrough and no other need for the cleanup,
@@ -803,7 +781,6 @@ void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough,
}
destroyOptimisticNormalEntry(*this, Scope);
- Scope.MarkEmitted();
EHStack.popCleanup();
EmitCleanup(*this, Fn, cleanupFlags, NormalActiveFlag);
@@ -939,7 +916,6 @@ void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough,
}
// IV. Pop the cleanup and emit it.
- Scope.MarkEmitted();
EHStack.popCleanup();
assert(EHStack.hasNormalCleanups() == HasEnclosingCleanups);
@@ -1008,8 +984,6 @@ void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough,
}
}
- if (NormalDeactivateOrigIP.isSet())
- Builder.restoreIP(NormalDeactivateOrigIP);
assert(EHStack.hasNormalCleanups() || EHStack.getNumBranchFixups() == 0);
// Emit the EH cleanup if required.
@@ -1299,8 +1273,17 @@ void CodeGenFunction::DeactivateCleanupBlock(EHScopeStack::stable_iterator C,
// to the current RunCleanupsScope.
if (C == EHStack.stable_begin() &&
CurrentCleanupScopeDepth.strictlyEncloses(C)) {
- PopCleanupBlock(/*FallthroughIsBranchThrough=*/false,
- /*ForDeactivation=*/true);
+ // 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);
+ }
return;
}
diff --git a/clang/lib/CodeGen/CGCleanup.h b/clang/lib/CodeGen/CGCleanup.h
index c73c97146abc4d..03e4a29d7b3dbf 100644
--- a/clang/lib/CodeGen/CGCleanup.h
+++ b/clang/lib/CodeGen/CGCleanup.h
@@ -16,11 +16,8 @@
#include "EHScopeStack.h"
#include "Address.h"
-#include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVector.h"
-#include "llvm/IR/Instruction.h"
namespace llvm {
class BasicBlock;
@@ -269,51 +266,6 @@ 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<llvm::Instruction *, 1> 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<llvm::Instruction *> 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<llvm::Instruction *> &Uses) {
- if (!I || !Uses.insert(I))
- return;
- for (auto *User : I->users())
- CollectUses(cast<llvm::Instruction>(User), Uses);
- }
- };
- mutable struct AuxillaryAllocas *AuxAllocas;
-
- AuxillaryAllocas &getAuxillaryAllocas() {
- if (!AuxAllocas) {
- AuxAllocas = new struct AuxillaryAllocas();
- }
- return *AuxAllocas;
- }
-
/// The number of fixups required by enclosing scopes (not including
/// this one). If this is the top cleanup scope, all the fixups
/// from this index onwards belong to this scope.
@@ -346,7 +298,7 @@ class alignas(8) EHCleanupScope : public EHScope {
EHScopeStack::stable_iterator enclosingEH)
: EHScope(EHScope::Cleanup, enclosingEH),
EnclosingNormal(enclosingNormal), NormalBlock(nullptr),
- ActiveFlag(Address::invalid()), ExtInfo(nullptr), AuxAllocas(nullptr),
+ ActiveFlag(Address::invalid()), ExtInfo(nullptr),
FixupDepth(fixupDepth) {
CleanupBits.IsNormalCleanup = isNormal;
CleanupBits.IsEHCleanup = isEH;
@@ -360,15 +312,8 @@ class alignas(8) EHCleanupScope : public EHScope {
}
void Destroy() {
- if (AuxAllocas)
- delete AuxAllocas;
delete ExtInfo;
}
- void AddAuxAllocas(llvm::SmallVector<llvm::AllocaInst *> Allocas) {
- for (auto *Alloca : Allocas)
- getAuxillaryAllocas().Add(Alloca);
- }
- void MarkEmitted() { getAuxillaryAllocas().MarkUsed(); }
// Objects of EHCleanupScope are not destructed. Use Destroy().
~EHCleanupScope() = delete;
diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index 3f05ebb561da57..ce6d6d8956076e 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -19,7 +19,6 @@
#include "CodeGenFunction.h"
#include "CodeGenModule.h"
#include "ConstantEmitter.h"
-#include "EHScopeStack.h"
#include "PatternInit.h"
#include "TargetInfo.h"
#include "clang/AST/ASTContext.h"
@@ -2202,27 +2201,6 @@ void CodeGenFunction::pushDestroy(CleanupKind cleanupKind, Address addr,
destroyer, useEHCleanupForArray);
}
-// Pushes a destroy and defers its deactivation until its
-// CleanupDeactivationScope is exited.
-void CodeGenFunction::pushDestroyAndDeferDeactivation(
- QualType::DestructionKind dtorKind, Address addr, QualType type) {
- assert(dtorKind && "cannot push destructor for trivial type");
-
- CleanupKind cleanupKind = getCleanupKind(dtorKind);
- pushDestroyAndDeferDeactivation(
- cleanupKind, addr, type, getDestroyer(dtorKind), cleanupKind & EHCleanup);
-}
-
-void CodeGenFunction::pushDestroyAndDeferDeactivation(
- CleanupKind cleanupKind, Address addr, QualType type, Destroyer *destroyer,
- bool useEHCleanupForArray) {
- llvm::Instruction *DominatingIP =
- Builder.CreateFlagLoad(llvm::Constant::getNullValue(Int8PtrTy));
- pushDestroy(cleanupKind, addr, type, destroyer, useEHCleanupForArray);
- DeferredDeactivationCleanupStack.push_back(
- {EHStack.stable_begin(), DominatingIP});
-}
-
void CodeGenFunction::pushStackRestore(CleanupKind Kind, Address SPMem) {
EHStack.pushCleanup<CallStackRestore>(Kind, SPMem);
}
@@ -2239,19 +2217,16 @@ void CodeGenFunction::pushLifetimeExtendedDestroy(CleanupKind cleanupKind,
// If we're not in a conditional branch, we don't need to bother generating a
// conditional cleanup.
if (!isInConditionalBranch()) {
+ // Push an EH-only cleanup for the object now.
// FIXME: When popping normal cleanups, we need to keep this EH cleanup
// around in case a temporary's destructor throws an exception.
+ if (cleanupKind & EHCleanup)
+ EHStack.pushCleanup<DestroyObject>(
+ static_cast<CleanupKind>(cleanupKind & ~NormalCleanup), addr, type,
+ destroyer, useEHCleanupForArray);
- // Add the cleanup to the EHStack. After the full-expr, this would be
- // deactivated before being popped from the stack.
- pushDestroyAndDeferDeactivation(cleanupKind, addr, type, destroyer,
- useEHCleanupForArray);
-
- // Since this is lifetime-extended, push it once again to the EHStack after
- // the full expression.
return pushCleanupAfterFullExprWithActiveFlag<DestroyObject>(
- cleanupKind, Address::invalid(), addr, type, destroyer,
- useEHCleanupForArray);
+ cleanupKind, Address::invalid(), addr, type, destroyer, useEHCleanupForArray);
}
// Otherwise, we should only destroy the object if it's been initialized.
@@ -2266,12 +2241,13 @@ void CodeGenFunction::pushLifetimeExtendedDestroy(CleanupKind cleanupKind,
Address ActiveFlag = createCleanupActiveFlag();
SavedType SavedAddr = saveValueInCond(addr);
- pushCleanupAndDeferDeactivation<ConditionalCleanupType>(
- cleanupKind, SavedAddr, type, destroyer, useEHCleanupForArray);
- initFullExprCleanupWithFlag(ActiveFlag);
+ if (cleanupKind & EHCleanup) {
+ EHStack.pushCleanup<ConditionalCleanupType>(
+ static_cast<CleanupKind>(cleanupKind & ~NormalCleanup), SavedAddr, type,
+ destroyer, useEHCleanupForArray);
+ initFullExprCleanupWithFlag(ActiveFlag);
+ }
- // Since this is lifetime-extended, push it once again to the EHStack after
- // the full expression.
pushCleanupAfterFullExprWithActiveFlag<ConditionalCleanupType>(
cleanupKind, ActiveFlag, SavedAddr, type, destroyer,
useEHCleanupForArray);
@@ -2466,9 +2442,9 @@ namespace {
};
} // end anonymous namespace
-/// pushIrregularPartialArrayCleanup - Push a NormalAndEHCleanup to
-/// destroy already-constructed elements of the given array. The cleanup may be
-/// popped with DeactivateCleanupBlock or PopCleanupBlock.
+/// pushIrregularPartialArrayCleanup - Push an EH cleanup to destroy
+/// already-constructed elements of the given array. The cleanup
+/// may be popped with DeactivateCleanupBlock or PopCleanupBlock.
///
/// \param elementType - the immediate element type of the array;
/// possibly still an array type
@@ -2477,9 +2453,10 @@ void CodeGenFunction::pushIrregularPartialArrayCleanup(llvm::Value *arrayBegin,
QualType elementType,
CharUnits elementAlign,
Destroyer *destroyer) {
- pushFullExprCleanup<IrregularPartialArrayDestroy>(
- NormalAndEHCleanup, arrayBegin, arrayEndPointer, elementType,
- elementAlign, destroyer);
+ pushFullExprCleanup<IrregularPartialArrayDestroy>(EHCleanup,
+ arrayBegin, arrayEndPointer,
+ elementType, elementAlign,
+ destroyer);
}
/// pushRegularPartialArrayCleanup - Push an EH cleanup to destroy
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index c85a339f5e3f88..cf696a1c9f560f 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -115,16 +115,10 @@ RawAddress CodeGenFunction::CreateTempAlloca(llvm::Type *Ty, CharUnits Align,
llvm::AllocaInst *CodeGenFunction::CreateTempAlloca(llvm::Type *Ty,
const Twine &Name,
llvm::Value *ArraySize) {
- llvm::AllocaInst *Alloca;
if (ArraySize)
- Alloca = Builder.CreateAlloca(Ty, ArraySize, Name);
- else
- Alloca = new llvm::AllocaInst(Ty, CGM.getDataLayout().getAllocaAddrSpace(),
- ArraySize, Name, AllocaInsertPt);
- if (Allocas) {
- Allocas->Add(Alloca);
- }
- return Alloca;
+ return Builder.CreateAlloca(Ty, ArraySize, Name);
+ return new llvm::AllocaInst(Ty, CGM.getDataLayout().getAllocaAddrSpace(),
+ ArraySize, Name, AllocaInsertPt);
}
/// CreateDefaultAlignTempAlloca - This creates an alloca with the
diff --git a/clang/lib/CodeGen/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp
index 560a9e2c5ead5c..1b9287ea239347 100644
--- a/clang/lib/CodeGen/CGExprAgg.cpp
+++ b/clang/lib/CodeGen/CGExprAgg.cpp
@@ -15,7 +15,6 @@
#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"
@@ -25,7 +24,6 @@
#include "llvm/IR/Constants.h"
#include "llvm/IR/Function.h"
#include "llvm/IR/GlobalVariable.h"
-#include "llvm/IR/Instruction.h"
#include "llvm/IR/IntrinsicInst.h"
#include "llvm/IR/Intrinsics.h"
using namespace clang;
@@ -560,27 +558,24 @@ void AggExprEmitter::EmitArrayInit(Address DestPtr, llvm::ArrayType *AType,
// For that, we'll need an EH cleanup.
QualType::DestructionKind dtorKind = elementType.isDestructedType();
Address endOfInit = Address::invalid();
- CodeGenFunction::CleanupDeactivationScope deactivation(CGF);
-
- if (dtorKind) {
- CodeGenFunction::AllocaTrackerRAII allocaTracker(CGF);
+ EHScopeStack::stable_iterator cleanup;
+ llvm::Instruction *cleanupDominator = nullptr;
+ if (CGF.needsEHCleanup(dtorKind)) {
// In principle we could tell the cleanup where we are more
// directly, but the control flow can get so varied here that it
// would actually be quite complex. Therefore we go through an
// alloca.
- llvm::Instruction *dominatingIP =
- Builder.CreateFlagLoad(llvm::ConstantInt::getNullValue(CGF.Int8PtrTy));
endOfInit = CGF.CreateTempAlloca(begin->getType(), CGF.getPointerAlign(),
"arrayinit.endOfInit");
- Builder.CreateStore(begin, endOfInit);
+ cleanupDominator = Builder.CreateStore(begin, endOfInit);
CGF.pushIrregularPartialArrayCleanup(begin, endOfInit, elementType,
elementAlign,
CGF.getDestroyer(dtorKind));
- cast<EHCleanupScope>(*CGF.EHStack.find(CGF.EHStack.stable_begin()))
- .AddAuxAllocas(allocaTracker.Take());
+ cleanup = CGF.EHStack.stable_begin();
- CGF.DeferredDeactivationCleanupStack.push_back(
- {CGF.EHStack.stable_begin(), dominatingIP});
+ // Otherwise, remember that we didn't need a cleanup.
+ } else {
+ dtorKind = QualType::DK_none;
}
llvm::Value *one = llvm::ConstantInt::get(CGF.SizeTy, 1);
@@ -676,6 +671,9 @@ void AggExprEmitter::EmitArrayInit(Address DestPtr, llvm::ArrayType *AType,
CGF.EmitBlock(endBB);
}
+
+ // Leave the partial-array cleanup if we entered one.
+ if (dtorKind) CGF.DeactivateCleanupBlock(cleanup, cleanupDominator);
}
//===----------------------------------------------------------------------===//
@@ -1376,8 +1374,9 @@ AggExprEmitter::VisitLambdaExpr(LambdaExpr *E) {
LValue SlotLV = CGF.MakeAddrLValue(Slot.getAddress(), E->getType());
// We'll need to enter cleanup scopes in case any of the element
- // initializers throws an exception or contains branch out of the expressions.
- CodeGenFunction::CleanupDeactivationScope scope(CGF);
+ // initializers throws an exception.
+ SmallVector<EHScopeStack::stable_iterator, 16> Clea...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/88884
More information about the cfe-commits
mailing list