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

Author: atrick

lib/SIL/MemAccessUtils.cpp

@@ -243,153 +243,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();
+
+  // 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) {

test/SILOptimizer/verifier.sil

@@ -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):
@@ -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 : $()
+}