Skip to content

Commit 9aca727

Browse files
swift-ciswift-ci
authored
Merge pull request #16844 from gottesmm/definite-init-fix
2 parents ccd2171 + 8886368 commit 9aca727

File tree

4 files changed

+195
-64
lines changed

4 files changed

+195
-64
lines changed

‎lib/SIL/SILVerifier.cpp

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1751,16 +1751,34 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
17511751
require(MU->getModule().getStage() == SILStage::Raw,
17521752
"mark_uninitialized instruction can only exist in raw SIL");
17531753
require(Src->getType().isAddress() ||
1754-
Src->getType().getClassOrBoundGenericClass() ||
1755-
Src->getType().getAs<SILBoxType>(),
1754+
Src->getType().getClassOrBoundGenericClass() ||
1755+
Src->getType().getAs<SILBoxType>(),
17561756
"mark_uninitialized must be an address, class, or box type");
17571757
require(Src->getType() == MU->getType(),"operand and result type mismatch");
17581758
// FIXME: When the work to force MUI to be on Allocations/SILArguments
17591759
// complete, turn on this assertion.
1760-
#if 0
1761-
require(isa<AllocationInst>(Src) || isa<SILArgument>(Src),
1762-
"Mark Uninitialized should always be on the storage location");
1763-
#endif
1760+
require(isa<AllocationInst>(Src)
1761+
|| isa<GlobalAddrInst>(Src)
1762+
// TODO: Should we support SILUndef on mark_uninitialized? We
1763+
// currently have a test that verifies this behavior, but it seems
1764+
// like this would always be a bug due to the implications around
1765+
// the code in DI. This just bakes in the current behavior.
1766+
|| isa<SILUndef>(Src)
1767+
// We allow SILArguments to support the case of initializing
1768+
// initializers. In such a case, the underlying case is allocated
1769+
// outside by the allocating initializer and we pass in the to be
1770+
// initialized value as a SILArgument.
1771+
|| isa<SILArgument>(Src)
1772+
// FIXME: Once the MarkUninitializedFixup pass is eliminated,
1773+
// mark_uninitialized should never be applied to a project_box. So
1774+
// at that point, this should be eliminated.
1775+
|| isa<ProjectBoxInst>(Src)
1776+
// FIXME: We only support pointer to address here to not break LLDB. It is
1777+
// important that long term we get rid of this since this is a situation
1778+
// where LLDB is breaking SILGen/DI invariants by not creating a new
1779+
// independent stack location for the pointer to address.
1780+
|| isa<PointerToAddressInst>(Src),
1781+
"Mark Uninitialized must be applied to a storage location");
17641782
}
17651783

