Revert "[codegen] Emit missing cleanups for stmt-expr and coro suspensions" and related commits #88884
+128
−796
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…cts (llvm#88751)" This reverts commit 5a46123.
This reverts commit 43b4e5b.
…sions [take-2] (llvm#85398)" This reverts commit 89ba7e1.
You can test this locally with the following command:git-clang-format --diff 668a58b8926473d731c41c55007f1fe4571ada86 3b835fd6d6404cce1a2d82fb3dfd56720fc90a4b -- clang/lib/CodeGen/CGCall.cpp clang/lib/CodeGen/CGCleanup.cpp clang/lib/CodeGen/CGCleanup.h clang/lib/CodeGen/CGDecl.cpp clang/lib/CodeGen/CGExpr.cpp clang/lib/CodeGen/CGExprAgg.cpp clang/lib/CodeGen/CGExprCXX.cpp clang/lib/CodeGen/CodeGenFunction.cpp clang/lib/CodeGen/CodeGenFunction.hView the diff from clang-format here.diff --git a/clang/lib/CodeGen/CGCleanup.cpp b/clang/lib/CodeGen/CGCleanup.cpp
index e6f8e68730..a79d453a0a 100644
--- a/clang/lib/CodeGen/CGCleanup.cpp
+++ b/clang/lib/CodeGen/CGCleanup.cpp
@@ -722,8 +722,7 @@ void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough) {
if (!RequiresNormalCleanup && !RequiresEHCleanup) {
destroyOptimisticNormalEntry(*this, Scope);
EHStack.popCleanup(); // safe because there are no fixups
- assert(EHStack.getNumBranchFixups() == 0 ||
- EHStack.hasNormalCleanups());
+ assert(EHStack.getNumBranchFixups() == 0 || EHStack.hasNormalCleanups());
return;
}
diff --git a/clang/lib/CodeGen/CGCleanup.h b/clang/lib/CodeGen/CGCleanup.h
index 03e4a29d7b..6620882d05 100644
--- a/clang/lib/CodeGen/CGCleanup.h
+++ b/clang/lib/CodeGen/CGCleanup.h
@@ -311,9 +311,7 @@ public:
assert(CleanupBits.CleanupSize == cleanupSize && "cleanup size overflow");
}
- void Destroy() {
- delete ExtInfo;
- }
+ void Destroy() { delete ExtInfo; }
// 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 ce6d6d8956..6f552f9b58 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -2226,7 +2226,8 @@ void CodeGenFunction::pushLifetimeExtendedDestroy(CleanupKind cleanupKind,
destroyer, useEHCleanupForArray);
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.
@@ -2453,10 +2454,9 @@ void CodeGenFunction::pushIrregularPartialArrayCleanup(llvm::Value *arrayBegin,
QualType elementType,
CharUnits elementAlign,
Destroyer *destroyer) {
- pushFullExprCleanup<IrregularPartialArrayDestroy>(EHCleanup,
- 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/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp
index 1b9287ea23..47796d9222 100644
--- a/clang/lib/CodeGen/CGExprAgg.cpp
+++ b/clang/lib/CodeGen/CGExprAgg.cpp
@@ -573,7 +573,7 @@ void AggExprEmitter::EmitArrayInit(Address DestPtr, llvm::ArrayType *AType,
CGF.getDestroyer(dtorKind));
cleanup = CGF.EHStack.stable_begin();
- // Otherwise, remember that we didn't need a cleanup.
+ // Otherwise, remember that we didn't need a cleanup.
} else {
dtorKind = QualType::DK_none;
}
@@ -673,7 +673,8 @@ void AggExprEmitter::EmitArrayInit(Address DestPtr, llvm::ArrayType *AType,
}
// Leave the partial-array cleanup if we entered one.
- if (dtorKind) CGF.DeactivateCleanupBlock(cleanup, cleanupDominator);
+ if (dtorKind)
+ CGF.DeactivateCleanupBlock(cleanup, cleanupDominator);
}
//===----------------------------------------------------------------------===//
@@ -1398,8 +1399,7 @@ AggExprEmitter::VisitLambdaExpr(LambdaExpr *E) {
if (CGF.needsEHCleanup(DtorKind)) {
if (!CleanupDominator)
CleanupDominator = CGF.Builder.CreateAlignedLoad(
- CGF.Int8Ty,
- llvm::Constant::getNullValue(CGF.Int8PtrTy),
+ CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy),
CharUnits::One()); // placeholder
CGF.pushDestroy(EHCleanup, LV.getAddress(CGF), CurField->getType(),
@@ -1412,7 +1412,7 @@ AggExprEmitter::VisitLambdaExpr(LambdaExpr *E) {
// Deactivate all the partial cleanups in reverse order, which
// generally means popping them.
for (unsigned i = Cleanups.size(); i != 0; --i)
- CGF.DeactivateCleanupBlock(Cleanups[i-1], CleanupDominator);
+ CGF.DeactivateCleanupBlock(Cleanups[i - 1], CleanupDominator);
// Destroy the placeholder if we made one.
if (CleanupDominator)
@@ -1835,7 +1835,7 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr(
assert((cleanupDominator || cleanups.empty()) &&
"Missing cleanupDominator before deactivating cleanup blocks");
for (unsigned i = cleanups.size(); i != 0; --i)
- CGF.DeactivateCleanupBlock(cleanups[i-1], cleanupDominator);
+ CGF.DeactivateCleanupBlock(cleanups[i - 1], cleanupDominator);
// Destroy the placeholder if we made one.
if (cleanupDominator)
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index ff1873325d..0f95a6f965 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -940,8 +940,7 @@ public:
public:
/// Enter a new cleanup scope.
explicit RunCleanupsScope(CodeGenFunction &CGF)
- : PerformCleanup(true), CGF(CGF)
- {
+ : PerformCleanup(true), CGF(CGF) {
CleanupStackDepth = CGF.EHStack.stable_begin();
LifetimeExtendedCleanupStackSize =
CGF.LifetimeExtendedCleanupStack.size();
|
You can test this locally with the following command:git-clang-format --diff c309dc6d0759b23b570c563f611530ff1a49e1bd b4a56290ff5d4fd2e1fb90dc974c5bb7030d61cb -- clang/lib/CodeGen/CGCall.cpp clang/lib/CodeGen/CGCleanup.cpp clang/lib/CodeGen/CGCleanup.h clang/lib/CodeGen/CGDecl.cpp clang/lib/CodeGen/CGExpr.cpp clang/lib/CodeGen/CGExprAgg.cpp clang/lib/CodeGen/CGExprCXX.cpp clang/lib/CodeGen/CodeGenFunction.cpp clang/lib/CodeGen/CodeGenFunction.hView the diff from clang-format here.diff --git a/clang/lib/CodeGen/CGCleanup.cpp b/clang/lib/CodeGen/CGCleanup.cpp
index e6f8e68730..a79d453a0a 100644
--- a/clang/lib/CodeGen/CGCleanup.cpp
+++ b/clang/lib/CodeGen/CGCleanup.cpp
@@ -722,8 +722,7 @@ void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough) {
if (!RequiresNormalCleanup && !RequiresEHCleanup) {
destroyOptimisticNormalEntry(*this, Scope);
EHStack.popCleanup(); // safe because there are no fixups
- assert(EHStack.getNumBranchFixups() == 0 ||
- EHStack.hasNormalCleanups());
+ assert(EHStack.getNumBranchFixups() == 0 || EHStack.hasNormalCleanups());
return;
}
diff --git a/clang/lib/CodeGen/CGCleanup.h b/clang/lib/CodeGen/CGCleanup.h
index 03e4a29d7b..6620882d05 100644
--- a/clang/lib/CodeGen/CGCleanup.h
+++ b/clang/lib/CodeGen/CGCleanup.h
@@ -311,9 +311,7 @@ public:
assert(CleanupBits.CleanupSize == cleanupSize && "cleanup size overflow");
}
- void Destroy() {
- delete ExtInfo;
- }
+ void Destroy() { delete ExtInfo; }
// 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 ce6d6d8956..6f552f9b58 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -2226,7 +2226,8 @@ void CodeGenFunction::pushLifetimeExtendedDestroy(CleanupKind cleanupKind,
destroyer, useEHCleanupForArray);
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.
@@ -2453,10 +2454,9 @@ void CodeGenFunction::pushIrregularPartialArrayCleanup(llvm::Value *arrayBegin,
QualType elementType,
CharUnits elementAlign,
Destroyer *destroyer) {
- pushFullExprCleanup<IrregularPartialArrayDestroy>(EHCleanup,
- 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/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp
index 1b9287ea23..47796d9222 100644
--- a/clang/lib/CodeGen/CGExprAgg.cpp
+++ b/clang/lib/CodeGen/CGExprAgg.cpp
@@ -573,7 +573,7 @@ void AggExprEmitter::EmitArrayInit(Address DestPtr, llvm::ArrayType *AType,
CGF.getDestroyer(dtorKind));
cleanup = CGF.EHStack.stable_begin();
- // Otherwise, remember that we didn't need a cleanup.
+ // Otherwise, remember that we didn't need a cleanup.
} else {
dtorKind = QualType::DK_none;
}
@@ -673,7 +673,8 @@ void AggExprEmitter::EmitArrayInit(Address DestPtr, llvm::ArrayType *AType,
}
// Leave the partial-array cleanup if we entered one.
- if (dtorKind) CGF.DeactivateCleanupBlock(cleanup, cleanupDominator);
+ if (dtorKind)
+ CGF.DeactivateCleanupBlock(cleanup, cleanupDominator);
}
//===----------------------------------------------------------------------===//
@@ -1398,8 +1399,7 @@ AggExprEmitter::VisitLambdaExpr(LambdaExpr *E) {
if (CGF.needsEHCleanup(DtorKind)) {
if (!CleanupDominator)
CleanupDominator = CGF.Builder.CreateAlignedLoad(
- CGF.Int8Ty,
- llvm::Constant::getNullValue(CGF.Int8PtrTy),
+ CGF.Int8Ty, llvm::Constant::getNullValue(CGF.Int8PtrTy),
CharUnits::One()); // placeholder
CGF.pushDestroy(EHCleanup, LV.getAddress(CGF), CurField->getType(),
@@ -1412,7 +1412,7 @@ AggExprEmitter::VisitLambdaExpr(LambdaExpr *E) {
// Deactivate all the partial cleanups in reverse order, which
// generally means popping them.
for (unsigned i = Cleanups.size(); i != 0; --i)
- CGF.DeactivateCleanupBlock(Cleanups[i-1], CleanupDominator);
+ CGF.DeactivateCleanupBlock(Cleanups[i - 1], CleanupDominator);
// Destroy the placeholder if we made one.
if (CleanupDominator)
@@ -1835,7 +1835,7 @@ void AggExprEmitter::VisitCXXParenListOrInitListExpr(
assert((cleanupDominator || cleanups.empty()) &&
"Missing cleanupDominator before deactivating cleanup blocks");
for (unsigned i = cleanups.size(); i != 0; --i)
- CGF.DeactivateCleanupBlock(cleanups[i-1], cleanupDominator);
+ CGF.DeactivateCleanupBlock(cleanups[i - 1], cleanupDominator);
// Destroy the placeholder if we made one.
if (cleanupDominator)
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index ff1873325d..0f95a6f965 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -940,8 +940,7 @@ public:
public:
/// Enter a new cleanup scope.
explicit RunCleanupsScope(CodeGenFunction &CGF)
- : PerformCleanup(true), CGF(CGF)
- {
+ : PerformCleanup(true), CGF(CGF) {
CleanupStackDepth = CGF.EHStack.stable_begin();
LifetimeExtendedCleanupStackSize =
CGF.LifetimeExtendedCleanupStack.size();
|
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-coroutines Author: Utkarsh Saxena (usx95) ChangesThe original change caused widespread breakages in msan/ubsan tests and causes 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:
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]
|
usx95
added a commit
to usx95/llvm-project
that referenced
this pull request
Apr 17, 2024
…nsions" and related commits (llvm#88884) This reverts commit 9d8be24.
usx95
added a commit
that referenced
this pull request
Apr 29, 2024
) Latest diff: https://github.com/llvm/llvm-project/pull/89154/files/f1ab4c2677394bbfc985d9680d5eecd7b2e6a882..adf9bc902baddb156c83ce0f8ec03c142e806d45 We address two additional bugs here: ### Problem 1: Deactivated normal cleanup still runs, leading to double-free Consider the following: ```cpp struct A { }; struct B { B(const A&); }; struct S { A a; B b; }; int AcceptS(S s); void Accept2(int x, int y); void Test() { Accept2(AcceptS({.a = A{}, .b = A{}}), ({ return; 0; })); } ``` We add cleanups as follows: 1. push dtor for field `S::a` 2. push dtor for temp `A{}` (used by ` B(const A&)` in `.b = A{}`) 3. push dtor for field `S::b` 4. Deactivate 3 `S::b`-> This pops the cleanup. 5. Deactivate 1 `S::a` -> Does not pop the cleanup as *2* is top. Should create _active flag_!! 6. push dtor for `~S()`. 7. ... It is important to deactivate **5** using active flags. Without the active flags, the `return` will fallthrough it and would run both `~S()` and dtor `S::a` leading to **double free** of `~A()`. In this patch, we unconditionally emit active flags while deactivating normal cleanups. These flags are deleted later by the `AllocaTracker` if the cleanup is not emitted. ### Problem 2: Missing cleanup for conditional lifetime extension We push 2 cleanups for lifetime-extended cleanup. The first cleanup is useful if we exit from the middle of the expression (stmt-expr/coro suspensions). This is deactivated after full-expr, and a new cleanup is pushed, extending the lifetime of the temporaries (to the scope of the reference being initialized). If this lifetime extension happens to be conditional, then we use active flags to remember whether the branch was taken and if the object was initialized. Previously, we used a **single** active flag, which was used by both cleanups. This is wrong because the first cleanup will be forced to deactivate after the full-expr and therefore this **active** flag will always be **inactive**. The dtor for the lifetime extended entity would not run as it always sees an **inactive** flag. In this patch, we solve this using two separate active flags for both cleanups. Both of them are activated if the conditional branch is taken, but only one of them is deactivated after the full-expr. --- Fixes #63818 Fixes #88478 --- Previous PR logs: 1. #85398 2. #88670 3. #88751 4. #88884
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The original change caused widespread breakages in msan/ubsan tests and causes
use-after-free. Most likely we are adding more cleanups than necessary.