Skip to content

Commit 82fbc0d

Browse files
committed
Fix findAccessedStorage to handle cyclic phis.
Fixes infinite recursion in findAccessedStorge. This routine was not designed to be recursive, but recursion was recently added as a hack for address-type phis. Naturally, phis can be cyclic, so this was not a correct workaround. This is only a problem with enforce-exclusivity=checked, with which findAccessedStorge is invoked after arbitrary optimization passes that introduce address-type phis. Ultimately, address phis will be disallowed in SIL, but some optimizer passes must be fixed first. Fixes <rdar://problem/47059671> swiftlang-1001.0.31.11 root: swiftc crashes
1 parent 4529083 commit 82fbc0d

File tree

2 files changed

+207
-134
lines changed

2 files changed

+207
-134
lines changed

lib/SIL/MemAccessUtils.cpp

Lines changed: 180 additions & 134 deletions
Original file line numberDiff line numberDiff line change
@@ -231,153 +231,199 @@ static bool isAddressForLocalInitOnly(SILValue sourceAddr) {
231231
}
232232
}
233233

234+
namespace {
235+
// The result of an accessed storage query. A complete result produces an
236+
// AccessedStorage object, which may or may not be valid. An incomplete result
237+
// produces a SILValue representing the source address for use with deeper
238+
// queries. The source address is not necessarily a SIL address type because
239+
// the query can extend past pointer-to-address casts.
240+
class AccessedStorageResult {
241+
AccessedStorage storage;
242+
SILValue address;
243+
bool complete;
244+
245+
explicit AccessedStorageResult(SILValue address)
246+
: address(address), complete(false) {}
247+
248+
public:
249+
AccessedStorageResult(const AccessedStorage &storage)
250+
: storage(storage), complete(true) {}
251+
252+
static AccessedStorageResult incomplete(SILValue address) {
253+
return AccessedStorageResult(address);
254+
}
255+
256+
bool isComplete() const { return complete; }
257+
258+
AccessedStorage getStorage() const { assert(complete); return storage; }
259+
260+
SILValue getAddress() const { assert(!complete); return address; }
261+
};
262+
} // namespace
263+
234264
// AccessEnforcementWMO makes a strong assumption that all accesses are either
235265
// identified or are *not* accessing a global variable or class property defined
236266
// in this module. Consequently, we cannot simply bail out on
237267
// PointerToAddressInst as an Unidentified access.
238-
AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
239-
SILValue address = sourceAddr;
240-
while (true) {
241-
AccessedStorage::Kind kind = AccessedStorage::classify(address);
242-
// First handle identified cases: these are always valid as the base of
243-
// a formal access.
244-
if (kind != AccessedStorage::Unidentified)
245-
return AccessedStorage(address, kind);
246-
247-
// If the address producer cannot immediately be classified, follow the
248-
// use-def chain of address, box, or RawPointer producers.
249-
assert(address->getType().isAddress()
250-
|| isa<SILBoxType>(address->getType().getASTType())
251-
|| isa<BuiltinRawPointerType>(address->getType().getASTType()));
252-
253-
// Handle other unidentified address sources.
254-
switch (address->getKind()) {
255-
default:
256-
if (isAddressForLocalInitOnly(address))
257-
return AccessedStorage(address, AccessedStorage::Unidentified);
258-
return AccessedStorage();
259-
260-
case ValueKind::SILUndef:
261-
return AccessedStorage(address, AccessedStorage::Unidentified);
262-
263-
case ValueKind::ApplyInst:
264-
if (isExternalGlobalAddressor(cast<ApplyInst>(address)))
265-
return AccessedStorage(address, AccessedStorage::Unidentified);
266-
267-
// Don't currently allow any other calls to return an accessed address.
268+
static AccessedStorageResult getAccessedStorageFromAddress(SILValue sourceAddr) {
269+
AccessedStorage::Kind kind = AccessedStorage::classify(sourceAddr);
270+
// First handle identified cases: these are always valid as the base of
271+
// a formal access.
272+
if (kind != AccessedStorage::Unidentified)
273+
return AccessedStorage(sourceAddr, kind);
274+
275+
// If the sourceAddr producer cannot immediately be classified, follow the
276+
// use-def chain of sourceAddr, box, or RawPointer producers.
277+
assert(sourceAddr->getType().isAddress()
278+
|| isa<SILBoxType>(sourceAddr->getType().getASTType())
279+
|| isa<BuiltinRawPointerType>(sourceAddr->getType().getASTType()));
280+
281+
// Handle other unidentified address sources.
282+
switch (sourceAddr->getKind()) {
283+
default:
284+
if (isAddressForLocalInitOnly(sourceAddr))
285+
return AccessedStorage(sourceAddr, AccessedStorage::Unidentified);
286+
return AccessedStorage();
287+
288+
case ValueKind::SILUndef:
289+
return AccessedStorage(sourceAddr, AccessedStorage::Unidentified);
290+
291+
case ValueKind::ApplyInst:
292+
if (isExternalGlobalAddressor(cast<ApplyInst>(sourceAddr)))
293+
return AccessedStorage(sourceAddr, AccessedStorage::Unidentified);
294+
295+
// Don't currently allow any other calls to return an accessed address.
296+
return AccessedStorage();
297+
298+
case ValueKind::StructExtractInst:
299+
// Handle nested access to a KeyPath projection. The projection itself
300+
// uses a Builtin. However, the returned UnsafeMutablePointer may be
301+
// converted to an address and accessed via an inout argument.
302+
if (isUnsafePointerExtraction(cast<StructExtractInst>(sourceAddr)))
303+
return AccessedStorage(sourceAddr, AccessedStorage::Unidentified);
304+
return AccessedStorage();
305+
306+
case ValueKind::SILPhiArgument: {
307+
auto *phiArg = cast<SILPhiArgument>(sourceAddr);
308+
if (phiArg->isPhiArgument())
309+
return AccessedStorageResult::incomplete(phiArg);
310+
311+
// A non-phi block argument may be a box value projected out of
312+
// switch_enum. Address-type block arguments are not allowed.
313+
if (sourceAddr->getType().isAddress())
268314
return AccessedStorage();
269315

270-
case ValueKind::StructExtractInst:
271-
// Handle nested access to a KeyPath projection. The projection itself
272-
// uses a Builtin. However, the returned UnsafeMutablePointer may be
273-
// converted to an address and accessed via an inout argument.
274-
if (isUnsafePointerExtraction(cast<StructExtractInst>(address)))
275-
return AccessedStorage(address, AccessedStorage::Unidentified);
276-
return AccessedStorage();
316+
checkSwitchEnumBlockArg(cast<SILPhiArgument>(sourceAddr));
317+
return AccessedStorage(sourceAddr, AccessedStorage::Unidentified);
318+
}
319+
// Load a box from an indirect payload of an opaque enum.
320+
// We must have peeked past the project_box earlier in this loop.
321+
// (the indirectness makes it a box, the load is for address-only).
322+
//
323+
// %payload_adr = unchecked_take_enum_data_addr %enum : $*Enum, #Enum.case
324+
// %box = load [take] %payload_adr : $*{ var Enum }
325+
//
326+
// FIXME: this case should go away with opaque values.
327+
//
328+
// Otherwise return invalid AccessedStorage.
329+
case ValueKind::LoadInst:
330+
if (sourceAddr->getType().is<SILBoxType>()) {
331+
SILValue operAddr = cast<LoadInst>(sourceAddr)->getOperand();
332+
assert(isa<UncheckedTakeEnumDataAddrInst>(operAddr));
333+
return AccessedStorageResult::incomplete(operAddr);
334+
}
335+
return AccessedStorage();
336+
337+
// ref_tail_addr project an address from a reference.
338+
// This is a valid address producer for nested @inout argument
339+
// access, but it is never used for formal access of identified objects.
340+
case ValueKind::RefTailAddrInst:
341+
return AccessedStorage(sourceAddr, AccessedStorage::Unidentified);
342+
343+
// Inductive single-operand cases:
344+
// Look through address casts to find the source address.
345+
case ValueKind::MarkUninitializedInst:
346+
case ValueKind::OpenExistentialAddrInst:
347+
case ValueKind::UncheckedAddrCastInst:
348+
// Inductive cases that apply to any type.
349+
case ValueKind::CopyValueInst:
350+
case ValueKind::MarkDependenceInst:
351+
// Look through a project_box to identify the underlying alloc_box as the
352+
// accesed object. It must be possible to reach either the alloc_box or the
353+
// containing enum in this loop, only looking through simple value
354+
// propagation such as copy_value.
355+
case ValueKind::ProjectBoxInst:
356+
// Handle project_block_storage just like project_box.
357+
case ValueKind::ProjectBlockStorageInst:
358+
// Look through begin_borrow in case a local box is borrowed.
359+
case ValueKind::BeginBorrowInst:
360+
return AccessedStorageResult::incomplete(
361+
cast<SingleValueInstruction>(sourceAddr)->getOperand(0));
362+
363+
// Access to a Builtin.RawPointer. Treat this like the inductive cases
364+
// above because some RawPointers originate from identified locations. See
365+
// the special case for global addressors, which return RawPointer, above.
366+
//
367+
// If the inductive search does not find a valid addressor, it will
368+
// eventually reach the default case that returns in invalid location. This
369+
// is correct for RawPointer because, although accessing a RawPointer is
370+
// legal SIL, there is no way to guarantee that it doesn't access class or
371+
// global storage, so returning a valid unidentified storage object would be
372+
// incorrect. It is the caller's responsibility to know that formal access
373+
// to such a location can be safely ignored.
374+
//
375+
// For example:
376+
//
377+
// - KeyPath Builtins access RawPointer. However, the caller can check
378+
// that the access `isFromBuilin` and ignore the storage.
379+
//
380+
// - lldb generates RawPointer access for debugger variables, but SILGen
381+
// marks debug VarDecl access as 'Unsafe' and SIL passes don't need the
382+
// AccessedStorage for 'Unsafe' access.
383+
case ValueKind::PointerToAddressInst:
384+
return AccessedStorageResult::incomplete(
385+
cast<SingleValueInstruction>(sourceAddr)->getOperand(0));
386+
387+
// Address-to-address subobject projections.
388+
case ValueKind::StructElementAddrInst:
389+
case ValueKind::TupleElementAddrInst:
390+
case ValueKind::UncheckedTakeEnumDataAddrInst:
391+
case ValueKind::TailAddrInst:
392+
case ValueKind::IndexAddrInst:
393+
return AccessedStorageResult::incomplete(
394+
cast<SingleValueInstruction>(sourceAddr)->getOperand(0));
395+
}
396+
}
277397

278-
case ValueKind::SILPhiArgument: {
279-
auto *phiArg = cast<SILPhiArgument>(address);
280-
if (phiArg->isPhiArgument()) {
281-
SmallVector<SILValue, 8> incomingValues;
282-
phiArg->getIncomingPhiValues(incomingValues);
283-
if (incomingValues.empty())
284-
return AccessedStorage();
285-
286-
auto storage = findAccessedStorage(incomingValues.pop_back_val());
287-
for (auto val : incomingValues) {
288-
auto otherStorage = findAccessedStorage(val);
289-
if (!accessingIdenticalLocations(storage, otherStorage))
290-
return AccessedStorage();
398+
AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
399+
SmallVector<SILValue, 8> addressWorklist({sourceAddr});
400+
SmallPtrSet<SILPhiArgument *, 4> visitedPhis;
401+
Optional<AccessedStorage> storage;
402+
403+
while (!addressWorklist.empty()) {
404+
AccessedStorageResult result =
405+
getAccessedStorageFromAddress(addressWorklist.pop_back_val());
406+
407+
if (!result.isComplete()) {
408+
SILValue operAddr = result.getAddress();
409+
if (auto *phiArg = dyn_cast<SILPhiArgument>(operAddr)) {
410+
if (phiArg->isPhiArgument()) {
411+
if (visitedPhis.insert(phiArg).second)
412+
phiArg->getIncomingPhiValues(addressWorklist);
413+
continue;
291414
}
292-
return storage;
293415
}
294-
// A non-phi block argument may be a box value projected out of
295-
// switch_enum. Address-type block arguments are not allowed.
296-
if (address->getType().isAddress())
297-
return AccessedStorage();
298-
299-
checkSwitchEnumBlockArg(cast<SILPhiArgument>(address));
300-
return AccessedStorage(address, AccessedStorage::Unidentified);
301-
}
302-
// Load a box from an indirect payload of an opaque enum.
303-
// We must have peeked past the project_box earlier in this loop.
304-
// (the indirectness makes it a box, the load is for address-only).
305-
//
306-
// %payload_adr = unchecked_take_enum_data_addr %enum : $*Enum, #Enum.case
307-
// %box = load [take] %payload_adr : $*{ var Enum }
308-
//
309-
// FIXME: this case should go away with opaque values.
310-
//
311-
// Otherwise return invalid AccessedStorage.
312-
case ValueKind::LoadInst: {
313-
if (address->getType().is<SILBoxType>()) {
314-
address = cast<LoadInst>(address)->getOperand();
315-
assert(isa<UncheckedTakeEnumDataAddrInst>(address));
316-
continue;
317-
}
318-
return AccessedStorage();
319-
}
320-
321-
// ref_tail_addr project an address from a reference.
322-
// This is a valid address producer for nested @inout argument
323-
// access, but it is never used for formal access of identified objects.
324-
case ValueKind::RefTailAddrInst:
325-
return AccessedStorage(address, AccessedStorage::Unidentified);
326-
327-
// Inductive cases:
328-
// Look through address casts to find the source address.
329-
case ValueKind::MarkUninitializedInst:
330-
case ValueKind::OpenExistentialAddrInst:
331-
case ValueKind::UncheckedAddrCastInst:
332-
// Inductive cases that apply to any type.
333-
case ValueKind::CopyValueInst:
334-
case ValueKind::MarkDependenceInst:
335-
// Look through a project_box to identify the underlying alloc_box as the
336-
// accesed object. It must be possible to reach either the alloc_box or the
337-
// containing enum in this loop, only looking through simple value
338-
// propagation such as copy_value.
339-
case ValueKind::ProjectBoxInst:
340-
// Handle project_block_storage just like project_box.
341-
case ValueKind::ProjectBlockStorageInst:
342-
// Look through begin_borrow in case a local box is borrowed.
343-
case ValueKind::BeginBorrowInst:
344-
address = cast<SingleValueInstruction>(address)->getOperand(0);
345-
continue;
346-
347-
// Access to a Builtin.RawPointer. Treat this like the inductive cases
348-
// above because some RawPointers originate from identified locations. See
349-
// the special case for global addressors, which return RawPointer, above.
350-
//
351-
// If the inductive search does not find a valid addressor, it will
352-
// eventually reach the default case that returns in invalid location. This
353-
// is correct for RawPointer because, although accessing a RawPointer is
354-
// legal SIL, there is no way to guarantee that it doesn't access class or
355-
// global storage, so returning a valid unidentified storage object would be
356-
// incorrect. It is the caller's responsibility to know that formal access
357-
// to such a location can be safely ignored.
358-
//
359-
// For example:
360-
//
361-
// - KeyPath Builtins access RawPointer. However, the caller can check
362-
// that the access `isFromBuilin` and ignore the storage.
363-
//
364-
// - lldb generates RawPointer access for debugger variables, but SILGen
365-
// marks debug VarDecl access as 'Unsafe' and SIL passes don't need the
366-
// AccessedStorage for 'Unsafe' access.
367-
case ValueKind::PointerToAddressInst:
368-
address = cast<SingleValueInstruction>(address)->getOperand(0);
416+
addressWorklist.push_back(operAddr);
369417
continue;
370-
371-
// Address-to-address subobject projections.
372-
case ValueKind::StructElementAddrInst:
373-
case ValueKind::TupleElementAddrInst:
374-
case ValueKind::UncheckedTakeEnumDataAddrInst:
375-
case ValueKind::TailAddrInst:
376-
case ValueKind::IndexAddrInst:
377-
address = cast<SingleValueInstruction>(address)->getOperand(0);
418+
}
419+
if (!storage.hasValue()) {
420+
storage = result.getStorage();
378421
continue;
379422
}
423+
if (!accessingIdenticalLocations(storage.getValue(), result.getStorage()))
424+
return AccessedStorage();
380425
}
426+
return storage.getValueOr(AccessedStorage());
381427
}
382428

383429
AccessedStorage swift::findAccessedStorageNonNested(SILValue sourceAddr) {

test/SILOptimizer/verifier.sil

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66

77
import Builtin
88

9+
sil_stage canonical
10+
911
// Don't fail in the verifier on a shared unreachable exit block of two loops.
1012
sil @dont_fail: $@convention(thin) (Builtin.Int1) -> () {
1113
bb0(%0 : $Builtin.Int1):
@@ -80,3 +82,28 @@ bb4:
8082
%10 = tuple ()
8183
return %10 : $()
8284
}
85+
86+
// Verify that findAccessedStorage handles cyclic phis.
87+
// <rdar://47059671> swiftc crashes
88+
class AClass {
89+
var aStoredProp: Builtin.Int32
90+
}
91+
92+
sil @testPhiCycle : $@convention(thin) (@guaranteed AClass) -> () {
93+
bb0(%0 : $AClass):
94+
%accessAddr = ref_element_addr %0 : $AClass, #AClass.aStoredProp
95+
br bbloop(%accessAddr : $*Builtin.Int32)
96+
97+
bbloop(%phiAddr : $*Builtin.Int32):
98+
%access = begin_access [read] [dynamic] [no_nested_conflict] %phiAddr : $*Builtin.Int32
99+
%val = load %access : $*Builtin.Int32
100+
end_access %access : $*Builtin.Int32
101+
cond_br undef, bbtail, bbreturn
102+
103+
bbtail:
104+
br bbloop(%phiAddr : $*Builtin.Int32)
105+
106+
bbreturn:
107+
%v = tuple ()
108+
return %v : $()
109+
}

0 commit comments

Comments
 (0)