17661784
void checkMarkUninitializedBehaviorInst(MarkUninitializedBehaviorInst *MU) {

‎lib/SILOptimizer/Mandatory/DIMemoryUseCollectorOwnership.cpp

Lines changed: 39 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,42 @@ using namespace swift;
2828
using namespace ownership;
2929

3030
//===----------------------------------------------------------------------===//
31-
// DIMemoryObjectInfo Implementation
31+
// Utility
32+
//===----------------------------------------------------------------------===//
33+
34+
static void gatherDestroysOfContainer(const MarkUninitializedInst *MUI,
35+
DIElementUseInfo &UseInfo) {
36+
// The MUI must be used on an alloc_box, alloc_stack, or global_addr. If we
37+
// have an alloc_stack or a global_addr, there is nothing further to do.
38+
if (isa<AllocStackInst>(MUI->getOperand()) ||
39+
isa<GlobalAddrInst>(MUI->getOperand()) ||
40+
isa<SILArgument>(MUI->getOperand()) ||
41+
// FIXME: We only support pointer to address here to not break LLDB. It is
42+
// important that long term we get rid of this since this is a situation
43+
// where LLDB is breaking SILGen/DI invariants by not creating a new
44+
// independent stack location for the pointer to address.
45+
isa<PointerToAddressInst>(MUI->getOperand()))
46+
return;
47+
48+
// Otherwise, we assume that we have a project_box. This is a hard cast to
49+
// ensure that we catch any new patterns emitted by SILGen and assert.
50+
auto *PBI = cast<ProjectBoxInst>(MUI->getOperand());
51+
auto *ABI = cast<AllocBoxInst>(PBI->getOperand());
52+
53+
// Treat destroys of the container as load+destroys of the original value.
54+
//
55+
// TODO: This should really be tracked separately from other destroys so that
56+
// we distinguish the lifetime of the container from the value itself.
57+
for (auto *Op : ABI->getUses()) {
58+
SILInstruction *User = Op->getUser();
59+
if (isa<StrongReleaseInst>(User) || isa<DestroyValueInst>(User)) {
60+
UseInfo.trackDestroy(User);
61+
}
62+
}
63+
}
64+
65+
//===----------------------------------------------------------------------===//
66+
// DIMemoryObjectInfo Implementation
3267
//===----------------------------------------------------------------------===//
3368

3469
static unsigned getElementCountRec(SILModule &Module, SILType T,
@@ -511,6 +546,7 @@ class ElementUseCollector {
511546
}
512547

513548
collectUses(TheMemory.MemoryInst, 0);
549+
gatherDestroysOfContainer(TheMemory.MemoryInst, UseInfo);
514550
}
515551

516552
void trackUse(DIMemoryUse Use) { UseInfo.trackUse(Use); }
@@ -1610,20 +1646,8 @@ void DelegatingClassInitElementUseCollector::collectClassInitSelfUses() {
16101646
assert(StoresOfArgumentToSelf == 1 &&
16111647
"The 'self' argument should have been stored into the box exactly once");
16121648

1613-
// The MUI must be used on an alloc_box or alloc_stack instruction. If we have
1614-
// an alloc_stack, there is nothing further to do.
1615-
if (isa<AllocStackInst>(MUI->getOperand()))
1616-
return;
1617-
1618-
auto *PBI = cast<ProjectBoxInst>(MUI->getOperand());
1619-
auto *ABI = cast<AllocBoxInst>(PBI->getOperand());
1620-
1621-
for (auto Op : ABI->getUses()) {
1622-
SILInstruction *User = Op->getUser();
1623-
if (isa<StrongReleaseInst>(User) || isa<DestroyValueInst>(User)) {
1624-
UseInfo.trackDestroy(User);
1625-
}
1626-
}
1649+
// Gather the uses of the
1650+
gatherDestroysOfContainer(MUI, UseInfo);
16271651
}
16281652

16291653
void DelegatingClassInitElementUseCollector::

‎lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp

Lines changed: 59 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -496,6 +496,10 @@ namespace {
496496
void processUninitializedRelease(SILInstruction *Release,
497497
bool consumed,
498498
SILBasicBlock::iterator InsertPt);
499+
void processUninitializedReleaseOfBox(AllocBoxInst *ABI,
500+
SILInstruction *Release,
501+
bool consumed,
502+
SILBasicBlock::iterator InsertPt);
499503
void deleteDeadRelease(unsigned ReleaseID);
500504

501505
void processNonTrivialRelease(unsigned ReleaseID);
@@ -1857,56 +1861,68 @@ void LifetimeChecker::updateInstructionForInitState(DIMemoryUse &Use) {
18571861
assert(isa<StoreInst>(Inst) && "Unknown store instruction!");
18581862
}
18591863

1864+
void LifetimeChecker::processUninitializedReleaseOfBox(
1865+
AllocBoxInst *ABI, SILInstruction *Release, bool consumed,
1866+
SILBasicBlock::iterator InsertPt) {
1867+
assert(ABI == Release->getOperand(0));
1868+
SILBuilderWithScope B(Release);
1869+
B.setInsertionPoint(InsertPt);
1870+
Destroys.push_back(B.createDeallocBox(Release->getLoc(), ABI));
1871+
}
1872+
18601873
void LifetimeChecker::processUninitializedRelease(SILInstruction *Release,
18611874
bool consumed,
18621875
SILBasicBlock::iterator InsertPt) {
18631876
// If this is an early release of a class instance, we need to emit a
18641877
// dealloc_partial_ref to free the memory. If this is a derived class, we
18651878
// may have to do a load of the 'self' box to get the class reference.
1866-
if (TheMemory.isClassInitSelf()) {
1867-
auto Loc = Release->getLoc();
1868-
1869-
SILBuilderWithScope B(Release);
1870-
B.setInsertionPoint(InsertPt);
1871-
1872-
SILValue Pointer = Release->getOperand(0);
1873-
1874-
// If we see an alloc_box as the pointer, then we're deallocating a 'box'
1875-
// for self. Make sure we're using its address result, not its refcount
1876-
// result, and make sure that the box gets deallocated (not released)
1877-
// since the pointer it contains will be manually cleaned up.
1878-
auto *ABI = dyn_cast<AllocBoxInst>(Release->getOperand(0));
1879-
if (ABI)
1880-
Pointer = getOrCreateProjectBox(ABI, 0);
1881-
1882-
if (!consumed) {
1883-
if (Pointer->getType().isAddress())
1884-
Pointer = B.createLoad(Loc, Pointer, LoadOwnershipQualifier::Take);
1885-
1886-
auto MetatypeTy = CanMetatypeType::get(
1887-
TheMemory.MemorySILType.getASTType(),
1888-
MetatypeRepresentation::Thick);
1889-
auto SILMetatypeTy = SILType::getPrimitiveObjectType(MetatypeTy);
1890-
SILValue Metatype;
1891-
1892-
// In an inherited convenience initializer, we must use the dynamic
1893-
// type of the object since nothing is initialized yet.
1894-
if (TheMemory.isDelegatingInit())
1895-
Metatype = B.createValueMetatype(Loc, SILMetatypeTy, Pointer);
1896-
else
1897-
Metatype = B.createMetatype(Loc, SILMetatypeTy);
1898-
1899-
// We've already destroyed any instance variables initialized by this
1900-
// constructor, now destroy instance variables initialized by subclass
1901-
// constructors that delegated to us, and finally free the memory.
1902-
B.createDeallocPartialRef(Loc, Pointer, Metatype);
1903-
}
1904-
1905-
// dealloc_box the self box if necessary.
1906-
if (ABI) {
1907-
auto DB = B.createDeallocBox(Loc, ABI);
1908-
Destroys.push_back(DB);
1879+
if (!TheMemory.isClassInitSelf()) {
1880+
if (auto *ABI = dyn_cast<AllocBoxInst>(Release->getOperand(0))) {
1881+
return processUninitializedReleaseOfBox(ABI, Release, consumed, InsertPt);
19091882
}
1883+
return;
1884+
}
1885+
1886+
auto Loc = Release->getLoc();
1887+
1888+
SILBuilderWithScope B(Release);
1889+
B.setInsertionPoint(InsertPt);
1890+
1891+
SILValue Pointer = Release->getOperand(0);
1892+
1893+
// If we see an alloc_box as the pointer, then we're deallocating a 'box' for
1894+
// self. Make sure that the box gets deallocated (not released) since the
1895+
// pointer it contains will be manually cleaned up.
1896+
auto *ABI = dyn_cast<AllocBoxInst>(Release->getOperand(0));
1897+
if (ABI)
1898+
Pointer = getOrCreateProjectBox(ABI, 0);
1899+
1900+
if (!consumed) {
1901+
if (Pointer->getType().isAddress())
1902+
Pointer = B.createLoad(Loc, Pointer, LoadOwnershipQualifier::Take);
1903+
1904+
auto MetatypeTy = CanMetatypeType::get(TheMemory.MemorySILType.getASTType(),
1905+
MetatypeRepresentation::Thick);
1906+
auto SILMetatypeTy = SILType::getPrimitiveObjectType(MetatypeTy);
1907+
SILValue Metatype;
1908+
1909+
// In an inherited convenience initializer, we must use the dynamic
1910+
// type of the object since nothing is initialized yet.
1911+
if (TheMemory.isDelegatingInit())
1912+
Metatype = B.createValueMetatype(Loc, SILMetatypeTy, Pointer);
1913+
else
1914+
Metatype = B.createMetatype(Loc, SILMetatypeTy);
1915+
1916+
// We've already destroyed any instance variables initialized by this
1917+
// constructor, now destroy instance variables initialized by subclass
1918+
// constructors that delegated to us, and finally free the memory.
1919+
B.createDeallocPartialRef(Loc, Pointer, Metatype);
1920+
}
1921+
1922+
// dealloc_box the self box if necessary.
1923+
if (ABI) {
1924+
auto DB = B.createDeallocBox(Loc, ABI);
1925+
Destroys.push_back(DB);
19101926
}
19111927
}
19121928

‎test/SILOptimizer/definite_init_markuninitialized_var.sil

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -542,3 +542,76 @@ bb2:
542542
%23 = tuple () // user: %24
543543
return %23 : $() // id: %24
544544
}
545+
// CHECK: } // end sil function 'conditionalInitOrAssign'
546+
547+
// Make sure that we use a dealloc_box on the path where we do not assign.
548+
//
549+
// CHECK-LABEL: sil @conditionalInitOrAssignAllocBox : $@convention(thin) () -> () {
550+
// CHECK: bb0:
551+
// CHECK: [[BOX:%.*]] = alloc_box
552+
//
553+
// CHECK: bb1:
554+
// CHECK: destroy_value [[BOX]]
555+
// CHECK: br bb3
556+
//
557+
// CHECK: bb2:
558+
// CHECK: dealloc_box [[BOX]]
559+
// CHECK: br bb3
560+
//
561+
// CHECK: bb3:
562+
// CHECK-NEXT: tuple
563+
// CHECK-NEXT: return
564+
// CHECK: } // end sil function 'conditionalInitOrAssignAllocBox'
565+
sil @conditionalInitOrAssignAllocBox : $@convention(thin) () -> () {
566+
bb0:
567+
%box = alloc_box $<τ_0_0> { var τ_0_0 } <SomeClass>
568+
%5 = project_box %box : $<τ_0_0> { var τ_0_0 } <SomeClass>, 0
569+
%7 = mark_uninitialized [var] %5 : $*SomeClass
570+
%2 = function_ref @getSomeClass : $@convention(thin) () -> @owned SomeClass
571+
cond_br undef, bb1, bb2
572+
573+
// Value is stored into box and then destroyed.
574+
bb1:
575+
%12 = apply %2() : $@convention(thin) () -> @owned SomeClass
576+
assign %12 to %7 : $*SomeClass
577+
destroy_value %box : $<τ_0_0> { var τ_0_0 } <SomeClass>
578+
br bb3
579+
580+
bb2:
581+
destroy_value %box : $<τ_0_0> { var τ_0_0 } <SomeClass>
582+
br bb3
583+
584+
bb3:
585+
%9999 = tuple()
586+
return %9999 : $()
587+
}
588+
589+
// LLDB uses mark_uninitialized [var] in an unsupported way via an address to
590+
// pointer. LLDB shouldn't do this, but until it is removed and validated we
591+
// need a test for this.
592+
// CHECK-LABEL: sil @lldb_unsupported_markuninitialized_testcase : $@convention(thin) (UnsafeMutablePointer<Any>) -> () {
593+
// CHECK-NOT: mark_uninitialized [var]
594+
// CHECK: } // end sil function 'lldb_unsupported_markuninitialized_testcase'
595+
sil @lldb_unsupported_markuninitialized_testcase : $@convention(thin) (UnsafeMutablePointer<Any>) -> () {
596+
bb0(%0 : @trivial $UnsafeMutablePointer<Any>):
597+
%2 = struct_extract %0 : $UnsafeMutablePointer<Any>, #UnsafeMutablePointer._rawValue
598+
%3 = integer_literal $Builtin.Int64, 8
599+
%4 = index_raw_pointer %2 : $Builtin.RawPointer, %3 : $Builtin.Int64
600+
%5 = pointer_to_address %4 : $Builtin.RawPointer to [strict] $*Builtin.RawPointer
601+
%6 = load [trivial] %5 : $*Builtin.RawPointer
602+
%7 = pointer_to_address %6 : $Builtin.RawPointer to [strict] $*Error
603+
%8 = mark_uninitialized [var] %7 : $*Error
604+
%9 = struct_extract %0 : $UnsafeMutablePointer<Any>, #UnsafeMutablePointer._rawValue
605+
%10 = integer_literal $Builtin.Int64, 0
606+
%11 = index_raw_pointer %9 : $Builtin.RawPointer, %10 : $Builtin.Int64
607+
%12 = pointer_to_address %11 : $Builtin.RawPointer to [strict] $*Builtin.RawPointer
608+
%13 = load [trivial] %12 : $*Builtin.RawPointer
609+
%14 = pointer_to_address %13 : $Builtin.RawPointer to [strict] $*@thick SomeClass.Type
610+
%15 = mark_uninitialized [var] %14 : $*@thick SomeClass.Type
611+
%16 = metatype $@thick SomeClass.Type
612+
%17 = begin_access [modify] [unsafe] %15 : $*@thick SomeClass.Type
613+
assign %16 to %17 : $*@thick SomeClass.Type
614+
end_access %17 : $*@thick SomeClass.Type
615+
%9999 = tuple()
616+
return %9999 : $()
617+
}

0 commit comments

Comments
 (0)