llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-coroutines Author: Utkarsh Saxena (usx95) <details> <summary>Changes</summary> Latest diff: https://github.com/llvm/llvm-project/pull/89154/files/f1ab4c2677394bbfc985d9680d5eecd7b2e6a882..26bb3b19c551348de5bf88bd16f90b60c28a8a2b 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&)`) 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. 6. push dtor for `~S()`. 7. ... It is important to deactivate **5** using active flags. Without the active flags, the `return` will fallthrough it as an active cleanup. It 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. Fixes https://github.com/llvm/llvm-project/issues/63818 Fixes https://github.com/llvm/llvm-project/issues/88478 --- Patch is 62.57 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/89154.diff 11 Files Affected: - (modified) clang/lib/CodeGen/CGCall.cpp (+7-6) - (modified) clang/lib/CodeGen/CGCleanup.cpp (+38-38) - (modified) clang/lib/CodeGen/CGCleanup.h (+56-1) - (modified) clang/lib/CodeGen/CGDecl.cpp (+42-19) - (modified) clang/lib/CodeGen/CGExpr.cpp (+9-3) - (modified) clang/lib/CodeGen/CGExprAgg.cpp (+26-61) - (modified) clang/lib/CodeGen/CGExprCXX.cpp (+19-19) - (modified) clang/lib/CodeGen/CodeGenFunction.cpp (+6) - (modified) clang/lib/CodeGen/CodeGenFunction.h (+96-3) - (added) clang/test/CodeGenCXX/control-flow-in-stmt-expr.cpp (+478) - (added) clang/test/CodeGenCoroutines/coro-suspend-cleanups.cpp (+93) ``````````diff diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp index 3f5463a9a70e9d..9004e96bc1a0cf 100644 --- a/clang/lib/CodeGen/CGCall.cpp +++ b/clang/lib/CodeGen/CGCall.cpp @@ -4693,11 +4693,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(); @@ -4706,14 +4706,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 e6f8e6873004f2..469e0363b744ac 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 = @@ -667,7 +674,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 @@ -692,7 +700,11 @@ 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) { + // FIXME: Come up with a program which would need forwarding prebranched + // fallthrough and add tests. Otherwise delete this and assert against it. + assert(!IsActive); llvm::BasicBlock *prebranchDest; // If the prebranch is semantically branching through the next @@ -724,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; } @@ -760,11 +774,19 @@ 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(); EHStack.popCleanup(); } else { // If we have a fallthrough and no other need for the cleanup, @@ -781,6 +803,7 @@ void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough) { } destroyOptimisticNormalEntry(*this, Scope); + Scope.MarkEmitted(); EHStack.popCleanup(); EmitCleanup(*this, Fn, cleanupFlags, NormalActiveFlag); @@ -916,6 +939,7 @@ void CodeGenFunction::PopCleanupBlock(bool FallthroughIsBranchThrough) { } // IV. Pop the cleanup and emit it. + Scope.MarkEmitted(); EHStack.popCleanup(); assert(EHStack.hasNormalCleanups() == HasEnclosingCleanups); @@ -984,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. @@ -1143,25 +1169,6 @@ void CodeGenFunction::EmitBranchThroughCleanup(JumpDest Dest) { Builder.ClearInsertionPoint(); } -static bool IsUsedAsNormalCleanup(EHScopeStack &EHStack, - EHScopeStack::stable_iterator C) { - // If we needed a normal block for any reason, that counts. - if (cast<EHCleanupScope>(*EHStack.find(C)).getNormalBlock()) - return true; - - // Check whether any enclosed cleanups were needed. - for (EHScopeStack::stable_iterator - I = EHStack.getInnermostNormalCleanup(); - I != C; ) { - assert(C.strictlyEncloses(I)); - EHCleanupScope &S = cast<EHCleanupScope>(*EHStack.find(I)); - if (S.getNormalBlock()) return true; - I = S.getEnclosingNormalCleanup(); - } - - return false; -} - static bool IsUsedAsEHCleanup(EHScopeStack &EHStack, EHScopeStack::stable_iterator cleanup) { // If we needed an EH block for any reason, that counts. @@ -1210,8 +1217,7 @@ static void SetupCleanupBlockActivation(CodeGenFunction &CGF, // Calculate whether the cleanup was used: // - as a normal cleanup - if (Scope.isNormalCleanup() && - (isActivatedInConditional || IsUsedAsNormalCleanup(CGF.EHStack, C))) { + if (Scope.isNormalCleanup()) { Scope.setTestFlagInNormalCleanup(); needFlag = true; } @@ -1224,13 +1230,16 @@ static void SetupCleanupBlockActivation(CodeGenFunction &CGF, } // If it hasn't yet been used as either, we're done. - if (!needFlag) return; + if (!needFlag) + return; Address var = Scope.getActiveFlag(); if (!var.isValid()) { + CodeGenFunction::AllocaTrackerRAII AllocaTracker(CGF); var = CGF.CreateTempAlloca(CGF.Builder.getInt1Ty(), CharUnits::One(), "cleanup.isactive"); Scope.setActiveFlag(var); + Scope.AddAuxAllocas(AllocaTracker.Take()); assert(dominatingIP && "no existing variable and no dominating IP!"); @@ -1273,17 +1282,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/CGCleanup.h b/clang/lib/CodeGen/CGCleanup.h index 03e4a29d7b3dbf..c73c97146abc4d 100644 --- a/clang/lib/CodeGen/CGCleanup.h +++ b/clang/lib/CodeGen/CGCleanup.h @@ -16,8 +16,11 @@ #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; @@ -266,6 +269,51 @@ 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. @@ -298,7 +346,7 @@ class alignas(8) EHCleanupScope : public EHScope { EHScopeStack::stable_iterator enclosingEH) : EHScope(EHScope::Cleanup, enclosingEH), EnclosingNormal(enclosingNormal), NormalBlock(nullptr), - ActiveFlag(Address::invalid()), ExtInfo(nullptr), + ActiveFlag(Address::invalid()), ExtInfo(nullptr), AuxAllocas(nullptr), FixupDepth(fixupDepth) { CleanupBits.IsNormalCleanup = isNormal; CleanupBits.IsEHCleanup = isEH; @@ -312,8 +360,15 @@ 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 ce6d6d8956076e..3f05ebb561da57 100644 --- a/clang/lib/CodeGen/CGDecl.cpp +++ b/clang/lib/CodeGen/CGDecl.cpp @@ -19,6 +19,7 @@ #include "CodeGenFunction.h" #include "CodeGenModule.h" #include "ConstantEmitter.h" +#include "EHScopeStack.h" #include "PatternInit.h" #include "TargetInfo.h" #include "clang/AST/ASTContext.h" @@ -2201,6 +2202,27 @@ 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); } @@ -2217,16 +2239,19 @@ 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. @@ -2241,13 +2266,12 @@ void CodeGenFunction::pushLifetimeExtendedDestroy(CleanupKind cleanupKind, Address ActiveFlag = createCleanupActiveFlag(); SavedType SavedAddr = saveValueInCond(addr); - if (cleanupKind & EHCleanup) { - EHStack.pushCleanup<ConditionalCleanupType>( - static_cast<CleanupKind>(cleanupKind & ~NormalCleanup), SavedAddr, type, - destroyer, useEHCleanupForArray); - initFullExprCleanupWithFlag(ActiveFlag); - } + pushCleanupAndDeferDeactivation<ConditionalCleanupType>( + cleanupKind, 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); @@ -2442,9 +2466,9 @@ namespace { }; } // end anonymous namespace -/// pushIrregularPartialArrayCleanup - Push an EH cleanup to destroy -/// already-constructed elements of the given array. The cleanup -/// may be popped with DeactivateCleanupBlock or PopCleanupBlock. +/// pushIrregularPartialArrayCleanup - Push a NormalAndEHCleanup 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 @@ -2453,10 +2477,9 @@ void CodeGenFunction::pushIrregularPartialArrayCleanup(llvm::Value *arrayBegin, QualType elementType, CharUnits elementAlign, Destroyer *destroyer) { - pushFullExprCleanup<IrregularPartialArrayDestroy>(EHCleanup, - arrayBegin, arrayEndPointer, - elementType, elementAlign, - destroyer); + pushFullExprCleanup<IrregularPartialArrayDestroy>( + NormalAndEHCleanup, 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 cf696a1c9f560f..c85a339f5e3f88 100644 --- a/clang/lib/CodeGen/CGExpr.cpp +++ b/clang/lib/CodeGen/CGExpr.cpp @@ -115,10 +115,16 @@ 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) - return Builder.CreateAlloca(Ty, ArraySize, Name); - return new llvm::AllocaInst(Ty, CGM.getDataLayout().getAllocaAddrSpace(), - ArraySize, Name, AllocaInsertPt); + 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; } /// CreateDefaultAlignTempAlloca - This creates an alloca with the diff --git a/clang/lib/CodeGen/CGExprAgg.cpp b/clang/lib/CodeGen/CGExprAgg.cpp index 1b9287ea239347..560a9e2c5ead5c 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" @@ -24,6 +25,7 @@ #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; @@ -558,24 +560,27 @@ 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(); - EHScopeStack::stable_iterator cleanup; - llvm::Instruction *cleanupDominator = nullptr; - if (CGF.needsEHCleanup(dtorKind)) { + CodeGenFunction::CleanupDeactivationScope deactivation(CGF); + + if (dtorKind) { + CodeGenFunction::AllocaTrackerRAII allocaTracker(CGF); // In principle we could tell the cleanup where... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/89154 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits