Skip to content

[definite-init] Treat destroys of mark_uninitialized [var] container to be a load + destroy. #16844

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 30, 2018
Merged

[definite-init] Treat destroys of mark_uninitialized [var] container to be a load + destroy. #16844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions lib/SIL/SILVerifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1751,16 +1751,34 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
require(MU->getModule().getStage() == SILStage::Raw,
"mark_uninitialized instruction can only exist in raw SIL");
require(Src->getType().isAddress() ||
Src->getType().getClassOrBoundGenericClass() ||
Src->getType().getAs<SILBoxType>(),
Src->getType().getClassOrBoundGenericClass() ||
Src->getType().getAs<SILBoxType>(),
"mark_uninitialized must be an address, class, or box type");
require(Src->getType() == MU->getType(),"operand and result type mismatch");
// FIXME: When the work to force MUI to be on Allocations/SILArguments
// complete, turn on this assertion.
#if 0
require(isa<AllocationInst>(Src) || isa<SILArgument>(Src),
"Mark Uninitialized should always be on the storage location");
#endif
require(isa<AllocationInst>(Src)
|| isa<GlobalAddrInst>(Src)
// TODO: Should we support SILUndef on mark_uninitialized? We
// currently have a test that verifies this behavior, but it seems
// like this would always be a bug due to the implications around
// the code in DI. This just bakes in the current behavior.
|| isa<SILUndef>(Src)
// We allow SILArguments to support the case of initializing
// initializers. In such a case, the underlying case is allocated
// outside by the allocating initializer and we pass in the to be
// initialized value as a SILArgument.
|| isa<SILArgument>(Src)
// FIXME: Once the MarkUninitializedFixup pass is eliminated,
// mark_uninitialized should never be applied to a project_box. So
// at that point, this should be eliminated.
|| isa<ProjectBoxInst>(Src)
// FIXME: We only support pointer to address here to not break LLDB. It is
// important that long term we get rid of this since this is a situation
// where LLDB is breaking SILGen/DI invariants by not creating a new
// independent stack location for the pointer to address.
|| isa<PointerToAddressInst>(Src),
"Mark Uninitialized must be applied to a storage location");
}

void checkMarkUninitializedBehaviorInst(MarkUninitializedBehaviorInst *MU) {
Expand Down
54 changes: 39 additions & 15 deletions lib/SILOptimizer/Mandatory/DIMemoryUseCollectorOwnership.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,42 @@ using namespace swift;
using namespace ownership;

//===----------------------------------------------------------------------===//
// DIMemoryObjectInfo Implementation
// Utility
//===----------------------------------------------------------------------===//

static void gatherDestroysOfContainer(const MarkUninitializedInst *MUI,
DIElementUseInfo &UseInfo) {
// The MUI must be used on an alloc_box, alloc_stack, or global_addr. If we
// have an alloc_stack or a global_addr, there is nothing further to do.
if (isa<AllocStackInst>(MUI->getOperand()) ||
isa<GlobalAddrInst>(MUI->getOperand()) ||
isa<SILArgument>(MUI->getOperand()) ||
// FIXME: We only support pointer to address here to not break LLDB. It is
// important that long term we get rid of this since this is a situation
// where LLDB is breaking SILGen/DI invariants by not creating a new
// independent stack location for the pointer to address.
isa<PointerToAddressInst>(MUI->getOperand()))
return;

// Otherwise, we assume that we have a project_box. This is a hard cast to
// ensure that we catch any new patterns emitted by SILGen and assert.
auto *PBI = cast<ProjectBoxInst>(MUI->getOperand());
auto *ABI = cast<AllocBoxInst>(PBI->getOperand());

// Treat destroys of the container as load+destroys of the original value.
//
// TODO: This should really be tracked separately from other destroys so that
// we distinguish the lifetime of the container from the value itself.
for (auto *Op : ABI->getUses()) {
SILInstruction *User = Op->getUser();
if (isa<StrongReleaseInst>(User) || isa<DestroyValueInst>(User)) {
UseInfo.trackDestroy(User);
}
}
}

//===----------------------------------------------------------------------===//
// DIMemoryObjectInfo Implementation
//===----------------------------------------------------------------------===//

static unsigned getElementCountRec(SILModule &Module, SILType T,
Expand Down Expand Up @@ -511,6 +546,7 @@ class ElementUseCollector {
}

collectUses(TheMemory.MemoryInst, 0);
gatherDestroysOfContainer(TheMemory.MemoryInst, UseInfo);
}

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

// The MUI must be used on an alloc_box or alloc_stack instruction. If we have
// an alloc_stack, there is nothing further to do.
if (isa<AllocStackInst>(MUI->getOperand()))
return;

auto *PBI = cast<ProjectBoxInst>(MUI->getOperand());
auto *ABI = cast<AllocBoxInst>(PBI->getOperand());

for (auto Op : ABI->getUses()) {
SILInstruction *User = Op->getUser();
if (isa<StrongReleaseInst>(User) || isa<DestroyValueInst>(User)) {
UseInfo.trackDestroy(User);
}
}
// Gather the uses of the
gatherDestroysOfContainer(MUI, UseInfo);
}

