From 29229a9410d99296e1d07e714acc15f45de9fb36 Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Tue, 18 Nov 2025 14:27:43 -0800 Subject: [PATCH 1/5] [sil] Add basic SIL support for alloc_stack [non_nested]. This means I just added the flag and added support for cloning/printing/serializing the bit on alloc_stack. --- include/swift/SIL/SILCloner.h | 1 + include/swift/SIL/SILInstruction.h | 45 ++++++++++++++++++++++++++++ include/swift/SIL/SILNode.h | 3 +- lib/SIL/IR/SILInstruction.cpp | 21 +++++++++++++ lib/SIL/IR/SILInstructions.cpp | 1 + lib/SIL/IR/SILPrinter.cpp | 10 +++++++ lib/SIL/Parser/ParseSIL.cpp | 17 +++++++---- lib/Serialization/DeserializeSIL.cpp | 5 +++- lib/Serialization/ModuleFormat.h | 2 +- lib/Serialization/SILFormat.h | 2 +- lib/Serialization/SerializeSIL.cpp | 1 + test/SIL/Parser/basic2.sil | 11 +++++++ test/SIL/Serialization/basic2.sil | 11 +++++++ 13 files changed, 120 insertions(+), 10 deletions(-) diff --git a/include/swift/SIL/SILCloner.h b/include/swift/SIL/SILCloner.h index 29029b1634e1d..9144c582f6855 100644 --- a/include/swift/SIL/SILCloner.h +++ b/include/swift/SIL/SILCloner.h @@ -1062,6 +1062,7 @@ SILCloner::visitAllocStackInst(AllocStackInst *Inst) { true #endif ); + NewInst->setStackAllocationIsNested(Inst->isStackAllocationNested()); recordClonedInstruction(Inst, NewInst); } diff --git a/include/swift/SIL/SILInstruction.h b/include/swift/SIL/SILInstruction.h index ff26cc48a88c5..a24c08d0e3fd9 100644 --- a/include/swift/SIL/SILInstruction.h +++ b/include/swift/SIL/SILInstruction.h @@ -141,6 +141,24 @@ class SILPrintContext; template class SILClonerWithScopes; +/// An enum that describes whether or not a stack allocation may violate the +/// stack discipline of swift. +/// +/// DISCUSSION: In swift, we require that all allocations on the stack be +/// strictly allocated in a LIFO order... that is the last stack allocation +/// created must be the first stack allocation destroyed. In some cases, we +/// cannot guarantee that behavior. In such cases we may need to use other +/// strategies that do not involve stack memory (e.x.: using heap memory +/// although we do not require heap memory to be used). +enum StackAllocationIsNested_t : bool { + /// The instruction may not obey the LIFO rule of stack allocation. + StackAllocationIsNotNested = false, + + /// The instruction obeys the LIFO rule of stack allocation and can allocate + /// memory on the stack normally. + StackAllocationIsNested = true, +}; + enum class MemoryBehavior { None, /// The instruction may read memory. @@ -858,6 +876,15 @@ class SILInstruction : public llvm::ilist_node { /// The stack allocation produced by the instruction, if any. SILValue getStackAllocation() const; + /// Returns the kind of stack memory that should be allocated. There are + /// certain (unfortunate) situations in which "stack" allocations may become + /// unnested and must use alternative allocation strategies. Rather than + /// requiring all of these to explicitly use heap allocation, which may be + /// to be significantly less efficient (e.g. ) + StackAllocationIsNested_t isStackAllocationNested() const; + + void setStackAllocationIsNested(StackAllocationIsNested_t isNested); + /// Returns true if this is the deallocation of a stack allocating instruction. /// The first operand must be the allocating instruction. bool isDeallocatingStack() const; @@ -2055,6 +2082,24 @@ class AllocStackInst final } } + StackAllocationIsNested_t isStackAllocationNested() const { + if (sharedUInt8().AllocStackInst.isNested) { + return StackAllocationIsNested; + } + return StackAllocationIsNotNested; + } + + void setStackAllocationIsNested(StackAllocationIsNested_t isNested) { + switch (isNested) { + case StackAllocationIsNotNested: + sharedUInt8().AllocStackInst.isNested = false; + break; + case StackAllocationIsNested: + sharedUInt8().AllocStackInst.isNested = true; + break; + } + } + void markUsesMoveableValueDebugInfo() { sharedUInt8().AllocStackInst.usesMoveableValueDebugInfo = (bool)UsesMoveableValueDebugInfo; diff --git a/include/swift/SIL/SILNode.h b/include/swift/SIL/SILNode.h index ca4220382463a..8cf66fbf9902c 100644 --- a/include/swift/SIL/SILNode.h +++ b/include/swift/SIL/SILNode.h @@ -229,7 +229,8 @@ class alignas(8) SILNode : lexical : 1, fromVarDecl : 1, usesMoveableValueDebugInfo : 1, - hasInvalidatedVarInfo : 1); + hasInvalidatedVarInfo : 1, + isNested : 1); SHARED_FIELD(AllocBoxInst, uint8_t dynamicLifetime : 1, diff --git a/lib/SIL/IR/SILInstruction.cpp b/lib/SIL/IR/SILInstruction.cpp index 1708f92a56918..a2db53c4c6d0d 100644 --- a/lib/SIL/IR/SILInstruction.cpp +++ b/lib/SIL/IR/SILInstruction.cpp @@ -1339,6 +1339,27 @@ SILValue SILInstruction::getStackAllocation() const { return cast(this); } +StackAllocationIsNested_t SILInstruction::isStackAllocationNested() const { + assert(isAllocatingStack()); + if (auto ASI = dyn_cast(this)) { + return ASI->isStackAllocationNested(); + } else { + // TODO: implement for all remaining allocations + return StackAllocationIsNested; + } +} + +void SILInstruction::setStackAllocationIsNested( + StackAllocationIsNested_t nested) { + assert(isAllocatingStack()); + if (auto ASI = dyn_cast(this)) { + assert(nested == StackAllocationIsNested); + ASI->setStackAllocationIsNested(nested); + } else if (!nested) { + llvm_unreachable("unimplemented"); + } +} + bool SILInstruction::isDeallocatingStack() const { // NOTE: If you're adding a new kind of deallocating instruction, // there are several places scattered around the SIL optimizer which diff --git a/lib/SIL/IR/SILInstructions.cpp b/lib/SIL/IR/SILInstructions.cpp index 3c959a16d6d80..f6abf477dbcfb 100644 --- a/lib/SIL/IR/SILInstructions.cpp +++ b/lib/SIL/IR/SILInstructions.cpp @@ -231,6 +231,7 @@ AllocStackInst::AllocStackInst( sharedUInt8().AllocStackInst.fromVarDecl = (bool)isFromVarDecl; sharedUInt8().AllocStackInst.usesMoveableValueDebugInfo = (bool)usesMoveableValueDebugInfo || elementType.isMoveOnly(); + setStackAllocationIsNested(StackAllocationIsNested); sharedUInt32().AllocStackInst.numOperands = TypeDependentOperands.size(); // VarInfo must be initialized after diff --git a/lib/SIL/IR/SILPrinter.cpp b/lib/SIL/IR/SILPrinter.cpp index 6e2b4fa6a82bc..233fa65d59f64 100644 --- a/lib/SIL/IR/SILPrinter.cpp +++ b/lib/SIL/IR/SILPrinter.cpp @@ -1568,7 +1568,17 @@ class SILPrinter : public SILInstructionVisitor { printDebugInfoExpression(Var->DIExpr); } + template + void printNonNested(T *inst) { + static_assert(&T::isStackAllocationNested != + &SILInstruction::isStackAllocationNested, + "Type doesn't override isStackAllocationNested"); + if (inst->isStackAllocationNested() == StackAllocationIsNotNested) + *this << "[non_nested] "; + } + void visitAllocStackInst(AllocStackInst *AVI) { + printNonNested(AVI); if (AVI->hasDynamicLifetime()) *this << "[dynamic_lifetime] "; if (AVI->isLexical()) diff --git a/lib/SIL/Parser/ParseSIL.cpp b/lib/SIL/Parser/ParseSIL.cpp index 8a265528714f7..f17b2675690aa 100644 --- a/lib/SIL/Parser/ParseSIL.cpp +++ b/lib/SIL/Parser/ParseSIL.cpp @@ -4994,6 +4994,7 @@ bool SILParser::parseSpecificSILInstruction(SILBuilder &B, auto isFromVarDecl = IsNotFromVarDecl; UsesMoveableValueDebugInfo_t usesMoveableValueDebugInfo = DoesNotUseMoveableValueDebugInfo; + auto isNested = StackAllocationIsNested; StringRef attributeName; SourceLoc attributeLoc; @@ -5006,6 +5007,8 @@ bool SILParser::parseSpecificSILInstruction(SILBuilder &B, isFromVarDecl = IsFromVarDecl; else if (attributeName == "moveable_value_debuginfo") usesMoveableValueDebugInfo = UsesMoveableValueDebugInfo; + else if (attributeName == "non_nested") + isNested = StackAllocationIsNotNested; else { P.diagnose(attributeLoc, diag::sil_invalid_attribute_for_instruction, attributeName, "alloc_stack"); @@ -5025,14 +5028,16 @@ bool SILParser::parseSpecificSILInstruction(SILBuilder &B, usesMoveableValueDebugInfo = UsesMoveableValueDebugInfo; // It doesn't make sense to attach a debug var info if the name is empty + AllocStackInst *ASI; if (VarInfo.Name.size()) - ResultVal = B.createAllocStack(InstLoc, Ty, VarInfo, hasDynamicLifetime, - isLexical, isFromVarDecl, - usesMoveableValueDebugInfo); + ASI = B.createAllocStack(InstLoc, Ty, VarInfo, hasDynamicLifetime, + isLexical, isFromVarDecl, + usesMoveableValueDebugInfo); else - ResultVal = - B.createAllocStack(InstLoc, Ty, {}, hasDynamicLifetime, isLexical, - isFromVarDecl, usesMoveableValueDebugInfo); + ASI = B.createAllocStack(InstLoc, Ty, {}, hasDynamicLifetime, isLexical, + isFromVarDecl, usesMoveableValueDebugInfo); + ASI->setStackAllocationIsNested(isNested); + ResultVal = ASI; break; } case SILInstructionKind::MetatypeInst: { diff --git a/lib/Serialization/DeserializeSIL.cpp b/lib/Serialization/DeserializeSIL.cpp index 430cdb0ff6c2a..eeefad61ed033 100644 --- a/lib/Serialization/DeserializeSIL.cpp +++ b/lib/Serialization/DeserializeSIL.cpp @@ -1818,9 +1818,12 @@ bool SILDeserializer::readSILInstruction(SILFunction *Fn, auto isLexical = IsLexical_t((Attr >> 1) & 0x1); auto isFromVarDecl = IsFromVarDecl_t((Attr >> 2) & 0x1); auto wasMoved = UsesMoveableValueDebugInfo_t((Attr >> 3) & 0x1); - ResultInst = Builder.createAllocStack( + auto isNested = StackAllocationIsNested_t((Attr >> 4) & 0x1); + auto ASI = Builder.createAllocStack( Loc, getSILType(MF->getType(TyID), (SILValueCategory)TyCategory, Fn), std::nullopt, hasDynamicLifetime, isLexical, isFromVarDecl, wasMoved); + ASI->setStackAllocationIsNested(isNested); + ResultInst = ASI; break; } case SILInstructionKind::AllocPackInst: { diff --git a/lib/Serialization/ModuleFormat.h b/lib/Serialization/ModuleFormat.h index 1e8ac35904c87..e22f2c9d507f0 100644 --- a/lib/Serialization/ModuleFormat.h +++ b/lib/Serialization/ModuleFormat.h @@ -58,7 +58,7 @@ const uint16_t SWIFTMODULE_VERSION_MAJOR = 0; /// describe what change you made. The content of this comment isn't important; /// it just ensures a conflict if two people change the module format. /// Don't worry about adhering to the 80-column limit for this line. -const uint16_t SWIFTMODULE_VERSION_MINOR = 975; // Lazy OpaqueTypeDecl underlying type substitutions +const uint16_t SWIFTMODULE_VERSION_MINOR = 976; // Expand one type layout /// A standard hash seed used for all string hashes in a serialized module. /// diff --git a/lib/Serialization/SILFormat.h b/lib/Serialization/SILFormat.h index 3cafe4e785a27..36e379d182335 100644 --- a/lib/Serialization/SILFormat.h +++ b/lib/Serialization/SILFormat.h @@ -542,7 +542,7 @@ namespace sil_block { // SIL instructions with one type. (alloc_stack) using SILOneTypeLayout = BCRecordLayout, // Optional attributes + BCFixed<5>, // Optional attributes TypeIDField, SILTypeCategoryField>; // SIL instructions with one typed valueref. (dealloc_stack, return) diff --git a/lib/Serialization/SerializeSIL.cpp b/lib/Serialization/SerializeSIL.cpp index 931e84ecae88e..c285a40304611 100644 --- a/lib/Serialization/SerializeSIL.cpp +++ b/lib/Serialization/SerializeSIL.cpp @@ -1339,6 +1339,7 @@ void SILSerializer::writeSILInstruction(const SILInstruction &SI) { attr |= unsigned(ASI->isLexical()) << 1; attr |= unsigned(ASI->isFromVarDecl()) << 2; attr |= unsigned(ASI->usesMoveableValueDebugInfo()) << 3; + attr |= unsigned(ASI->isStackAllocationNested()) << 4; writeOneTypeLayout(ASI->getKind(), attr, ASI->getElementType()); break; } diff --git a/test/SIL/Parser/basic2.sil b/test/SIL/Parser/basic2.sil index 1a87c8280a933..833beb6c32073 100644 --- a/test/SIL/Parser/basic2.sil +++ b/test/SIL/Parser/basic2.sil @@ -494,3 +494,14 @@ bb0(%0 : @guaranteed $Klass): %9999 = tuple () return %9999 : $() } + +// CHECK-LABEL: sil [ossa] @alloc_stack_nonnested : $@convention(thin) () -> () { +// CHECK: alloc_stack [non_nested] $Klass +// CHECK: } // end sil function 'alloc_stack_nonnested' +sil [ossa] @alloc_stack_nonnested : $@convention(thin) () -> () { +bb0: + %0 = alloc_stack [non_nested] $Klass + dealloc_stack %0 + %9999 = tuple () + return %9999 : $() +} \ No newline at end of file diff --git a/test/SIL/Serialization/basic2.sil b/test/SIL/Serialization/basic2.sil index 53f13dfb6ec8f..8746bad59ddc8 100644 --- a/test/SIL/Serialization/basic2.sil +++ b/test/SIL/Serialization/basic2.sil @@ -13,6 +13,17 @@ class Klass {} sil @getC : $@convention(thin) () -> (@owned C) sil @getKlass : $@convention(thin) () -> (@owned Klass) +// CHECK-LABEL: sil [ossa] @alloc_stack_nonnested : $@convention(thin) () -> () { +// CHECK: alloc_stack [non_nested] $Klass +// CHECK: } // end sil function 'alloc_stack_nonnested' +sil [ossa] @alloc_stack_nonnested : $@convention(thin) () -> () { +bb0: + %0 = alloc_stack [non_nested] $Klass + dealloc_stack %0 + %9999 = tuple () + return %9999 : $() +} + // CHECK-LABEL: sil [ossa] @ignored_use_test : $@convention(thin) (@guaranteed Klass) -> () { // CHECK: ignored_use %0 : $Klass // CHECK: } // end sil function 'ignored_use_test' From 682ef268d2d0e340f28a2617e94293d5ff203949 Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Tue, 18 Nov 2025 14:28:42 -0800 Subject: [PATCH 2/5] [optimizer] Teach SIL optimizer that stack nesting should ignore nested stack allocations. --- lib/SIL/Verifier/SILVerifier.cpp | 55 ++++++++++++++++--------- lib/SILOptimizer/Utils/StackNesting.cpp | 20 ++++++--- test/SILOptimizer/stack-nesting.sil | 28 +++++++++++++ 3 files changed, 78 insertions(+), 25 deletions(-) diff --git a/lib/SIL/Verifier/SILVerifier.cpp b/lib/SIL/Verifier/SILVerifier.cpp index f454cbcf64196..5d30bfcf5becb 100644 --- a/lib/SIL/Verifier/SILVerifier.cpp +++ b/lib/SIL/Verifier/SILVerifier.cpp @@ -7052,6 +7052,22 @@ class SILVerifier : public SILVerifierBase { } } } + + void handleScopeInst(SILInstruction &i) { + if (!ActiveOps.insert(&i).second) { + verificationFailure("operation was not ended before re-beginning it", + &i, nullptr, nullptr); + } + } + + void handleScopeEndingInst(SILInstruction &i) { + if (auto beginOp = i.getOperand(0)->getDefiningInstruction()) { + if (!ActiveOps.erase(beginOp)) { + verificationFailure("operation has already been ended", &i, nullptr, + nullptr); + } + } + } }; struct DeadEndRegionState { @@ -7152,22 +7168,31 @@ class SILVerifier : public SILVerifierBase { } if (i.isAllocatingStack()) { - if (auto *BAI = dyn_cast(&i)) { - state.Stack.push_back(BAI->getCalleeAllocationResult()); + // Nested stack allocations are pushed on the stack. Non-nested stack + // allocations are treated as active ops so that we can at least + // verify their joint post-dominance. + if (i.isStackAllocationNested()) { + state.Stack.push_back(i.getStackAllocation()); + + // Also track begin_apply as a scope instruction. + if (isa(i)) + state.handleScopeInst(i); } else { - state.Stack.push_back(cast(&i)); + state.handleScopeInst(i); } - // Not "else if": begin_apply both allocates stack and begins an - // operation. - } - if (i.isDeallocatingStack()) { + } else if (i.isDeallocatingStack()) { SILValue op = i.getOperand(0); while (auto *mvi = dyn_cast(op)) { op = mvi->getOperand(); } - if (!state.Stack.empty() && op == state.Stack.back()) { + auto beginInst = op->getDefiningInstruction(); + if (beginInst && !beginInst->isStackAllocationNested()) { + state.handleScopeEndingInst(i); + } else if (!state.Stack.empty() && op == state.Stack.back()) { state.Stack.pop_back(); + if (beginInst && isa(beginInst)) + state.handleScopeEndingInst(i); } else { verificationFailure("deallocating allocation that is not the top of the stack", &i, nullptr, @@ -7178,20 +7203,12 @@ class SILVerifier : public SILVerifierBase { }); } } else if (isScopeInst(&i)) { - bool notAlreadyPresent = state.ActiveOps.insert(&i).second; - require(notAlreadyPresent, - "operation was not ended before re-beginning it"); + state.handleScopeInst(i); } else if (isScopeEndingInst(&i)) { - if (auto beginOp = i.getOperand(0)->getDefiningInstruction()) { - bool present = state.ActiveOps.erase(beginOp); - require(present, "operation has already been ended"); - } + state.handleScopeEndingInst(i); } else if (auto *endBorrow = dyn_cast(&i)) { if (isa(endBorrow->getOperand())) { - if (auto beginOp = i.getOperand(0)->getDefiningInstruction()) { - bool present = state.ActiveOps.erase(beginOp); - require(present, "operation has already been ended"); - } + state.handleScopeEndingInst(i); } } else if (auto gaci = dyn_cast(&i)) { require(!state.GotAsyncContinuation, diff --git a/lib/SILOptimizer/Utils/StackNesting.cpp b/lib/SILOptimizer/Utils/StackNesting.cpp index 050a5d61f8385..d6feb7589333e 100644 --- a/lib/SILOptimizer/Utils/StackNesting.cpp +++ b/lib/SILOptimizer/Utils/StackNesting.cpp @@ -579,7 +579,9 @@ StackNesting::Changes StackNesting::fixNesting(SILFunction *F) { // In the formal presentation, the state change is // state = STATE_PUSH(state, alloc) if (I->isAllocatingStack()) { - state.allocations.push_back(I); + // Only handle nested stack allocations. + if (I->isStackAllocationNested() == StackAllocationIsNested) + state.allocations.push_back(I); continue; } @@ -594,6 +596,11 @@ StackNesting::Changes StackNesting::fixNesting(SILFunction *F) { SILInstruction *dealloc = I; SILInstruction *alloc = getAllocForDealloc(dealloc); + // Since we only processed nested allocations above, ignore deallocations + // from non-nested allocations. + if (alloc->isStackAllocationNested() == StackAllocationIsNotNested) + continue; + #ifndef NDEBUG if (state.allocations.empty()) { state.abortForUnknownAllocation(alloc, dealloc); @@ -657,9 +664,10 @@ StackNesting::Changes StackNesting::fixNesting(SILFunction *F) { } namespace swift::test { -static FunctionTest MyNewTest("stack_nesting_fixup", - [](auto &function, auto &arguments, auto &test) { - StackNesting::fixNesting(&function); - function.print(llvm::outs()); - }); +static FunctionTest StackNestingTests("stack_nesting_fixup", + [](auto &function, auto &arguments, + auto &test) { + StackNesting::fixNesting(&function); + function.print(llvm::outs()); + }); } // end namespace swift::test diff --git a/test/SILOptimizer/stack-nesting.sil b/test/SILOptimizer/stack-nesting.sil index fb74577068316..264109f859cf7 100644 --- a/test/SILOptimizer/stack-nesting.sil +++ b/test/SILOptimizer/stack-nesting.sil @@ -281,3 +281,31 @@ bb5: dealloc_stack %d br bb4 } + +// We only re-order b and c. We leave a alone. +// CHECK-LABEL: sil [ossa] @test_simple_non_nested +// CHECK: integer_literal $Builtin.Int32, 1 +// CHECK-NEXT: [[A:%.*]] = alloc_stack [non_nested] $Int +// CHECK-NEXT: [[B:%.*]] = alloc_stack $Int +// CHECK-NEXT: [[C:%.*]] = alloc_stack $Int +// CHECK-NEXT: integer_literal $Builtin.Int32, 2 +// CHECK-NEXT: dealloc_stack [[A]] +// CHECK-NEXT: dealloc_stack [[C]] +// CHECK-NEXT: dealloc_stack [[B]] +// CHECK-NEXT: integer_literal $Builtin.Int32, 3 +// CHECK-LABEL: // end sil function 'test_simple_non_nested' +sil [ossa] @test_simple_non_nested : $@convention(thin) (Builtin.Int1) -> () { +bb0(%cond: $Builtin.Int1): + specify_test "stack_nesting_fixup" + integer_literal $Builtin.Int32, 1 + %a = alloc_stack [non_nested] $Int + %b = alloc_stack $Int + %c = alloc_stack $Int + integer_literal $Builtin.Int32, 2 + dealloc_stack %a + dealloc_stack %b + dealloc_stack %c + integer_literal $Builtin.Int32, 3 + %ret = tuple () + return %ret : $() +} From b6dfb30f1ef6e671c5e3029ef653b2b6aeba6f70 Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Mon, 17 Nov 2025 11:32:56 -0800 Subject: [PATCH 3/5] [irgen] Add support for alloc_stack [non_nested]. --- lib/IRGen/FixedTypeInfo.h | 8 ++++++-- lib/IRGen/GenArray.cpp | 16 +++++++++------- lib/IRGen/GenInit.cpp | 8 +++++--- lib/IRGen/GenOpaque.cpp | 32 ++++++++++++++++++++++++++++++++ lib/IRGen/GenType.cpp | 9 +++++---- lib/IRGen/IRGenFunction.h | 7 +++++++ lib/IRGen/IRGenSIL.cpp | 26 ++++++++++++++++++++------ lib/IRGen/NonFixedTypeInfo.h | 13 ++++++++----- lib/IRGen/TypeInfo.h | 11 +++++++---- 9 files changed, 99 insertions(+), 31 deletions(-) diff --git a/lib/IRGen/FixedTypeInfo.h b/lib/IRGen/FixedTypeInfo.h index bad9f031b9f29..b5cd5a88adf36 100644 --- a/lib/IRGen/FixedTypeInfo.h +++ b/lib/IRGen/FixedTypeInfo.h @@ -82,8 +82,12 @@ class FixedTypeInfo : public TypeInfo { } StackAddress allocateStack(IRGenFunction &IGF, SILType T, - const llvm::Twine &name) const override; - void deallocateStack(IRGenFunction &IGF, StackAddress addr, SILType T) const override; + const llvm::Twine &name, + StackAllocationIsNested_t isNested = + StackAllocationIsNested) const override; + void deallocateStack(IRGenFunction &IGF, StackAddress addr, SILType T, + StackAllocationIsNested_t isNested = + StackAllocationIsNested) const override; void destroyStack(IRGenFunction &IGF, StackAddress addr, SILType T, bool isOutlined) const override; diff --git a/lib/IRGen/GenArray.cpp b/lib/IRGen/GenArray.cpp index b95766f74e5e7..2859bf7c785ba 100644 --- a/lib/IRGen/GenArray.cpp +++ b/lib/IRGen/GenArray.cpp @@ -638,24 +638,26 @@ class NonFixedArrayTypeInfo final return nullptr; } - StackAddress allocateStack(IRGenFunction &IGF, SILType T, - const llvm::Twine &name) const override { + StackAddress + allocateStack(IRGenFunction &IGF, SILType T, const llvm::Twine &name, + StackAllocationIsNested_t isNested) const override { // Allocate memory on the stack. - auto alloca = IGF.emitDynamicAlloca(T, name); + auto alloca = IGF.emitDynamicStackAllocation(T, isNested, name); IGF.Builder.CreateLifetimeStart(alloca.getAddressPointer()); return alloca.withAddress(getAddressForPointer(alloca.getAddressPointer())); } - void deallocateStack(IRGenFunction &IGF, StackAddress stackAddress, - SILType T) const override { + void deallocateStack(IRGenFunction &IGF, StackAddress stackAddress, SILType T, + StackAllocationIsNested_t isNested) const override { IGF.Builder.CreateLifetimeEnd(stackAddress.getAddress().getAddress()); - IGF.emitDeallocateDynamicAlloca(stackAddress); + IGF.emitDynamicStackDeallocation(stackAddress, isNested); } void destroyStack(IRGenFunction &IGF, StackAddress stackAddress, SILType T, bool isOutlined) const override { emitDestroyCall(IGF, T, stackAddress.getAddress()); - deallocateStack(IGF, stackAddress, T); + // For now, just always do nested. + deallocateStack(IGF, stackAddress, T, StackAllocationIsNested); } TypeLayoutEntry * diff --git a/lib/IRGen/GenInit.cpp b/lib/IRGen/GenInit.cpp index 0e6756a3ea736..86a4aec5be917 100644 --- a/lib/IRGen/GenInit.cpp +++ b/lib/IRGen/GenInit.cpp @@ -59,8 +59,9 @@ void IRGenModule::emitSILGlobalVariable(SILGlobalVariable *var) { var->isDefinition() ? ForDefinition : NotForDefinition); } -StackAddress FixedTypeInfo::allocateStack(IRGenFunction &IGF, SILType T, - const Twine &name) const { +StackAddress +FixedTypeInfo::allocateStack(IRGenFunction &IGF, SILType T, const Twine &name, + StackAllocationIsNested_t isNested) const { // If the type is known to be empty, don't actually allocate anything. if (isKnownEmpty(ResilienceExpansion::Maximal)) { auto addr = getUndefAddress(); @@ -81,7 +82,8 @@ void FixedTypeInfo::destroyStack(IRGenFunction &IGF, StackAddress addr, } void FixedTypeInfo::deallocateStack(IRGenFunction &IGF, StackAddress addr, - SILType T) const { + SILType T, + StackAllocationIsNested_t isNested) const { if (isKnownEmpty(ResilienceExpansion::Maximal)) return; IGF.Builder.CreateLifetimeEnd(addr.getAddress(), getFixedSize()); diff --git a/lib/IRGen/GenOpaque.cpp b/lib/IRGen/GenOpaque.cpp index 1ef51b261667c..6c068e00ffae0 100644 --- a/lib/IRGen/GenOpaque.cpp +++ b/lib/IRGen/GenOpaque.cpp @@ -553,6 +553,38 @@ irgen::emitInitializeBufferWithCopyOfBufferCall(IRGenFunction &IGF, return call; } +StackAddress IRGenFunction::emitDynamicStackAllocation( + SILType T, StackAllocationIsNested_t isNested, const llvm::Twine &name) { + if (isNested) { + return emitDynamicAlloca(T, name); + } + + // First malloc the memory. + auto mallocFn = IGM.getMallocFunctionPointer(); + auto *size = emitLoadOfSize(*this, T); + auto *call = Builder.CreateCall(mallocFn, {size}); + call->setDoesNotThrow(); + call->setCallingConv(IGM.C_CC); + + // Then create the dynamic alloca to store the malloc pointer into. + + // TODO: Alignment should be platform specific. + auto address = Address(call, IGM.Int8Ty, Alignment(16)); + return StackAddress{address}; +} + +void IRGenFunction::emitDynamicStackDeallocation( + StackAddress address, StackAllocationIsNested_t isNested) { + if (isNested) { + return emitDeallocateDynamicAlloca(address); + } + + auto *call = Builder.CreateCall(IGM.getFreeFunctionPointer(), + {address.getAddressPointer()}); + call->setDoesNotThrow(); + call->setCallingConv(IGM.C_CC); +} + /// Emit a dynamic alloca call to allocate enough memory to hold an object of /// type 'T' and an optional llvm.stackrestore point if 'isInEntryBlock' is /// false. diff --git a/lib/IRGen/GenType.cpp b/lib/IRGen/GenType.cpp index ad46b6e20bdc3..262b9d370edeb 100644 --- a/lib/IRGen/GenType.cpp +++ b/lib/IRGen/GenType.cpp @@ -1373,12 +1373,13 @@ namespace { llvm::Constant *getStaticStride(IRGenModule &IGM) const override { return nullptr; } - StackAddress allocateStack(IRGenFunction &IGF, SILType T, - const llvm::Twine &name) const override { + StackAddress + allocateStack(IRGenFunction &IGF, SILType T, const llvm::Twine &name, + StackAllocationIsNested_t isNested) const override { llvm_unreachable("should not call on an immovable opaque type"); } - void deallocateStack(IRGenFunction &IGF, StackAddress addr, - SILType T) const override { + void deallocateStack(IRGenFunction &IGF, StackAddress addr, SILType T, + StackAllocationIsNested_t isNested) const override { llvm_unreachable("should not call on an immovable opaque type"); } void destroyStack(IRGenFunction &IGF, StackAddress addr, SILType T, diff --git a/lib/IRGen/IRGenFunction.h b/lib/IRGen/IRGenFunction.h index c25fb901e8b77..b72ad37dbc8d8 100644 --- a/lib/IRGen/IRGenFunction.h +++ b/lib/IRGen/IRGenFunction.h @@ -25,6 +25,7 @@ #include "swift/AST/ReferenceCounting.h" #include "swift/AST/Type.h" #include "swift/Basic/LLVM.h" +#include "swift/SIL/SILInstruction.h" #include "swift/SIL/SILLocation.h" #include "swift/SIL/SILType.h" #include "llvm/ADT/DenseMap.h" @@ -317,6 +318,12 @@ class IRGenFunction { bool useTaskDeallocThrough = false, bool forCalleeCoroutineFrame = false); + StackAddress emitDynamicStackAllocation(SILType type, + StackAllocationIsNested_t isNested, + const llvm::Twine &name = ""); + void emitDynamicStackDeallocation(StackAddress address, + StackAllocationIsNested_t isNested); + llvm::BasicBlock *createBasicBlock(const llvm::Twine &Name); const TypeInfo &getTypeInfoForUnlowered(Type subst); const TypeInfo &getTypeInfoForUnlowered(AbstractionPattern orig, Type subst); diff --git a/lib/IRGen/IRGenSIL.cpp b/lib/IRGen/IRGenSIL.cpp index c1dabec314a35..2cb8810179052 100644 --- a/lib/IRGen/IRGenSIL.cpp +++ b/lib/IRGen/IRGenSIL.cpp @@ -882,6 +882,14 @@ class IRGenSILFunction : return isTaskAlloc; } + static bool isCallToMalloc(llvm::Value *val) { + auto *call = dyn_cast(val); + if (!call) + return false; + auto *callee = call->getCalledFunction(); + return callee && callee->getName() == "malloc"; + } + static bool isTaskAlloc(llvm::Value *Storage) { while (Storage) { if (auto *LdInst = dyn_cast(Storage)) @@ -6518,7 +6526,8 @@ void IRGenSILFunction::emitDebugInfoAfterAllocStack(AllocStackInst *i, // At this point addr must be an alloca or an undef. assert(isa(addr) || isa(addr) || - isa(addr) || isCallToSwiftTaskAlloc(addr)); + isa(addr) || isCallToSwiftTaskAlloc(addr) || + isCallToMalloc(addr)); auto Indirection = DirectValue; if (InCoroContext(*CurSILFn, *i)) @@ -6575,7 +6584,8 @@ void IRGenSILFunction::visitAllocStackInst(swift::AllocStackInst *i) { DebugTypeInfo DbgTy; emitDebugInfoBeforeAllocStack(i, type, DbgTy); - auto stackAddr = type.allocateStack(*this, i->getElementType(), dbgname); + auto stackAddr = type.allocateStack(*this, i->getElementType(), dbgname, + i->isStackAllocationNested()); setLoweredStackAddress(i, stackAddr); Address addr = stackAddr.getAddress(); @@ -6668,23 +6678,27 @@ void IRGenSILFunction::visitDeallocStackInst(swift::DeallocStackInst *i) { if (auto *closure = dyn_cast(i->getOperand())) { assert(closure->isOnStack()); auto stackAddr = LoweredPartialApplyAllocations[i->getOperand()]; - emitDeallocateDynamicAlloca(stackAddr); + emitDeallocateDynamicAlloca(stackAddr, closure->isStackAllocationNested()); return; } if (isaResultOf(i->getOperand())) { auto *mvi = getAsResultOf(i->getOperand()); auto *bai = cast(mvi->getParent()); + // FIXME: [non_nested] const auto &coroutine = getLoweredCoroutine(bai->getTokenResult()); emitDeallocYieldOnce2CoroutineFrame(*this, coroutine.getCalleeAllocatedFrame()); return; } - auto allocatedType = i->getOperand()->getType(); + auto *asi = cast(i->getOperand()); + + auto allocatedType = asi->getType(); const TypeInfo &allocatedTI = getTypeInfo(allocatedType); - StackAddress stackAddr = getLoweredStackAddress(i->getOperand()); + StackAddress stackAddr = getLoweredStackAddress(asi); + auto isNested = asi->isStackAllocationNested(); - allocatedTI.deallocateStack(*this, stackAddr, allocatedType); + allocatedTI.deallocateStack(*this, stackAddr, allocatedType, isNested); } void IRGenSILFunction::visitDeallocStackRefInst(DeallocStackRefInst *i) { diff --git a/lib/IRGen/NonFixedTypeInfo.h b/lib/IRGen/NonFixedTypeInfo.h index 50a00e5ccdf07..697edff012313 100644 --- a/lib/IRGen/NonFixedTypeInfo.h +++ b/lib/IRGen/NonFixedTypeInfo.h @@ -63,18 +63,21 @@ class WitnessSizedTypeInfo : public IndirectTypeInfo { static bool isFixed() { return false; } StackAddress allocateStack(IRGenFunction &IGF, SILType T, - const llvm::Twine &name) const override { + const llvm::Twine &name, + StackAllocationIsNested_t isNested = + StackAllocationIsNested) const override { // Allocate memory on the stack. - auto alloca = IGF.emitDynamicAlloca(T, name); + auto alloca = IGF.emitDynamicStackAllocation(T, isNested, name); IGF.Builder.CreateLifetimeStart(alloca.getAddressPointer()); return alloca.withAddress( getAsBitCastAddress(IGF, alloca.getAddressPointer())); } - void deallocateStack(IRGenFunction &IGF, StackAddress stackAddress, - SILType T) const override { + void deallocateStack(IRGenFunction &IGF, StackAddress stackAddress, SILType T, + StackAllocationIsNested_t isNested = + StackAllocationIsNested) const override { IGF.Builder.CreateLifetimeEnd(stackAddress.getAddress().getAddress()); - IGF.emitDeallocateDynamicAlloca(stackAddress); + IGF.emitDynamicStackDeallocation(stackAddress, isNested); } void destroyStack(IRGenFunction &IGF, StackAddress stackAddress, SILType T, diff --git a/lib/IRGen/TypeInfo.h b/lib/IRGen/TypeInfo.h index 91a7c7bdd3d1b..733598177b4af 100644 --- a/lib/IRGen/TypeInfo.h +++ b/lib/IRGen/TypeInfo.h @@ -28,6 +28,7 @@ #include "IRGen.h" #include "Outlining.h" #include "swift/AST/ReferenceCounting.h" +#include "swift/SIL/SILInstruction.h" #include "llvm/ADT/MapVector.h" namespace llvm { @@ -354,12 +355,14 @@ class TypeInfo { bool useStructLayouts) const = 0; /// Allocate a variable of this type on the stack. - virtual StackAddress allocateStack(IRGenFunction &IGF, SILType T, - const llvm::Twine &name) const = 0; + virtual StackAddress allocateStack( + IRGenFunction &IGF, SILType T, const llvm::Twine &name, + StackAllocationIsNested_t isNested = StackAllocationIsNested) const = 0; /// Deallocate a variable of this type. - virtual void deallocateStack(IRGenFunction &IGF, StackAddress addr, - SILType T) const = 0; + virtual void deallocateStack( + IRGenFunction &IGF, StackAddress addr, SILType T, + StackAllocationIsNested_t isNested = StackAllocationIsNested) const = 0; /// Destroy the value of a variable of this type, then deallocate its /// memory. From c876c1ee88bf7d4d0b2733cd096281348d5925d2 Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Thu, 20 Nov 2025 13:03:18 -0800 Subject: [PATCH 4/5] [sil-verifier] Split SILVerifier verificationFailure into separate functions for Instructions, Arguments, and add a variant for Values. This also required me to change how we handled which instruction/argument we emit an error about in the verifier. Previously we were using two global variables that we made nullptr to control which thing we emitted an error about. This was unnecessary. Instead I added a little helper struct that internally controls what we will emit an error about and an external "guard" RAII struct that makes sure we push/pop the instruction/argument we are erroring upon correctly. --- .../Sources/SIL/Utilities/Verifier.swift | 2 +- include/swift/SIL/SILBridging.h | 6 +- include/swift/SIL/SILModule.h | 16 +- lib/SIL/Utils/SILBridging.cpp | 17 +- lib/SIL/Verifier/SILVerifier.cpp | 280 +++++++++++++++--- test/SIL/verifier_failures.sil | 23 +- test/SIL/verifier_loweredsil_failures.sil | 4 +- test/SILOptimizer/cyclic_entry.sil | 3 +- 8 files changed, 291 insertions(+), 60 deletions(-) diff --git a/SwiftCompilerSources/Sources/SIL/Utilities/Verifier.swift b/SwiftCompilerSources/Sources/SIL/Utilities/Verifier.swift index 438a1f285d6f6..a12e0f5943f2f 100644 --- a/SwiftCompilerSources/Sources/SIL/Utilities/Verifier.swift +++ b/SwiftCompilerSources/Sources/SIL/Utilities/Verifier.swift @@ -22,7 +22,7 @@ private func require(_ condition: Bool, _ message: @autoclosure () -> String, at if !condition { let msg = message() msg._withBridgedStringRef { stringRef in - BridgedVerifier.verifierError(stringRef, atInstruction.bridged, Optional.none.bridged) + BridgedVerifier.verifierError(stringRef, atInstruction.bridged) } } } diff --git a/include/swift/SIL/SILBridging.h b/include/swift/SIL/SILBridging.h index 4cb70f7a563c7..18ca2b046c95a 100644 --- a/include/swift/SIL/SILBridging.h +++ b/include/swift/SIL/SILBridging.h @@ -1627,8 +1627,12 @@ struct BridgedVerifier { static void runSwiftFunctionVerification(swift::SILFunction * _Nonnull f, swift::SILContext * _Nonnull context); static void registerVerifier(VerifyFunctionFn verifyFunctionFn); - static void verifierError(BridgedStringRef message, OptionalBridgedInstruction atInstruction, + static void verifierError(BridgedStringRef message, + OptionalBridgedInstruction atInstruction); + static void verifierError(BridgedStringRef message, OptionalBridgedArgument atArgument); + static void verifierError(BridgedStringRef message, + OptionalBridgedValue atValue); }; struct BridgedUtilities { diff --git a/include/swift/SIL/SILModule.h b/include/swift/SIL/SILModule.h index 4f1dfbe55b192..78d0d2b9cc67c 100644 --- a/include/swift/SIL/SILModule.h +++ b/include/swift/SIL/SILModule.h @@ -1161,10 +1161,18 @@ inline llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const SILModule &M){ return OS; } -void verificationFailure(const Twine &complaint, - const SILInstruction *atInstruction, - const SILArgument *atArgument, - llvm::function_ref extraContext); +void verificationFailure( + const Twine &complaint, const SILFunction *fn, + llvm::function_ref extraContext); +void verificationFailure( + const Twine &complaint, const SILInstruction *atInstruction, + llvm::function_ref extraContext); +void verificationFailure( + const Twine &complaint, const SILArgument *atArgument, + llvm::function_ref extraContext); +void verificationFailure( + const Twine &complaint, SILValue atValue, + llvm::function_ref extraContext); inline bool SILOptions::supportsLexicalLifetimes(const SILModule &mod) const { switch (mod.getStage()) { diff --git a/lib/SIL/Utils/SILBridging.cpp b/lib/SIL/Utils/SILBridging.cpp index b4f9c97941ae9..e798f91d4a78c 100644 --- a/lib/SIL/Utils/SILBridging.cpp +++ b/lib/SIL/Utils/SILBridging.cpp @@ -918,8 +918,19 @@ void BridgedVerifier::runSwiftFunctionVerification(SILFunction * _Nonnull f, SIL } void BridgedVerifier::verifierError(BridgedStringRef message, - OptionalBridgedInstruction atInstruction, + OptionalBridgedInstruction atInstruction) { + verificationFailure(message.unbridged(), atInstruction.unbridged(), + /*extraContext=*/nullptr); +} + +void BridgedVerifier::verifierError(BridgedStringRef message, OptionalBridgedArgument atArgument) { - Twine msg(message.unbridged()); - verificationFailure(msg, atInstruction.unbridged(), atArgument.unbridged(), /*extraContext=*/nullptr); + verificationFailure(message.unbridged(), atArgument.unbridged(), + /*extraContext=*/nullptr); +} + +void BridgedVerifier::verifierError(BridgedStringRef message, + OptionalBridgedValue atValue) { + verificationFailure(message.unbridged(), SILValue(atValue.getSILValue()), + /*extraContext=*/nullptr); } diff --git a/lib/SIL/Verifier/SILVerifier.cpp b/lib/SIL/Verifier/SILVerifier.cpp index 5d30bfcf5becb..b033639b2eb33 100644 --- a/lib/SIL/Verifier/SILVerifier.cpp +++ b/lib/SIL/Verifier/SILVerifier.cpp @@ -95,22 +95,56 @@ static llvm::cl::opt AllowCriticalEdges("allow-critical-edges", llvm::cl::init(true)); extern llvm::cl::opt SILPrintDebugInfo; -void swift::verificationFailure(const Twine &complaint, - const SILInstruction *atInstruction, - const SILArgument *atArgument, - llvm::function_ref extraContext) { +void swift::verificationFailure( + const Twine &complaint, const SILInstruction *atInstruction, + llvm::function_ref extraContext) { llvm::raw_ostream &out = llvm::dbgs(); SILPrintContext ctx(out); - const SILFunction *f = nullptr; - StringRef funcName = "?"; - if (atInstruction) { - f = atInstruction->getFunction(); - funcName = f->getName(); - } else if (atArgument) { - f = atArgument->getFunction(); - funcName = f->getName(); + auto *f = atInstruction->getFunction(); + StringRef funcName = f->getName(); + if (ContinueOnFailure) { + out << "Begin Error in function " << funcName << "\n"; } + + out << "SIL verification failed: " << complaint << "\n"; + if (extraContext) + extraContext(ctx); + + out << "Verifying instruction:\n"; + atInstruction->printInContext(ctx); + if (ContinueOnFailure) { + out << "End Error in function " << funcName << "\n"; + return; + } + + if (f) { + out << "In function:\n"; + f->print(out); + if (DumpModuleOnFailure) { + // Don't do this by default because modules can be _very_ large. + out << "In module:\n"; + f->getModule().print(out); + } + } + + // We abort by default because we want to always crash in + // the debugger. + if (AbortOnFailure) + abort(); + else + exit(1); +} + +void swift::verificationFailure( + const Twine &complaint, const SILArgument *atArgument, + llvm::function_ref extraContext) { + llvm::raw_ostream &out = llvm::dbgs(); + SILPrintContext ctx(out); + + auto *f = atArgument->getFunction(); + StringRef funcName = f->getName(); + if (ContinueOnFailure) { out << "Begin Error in function " << funcName << "\n"; } @@ -119,13 +153,51 @@ void swift::verificationFailure(const Twine &complaint, if (extraContext) extraContext(ctx); - if (atInstruction) { - out << "Verifying instruction:\n"; - atInstruction->printInContext(ctx); - } else if (atArgument) { - out << "Verifying argument:\n"; - atArgument->printInContext(ctx); + out << "Verifying argument:\n"; + atArgument->printInContext(ctx); + + if (ContinueOnFailure) { + out << "End Error in function " << funcName << "\n"; + return; + } + + if (f) { + out << "In function:\n"; + f->print(out); + if (DumpModuleOnFailure) { + // Don't do this by default because modules can be _very_ large. + out << "In module:\n"; + f->getModule().print(out); + } + } + + // We abort by default because we want to always crash in + // the debugger. + if (AbortOnFailure) + abort(); + else + exit(1); +} + +void swift::verificationFailure( + const Twine &complaint, SILValue atValue, + llvm::function_ref extraContext) { + llvm::raw_ostream &out = llvm::dbgs(); + SILPrintContext ctx(out); + + const SILFunction *f = atValue->getFunction(); + StringRef funcName = f->getName(); + if (ContinueOnFailure) { + out << "Begin Error in function " << funcName << "\n"; } + + out << "SIL verification failed: " << complaint << "\n"; + if (extraContext) + extraContext(ctx); + + out << "Verifying value:\n"; + atValue->printInContext(ctx); + if (ContinueOnFailure) { out << "End Error in function " << funcName << "\n"; return; @@ -149,6 +221,44 @@ void swift::verificationFailure(const Twine &complaint, exit(1); } +void swift::verificationFailure( + const Twine &complaint, const SILFunction *f, + llvm::function_ref extraContext) { + llvm::raw_ostream &out = llvm::dbgs(); + SILPrintContext ctx(out); + + StringRef funcName = f->getName(); + + if (ContinueOnFailure) { + out << "Begin Error in function " << funcName << "\n"; + } + + out << "SIL verification failed: " << complaint << "\n"; + if (extraContext) + extraContext(ctx); + + if (ContinueOnFailure) { + out << "End Error in function " << funcName << "\n"; + return; + } + + if (f) { + out << "In function:\n"; + f->print(out); + if (DumpModuleOnFailure) { + // Don't do this by default because modules can be _very_ large. + out << "In module:\n"; + f->getModule().print(out); + } + } + + // We abort by default because we want to always crash in + // the debugger. + if (AbortOnFailure) + abort(); + else + exit(1); +} // The verifier is basically all assertions, so don't compile it with NDEBUG to // prevent release builds from triggering spurious unused variable warnings. @@ -258,11 +368,13 @@ class SILVerifierBase : public SILInstructionVisitor { public: // visitCLASS calls visitPARENT and checkCLASS. // checkCLASS does nothing by default. -#define INST(CLASS, PARENT) \ - void visit##CLASS(CLASS *I) { \ - static_cast(this)->visit##PARENT(I); \ - static_cast(this)->check##CLASS(I); \ - } \ +#define INST(CLASS, PARENT) \ + void visit##CLASS(CLASS *I) { \ + typename Impl::VerifierErrorEmitterGuard guard(static_cast(this), \ + I); \ + static_cast(this)->visit##PARENT(I); \ + static_cast(this)->check##CLASS(I); \ + } \ void check##CLASS(CLASS *I) {} #include "swift/SIL/SILNodes.def" @@ -924,8 +1036,79 @@ class SILVerifier : public SILVerifierBase { bool checkLinearLifetime = false; SmallVector, 16> DebugVars; - const SILInstruction *CurInstruction = nullptr; - const SILArgument *CurArgument = nullptr; + +public: + class VerifierErrorEmitter { + std::optional> + value; + + public: + class VerifierErrorEmitterGuard { + SILVerifier *verifier; + + VerifierErrorEmitter &getEmitter() const { + return verifier->ErrorEmitter; + } + + public: + VerifierErrorEmitterGuard(SILVerifier *verifier, + const SILInstruction *inst) + : verifier(verifier) { + assert(!bool(getEmitter().value) && + "Cannot emit errors for two different " + "constructs at the same time?!"); + getEmitter().value = inst; + } + + VerifierErrorEmitterGuard(SILVerifier *verifier, const SILFunction *f) + : verifier(verifier) { + assert(!bool(getEmitter().value) && + "Cannot emit errors for two different " + "constructs at the same time?!"); + getEmitter().value = f; + } + + VerifierErrorEmitterGuard(SILVerifier *verifier, const SILArgument *arg) + : verifier(verifier) { + assert(!bool(getEmitter().value) && + "Cannot emit errors for two different " + "constructs at the same time?!"); + getEmitter().value = arg; + } + ~VerifierErrorEmitterGuard() { getEmitter().value = {}; } + }; + + void emitError(const Twine &complaint, + llvm::function_ref extraContext) { + if (!value.has_value()) { + llvm::report_fatal_error("Must have a construct to emit for"); + } + + auto v = *value; + if (std::holds_alternative(v)) { + return verificationFailure( + complaint, std::get(v), extraContext); + } + + if (std::holds_alternative(v)) { + return verificationFailure(complaint, std::get(v), + extraContext); + } + + if (std::holds_alternative(v)) { + return verificationFailure(complaint, std::get(v), + extraContext); + } + + llvm::report_fatal_error("Unhandled case?!"); + } + }; + using VerifierErrorEmitterGuard = + VerifierErrorEmitter::VerifierErrorEmitterGuard; + +private: + VerifierErrorEmitter ErrorEmitter; std::unique_ptr Dominance; // Used for dominance checking within a basic block. @@ -983,7 +1166,7 @@ class SILVerifier : public SILVerifierBase { = nullptr) { if (condition) return; - verificationFailure(complaint, CurInstruction, CurArgument, extraContext); + ErrorEmitter.emitError(complaint, extraContext); } #define require(condition, complaint) \ _require(bool(condition), complaint ": " #condition) @@ -1292,7 +1475,7 @@ class SILVerifier : public SILVerifierBase { } void visitSILArgument(SILArgument *arg) { - CurArgument = arg; + VerifierErrorEmitterGuard guard(this, arg); checkLegalType(arg->getFunction(), arg, nullptr); if (checkLinearLifetime) { @@ -1327,7 +1510,6 @@ class SILVerifier : public SILVerifierBase { } void visitSILInstruction(SILInstruction *I) { - CurInstruction = I; checkSILInstruction(I); // Check the SILLLocation attached to the instruction, @@ -6595,6 +6777,7 @@ class SILVerifier : public SILVerifierBase { // This verifies that the entry block of a SIL function doesn't have // any predecessors and also verifies the entry point arguments. void verifyEntryBlock(SILBasicBlock *entry) { + VerifierErrorEmitterGuard guard(this, entry->getParent()); require(entry->pred_empty(), "entry block cannot have predecessors"); LLVM_DEBUG( @@ -6834,6 +7017,7 @@ class SILVerifier : public SILVerifierBase { } void verifyEpilogBlocks(SILFunction *F) { + VerifierErrorEmitterGuard guard(this, F); bool FoundReturnBlock = false; bool FoundThrowBlock = false; bool FoundUnwindBlock = false; @@ -6990,13 +7174,13 @@ class SILVerifier : public SILVerifierBase { // conservative merge regardless of failure so that we're in a // coherent state in the successor block. auto fail = [&](StringRef complaint, - llvm::function_ref extra - = nullptr) { - verificationFailure(complaint, term, nullptr, - [&](SILPrintContext &ctx) { + llvm::function_ref extra = + nullptr) { + verificationFailure(complaint, term, [&](SILPrintContext &ctx) { ctx.OS() << "Entering basic block " << ctx.getID(succBB) << "\n"; - if (extra) extra(ctx); + if (extra) + extra(ctx); }); }; @@ -7056,14 +7240,14 @@ class SILVerifier : public SILVerifierBase { void handleScopeInst(SILInstruction &i) { if (!ActiveOps.insert(&i).second) { verificationFailure("operation was not ended before re-beginning it", - &i, nullptr, nullptr); + &i, nullptr); } } void handleScopeEndingInst(SILInstruction &i) { if (auto beginOp = i.getOperand(0)->getDefiningInstruction()) { if (!ActiveOps.erase(beginOp)) { - verificationFailure("operation has already been ended", &i, nullptr, + verificationFailure("operation has already been ended", &i, nullptr); } } @@ -7151,7 +7335,7 @@ class SILVerifier : public SILVerifierBase { BBState state = visitedBBs[BB]; for (SILInstruction &i : *BB) { - CurInstruction = &i; + VerifierErrorEmitterGuard guard(this, &i); if (i.maySuspend()) { // Instructions that may suspend an async context must not happen @@ -7187,20 +7371,22 @@ class SILVerifier : public SILVerifierBase { } auto beginInst = op->getDefiningInstruction(); - if (beginInst && !beginInst->isStackAllocationNested()) { + if (beginInst && (!beginInst->isAllocatingStack() || + !beginInst->isStackAllocationNested())) { state.handleScopeEndingInst(i); } else if (!state.Stack.empty() && op == state.Stack.back()) { state.Stack.pop_back(); if (beginInst && isa(beginInst)) state.handleScopeEndingInst(i); } else { - verificationFailure("deallocating allocation that is not the top of the stack", - &i, nullptr, - [&](SILPrintContext &ctx) { - state.printStack(ctx, "Current stack state"); - ctx.OS() << "Stack allocation:\n" << *op; - // The deallocation is printed out as the focus of the failure. - }); + verificationFailure( + "deallocating allocation that is not the top of the stack", &i, + [&](SILPrintContext &ctx) { + state.printStack(ctx, "Current stack state"); + ctx.OS() << "Stack allocation:\n" << *op; + // The deallocation is printed out as the focus of the + // failure. + }); } } else if (isScopeInst(&i)) { state.handleScopeInst(i); @@ -7409,7 +7595,7 @@ class SILVerifier : public SILVerifierBase { for (auto &bb : *F) { const TermInst *termInst = bb.getTerminator(); - CurInstruction = termInst; + VerifierErrorEmitterGuard guard(this, termInst); if (isSILOwnershipEnabled() && F->hasOwnership()) { requireNonCriticalSucc(termInst, "critical edges not allowed in OSSA"); @@ -7423,6 +7609,7 @@ class SILVerifier : public SILVerifierBase { /// This pass verifies that there are no hole in debug scopes at -Onone. void verifyDebugScopeHoles(SILBasicBlock *BB) { + VerifierErrorEmitterGuard guard(this, BB->getParent()); if (!VerifyDIHoles) return; @@ -7539,7 +7726,6 @@ class SILVerifier : public SILVerifierBase { } void visitBasicBlockArguments(SILBasicBlock *BB) { - CurInstruction = nullptr; for (auto argI = BB->args_begin(), argEnd = BB->args_end(); argI != argEnd; ++argI) visitSILArgument(*argI); @@ -7629,6 +7815,7 @@ class SILVerifier : public SILVerifierBase { void visitSILFunction(SILFunction *F) { PrettyStackTraceSILFunction stackTrace("verifying", F); + std::optional functionTypeErrorGuard{{this, F}}; CanSILFunctionType FTy = F->getLoweredFunctionType(); verifySILFunctionType(FTy); verifyParentFunctionSILFunctionType(FTy); @@ -7716,6 +7903,7 @@ class SILVerifier : public SILVerifierBase { require(!FTy->isPolymorphic(), "generic function definition must have a generic environment"); } + functionTypeErrorGuard.reset(); // Before verifying the body of the function, validate the SILUndef map to // make sure that all SILUndef in the function's map point at the function diff --git a/test/SIL/verifier_failures.sil b/test/SIL/verifier_failures.sil index 38a8e4ed58159..a82446fac54f5 100644 --- a/test/SIL/verifier_failures.sil +++ b/test/SIL/verifier_failures.sil @@ -1,4 +1,9 @@ -// RUN: %target-sil-opt -emit-sorted-sil -verify-continue-on-failure -o /dev/null %s 2>&1 | %FileCheck %s +// RUN: %target-sil-opt -sil-verify-all=false -enable-sil-verify-all=false -emit-sorted-sil -verify-continue-on-failure -o /dev/null %s 2>&1 | %FileCheck '--implicit-check-not=Begin Error' %s + +// NOTE: We run the verification when we parse. We turn off sil-verify-all so +// that we do not emit the diagnostics a second time after sil-opt ends. We do +// this so that we can use an implicit-check-not line to make sure that +// additional diagnostics are not emitted. // REQUIRES: asserts @@ -68,3 +73,19 @@ failure(%error : @owned $any Error): dealloc_stack %allocation : $*Builtin.SILToken throw %error : $any Error } + +// CHECK-LABEL: Begin Error in function abort_apply_callee_allocated_coro_2 +// CHECK: SIL verification failed: abort_apply of callee-allocated yield-once coroutine!?: !bai->getSubstCalleeType()->isCalleeAllocatedCoroutine() || AI->getFunction()->getASTContext().LangOpts.hasFeature( Feature::CoroutineAccessorsUnwindOnCallerError) +// CHECK: End Error in function abort_apply_callee_allocated_coro_2 +sil [ossa] @abort_apply_callee_allocated_coro_2 : $@convention(thin) () -> (@error any Error) { +entry: + (%value, %token, %allocation) = begin_apply undef() : $@yield_once_2 @convention(thin) () -> @yields @in_guaranteed () + end_apply %token as $() + dealloc_stack %allocation : $*Builtin.SILToken + return undef : $() + +failure(%error : @owned $any Error): + abort_apply %token + dealloc_stack %allocation : $*Builtin.SILToken + throw %error : $any Error +} diff --git a/test/SIL/verifier_loweredsil_failures.sil b/test/SIL/verifier_loweredsil_failures.sil index 55bf8d1490e36..0512cf98035ea 100644 --- a/test/SIL/verifier_loweredsil_failures.sil +++ b/test/SIL/verifier_loweredsil_failures.sil @@ -1,4 +1,4 @@ -// RUN: %target-sil-opt -emit-sorted-sil -verify-continue-on-failure -o /dev/null %s 2>&1 | %FileCheck %s +// RUN: %target-sil-opt -sil-verify-all=false -enable-sil-verify-all=false -emit-sorted-sil -verify-continue-on-failure -o /dev/null %s 2>&1 | %FileCheck %s // REQUIRES: asserts @@ -19,7 +19,7 @@ sil @alloc_pack_metadata_before_tuple : $@convention(thin) () -> () { } // CHECK-LABEL: Begin Error in function dealloc_pack_metadata_with_bad_operand -// CHECK: SIL verification failed: deallocating allocation that is not the top of the stack +// CHECK: SIL verification failed: operation has already been ended // CHECK-LABEL: End Error in function dealloc_pack_metadata_with_bad_operand // CHECK-LABEL: Begin Error in function dealloc_pack_metadata_with_bad_operand // CHECK: SIL verification failed: return with stack allocs that haven't been deallocated diff --git a/test/SILOptimizer/cyclic_entry.sil b/test/SILOptimizer/cyclic_entry.sil index aba5985e6300e..0400e135e1844 100644 --- a/test/SILOptimizer/cyclic_entry.sil +++ b/test/SILOptimizer/cyclic_entry.sil @@ -1,6 +1,5 @@ // RUN: %empty-directory(%t) -// RUN: not --crash %target-sil-opt -enable-sil-verify-all %s 2> %t/err.txt -// RUN: %FileCheck %s < %t/err.txt +// RUN: %target-sil-opt -sil-verify-all=false -enable-sil-verify-all=false -emit-sorted-sil -verify-continue-on-failure %s 2>&1 | %FileCheck %s // REQUIRES: asserts From 94e9166706de87a17362d7a4b97855d4253d6bcf Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Thu, 20 Nov 2025 13:46:43 -0800 Subject: [PATCH 5/5] [sil-verifier] Use the scope result rather than the scope instruction when verifying that we end scope instructions. We do this since we need to be able to handle instructions that may have two different results that independently need their scopes lifetime to be ended. --- lib/SIL/Verifier/SILVerifier.cpp | 60 ++++++++++++++++++-------------- test/SIL/verifier_failures.sil | 15 ++++++++ 2 files changed, 49 insertions(+), 26 deletions(-) diff --git a/lib/SIL/Verifier/SILVerifier.cpp b/lib/SIL/Verifier/SILVerifier.cpp index b033639b2eb33..d5515e6243b2d 100644 --- a/lib/SIL/Verifier/SILVerifier.cpp +++ b/lib/SIL/Verifier/SILVerifier.cpp @@ -7083,8 +7083,8 @@ class SILVerifier : public SILVerifierBase { struct BBState { std::vector Stack; - /// Contents: BeginAccessInst*, BeginApplyInst*. - std::set ActiveOps; + /// Contents: Results of BeginAccessInst*, BeginApplyInst*. + std::set ActiveOps; CFGState CFG = Normal; @@ -7237,19 +7237,16 @@ class SILVerifier : public SILVerifierBase { } } - void handleScopeInst(SILInstruction &i) { - if (!ActiveOps.insert(&i).second) { + void handleScopeInst(SILValue i) { + if (!ActiveOps.insert(i).second) { verificationFailure("operation was not ended before re-beginning it", - &i, nullptr); + i, nullptr); } } - void handleScopeEndingInst(SILInstruction &i) { - if (auto beginOp = i.getOperand(0)->getDefiningInstruction()) { - if (!ActiveOps.erase(beginOp)) { - verificationFailure("operation has already been ended", &i, - nullptr); - } + void handleScopeEndingInst(const SILInstruction &i) { + if (!ActiveOps.erase(i.getOperand(0))) { + verificationFailure("operation has already been ended", &i, nullptr); } } }; @@ -7263,23 +7260,30 @@ class SILVerifier : public SILVerifierBase { }; }; - static bool isScopeInst(SILInstruction *i) { - if (isa(i) || - isa(i) || - isa(i)) { - return true; - } else if (auto bi = dyn_cast(i)) { + /// If this is a scope ending inst, return the result from the instruction + /// that provides the scoped value whose lifetime must be ended by some other + /// scope ending instruction. + static SILValue isScopeInst(SILInstruction *i) { + if (auto *bai = dyn_cast(i)) + return bai; + if (auto *bai = dyn_cast(i)) + return bai->getTokenResult(); + if (auto *sbi = dyn_cast(i)) + return sbi; + + if (auto bi = dyn_cast(i)) { if (auto bk = bi->getBuiltinKind()) { switch (*bk) { case BuiltinValueKind::StartAsyncLetWithLocalBuffer: - return true; + return bi->getResult(0); default: - return false; + return SILValue(); } } } - return false; + + return SILValue(); } static bool isScopeEndingInst(SILInstruction *i) { @@ -7287,7 +7291,9 @@ class SILVerifier : public SILVerifierBase { isa(i) || isa(i)) { return true; - } else if (auto bi = dyn_cast(i)) { + } + + if (auto bi = dyn_cast(i)) { if (auto bk = bi->getBuiltinKind()) { switch (*bk) { case BuiltinValueKind::FinishAsyncLet: @@ -7359,10 +7365,12 @@ class SILVerifier : public SILVerifierBase { state.Stack.push_back(i.getStackAllocation()); // Also track begin_apply as a scope instruction. - if (isa(i)) - state.handleScopeInst(i); + if (auto *bai = dyn_cast(&i)) { + state.handleScopeInst(bai->getStackAllocation()); + state.handleScopeInst(bai->getTokenResult()); + } } else { - state.handleScopeInst(i); + state.handleScopeInst(i.getStackAllocation()); } } else if (i.isDeallocatingStack()) { SILValue op = i.getOperand(0); @@ -7388,8 +7396,8 @@ class SILVerifier : public SILVerifierBase { // failure. }); } - } else if (isScopeInst(&i)) { - state.handleScopeInst(i); + } else if (auto scopeEndingValue = isScopeInst(&i)) { + state.handleScopeInst(scopeEndingValue); } else if (isScopeEndingInst(&i)) { state.handleScopeEndingInst(i); } else if (auto *endBorrow = dyn_cast(&i)) { diff --git a/test/SIL/verifier_failures.sil b/test/SIL/verifier_failures.sil index a82446fac54f5..2453e1c7213e1 100644 --- a/test/SIL/verifier_failures.sil +++ b/test/SIL/verifier_failures.sil @@ -89,3 +89,18 @@ failure(%error : @owned $any Error): dealloc_stack %allocation : $*Builtin.SILToken throw %error : $any Error } + +sil [ossa] @yield_test : $@yield_once @convention(thin) (@inout Builtin.Int32) -> @yields @inout Builtin.Int32 { +bb0(%0 : $*Builtin.Int32): + (%2, %3) = begin_apply undef(%0) : $@yield_once @convention(method) (@inout Builtin.Int32) -> @yields @inout Builtin.Int32 + yield %2, resume bb1, unwind bb2 + +bb1: + %5 = end_apply %3 as $() + %6 = tuple () + return %6 + +bb2: + abort_apply %3 + unwind +}