From f94168c8757aa400fd93fa82d9366d027cfa2303 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Mon, 25 Aug 2025 16:44:05 -0700 Subject: [PATCH 1/4] [AllocBoxToStack] Restore isDeadEnd check. The rewrite was missing the intentional omission of `dealloc_stack`s corresponding to `[dead_end]` `dealloc_box`es. Add the necessary bridging to get to parity with the original. Without this check, `dealloc_box [dead_end]`s are promoted to `dealloc_stack`s but the memory projected out of such `alloc_box`s need not be valid. rdar://159271158 --- .../Sources/Optimizer/FunctionPasses/AllocBoxToStack.swift | 4 ++++ SwiftCompilerSources/Sources/SIL/Instruction.swift | 4 +++- include/swift/SIL/SILBridging.h | 1 + include/swift/SIL/SILBridgingImpl.h | 4 ++++ test/SILOptimizer/allocbox_to_stack_lifetime.sil | 1 - 5 files changed, 12 insertions(+), 2 deletions(-) diff --git a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/AllocBoxToStack.swift b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/AllocBoxToStack.swift index 5d09c6d9a137f..bee2d8bb0b6c1 100644 --- a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/AllocBoxToStack.swift +++ b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/AllocBoxToStack.swift @@ -365,6 +365,10 @@ private func createAllocStack(for allocBox: AllocBoxInst, flags: Flags, _ contex if !unboxedType.isTrivial(in: allocBox.parentFunction), !(destroy is DeallocBoxInst) { builder.createDestroyAddr(address: stackLocation) } + if let dbi = destroy as? DeallocBoxInst, dbi.isDeadEnd { + // Don't bother to create dealloc_stack instructions in dead-ends. + return + } builder.createDeallocStack(asi) } } diff --git a/SwiftCompilerSources/Sources/SIL/Instruction.swift b/SwiftCompilerSources/Sources/SIL/Instruction.swift index fd5e986ac4096..0bc516cb8663b 100644 --- a/SwiftCompilerSources/Sources/SIL/Instruction.swift +++ b/SwiftCompilerSources/Sources/SIL/Instruction.swift @@ -699,7 +699,9 @@ final public class DeallocRefInst : Instruction, UnaryInstruction, Deallocation final public class DeallocPartialRefInst : Instruction, Deallocation {} -final public class DeallocBoxInst : Instruction, UnaryInstruction, Deallocation {} +final public class DeallocBoxInst : Instruction, UnaryInstruction, Deallocation { + public var isDeadEnd: Bool { bridged.DeallocBoxInst_isDeadEnd() } +} final public class DeallocExistentialBoxInst : Instruction, UnaryInstruction, Deallocation {} diff --git a/include/swift/SIL/SILBridging.h b/include/swift/SIL/SILBridging.h index b3c4475267534..42f97ba51b1e8 100644 --- a/include/swift/SIL/SILBridging.h +++ b/include/swift/SIL/SILBridging.h @@ -827,6 +827,7 @@ struct BridgedInstruction { BRIDGED_INLINE bool CopyAddrInst_isInitializationOfDest() const; BRIDGED_INLINE void CopyAddrInst_setIsTakeOfSrc(bool isTakeOfSrc) const; BRIDGED_INLINE void CopyAddrInst_setIsInitializationOfDest(bool isInitializationOfDest) const; + BRIDGED_INLINE bool DeallocBoxInst_isDeadEnd() const; BRIDGED_INLINE bool ExplicitCopyAddrInst_isTakeOfSrc() const; BRIDGED_INLINE bool ExplicitCopyAddrInst_isInitializationOfDest() const; BRIDGED_INLINE SwiftInt MarkUninitializedInst_getKind() const; diff --git a/include/swift/SIL/SILBridgingImpl.h b/include/swift/SIL/SILBridgingImpl.h index c1229e7231457..24295ef76a591 100644 --- a/include/swift/SIL/SILBridgingImpl.h +++ b/include/swift/SIL/SILBridgingImpl.h @@ -1508,6 +1508,10 @@ void BridgedInstruction::CopyAddrInst_setIsInitializationOfDest(bool isInitializ isInitializationOfDest ? swift::IsInitialization : swift::IsNotInitialization); } +bool BridgedInstruction::DeallocBoxInst_isDeadEnd() const { + return getAs()->isDeadEnd(); +} + bool BridgedInstruction::ExplicitCopyAddrInst_isTakeOfSrc() const { return getAs()->isTakeOfSrc(); } diff --git a/test/SILOptimizer/allocbox_to_stack_lifetime.sil b/test/SILOptimizer/allocbox_to_stack_lifetime.sil index 86f2cfa6d46c3..33cc1cc2f947a 100644 --- a/test/SILOptimizer/allocbox_to_stack_lifetime.sil +++ b/test/SILOptimizer/allocbox_to_stack_lifetime.sil @@ -54,7 +54,6 @@ bb0(%0 : $Int): // CHECK: [[STACK:%[^,]+]] = alloc_stack // CHECK: cond_br undef, [[DIE:bb[0-9]+]] // CHECK: [[DIE]]: -// CHECK-NEXT: dealloc_stack [[STACK]] // CHECK-NEXT: unreachable // CHECK-LABEL: } // end sil function 'keep_dead_end' sil [ossa] @keep_dead_end : $@convention(thin) () -> () { From 74528eef8c2455995d595a3fcb5264b223e940ff Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Wed, 27 Aug 2025 07:43:06 -0700 Subject: [PATCH 2/4] [NFC] OptUtils: Promote isInLoop to internal. It will be used by both AllocBoxToStack in addition to StackPromotion and for the same reason. --- .../Optimizer/FunctionPasses/StackPromotion.swift | 14 -------------- .../Sources/Optimizer/Utilities/OptUtils.swift | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/StackPromotion.swift b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/StackPromotion.swift index 7d522eade4bb5..30994a76c5bf7 100644 --- a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/StackPromotion.swift +++ b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/StackPromotion.swift @@ -330,17 +330,3 @@ private extension BasicBlockRange { exits.contains { !deadEndBlocks.isDeadEnd($0) && !$0.hasSinglePredecessor } } } - -private func isInLoop(block startBlock: BasicBlock, _ context: FunctionPassContext) -> Bool { - var worklist = BasicBlockWorklist(context) - defer { worklist.deinitialize() } - - worklist.pushIfNotVisited(contentsOf: startBlock.successors) - while let block = worklist.pop() { - if block == startBlock { - return true - } - worklist.pushIfNotVisited(contentsOf: block.successors) - } - return false -} diff --git a/SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift b/SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift index d3a9302424cfe..98cc1079c1c25 100644 --- a/SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift +++ b/SwiftCompilerSources/Sources/Optimizer/Utilities/OptUtils.swift @@ -1012,3 +1012,17 @@ func eraseIfDead(functions: [Function], _ context: ModulePassContext) { toDelete = remaining } } + +func isInLoop(block startBlock: BasicBlock, _ context: FunctionPassContext) -> Bool { + var worklist = BasicBlockWorklist(context) + defer { worklist.deinitialize() } + + worklist.pushIfNotVisited(contentsOf: startBlock.successors) + while let block = worklist.pop() { + if block == startBlock { + return true + } + worklist.pushIfNotVisited(contentsOf: block.successors) + } + return false +} From 1eafced6a2fef64c6caffb1d34327614d799ecc2 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Mon, 25 Aug 2025 16:45:46 -0700 Subject: [PATCH 3/4] [AllocBoxToStack] Don't destroy in dead-ends. It is valid to leak a value on paths into dead-end regions. Specifically, it is valid to leak an `alloc_box`. Thus, "final releases" in dead-end regions may not destroy the box and consequently may not release its contents. Therefore it's invalid to lower such final releases to `dealloc_stack`s, let alone `destroy_addr`s. The in-general invalidity of that transformation results in miscompiling whenever a box is leaked and its projected address is used after such final releases. Fix this by not treating final releases as boundary markers of the `alloc_box` and not lowering them to `destroy_addr`s and `dealloc_stack`s. rdar://158149082 --- .../FunctionPasses/AllocBoxToStack.swift | 23 +- .../Transforms/AllocBoxToStack.cpp | 41 +++- test/SILOptimizer/allocbox_to_stack.sil | 203 ++++++++++++++++- .../allocbox_to_stack_ownership.sil | 206 +++++++++++++++++- 4 files changed, 462 insertions(+), 11 deletions(-) diff --git a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/AllocBoxToStack.swift b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/AllocBoxToStack.swift index bee2d8bb0b6c1..f56dd90f20a4b 100644 --- a/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/AllocBoxToStack.swift +++ b/SwiftCompilerSources/Sources/Optimizer/FunctionPasses/AllocBoxToStack.swift @@ -362,7 +362,28 @@ private func createAllocStack(for allocBox: AllocBoxInst, flags: Flags, _ contex for destroy in getFinalDestroys(of: allocBox, context) { let loc = allocBox.location.asCleanup.withScope(of: destroy.location) Builder.insert(after: destroy, location: loc, context) { builder in - if !unboxedType.isTrivial(in: allocBox.parentFunction), !(destroy is DeallocBoxInst) { + if !(destroy is DeallocBoxInst), + context.deadEndBlocks.isDeadEnd(destroy.parentBlock), + !isInLoop(block: destroy.parentBlock, context) { + // "Last" releases in dead-end regions may not actually destroy the box + // and consequently may not actually release the stored value. That's + // because values (including boxes) may be leaked along paths into + // dead-end regions. Thus it is invalid to lower such final releases of + // the box to destroy_addr's/dealloc_box's of the stack-promoted storage. + // + // There is one exception: if the alloc_box is in a dead-end loop. In + // that case SIL invariants require that the final releases actually + // destroy the box; otherwise, a box would leak once per loop. To check + // for this, it is sufficient check that the LastRelease is in a dead-end + // loop: if the alloc_box is not in that loop, then the entire loop is in + // the live range, so no release within the loop would be a "final + // release". + // + // None of this applies to dealloc_box instructions which always destroy + // the box. + return + } + if !unboxedType.isTrivial(in: allocBox.parentFunction), !(destroy is DeallocBoxInst) { builder.createDestroyAddr(address: stackLocation) } if let dbi = destroy as? DeallocBoxInst, dbi.isDeadEnd { diff --git a/lib/SILOptimizer/Transforms/AllocBoxToStack.cpp b/lib/SILOptimizer/Transforms/AllocBoxToStack.cpp index c780a9be1f342..7779266d3a626 100644 --- a/lib/SILOptimizer/Transforms/AllocBoxToStack.cpp +++ b/lib/SILOptimizer/Transforms/AllocBoxToStack.cpp @@ -25,6 +25,8 @@ #include "swift/SIL/SILArgument.h" #include "swift/SIL/SILBuilder.h" #include "swift/SIL/SILCloner.h" +#include "swift/SILOptimizer/Analysis/DeadEndBlocksAnalysis.h" +#include "swift/SILOptimizer/Analysis/LoopAnalysis.h" #include "swift/SILOptimizer/PassManager/Passes.h" #include "swift/SILOptimizer/PassManager/Transforms.h" #include "swift/SILOptimizer/Utils/InstOptUtils.h" @@ -601,7 +603,9 @@ static void hoistMarkUnresolvedNonCopyableValueInsts( /// rewriteAllocBoxAsAllocStack - Replace uses of the alloc_box with a /// new alloc_stack, but do not delete the alloc_box yet. -static bool rewriteAllocBoxAsAllocStack(AllocBoxInst *ABI) { +static bool rewriteAllocBoxAsAllocStack(AllocBoxInst *ABI, + DeadEndBlocksAnalysis &deba, + SILLoopAnalysis &la) { LLVM_DEBUG(llvm::dbgs() << "*** Promoting alloc_box to stack: " << *ABI); SILValue HeapBox = ABI; @@ -693,9 +697,31 @@ static bool rewriteAllocBoxAsAllocStack(AllocBoxInst *ABI) { ABI->getBoxType(), ABI->getModule().Types, 0)); auto Loc = CleanupLocation(ABI->getLoc()); + auto *deb = deba.get(ABI->getFunction()); for (auto LastRelease : FinalReleases) { + auto *dbi = dyn_cast(LastRelease); + if (!dbi && deb->isDeadEnd(LastRelease->getParent()) && + !la.get(ABI->getFunction())->getLoopFor(LastRelease->getParent())) { + // "Last" releases in dead-end regions may not actually destroy the box + // and consequently may not actually release the stored value. That's + // because values (including boxes) may be leaked along paths into + // dead-end regions. Thus it is invalid to lower such final releases of + // the box to destroy_addr's/dealloc_box's of the stack-promoted storage. + // + // There is one exception: if the alloc_box is in a dead-end loop. In + // that case SIL invariants require that the final releases actually + // destroy the box; otherwise, a box would leak once per loop. To check + // for this, it is sufficient check that the LastRelease is in a dead-end + // loop: if the alloc_box is not in that loop, then the entire loop is in + // the live range, so no release within the loop would be a "final + // release". + // + // None of this applies to dealloc_box instructions which always destroy + // the box. + continue; + } SILBuilderWithScope Builder(LastRelease); - if (!isa(LastRelease)&& !Lowering.isTrivial()) { + if (!dbi && !Lowering.isTrivial()) { // If we have a mark_unresolved_non_copyable_value use of our stack box, // we want to destroy that. SILValue valueToDestroy = StackBox; @@ -709,7 +735,6 @@ static bool rewriteAllocBoxAsAllocStack(AllocBoxInst *ABI) { // instruction we found that isn't an explicit dealloc_box. Builder.emitDestroyAddrAndFold(Loc, valueToDestroy); } - auto *dbi = dyn_cast(LastRelease); if (dbi && dbi->isDeadEnd()) { // Don't bother to create dealloc_stack instructions in dead-ends. continue; @@ -1265,7 +1290,9 @@ static void rewriteApplySites(AllocBoxToStackState &pass) { /// Clone closure bodies and rewrite partial applies. Returns the number of /// alloc_box allocations promoted. -static unsigned rewritePromotedBoxes(AllocBoxToStackState &pass) { +static unsigned rewritePromotedBoxes(AllocBoxToStackState &pass, + DeadEndBlocksAnalysis &deba, + SILLoopAnalysis &la) { // First we'll rewrite any ApplySite that we can to remove // the box container pointer from the operands. rewriteApplySites(pass); @@ -1274,7 +1301,7 @@ static unsigned rewritePromotedBoxes(AllocBoxToStackState &pass) { auto rend = pass.Promotable.rend(); for (auto I = pass.Promotable.rbegin(); I != rend; ++I) { auto *ABI = *I; - if (rewriteAllocBoxAsAllocStack(ABI)) { + if (rewriteAllocBoxAsAllocStack(ABI, deba, la)) { ++Count; ABI->eraseFromParent(); } @@ -1299,7 +1326,9 @@ class AllocBoxToStack : public SILFunctionTransform { } if (!pass.Promotable.empty()) { - auto Count = rewritePromotedBoxes(pass); + auto *deba = getAnalysis(); + auto *la = getAnalysis(); + auto Count = rewritePromotedBoxes(pass, *deba, *la); NumStackPromoted += Count; if (Count) { if (StackNesting::fixNesting(getFunction()) == StackNesting::Changes::CFG) diff --git a/test/SILOptimizer/allocbox_to_stack.sil b/test/SILOptimizer/allocbox_to_stack.sil index 0642d8cd352eb..26f568cf31549 100644 --- a/test/SILOptimizer/allocbox_to_stack.sil +++ b/test/SILOptimizer/allocbox_to_stack.sil @@ -14,6 +14,8 @@ struct Bool { protocol Error {} +class C {} + // CHECK-LABEL: sil @simple_promotion sil @simple_promotion : $(Int) -> Int { bb0(%0 : $Int): @@ -900,7 +902,6 @@ bb2: // CHECK: bb1: // CHECK-NEXT: [[STACK2:%[0-9]+]] = alloc_stack $Bool // CHECK-NEXT: dealloc_stack [[STACK2]] -// CHECK-NEXT: dealloc_stack [[BOX]] // CHECK-NEXT: unreachable // CHECK: bb2: // CHECK: store {{%[0-9]+}} @@ -1058,10 +1059,8 @@ bb3: // CHECK-NEXT: [[AI:%[0-9]+]] = alloc_stack $Int // CHECK-NEXT: cond_br // CHECK: bb1: -// CHECK-NEXT: dealloc_stack [[AI]] // CHECK-NEXT: br bb3 // CHECK: bb2: -// CHECK-NEXT: dealloc_stack [[AI]] // CHECK-NEXT: br bb3 // CHECK: bb3: // CHECK-NEXT: store {{%[0-9]+}} @@ -1225,3 +1224,201 @@ bb0: destroy_value %box : ${ var Builtin.Int32 } return %value : $Builtin.Int32 } + +sil @getC : $@convention(thin) () -> (@owned C) + +// CHECK-LABEL: sil @leak_to_inf_loop_1 : {{.*}} { +// CHECK-NOT: destroy_addr +// CHECK-NOT: dealloc_stack +// CHECK-LABEL: } // end sil function 'leak_to_inf_loop_1' +sil @leak_to_inf_loop_1 : $@convention(thin) (@owned C) -> () { +entry(%c : $C): + %box = alloc_box ${ var C } + %c_addr = project_box %box, 0 + store %c to %c_addr + strong_retain %box + strong_release %box + br loop + +loop: + %getC = function_ref @getC : $@convention(thin) () -> (@owned C) + %c2 = apply %getC() : $@convention(thin) () -> (@owned C) + %c_old = load %c_addr + store %c2 to %c_addr + release_value %c_old + br loop +} + +// CHECK-LABEL: sil @leak_to_inf_loop_2 : {{.*}} { +// CHECK: cond_br undef, [[EXIT:bb[0-9]+]], [[TO_LOOP:bb[0-9]+]] +// CHECK: [[EXIT]] +// CHECK: destroy_addr +// CHECK: dealloc_stack +// CHECK: [[TO_LOOP]] +// CHECK-NOT: destroy_addr +// CHECK-NOT: dealloc_stack +// CHECK-LABEL: } // end sil function 'leak_to_inf_loop_2' +sil @leak_to_inf_loop_2 : $@convention(thin) (@owned C) -> () { +entry(%c : $C): + %box = alloc_box ${ var C } + %c_addr = project_box %box, 0 + store %c to %c_addr + strong_retain %box + strong_release %box + cond_br undef, exit, to_loop + +exit: + strong_release %box + return undef : $() + +to_loop: + strong_retain %box + strong_release %box + br loop + +loop: + %getC = function_ref @getC : $@convention(thin) () -> (@owned C) + %c2 = apply %getC() : $@convention(thin) () -> (@owned C) + %c_old = load %c_addr + store %c2 to %c_addr + release_value %c_old + br loop +} + +// CHECK-LABEL: sil @leak_to_inf_loop_3 : {{.*}} { +// CHECK-NOT: destroy_addr +// CHECK-NOT: dealloc_stack +// CHECK-LABEL: } // end sil function 'leak_to_inf_loop_3' +sil @leak_to_inf_loop_3 : $@convention(thin) (@owned C) -> () { +entry(%c : $C): + %box = alloc_box ${ var C } + %c_addr = project_box %box, 0 + store %c to %c_addr + br to_loop + +to_loop: + strong_retain %box + strong_release %box + br loop + +loop: + %getC = function_ref @getC : $@convention(thin) () -> (@owned C) + %c2 = apply %getC() : $@convention(thin) () -> (@owned C) + %c_old = load %c_addr + store %c2 to %c_addr + release_value %c_old + br loop +} + +// CHECK-LABEL: sil @dealloc_box_before_loop +// CHECK: dealloc_stack +// CHECK-LABEL: } // end sil function 'dealloc_box_before_loop' +sil @dealloc_box_before_loop : $@convention(thin) (@owned C) -> () { +entry(%c : $C): + %box = alloc_box ${ var C } + %c_addr = project_box %box, 0 + store %c to %c_addr + cond_br undef, exit, to_loop + +exit: + strong_release %box + return undef : $() + +to_loop: + dealloc_box %box + br loop + +loop: + br loop +} + +// CHECK-LABEL: sil @alloc_box_in_deadend_loop +// CHECK: alloc_stack +// CHECK: dealloc_stack +// CHECK-LABEL: } // end sil function 'alloc_box_in_deadend_loop' +sil @alloc_box_in_deadend_loop : $@convention(thin) () -> () { +entry: + br loop + +loop: + %box = alloc_box ${ var C } + %c_addr = project_box %box, 0 + %getC = function_ref @getC : $@convention(thin) () -> (@owned C) + %c = apply %getC() : $@convention(thin) () -> (@owned C) + store %c to %c_addr + strong_release %box + br loop +} + +// CHECK-LABEL: sil @alloc_box_in_exiting_loop +// CHECK: br [[EXITING_LOOP:bb[0-9]+]] +// CHECK: [[EXITING_LOOP]]: +// CHECK: alloc_stack +// CHECK: cond_br undef, [[EXIT:bb[0-9]+]], [[LATCH:bb[0-9]+]] +// CHECK: [[LATCH]]: +// CHECK: cond_br undef, [[BACKEDGE:bb[0-9]+]], [[INFINITE_LOOP:bb[0-9]+]] +// CHECK: [[BACKEDGE]]: +// CHECK: dealloc_stack +// CHECK: br [[EXITING_LOOP]] +// CHECK: [[EXIT]]: +// CHECK: dealloc_stack +// CHECK: [[INFINITE_LOOP]]: +// CHECK-NOT: dealloc_stack +// CHECK-LABEL: } // end sil function 'alloc_box_in_exiting_loop' +sil @alloc_box_in_exiting_loop : $@convention(thin) () -> () { +entry: + br exiting_loop + +exiting_loop: + %box = alloc_box ${ var C } + %c_addr = project_box %box, 0 + cond_br undef, exit, latch + +latch: + cond_br undef, backedge, infinite_loop + +backedge: + strong_release %box + br exiting_loop + +exit: + strong_release %box + return undef : $() + +infinite_loop: + %getC = function_ref @getC : $@convention(thin) () -> (@owned C) + %c = apply %getC() : $@convention(thin) () -> (@owned C) + store %c to %c_addr + strong_retain %box + strong_release %box + br infinite_loop +} + +// CHECK-LABEL: sil @alloc_box_in_deadend_loop_with_another_deadend +// CHECK: br [[LOOP:bb[0-9]+]] +// CHECK: [[LOOP]]: +// CHECK: alloc_stack +// CHECK: cond_br undef, [[BACKEDGE:bb[0-9]+]], [[DIE:bb[0-9]+]] +// CHECK: [[BACKEDGE]]: +// CHECK: dealloc_stack +// CHECK: br [[LOOP]] +// CHECK: [[DIE]]: +// CHECK-NOT: dealloc_stack +// CHECK-LABEL: } // end sil function 'alloc_box_in_deadend_loop_with_another_deadend' +sil @alloc_box_in_deadend_loop_with_another_deadend : $@convention(thin) () -> () { +entry: + br loop + +loop: + %box = alloc_box ${ var C } + cond_br undef, backedge, die + +backedge: + strong_release %box + br loop + +die: + strong_retain %box + strong_release %box + unreachable +} diff --git a/test/SILOptimizer/allocbox_to_stack_ownership.sil b/test/SILOptimizer/allocbox_to_stack_ownership.sil index d0758779f0685..3aa046df52537 100644 --- a/test/SILOptimizer/allocbox_to_stack_ownership.sil +++ b/test/SILOptimizer/allocbox_to_stack_ownership.sil @@ -15,6 +15,8 @@ struct Bool { protocol Error {} +class C {} + // CHECK-LABEL: sil [ossa] @simple_promotion sil [ossa] @simple_promotion : $@convention(thin) (Int) -> Int { bb0(%0 : $Int): @@ -1006,7 +1008,6 @@ bb2: // CHECK: bb1: // CHECK-NEXT: [[STACK2:%[0-9]+]] = alloc_stack $Bool // CHECK-NEXT: dealloc_stack [[STACK2]] -// CHECK-NEXT: dealloc_stack [[BOX]] // CHECK-NEXT: unreachable // CHECK: bb2: // CHECK: store @@ -1231,3 +1232,206 @@ bb0(%0 : @guaranteed ${ var Int }): %r = tuple () return %r } + +sil @getC : $@convention(thin) () -> (@owned C) + +// CHECK-LABEL: sil [ossa] @leak_to_inf_loop_1 : {{.*}} { +// CHECK-NOT: destroy_addr +// CHECK-NOT: dealloc_stack +// CHECK-LABEL: } // end sil function 'leak_to_inf_loop_1' +sil [ossa] @leak_to_inf_loop_1 : $@convention(thin) (@owned C) -> () { +entry(%c : @owned $C): + %box = alloc_box ${ var C } + %c_addr = project_box %box, 0 + store %c to [init] %c_addr + %copy = copy_value %box + destroy_value %box + br loop + +loop: + %getC = function_ref @getC : $@convention(thin) () -> (@owned C) + %c2 = apply %getC() : $@convention(thin) () -> (@owned C) + %c_old = load [take] %c_addr + store %c2 to [init] %c_addr + destroy_value %c_old + br loop +} + +// CHECK-LABEL: sil [ossa] @leak_to_inf_loop_2 : {{.*}} { +// CHECK: cond_br undef, [[EXIT:bb[0-9]+]], [[TO_LOOP:bb[0-9]+]] +// CHECK: [[EXIT]] +// CHECK: destroy_addr +// CHECK: dealloc_stack +// CHECK: [[TO_LOOP]] +// CHECK-NOT: destroy_addr +// CHECK-NOT: dealloc_stack +// CHECK-LABEL: } // end sil function 'leak_to_inf_loop_2' +sil [ossa] @leak_to_inf_loop_2 : $@convention(thin) (@owned C) -> () { +entry(%c : @owned $C): + %box = alloc_box ${ var C } + %c_addr = project_box %box, 0 + store %c to [init] %c_addr + %copy = copy_value %box + destroy_value %box + cond_br undef, exit, to_loop + +exit: + destroy_value %copy + return undef : $() + +to_loop: + %copy2 = copy_value %copy + destroy_value %copy + br loop + +loop: + %getC = function_ref @getC : $@convention(thin) () -> (@owned C) + %c2 = apply %getC() : $@convention(thin) () -> (@owned C) + %c_old = load [take] %c_addr + store %c2 to [init] %c_addr + destroy_value %c_old + br loop +} + +// CHECK-LABEL: sil [ossa] @leak_to_inf_loop_3 : {{.*}} { +// CHECK-NOT: destroy_addr +// CHECK-NOT: dealloc_stack +// CHECK-LABEL: } // end sil function 'leak_to_inf_loop_3' +sil [ossa] @leak_to_inf_loop_3 : $@convention(thin) (@owned C) -> () { +entry(%c : @owned $C): + %box = alloc_box ${ var C } + %c_addr = project_box %box, 0 + store %c to [init] %c_addr + br to_loop + +to_loop: + %copy = copy_value %box + destroy_value %box + br loop + +loop: + %getC = function_ref @getC : $@convention(thin) () -> (@owned C) + %c2 = apply %getC() : $@convention(thin) () -> (@owned C) + %c_old = load [take] %c_addr + store %c2 to [init] %c_addr + destroy_value %c_old + br loop +} + +// CHECK-LABEL: sil [ossa] @dealloc_box_before_loop +// CHECK: dealloc_stack +// CHECK-LABEL: } // end sil function 'dealloc_box_before_loop' +sil [ossa] @dealloc_box_before_loop : $@convention(thin) (@owned C) -> () { +entry(%c : @owned $C): + %box = alloc_box ${ var C } + %c_addr = project_box %box, 0 + store %c to [init] %c_addr + cond_br undef, exit, to_loop + +exit: + destroy_value %box + return undef : $() + +to_loop: + dealloc_box %box + br loop + +loop: + br loop +} + +// CHECK-LABEL: sil [ossa] @alloc_box_in_deadend_loop +// CHECK: alloc_stack +// CHECK: dealloc_stack +// CHECK-LABEL: } // end sil function 'alloc_box_in_deadend_loop' +sil [ossa] @alloc_box_in_deadend_loop : $@convention(thin) () -> () { +entry: + br loop + +loop: + %box = alloc_box ${ var C } + %c_addr = project_box %box, 0 + %getC = function_ref @getC : $@convention(thin) () -> (@owned C) + %c = apply %getC() : $@convention(thin) () -> (@owned C) + store %c to [init] %c_addr + destroy_value %box + br loop +} + +// CHECK-LABEL: sil [ossa] @alloc_box_in_exiting_loop +// CHECK: br [[EXITING_LOOP:bb[0-9]+]] +// CHECK: [[EXITING_LOOP]]: +// CHECK: alloc_stack +// CHECK: cond_br undef, [[EXIT:bb[0-9]+]], [[LATCH:bb[0-9]+]] +// CHECK: [[LATCH]]: +// CHECK: cond_br undef, [[BACKEDGE:bb[0-9]+]], [[INFINITE_LOOP:bb[0-9]+]] +// CHECK: [[BACKEDGE]]: +// CHECK: dealloc_stack +// CHECK: br [[EXITING_LOOP]] +// CHECK: [[EXIT]]: +// CHECK: dealloc_stack +// CHECK: [[INFINITE_LOOP]]: +// CHECK-NOT: dealloc_stack +// CHECK-LABEL: } // end sil function 'alloc_box_in_exiting_loop' +sil [ossa] @alloc_box_in_exiting_loop : $@convention(thin) () -> () { +entry: + br exiting_loop + +exiting_loop: + %box = alloc_box ${ var C } + %c_addr = project_box %box, 0 + %getC = function_ref @getC : $@convention(thin) () -> (@owned C) + %c = apply %getC() : $@convention(thin) () -> (@owned C) + store %c to [init] %c_addr + cond_br undef, exit, latch + +latch: + cond_br undef, backedge, to_infinite_loop + +backedge: + destroy_value %box + br exiting_loop + +exit: + destroy_value %box + return undef : $() + +to_infinite_loop: + br infinite_loop + +infinite_loop: + %c2 = apply %getC() : $@convention(thin) () -> (@owned C) + store %c2 to [assign] %c_addr + %copy = copy_value %box + destroy_value %copy + br infinite_loop +} + +// CHECK-LABEL: sil [ossa] @alloc_box_in_deadend_loop_with_another_deadend +// CHECK: br [[LOOP:bb[0-9]+]] +// CHECK: [[LOOP]]: +// CHECK: alloc_stack +// CHECK: cond_br undef, [[BACKEDGE:bb[0-9]+]], [[DIE:bb[0-9]+]] +// CHECK: [[BACKEDGE]]: +// CHECK: dealloc_stack +// CHECK: br [[LOOP]] +// CHECK: [[DIE]]: +// CHECK-NOT: dealloc_stack +// CHECK-LABEL: } // end sil function 'alloc_box_in_deadend_loop_with_another_deadend' +sil [ossa] @alloc_box_in_deadend_loop_with_another_deadend : $@convention(thin) () -> () { +entry: + br loop + +loop: + %box = alloc_box ${ var C } + cond_br undef, backedge, die + +backedge: + destroy_value %box + br loop + +die: + %copy = copy_value %box + destroy_value %copy + unreachable +} From a241cf5e5994ea98a878c76df517b19d169f4d40 Mon Sep 17 00:00:00 2001 From: Nate Chandler Date: Wed, 27 Aug 2025 18:03:53 -0700 Subject: [PATCH 4/4] [MemoryLifetimeVerifier] Permit leaks in dead-ends --- include/swift/SIL/SILFunction.h | 3 +- lib/SIL/Verifier/MemoryLifetimeVerifier.cpp | 31 +++++++++++++-------- lib/SIL/Verifier/SILVerifier.cpp | 2 +- test/SIL/memory_lifetime.sil | 8 ++++++ 4 files changed, 31 insertions(+), 13 deletions(-) diff --git a/include/swift/SIL/SILFunction.h b/include/swift/SIL/SILFunction.h index 1728baf431a54..21a504bc36050 100644 --- a/include/swift/SIL/SILFunction.h +++ b/include/swift/SIL/SILFunction.h @@ -1692,7 +1692,8 @@ class SILFunction } /// Verifies the lifetime of memory locations in the function. - void verifyMemoryLifetime(CalleeCache *calleeCache); + void verifyMemoryLifetime(CalleeCache *calleeCache, + DeadEndBlocks *deadEndBlocks); /// Verifies ownership of the function. /// Since we don't have complete lifetimes everywhere, computes DeadEndBlocks diff --git a/lib/SIL/Verifier/MemoryLifetimeVerifier.cpp b/lib/SIL/Verifier/MemoryLifetimeVerifier.cpp index aa79c1be31d76..0ee5cd00f3d5b 100644 --- a/lib/SIL/Verifier/MemoryLifetimeVerifier.cpp +++ b/lib/SIL/Verifier/MemoryLifetimeVerifier.cpp @@ -12,13 +12,14 @@ #define DEBUG_TYPE "sil-memory-lifetime-verifier" #include "swift/Basic/Assertions.h" -#include "swift/SIL/MemoryLocations.h" +#include "swift/SIL/ApplySite.h" +#include "swift/SIL/BasicBlockDatastructures.h" +#include "swift/SIL/BasicBlockUtils.h" #include "swift/SIL/BitDataflow.h" #include "swift/SIL/CalleeCache.h" +#include "swift/SIL/MemoryLocations.h" #include "swift/SIL/SILBasicBlock.h" #include "swift/SIL/SILFunction.h" -#include "swift/SIL/ApplySite.h" -#include "swift/SIL/BasicBlockDatastructures.h" #include "llvm/Support/CommandLine.h" using namespace swift; @@ -43,6 +44,7 @@ class MemoryLifetimeVerifier { SILFunction *function; CalleeCache *calleeCache; + DeadEndBlocks *deadEndBlocks; MemoryLocations locations; /// alloc_stack memory locations which are used for store_borrow. @@ -140,11 +142,12 @@ class MemoryLifetimeVerifier { } public: - MemoryLifetimeVerifier(SILFunction *function, CalleeCache *calleeCache) : - function(function), - calleeCache(calleeCache), - locations(/*handleNonTrivialProjections*/ true, - /*handleTrivialLocations*/ true) {} + MemoryLifetimeVerifier(SILFunction *function, CalleeCache *calleeCache, + DeadEndBlocks *deadEndBlocks) + : function(function), calleeCache(calleeCache), + deadEndBlocks(deadEndBlocks), + locations(/*handleNonTrivialProjections*/ true, + /*handleTrivialLocations*/ true) {} /// The main entry point to verify the lifetime of all memory locations in /// the function. @@ -884,7 +887,12 @@ void MemoryLifetimeVerifier::checkBlock(SILBasicBlock *block, Bits &bits) { } case SILInstructionKind::DeallocStackInst: { SILValue opVal = cast(&I)->getOperand(); - requireBitsClear(bits & nonTrivialLocations, opVal, &I); + if (!deadEndBlocks->isDeadEnd(I.getParent())) { + // TODO: rdar://159311784: Maybe at some point the invariant will be + // enforced that values stored into addresses + // don't leak in dead-ends. + requireBitsClear(bits & nonTrivialLocations, opVal, &I); + } // Needed to clear any bits of trivial locations (which are not required // to be zero). locations.clearBits(bits, opVal); @@ -974,7 +982,8 @@ void MemoryLifetimeVerifier::verify() { } // anonymous namespace -void SILFunction::verifyMemoryLifetime(CalleeCache *calleeCache) { - MemoryLifetimeVerifier verifier(this, calleeCache); +void SILFunction::verifyMemoryLifetime(CalleeCache *calleeCache, + DeadEndBlocks *deadEndBlocks) { + MemoryLifetimeVerifier verifier(this, calleeCache, deadEndBlocks); verifier.verify(); } diff --git a/lib/SIL/Verifier/SILVerifier.cpp b/lib/SIL/Verifier/SILVerifier.cpp index a3a4504930a2f..fd757fd08997b 100644 --- a/lib/SIL/Verifier/SILVerifier.cpp +++ b/lib/SIL/Verifier/SILVerifier.cpp @@ -7385,7 +7385,7 @@ class SILVerifier : public SILVerifierBase { if (F->hasOwnership() && F->shouldVerifyOwnership() && !mod.getASTContext().hadError()) { - F->verifyMemoryLifetime(calleeCache); + F->verifyMemoryLifetime(calleeCache, &getDeadEndBlocks()); } } diff --git a/test/SIL/memory_lifetime.sil b/test/SIL/memory_lifetime.sil index 76c02a3538db9..406d77ad8958e 100644 --- a/test/SIL/memory_lifetime.sil +++ b/test/SIL/memory_lifetime.sil @@ -864,3 +864,11 @@ bb0: %10 = tuple () return %10 } + +sil [ossa] @storage_leaked_into_dead_ends : $@convention(thin) () -> () { + %t = apply undef() : $@convention(thin) () -> (@owned T) + %t_addr = alloc_stack $T + store %t to [init] %t_addr + dealloc_stack %t_addr + unreachable +}