void DelegatingClassInitElementUseCollector::
Expand Down
102 changes: 59 additions & 43 deletions lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,10 @@ namespace {
void processUninitializedRelease(SILInstruction *Release,
bool consumed,
SILBasicBlock::iterator InsertPt);
void processUninitializedReleaseOfBox(AllocBoxInst *ABI,
SILInstruction *Release,
bool consumed,
SILBasicBlock::iterator InsertPt);
void deleteDeadRelease(unsigned ReleaseID);

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

void LifetimeChecker::processUninitializedReleaseOfBox(
AllocBoxInst *ABI, SILInstruction *Release, bool consumed,
SILBasicBlock::iterator InsertPt) {
assert(ABI == Release->getOperand(0));
SILBuilderWithScope B(Release);
B.setInsertionPoint(InsertPt);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you're updating the insertion point but not the debug scope. Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just followed the model in other parts of the file. If this is wrong then the other places are wrong as well.

Destroys.push_back(B.createDeallocBox(Release->getLoc(), ABI));
}

void LifetimeChecker::processUninitializedRelease(SILInstruction *Release,
bool consumed,
SILBasicBlock::iterator InsertPt) {
// If this is an early release of a class instance, we need to emit a
// dealloc_partial_ref to free the memory. If this is a derived class, we
// may have to do a load of the 'self' box to get the class reference.
if (TheMemory.isClassInitSelf()) {
auto Loc = Release->getLoc();

SILBuilderWithScope B(Release);
B.setInsertionPoint(InsertPt);

SILValue Pointer = Release->getOperand(0);

// If we see an alloc_box as the pointer, then we're deallocating a 'box'
// for self. Make sure we're using its address result, not its refcount
// result, and make sure that the box gets deallocated (not released)
// since the pointer it contains will be manually cleaned up.
auto *ABI = dyn_cast<AllocBoxInst>(Release->getOperand(0));
if (ABI)
Pointer = getOrCreateProjectBox(ABI, 0);

if (!consumed) {
if (Pointer->getType().isAddress())
Pointer = B.createLoad(Loc, Pointer, LoadOwnershipQualifier::Take);

auto MetatypeTy = CanMetatypeType::get(
TheMemory.MemorySILType.getASTType(),
MetatypeRepresentation::Thick);
auto SILMetatypeTy = SILType::getPrimitiveObjectType(MetatypeTy);
SILValue Metatype;

// In an inherited convenience initializer, we must use the dynamic
// type of the object since nothing is initialized yet.
if (TheMemory.isDelegatingInit())
Metatype = B.createValueMetatype(Loc, SILMetatypeTy, Pointer);
else
Metatype = B.createMetatype(Loc, SILMetatypeTy);

// We've already destroyed any instance variables initialized by this
// constructor, now destroy instance variables initialized by subclass
// constructors that delegated to us, and finally free the memory.
B.createDeallocPartialRef(Loc, Pointer, Metatype);
}

// dealloc_box the self box if necessary.
if (ABI) {
auto DB = B.createDeallocBox(Loc, ABI);
Destroys.push_back(DB);
if (!TheMemory.isClassInitSelf()) {
if (auto *ABI = dyn_cast<AllocBoxInst>(Release->getOperand(0))) {
return processUninitializedReleaseOfBox(ABI, Release, consumed, InsertPt);
}
return;
}

auto Loc = Release->getLoc();

SILBuilderWithScope B(Release);
B.setInsertionPoint(InsertPt);

SILValue Pointer = Release->getOperand(0);

// If we see an alloc_box as the pointer, then we're deallocating a 'box' for
// self. Make sure that the box gets deallocated (not released) since the
// pointer it contains will be manually cleaned up.
auto *ABI = dyn_cast<AllocBoxInst>(Release->getOperand(0));
if (ABI)
Pointer = getOrCreateProjectBox(ABI, 0);

if (!consumed) {
if (Pointer->getType().isAddress())
Pointer = B.createLoad(Loc, Pointer, LoadOwnershipQualifier::Take);

auto MetatypeTy = CanMetatypeType::get(TheMemory.MemorySILType.getASTType(),
MetatypeRepresentation::Thick);
auto SILMetatypeTy = SILType::getPrimitiveObjectType(MetatypeTy);
SILValue Metatype;

// In an inherited convenience initializer, we must use the dynamic
// type of the object since nothing is initialized yet.
if (TheMemory.isDelegatingInit())
Metatype = B.createValueMetatype(Loc, SILMetatypeTy, Pointer);
else
Metatype = B.createMetatype(Loc, SILMetatypeTy);

// We've already destroyed any instance variables initialized by this
// constructor, now destroy instance variables initialized by subclass
// constructors that delegated to us, and finally free the memory.
B.createDeallocPartialRef(Loc, Pointer, Metatype);
}

// dealloc_box the self box if necessary.
if (ABI) {
auto DB = B.createDeallocBox(Loc, ABI);
Destroys.push_back(DB);
}
}

Expand Down
73 changes: 73 additions & 0 deletions test/SILOptimizer/definite_init_markuninitialized_var.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,3 +542,76 @@ bb2:
%23 = tuple () // user: %24
return %23 : $() // id: %24
}
// CHECK: } // end sil function 'conditionalInitOrAssign'

// Make sure that we use a dealloc_box on the path where we do not assign.
//
// CHECK-LABEL: sil @conditionalInitOrAssignAllocBox : $@convention(thin) () -> () {
// CHECK: bb0:
// CHECK: [[BOX:%.*]] = alloc_box
//
// CHECK: bb1:
// CHECK: destroy_value [[BOX]]
// CHECK: br bb3
//
// CHECK: bb2:
// CHECK: dealloc_box [[BOX]]
// CHECK: br bb3
//
// CHECK: bb3:
// CHECK-NEXT: tuple
// CHECK-NEXT: return
// CHECK: } // end sil function 'conditionalInitOrAssignAllocBox'
sil @conditionalInitOrAssignAllocBox : $@convention(thin) () -> () {
bb0:
%box = alloc_box $<τ_0_0> { var τ_0_0 } <SomeClass>
%5 = project_box %box : $<τ_0_0> { var τ_0_0 } <SomeClass>, 0
%7 = mark_uninitialized [var] %5 : $*SomeClass
%2 = function_ref @getSomeClass : $@convention(thin) () -> @owned SomeClass
cond_br undef, bb1, bb2

// Value is stored into box and then destroyed.
bb1:
%12 = apply %2() : $@convention(thin) () -> @owned SomeClass
assign %12 to %7 : $*SomeClass
destroy_value %box : $<τ_0_0> { var τ_0_0 } <SomeClass>
br bb3

bb2:
destroy_value %box : $<τ_0_0> { var τ_0_0 } <SomeClass>
br bb3

bb3:
%9999 = tuple()
return %9999 : $()
}

// LLDB uses mark_uninitialized [var] in an unsupported way via an address to
// pointer. LLDB shouldn't do this, but until it is removed and validated we
// need a test for this.
// CHECK-LABEL: sil @lldb_unsupported_markuninitialized_testcase : $@convention(thin) (UnsafeMutablePointer<Any>) -> () {
// CHECK-NOT: mark_uninitialized [var]
// CHECK: } // end sil function 'lldb_unsupported_markuninitialized_testcase'
sil @lldb_unsupported_markuninitialized_testcase : $@convention(thin) (UnsafeMutablePointer<Any>) -> () {
bb0(%0 : @trivial $UnsafeMutablePointer<Any>):
%2 = struct_extract %0 : $UnsafeMutablePointer<Any>, #UnsafeMutablePointer._rawValue
%3 = integer_literal $Builtin.Int64, 8
%4 = index_raw_pointer %2 : $Builtin.RawPointer, %3 : $Builtin.Int64
%5 = pointer_to_address %4 : $Builtin.RawPointer to [strict] $*Builtin.RawPointer
%6 = load [trivial] %5 : $*Builtin.RawPointer
%7 = pointer_to_address %6 : $Builtin.RawPointer to [strict] $*Error
%8 = mark_uninitialized [var] %7 : $*Error
%9 = struct_extract %0 : $UnsafeMutablePointer<Any>, #UnsafeMutablePointer._rawValue
%10 = integer_literal $Builtin.Int64, 0
%11 = index_raw_pointer %9 : $Builtin.RawPointer, %10 : $Builtin.Int64
%12 = pointer_to_address %11 : $Builtin.RawPointer to [strict] $*Builtin.RawPointer
%13 = load [trivial] %12 : $*Builtin.RawPointer
%14 = pointer_to_address %13 : $Builtin.RawPointer to [strict] $*@thick SomeClass.Type
%15 = mark_uninitialized [var] %14 : $*@thick SomeClass.Type
%16 = metatype $@thick SomeClass.Type
%17 = begin_access [modify] [unsafe] %15 : $*@thick SomeClass.Type
assign %16 to %17 : $*@thick SomeClass.Type
end_access %17 : $*@thick SomeClass.Type
%9999 = tuple()
return %9999 : $()
}