Skip to content

Fix findAccessedStorage to handle cyclic phis. #21665

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
Jan 8, 2019
Merged

Fix findAccessedStorage to handle cyclic phis. #21665

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
314 changes: 180 additions & 134 deletions lib/SIL/MemAccessUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,153 +231,199 @@ static bool isAddressForLocalInitOnly(SILValue sourceAddr) {
}
}

namespace {
// The result of an accessed storage query. A complete result produces an
// AccessedStorage object, which may or may not be valid. An incomplete result
// produces a SILValue representing the source address for use with deeper
// queries. The source address is not necessarily a SIL address type because
// the query can extend past pointer-to-address casts.
class AccessedStorageResult {
AccessedStorage storage;
SILValue address;
bool complete;

explicit AccessedStorageResult(SILValue address)
: address(address), complete(false) {}

public:
AccessedStorageResult(const AccessedStorage &storage)
: storage(storage), complete(true) {}

static AccessedStorageResult incomplete(SILValue address) {
return AccessedStorageResult(address);
}

bool isComplete() const { return complete; }

AccessedStorage getStorage() const { assert(complete); return storage; }

SILValue getAddress() const { assert(!complete); return address; }
};
} // namespace

// AccessEnforcementWMO makes a strong assumption that all accesses are either
// identified or are *not* accessing a global variable or class property defined
// in this module. Consequently, we cannot simply bail out on
// PointerToAddressInst as an Unidentified access.
AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
SILValue address = sourceAddr;
while (true) {
AccessedStorage::Kind kind = AccessedStorage::classify(address);
// First handle identified cases: these are always valid as the base of
// a formal access.
if (kind != AccessedStorage::Unidentified)
return AccessedStorage(address, kind);

// If the address producer cannot immediately be classified, follow the
// use-def chain of address, box, or RawPointer producers.
assert(address->getType().isAddress()
|| isa<SILBoxType>(address->getType().getASTType())
|| isa<BuiltinRawPointerType>(address->getType().getASTType()));

// Handle other unidentified address sources.
switch (address->getKind()) {
default:
if (isAddressForLocalInitOnly(address))
return AccessedStorage(address, AccessedStorage::Unidentified);
return AccessedStorage();

case ValueKind::SILUndef:
return AccessedStorage(address, AccessedStorage::Unidentified);

case ValueKind::ApplyInst:
if (isExternalGlobalAddressor(cast<ApplyInst>(address)))
return AccessedStorage(address, AccessedStorage::Unidentified);

// Don't currently allow any other calls to return an accessed address.
static AccessedStorageResult getAccessedStorageFromAddress(SILValue sourceAddr) {
AccessedStorage::Kind kind = AccessedStorage::classify(sourceAddr);
// First handle identified cases: these are always valid as the base of
// a formal access.
if (kind != AccessedStorage::Unidentified)
return AccessedStorage(sourceAddr, kind);

// If the sourceAddr producer cannot immediately be classified, follow the
// use-def chain of sourceAddr, box, or RawPointer producers.
assert(sourceAddr->getType().isAddress()
|| isa<SILBoxType>(sourceAddr->getType().getASTType())
|| isa<BuiltinRawPointerType>(sourceAddr->getType().getASTType()));

// Handle other unidentified address sources.
switch (sourceAddr->getKind()) {
default:
if (isAddressForLocalInitOnly(sourceAddr))
return AccessedStorage(sourceAddr, AccessedStorage::Unidentified);
return AccessedStorage();

case ValueKind::SILUndef:
return AccessedStorage(sourceAddr, AccessedStorage::Unidentified);

case ValueKind::ApplyInst:
if (isExternalGlobalAddressor(cast<ApplyInst>(sourceAddr)))
return AccessedStorage(sourceAddr, AccessedStorage::Unidentified);

// Don't currently allow any other calls to return an accessed address.
return AccessedStorage();

case ValueKind::StructExtractInst:
// Handle nested access to a KeyPath projection. The projection itself
// uses a Builtin. However, the returned UnsafeMutablePointer may be
// converted to an address and accessed via an inout argument.
if (isUnsafePointerExtraction(cast<StructExtractInst>(sourceAddr)))
return AccessedStorage(sourceAddr, AccessedStorage::Unidentified);
return AccessedStorage();

case ValueKind::SILPhiArgument: {
auto *phiArg = cast<SILPhiArgument>(sourceAddr);
if (phiArg->isPhiArgument())
return AccessedStorageResult::incomplete(phiArg);

// A non-phi block argument may be a box value projected out of
// switch_enum. Address-type block arguments are not allowed.
if (sourceAddr->getType().isAddress())
return AccessedStorage();

case ValueKind::StructExtractInst:
// Handle nested access to a KeyPath projection. The projection itself
// uses a Builtin. However, the returned UnsafeMutablePointer may be
// converted to an address and accessed via an inout argument.
if (isUnsafePointerExtraction(cast<StructExtractInst>(address)))
return AccessedStorage(address, AccessedStorage::Unidentified);
return AccessedStorage();
checkSwitchEnumBlockArg(cast<SILPhiArgument>(sourceAddr));
return AccessedStorage(sourceAddr, AccessedStorage::Unidentified);
}
// Load a box from an indirect payload of an opaque enum.
// We must have peeked past the project_box earlier in this loop.
// (the indirectness makes it a box, the load is for address-only).
//
// %payload_adr = unchecked_take_enum_data_addr %enum : $*Enum, #Enum.case
// %box = load [take] %payload_adr : $*{ var Enum }
//
// FIXME: this case should go away with opaque values.
//
// Otherwise return invalid AccessedStorage.
case ValueKind::LoadInst:
if (sourceAddr->getType().is<SILBoxType>()) {
SILValue operAddr = cast<LoadInst>(sourceAddr)->getOperand();
assert(isa<UncheckedTakeEnumDataAddrInst>(operAddr));
return AccessedStorageResult::incomplete(operAddr);
}
return AccessedStorage();

Copy link

Choose a reason for hiding this comment

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

The following huge switch case works / I don't clearly see a better option, but I can see potential future bugs if someone adds new (relevant) SIL instructions / does not update it.

// ref_tail_addr project an address from a reference.
// This is a valid address producer for nested @inout argument
// access, but it is never used for formal access of identified objects.
case ValueKind::RefTailAddrInst:
return AccessedStorage(sourceAddr, AccessedStorage::Unidentified);

// Inductive single-operand cases:
// Look through address casts to find the source address.
case ValueKind::MarkUninitializedInst:
case ValueKind::OpenExistentialAddrInst:
case ValueKind::UncheckedAddrCastInst:
// Inductive cases that apply to any type.
case ValueKind::CopyValueInst:
case ValueKind::MarkDependenceInst:
// Look through a project_box to identify the underlying alloc_box as the
// accesed object. It must be possible to reach either the alloc_box or the
// containing enum in this loop, only looking through simple value
// propagation such as copy_value.
case ValueKind::ProjectBoxInst:
// Handle project_block_storage just like project_box.
case ValueKind::ProjectBlockStorageInst:
// Look through begin_borrow in case a local box is borrowed.
case ValueKind::BeginBorrowInst:
return AccessedStorageResult::incomplete(
cast<SingleValueInstruction>(sourceAddr)->getOperand(0));

// Access to a Builtin.RawPointer. Treat this like the inductive cases
// above because some RawPointers originate from identified locations. See
// the special case for global addressors, which return RawPointer, above.
//
// If the inductive search does not find a valid addressor, it will
// eventually reach the default case that returns in invalid location. This
// is correct for RawPointer because, although accessing a RawPointer is
// legal SIL, there is no way to guarantee that it doesn't access class or
// global storage, so returning a valid unidentified storage object would be
// incorrect. It is the caller's responsibility to know that formal access
// to such a location can be safely ignored.
//
// For example:
//
// - KeyPath Builtins access RawPointer. However, the caller can check
// that the access `isFromBuilin` and ignore the storage.
//
// - lldb generates RawPointer access for debugger variables, but SILGen
// marks debug VarDecl access as 'Unsafe' and SIL passes don't need the
// AccessedStorage for 'Unsafe' access.
case ValueKind::PointerToAddressInst:
return AccessedStorageResult::incomplete(
cast<SingleValueInstruction>(sourceAddr)->getOperand(0));

// Address-to-address subobject projections.
case ValueKind::StructElementAddrInst:
case ValueKind::TupleElementAddrInst:
case ValueKind::UncheckedTakeEnumDataAddrInst:
case ValueKind::TailAddrInst:
case ValueKind::IndexAddrInst:
return AccessedStorageResult::incomplete(
cast<SingleValueInstruction>(sourceAddr)->getOperand(0));
}
}

case ValueKind::SILPhiArgument: {
auto *phiArg = cast<SILPhiArgument>(address);
if (phiArg->isPhiArgument()) {
SmallVector<SILValue, 8> incomingValues;
phiArg->getIncomingPhiValues(incomingValues);
if (incomingValues.empty())
return AccessedStorage();

auto storage = findAccessedStorage(incomingValues.pop_back_val());
for (auto val : incomingValues) {
auto otherStorage = findAccessedStorage(val);
if (!accessingIdenticalLocations(storage, otherStorage))
return AccessedStorage();
AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
SmallVector<SILValue, 8> addressWorklist({sourceAddr});
SmallPtrSet<SILPhiArgument *, 4> visitedPhis;
Optional<AccessedStorage> storage;

while (!addressWorklist.empty()) {
AccessedStorageResult result =
getAccessedStorageFromAddress(addressWorklist.pop_back_val());

if (!result.isComplete()) {
SILValue operAddr = result.getAddress();
if (auto *phiArg = dyn_cast<SILPhiArgument>(operAddr)) {
if (phiArg->isPhiArgument()) {
if (visitedPhis.insert(phiArg).second)
phiArg->getIncomingPhiValues(addressWorklist);
continue;
}
return storage;
}
// A non-phi block argument may be a box value projected out of
// switch_enum. Address-type block arguments are not allowed.
if (address->getType().isAddress())
return AccessedStorage();

checkSwitchEnumBlockArg(cast<SILPhiArgument>(address));
return AccessedStorage(address, AccessedStorage::Unidentified);
}
// Load a box from an indirect payload of an opaque enum.
// We must have peeked past the project_box earlier in this loop.
// (the indirectness makes it a box, the load is for address-only).
//
// %payload_adr = unchecked_take_enum_data_addr %enum : $*Enum, #Enum.case
// %box = load [take] %payload_adr : $*{ var Enum }
//
// FIXME: this case should go away with opaque values.
//
// Otherwise return invalid AccessedStorage.
case ValueKind::LoadInst: {
if (address->getType().is<SILBoxType>()) {
address = cast<LoadInst>(address)->getOperand();
assert(isa<UncheckedTakeEnumDataAddrInst>(address));
continue;
}
return AccessedStorage();
}

// ref_tail_addr project an address from a reference.
// This is a valid address producer for nested @inout argument
// access, but it is never used for formal access of identified objects.
case ValueKind::RefTailAddrInst:
return AccessedStorage(address, AccessedStorage::Unidentified);

// Inductive cases:
// Look through address casts to find the source address.
case ValueKind::MarkUninitializedInst:
case ValueKind::OpenExistentialAddrInst:
case ValueKind::UncheckedAddrCastInst:
// Inductive cases that apply to any type.
case ValueKind::CopyValueInst:
case ValueKind::MarkDependenceInst:
// Look through a project_box to identify the underlying alloc_box as the
// accesed object. It must be possible to reach either the alloc_box or the
// containing enum in this loop, only looking through simple value
// propagation such as copy_value.
case ValueKind::ProjectBoxInst:
// Handle project_block_storage just like project_box.
case ValueKind::ProjectBlockStorageInst:
// Look through begin_borrow in case a local box is borrowed.
case ValueKind::BeginBorrowInst:
address = cast<SingleValueInstruction>(address)->getOperand(0);
continue;

// Access to a Builtin.RawPointer. Treat this like the inductive cases
// above because some RawPointers originate from identified locations. See
// the special case for global addressors, which return RawPointer, above.
//
// If the inductive search does not find a valid addressor, it will
// eventually reach the default case that returns in invalid location. This
// is correct for RawPointer because, although accessing a RawPointer is
// legal SIL, there is no way to guarantee that it doesn't access class or
// global storage, so returning a valid unidentified storage object would be
// incorrect. It is the caller's responsibility to know that formal access
// to such a location can be safely ignored.
//
// For example:
//
// - KeyPath Builtins access RawPointer. However, the caller can check
// that the access `isFromBuilin` and ignore the storage.
//
// - lldb generates RawPointer access for debugger variables, but SILGen
// marks debug VarDecl access as 'Unsafe' and SIL passes don't need the
// AccessedStorage for 'Unsafe' access.
case ValueKind::PointerToAddressInst:
address = cast<SingleValueInstruction>(address)->getOperand(0);
addressWorklist.push_back(operAddr);
continue;

// Address-to-address subobject projections.
case ValueKind::StructElementAddrInst:
case ValueKind::TupleElementAddrInst:
case ValueKind::UncheckedTakeEnumDataAddrInst:
case ValueKind::TailAddrInst:
case ValueKind::IndexAddrInst:
address = cast<SingleValueInstruction>(address)->getOperand(0);
}
if (!storage.hasValue()) {
storage = result.getStorage();
continue;
}
if (!accessingIdenticalLocations(storage.getValue(), result.getStorage()))
return AccessedStorage();
}
return storage.getValueOr(AccessedStorage());
}

AccessedStorage swift::findAccessedStorageNonNested(SILValue sourceAddr) {
Expand Down
27 changes: 27 additions & 0 deletions test/SILOptimizer/verifier.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

import Builtin

sil_stage canonical

// Don't fail in the verifier on a shared unreachable exit block of two loops.
sil @dont_fail: $@convention(thin) (Builtin.Int1) -> () {
bb0(%0 : $Builtin.Int1):
Expand Down Expand Up @@ -80,3 +82,28 @@ bb4:
%10 = tuple ()
return %10 : $()
}

// Verify that findAccessedStorage handles cyclic phis.
// <rdar://47059671> swiftc crashes
class AClass {
var aStoredProp: Builtin.Int32
}

sil @testPhiCycle : $@convention(thin) (@guaranteed AClass) -> () {
bb0(%0 : $AClass):
%accessAddr = ref_element_addr %0 : $AClass, #AClass.aStoredProp
br bbloop(%accessAddr : $*Builtin.Int32)

bbloop(%phiAddr : $*Builtin.Int32):
%access = begin_access [read] [dynamic] [no_nested_conflict] %phiAddr : $*Builtin.Int32
%val = load %access : $*Builtin.Int32
end_access %access : $*Builtin.Int32
cond_br undef, bbtail, bbreturn

bbtail:
br bbloop(%phiAddr : $*Builtin.Int32)

bbreturn:
%v = tuple ()
return %v : $()
}