From d0e519fabf9f75190846badf09dc85cf25bc4c54 Mon Sep 17 00:00:00 2001 From: Jamie <2119834+jamieQ@users.noreply.github.com> Date: Thu, 16 Oct 2025 06:00:22 -0500 Subject: [PATCH 1/3] [SILGen]: ensure DI checks address-only let temporary allocations Previously it was possible to reference uninitialized memory if a closure that was part of a variable binding initializer referenced the name of the uninitialized value. DI was ignoring these cases before because no mark_uninitialized instruction was produced. Instead of trying to skip emitting that instruction sometimes, just always emit it if a temporary allocation is used so DI will catch liveness issues. --- lib/SILGen/SILGenDecl.cpp | 10 +++++----- test/SILOptimizer/definite_init_address_only_let.swift | 10 ++++++++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/SILGen/SILGenDecl.cpp b/lib/SILGen/SILGenDecl.cpp index 65332d83edbe0..fee27bf83ad94 100644 --- a/lib/SILGen/SILGenDecl.cpp +++ b/lib/SILGen/SILGenDecl.cpp @@ -781,7 +781,6 @@ class LetValueInitialization : public Initialization { // There are four cases we need to handle here: parameters, initialized (or // bound) decls, uninitialized ones, and async let declarations. bool needsTemporaryBuffer; - bool isUninitialized = false; assert(!isa(vd) && "should not bind function params on this path"); @@ -790,14 +789,12 @@ class LetValueInitialization : public Initialization { // If this is a let-value without an initializer, then we need a temporary // buffer. DI will make sure it is only assigned to once. needsTemporaryBuffer = true; - isUninitialized = true; } else if (vd->isAsyncLet()) { // If this is an async let, treat it like a let-value without an // initializer. The initializer runs concurrently in a child task, // and value will be initialized at the point the variable in the // async let is used. needsTemporaryBuffer = true; - isUninitialized = true; } else { // If this is a let with an initializer or bound value, we only need a // buffer if the type is address only or is noncopyable. @@ -824,8 +821,11 @@ class LetValueInitialization : public Initialization { address = SGF.emitTemporaryAllocation(vd, lowering->getLoweredType(), DoesNotHaveDynamicLifetime, isLexical, IsFromVarDecl); - if (isUninitialized) - address = SGF.B.createMarkUninitializedVar(vd, address); + + // Ensure DI always checks this to avoid cases where an address-only + // value is referenced in a closure that is part of its initializer. + address = SGF.B.createMarkUninitializedVar(vd, address); + DestroyCleanup = SGF.enterDormantTemporaryCleanup(address, *lowering); SGF.VarLocs[vd] = SILGenFunction::VarLoc(address, SILAccessEnforcement::Unknown); diff --git a/test/SILOptimizer/definite_init_address_only_let.swift b/test/SILOptimizer/definite_init_address_only_let.swift index 3500154159ad5..3ed56dfd9d3d0 100644 --- a/test/SILOptimizer/definite_init_address_only_let.swift +++ b/test/SILOptimizer/definite_init_address_only_let.swift @@ -39,3 +39,13 @@ func quz(a: Bool, t: T) { return } } + +// https://github.com/swiftlang/swift/issues/84909 + +func uninit_closure_reference() { + func passthrough(_ a: () -> Any) -> Any { a() } + + let initMe = passthrough { initMe } + // expected-error @-1 {{constant 'initMe' used before being initialized}} + // expected-note @-2 {{defined here}} +} From 681e226b087b4211d371e049e1e470bb00bf0cd2 Mon Sep 17 00:00:00 2001 From: Jamie <2119834+jamieQ@users.noreply.github.com> Date: Sat, 18 Oct 2025 18:55:19 -0500 Subject: [PATCH 2/3] [WIP]: try to fix things & add many debug logs --- lib/SILGen/SILGenDecl.cpp | 20 ++++- lib/SILGen/SILGenFunction.cpp | 74 +++++++++++++++++++ .../Mandatory/DIMemoryUseCollector.cpp | 21 ++++++ .../definite_init_address_only_let.swift | 21 ++++++ 4 files changed, 134 insertions(+), 2 deletions(-) diff --git a/lib/SILGen/SILGenDecl.cpp b/lib/SILGen/SILGenDecl.cpp index fee27bf83ad94..f853d4c63dcb4 100644 --- a/lib/SILGen/SILGenDecl.cpp +++ b/lib/SILGen/SILGenDecl.cpp @@ -40,6 +40,8 @@ #include "llvm/Support/ErrorHandling.h" #include +#define DEBUG_TYPE "SILGenDecl" + using namespace swift; using namespace Lowering; @@ -781,6 +783,7 @@ class LetValueInitialization : public Initialization { // There are four cases we need to handle here: parameters, initialized (or // bound) decls, uninitialized ones, and async let declarations. bool needsTemporaryBuffer; + bool isUninitialized = false; assert(!isa(vd) && "should not bind function params on this path"); @@ -789,12 +792,14 @@ class LetValueInitialization : public Initialization { // If this is a let-value without an initializer, then we need a temporary // buffer. DI will make sure it is only assigned to once. needsTemporaryBuffer = true; + isUninitialized = true; } else if (vd->isAsyncLet()) { // If this is an async let, treat it like a let-value without an // initializer. The initializer runs concurrently in a child task, // and value will be initialized at the point the variable in the // async let is used. needsTemporaryBuffer = true; + isUninitialized = true; } else { // If this is a let with an initializer or bound value, we only need a // buffer if the type is address only or is noncopyable. @@ -805,6 +810,12 @@ class LetValueInitialization : public Initialization { lowering->getLoweredType().isMoveOnly(/*orWrapped=*/false); } +// auto parentInit = vd->getParentInitializer(); +// if (isa(parentInit)) { +// auto fn = cast(parentInit)->getSemanticFn(); +// isUninitialized = isa(fn); +// } + // Make sure that we have a non-address only type when binding a // @_noImplicitCopy let. if (lowering->isAddressOnly() && vd->isNoImplicitCopy()) { @@ -813,6 +824,10 @@ class LetValueInitialization : public Initialization { } if (needsTemporaryBuffer) { + LLVM_DEBUG({ + llvm::dbgs() << "JQ: need tmp buffer\n"; + vd->dump(llvm::dbgs()); + }); bool lexicalLifetimesEnabled = SGF.getASTContext().SILOpts.supportsLexicalLifetimes(SGF.getModule()); auto lifetime = SGF.F.getLifetime(vd, lowering->getLoweredType()); @@ -824,7 +839,8 @@ class LetValueInitialization : public Initialization { // Ensure DI always checks this to avoid cases where an address-only // value is referenced in a closure that is part of its initializer. - address = SGF.B.createMarkUninitializedVar(vd, address); + if (true || isUninitialized) + address = SGF.B.createMarkUninitializedVar(vd, address); DestroyCleanup = SGF.enterDormantTemporaryCleanup(address, *lowering); SGF.VarLocs[vd] = SILGenFunction::VarLoc(address, @@ -834,7 +850,7 @@ class LetValueInitialization : public Initialization { // inactive until the variable is initialized: if control flow exits the // before the value is bound, we don't want to destroy the value. // - // Cleanups are required for all lexically scoped variables to delimite + // Cleanups are required for all lexically scoped variables to delimit // the variable scope, even if the cleanup does nothing. SGF.Cleanups.pushCleanupInState( CleanupState::Dormant, vd); diff --git a/lib/SILGen/SILGenFunction.cpp b/lib/SILGen/SILGenFunction.cpp index e39ebbfc0b8e5..4596f2bf91bdc 100644 --- a/lib/SILGen/SILGenFunction.cpp +++ b/lib/SILGen/SILGenFunction.cpp @@ -590,6 +590,16 @@ void SILGenFunction::emitCaptures(SILLocation loc, break; } + LLVM_DEBUG({ + llvm::dbgs() << "JQ: emit captures running\n"; + for (auto capture : captureInfo.getCaptures()) { + llvm::dbgs() << " cap: '" << capture.getDecl()->getName() << "'\n"; + } + llvm::dbgs() + << " canGuarantee: '" << canGuarantee << "'\n" + << " capCanEscape: '" << captureCanEscape << "'\n"; + }); + auto expansion = getTypeExpansionContext(); for (auto capture : captureInfo.getCaptures()) { @@ -609,6 +619,7 @@ void SILGenFunction::emitCaptures(SILLocation loc, if (capture.isOpaqueValue() || capture.isPackElement()) { capturedArgs.push_back( emitRValueAsSingleValue(capture.getExpr()).ensurePlusOne(*this, loc)); + LLVM_DEBUG(llvm::dbgs() << "JQ: cap emit early ret: opaque || pack\n"); continue; } @@ -632,6 +643,52 @@ void SILGenFunction::emitCaptures(SILLocation loc, auto valueType = FunctionDC->mapTypeIntoContext( interfaceType->getReferenceStorageReferent()); + LLVM_DEBUG( + { + llvm::dbgs() << "=== DEBUG: VarLocs contents for capture of '" + << vd->getBaseIdentifier() << "' ===\n"; + llvm::dbgs() << "Total entries in VarLocs: " << VarLocs.size() << "\n"; + + for (auto &entry : VarLocs) { + auto *var = entry.first; + auto &loc = entry.second; + + llvm::dbgs() << " - Variable: " << var->getBaseIdentifier() << "\n"; + // llvm::errs() << " Type: " << var->getType() << "\n"; + llvm::dbgs() << " Value type: " << loc.value->getType() << "\n"; + llvm::dbgs() << " Value kind: "; + + if (isa(loc.value)) { + llvm::dbgs() << "SILUndef (UNINITIALIZED)\n"; + } else if (isa(loc.value)) { + llvm::dbgs() << "SILArgument\n"; + } else if (isa(loc.value)) { + llvm::dbgs() << "AllocStackInst\n"; + } else if (isa(loc.value)) { + llvm::dbgs() << "AllocBoxInst\n"; + } else { + llvm::dbgs() << "some other inst\n"; + } + + if (loc.box) { + llvm::dbgs() << " Has box: yes\n"; + } + + llvm::dbgs() << "\n"; + } + + llvm::dbgs() << "Looking for variable: " << vd->getBaseIdentifier() << "\n"; + auto found = VarLocs.find(vd); + if (found == VarLocs.end()) { + llvm::dbgs() << " Result: NOT FOUND in VarLocs\n"; + } else { + llvm::dbgs() << " Result: FOUND in VarLocs\n"; + llvm::dbgs() << " Value is undef: " + << (isa(found->second.value) ? "YES" : "NO") << "\n"; + } + llvm::dbgs() << "===================================\n\n"; + }); + // // If we haven't emitted the captured value yet, we're forming a closure // to a local function before all of its captures have been emitted. Eg, @@ -695,6 +752,12 @@ void SILGenFunction::emitCaptures(SILLocation loc, // expansion context without opaque archetype substitution. auto getAddressValue = [&](SILValue entryValue, bool forceCopy, bool forLValue) -> SILValue { + LLVM_DEBUG({ + llvm::dbgs() << "JQ: get addr value, force copy: " << forceCopy + << ", for lval: " << forLValue << "\n"; + entryValue->getDefiningInstruction()->print(llvm::dbgs()); + }); + if (!SGM.M.useLoweredAddresses() && !forLValue && !isPack) { // In opaque values mode, addresses aren't used except by lvalues. auto &lowering = getTypeLowering(entryValue->getType()); @@ -775,6 +838,11 @@ void SILGenFunction::emitCaptures(SILLocation loc, auto &Entry = found->second; auto val = Entry.value; + LLVM_DEBUG({ + auto capKind = SGM.Types.getDeclCaptureKind(capture, expansion); + llvm::dbgs() << "JQ: cap kind:: " << (unsigned)capKind << "\n"; + }); + switch (SGM.Types.getDeclCaptureKind(capture, expansion)) { case CaptureKind::Constant: { assert(!isPack); @@ -855,6 +923,12 @@ void SILGenFunction::emitCaptures(SILLocation loc, } capturedArgs.push_back(ManagedValue::forOwnedAddressRValue( addr, CleanupHandle::invalid())); + +// LLVM_DEBUG({ +// llvm::dbgs() << "JQ: adding escape to mark for: \n"; +// val->getDefiningInstruction()->print(llvm::dbgs()); +// }); +// escapesToMark.push_back(val); } break; } diff --git a/lib/SILOptimizer/Mandatory/DIMemoryUseCollector.cpp b/lib/SILOptimizer/Mandatory/DIMemoryUseCollector.cpp index 3ca1f6c06661c..1aeffd7e00edc 100644 --- a/lib/SILOptimizer/Mandatory/DIMemoryUseCollector.cpp +++ b/lib/SILOptimizer/Mandatory/DIMemoryUseCollector.cpp @@ -823,6 +823,27 @@ void ElementUseCollector::collectUses(SILValue Pointer, unsigned BaseEltNo) { continue; } + if (false || (isa(User) || + isa(User))) { + LLVM_DEBUG({ + llvm::dbgs() << "JQ: DI found unchecked addr cast:\n" + << Op; + }); + + DIUseKind Kind; + Kind = DIUseKind::Initialization; +// if (Op->getOperandNumber() == 0) +// Kind = DIUseKind::Load; +// else +// Kind = InStructSubElement ? DIUseKind::PartialStore +// : DIUseKind::Initialization; + + // TODO: what about enum sub elt? do we need to even special case things here? + + addElementUses(BaseEltNo, PointeeType, User, Kind); + continue; + } + // Look through mark_unresolved_non_copyable_value. To us, it is not // interesting. if (auto *mmi = dyn_cast(User)) { diff --git a/test/SILOptimizer/definite_init_address_only_let.swift b/test/SILOptimizer/definite_init_address_only_let.swift index 3ed56dfd9d3d0..8b3b001424f12 100644 --- a/test/SILOptimizer/definite_init_address_only_let.swift +++ b/test/SILOptimizer/definite_init_address_only_let.swift @@ -48,4 +48,25 @@ func uninit_closure_reference() { let initMe = passthrough { initMe } // expected-error @-1 {{constant 'initMe' used before being initialized}} // expected-note @-2 {{defined here}} + + let inline = { () -> Any in inline }() + // expected-error @-1 {{constant 'inline' used before being initialized}} + // expected-note @-2 {{defined here}} + + // these should not regress + func castAny(_ a: Any) { + let directUncond = a as! Int + _ = directUncond + + let directCond = a as? Int + _ = directCond + + let twoPhaseUncond: Int + twoPhaseUncond = a as! Int + _ = twoPhaseUncond + + let twoPhasCond: Int? + twoPhasCond = a as? Int + _ = twoPhasCond + } } From e0f16ca9f1b51e79ddff89c65f2bd13e8d94a6e5 Mon Sep 17 00:00:00 2001 From: Jamie <2119834+jamieQ@users.noreply.github.com> Date: Sun, 19 Oct 2025 05:24:57 -0500 Subject: [PATCH 3/3] try fixing from Sema instead --- lib/SILGen/SILGenDecl.cpp | 2 +- .../Mandatory/DIMemoryUseCollector.cpp | 2 +- lib/Sema/TypeCheckCaptures.cpp | 72 +++++++++++++++++++ .../definite_init_address_only_let.swift | 12 ++-- test/Sema/diag_variable_used_in_initial.swift | 33 +++++++++ 5 files changed, 113 insertions(+), 8 deletions(-) diff --git a/lib/SILGen/SILGenDecl.cpp b/lib/SILGen/SILGenDecl.cpp index f853d4c63dcb4..fb544dac416d4 100644 --- a/lib/SILGen/SILGenDecl.cpp +++ b/lib/SILGen/SILGenDecl.cpp @@ -839,7 +839,7 @@ class LetValueInitialization : public Initialization { // Ensure DI always checks this to avoid cases where an address-only // value is referenced in a closure that is part of its initializer. - if (true || isUninitialized) + if (false || isUninitialized) address = SGF.B.createMarkUninitializedVar(vd, address); DestroyCleanup = SGF.enterDormantTemporaryCleanup(address, *lowering); diff --git a/lib/SILOptimizer/Mandatory/DIMemoryUseCollector.cpp b/lib/SILOptimizer/Mandatory/DIMemoryUseCollector.cpp index 1aeffd7e00edc..ed26d0f3b57ee 100644 --- a/lib/SILOptimizer/Mandatory/DIMemoryUseCollector.cpp +++ b/lib/SILOptimizer/Mandatory/DIMemoryUseCollector.cpp @@ -823,7 +823,7 @@ void ElementUseCollector::collectUses(SILValue Pointer, unsigned BaseEltNo) { continue; } - if (false || (isa(User) || + if (false && (isa(User) || isa(User))) { LLVM_DEBUG({ llvm::dbgs() << "JQ: DI found unchecked addr cast:\n" diff --git a/lib/Sema/TypeCheckCaptures.cpp b/lib/Sema/TypeCheckCaptures.cpp index be0054a709067..8ba29616fcd76 100644 --- a/lib/Sema/TypeCheckCaptures.cpp +++ b/lib/Sema/TypeCheckCaptures.cpp @@ -289,6 +289,27 @@ class FindCapturedVars : public ASTWalker { return Action::SkipChildren(PEE); } + bool isExprContainedIn(Expr *inner, Expr *outer) { + class ContainmentChecker : public ASTWalker { + Expr *target; + public: + bool found = false; + ContainmentChecker(Expr *target) : target(target) {} + + PreWalkResult walkToExprPre(Expr *E) override { + if (E == target) { + found = true; + return Action::Stop(); + } + return Action::Continue(E); + } + }; + + ContainmentChecker checker(inner); + outer->walk(checker); + return checker.found; + } + PreWalkResult walkToDeclRefExpr(DeclRefExpr *DRE) { auto *D = DRE->getDecl(); @@ -300,6 +321,41 @@ class FindCapturedVars : public ASTWalker { if (D->getBaseName() == Context.Id_dollarInterpolation) return Action::SkipNode(DRE); + auto isCapturingFromOwnInitializer = [&]() -> bool { +// return false; + + // assume synthesized code is okay? + if (D->isImplicit() || DRE->isImplicit()) + return false; + + auto *var = dyn_cast(D); + if (!var) { + return false; + } + + auto *pbd = var->getParentPatternBinding(); + if (!pbd) { + return false; + } + + unsigned index = pbd->getPatternEntryIndexForVarDecl(var); + auto *initExpr = pbd->getInit(index); + if (!initExpr) { + return false; + } + + auto loc = DRE->getLoc(); + if (!loc.isValid()) + return false; + + auto srcRange = initExpr->getSourceRange(); + if (!srcRange.isValid()) + return false; + +// return isExprContainedIn(DRE, initExpr); + return initExpr->getSourceRange().contains(DRE->getLoc()); + }; + // DC is the DeclContext where D was defined // CurDC is the DeclContext where D was referenced auto DC = D->getDeclContext(); @@ -321,6 +377,14 @@ class FindCapturedVars : public ASTWalker { if (isa(D)) return Action::SkipNode(DRE); + if (isCapturingFromOwnInitializer()) { + Context.Diags.diagnose(DRE->getLoc(), + diag::use_local_before_declaration + ,DRE->getDecl()->createNameRef()); + Context.Diags.diagnose(DRE->getDecl()->getLoc(), diag::default_value_declared_here); + return Action::SkipNode(DRE); + } + // A local reference is not a capture. if (CurDC == DC || isa(CurDC)) return Action::SkipNode(DRE); @@ -389,6 +453,14 @@ class FindCapturedVars : public ASTWalker { if (TmpDC == nullptr) return Action::SkipNode(DRE); +// if (isCapturingFromOwnInitializer()) { +// Context.Diags.diagnose(DRE->getLoc(), +// diag::use_local_before_declaration +// ,DRE->getDecl()->createNameRef()); +// Context.Diags.diagnose(DRE->getDecl()->getLoc(), diag::default_value_declared_here); +// return Action::SkipNode(DRE); +// } + // Only capture var decls at global scope. Other things can be captured // if they are local. if (!isa(D) && !D->isLocalCapture()) diff --git a/test/SILOptimizer/definite_init_address_only_let.swift b/test/SILOptimizer/definite_init_address_only_let.swift index 8b3b001424f12..8f404309f2733 100644 --- a/test/SILOptimizer/definite_init_address_only_let.swift +++ b/test/SILOptimizer/definite_init_address_only_let.swift @@ -45,13 +45,13 @@ func quz(a: Bool, t: T) { func uninit_closure_reference() { func passthrough(_ a: () -> Any) -> Any { a() } - let initMe = passthrough { initMe } - // expected-error @-1 {{constant 'initMe' used before being initialized}} - // expected-note @-2 {{defined here}} + // let initMe = passthrough { initMe } + // ezxpected-error @-1 {{constant 'initMe' used before being initialized}} + // ezxpected-note @-2 {{defined here}} - let inline = { () -> Any in inline }() - // expected-error @-1 {{constant 'inline' used before being initialized}} - // expected-note @-2 {{defined here}} + // let inline = { () -> Any in inline }() + // ezxpected-error @-1 {{constant 'inline' used before being initialized}} + // ezxpected-note @-2 {{defined here}} // these should not regress func castAny(_ a: Any) { diff --git a/test/Sema/diag_variable_used_in_initial.swift b/test/Sema/diag_variable_used_in_initial.swift index b79c798877a13..2e7f4a20a3538 100644 --- a/test/Sema/diag_variable_used_in_initial.swift +++ b/test/Sema/diag_variable_used_in_initial.swift @@ -58,3 +58,36 @@ func localContext() { } } } + +// https://github.com/swiftlang/swift/issues/84909 + +func uninit_closure_reference() { + func passthrough(_ a: () -> Any) -> Any { a() } + + let initMe = passthrough { initMe } + // expected-error @-1 {{use of local variable 'initMe' before its declaration}} + // expected-note @-2 {{default value declared here}} + + let inline = // expected-note {{default value declared here}} + { + () -> Any in + inline // expected-error {{use of local variable 'inline' before its declaration}} + }() + + // these should not regress + func castAny(_ a: Any) { + let directUncond = a as! Int + _ = directUncond + + let directCond = a as? Int + _ = directCond + + let twoPhaseUncond: Int + twoPhaseUncond = a as! Int + _ = twoPhaseUncond + + let twoPhaseCond: Int? + twoPhaseCond = a as? Int + _ = twoPhaseCond + } +}