Merge pull request #17935 from atrick/fix-keypath-exclusivity-verify

Teach exclusivity access marker verifier to handle UnsafePointer.

Author: atrick

lib/SIL/MemAccessUtils.cpp

@@ -181,6 +181,16 @@ static bool isExternalGlobalAddressor(ApplyInst *AI) {
   return funcRef->isGlobalInit() && funcRef->isExternalDeclaration();
 }
 
+// Return true if the given StructExtractInst extracts the RawPointer from
+// Unsafe[Mutable]Pointer.
+static bool isUnsafePointerExtraction(StructExtractInst *SEI) {
+  assert(isa<BuiltinRawPointerType>(SEI->getType().getASTType()));
+  auto &C = SEI->getModule().getASTContext();
+  auto *decl = SEI->getStructDecl();
+  return decl == C.getUnsafeMutablePointerDecl()
+    || decl == C.getUnsafePointerDecl();
+}
+
 // Given an address base is a block argument, verify that it is actually a box
 // projected from a switch_enum. This is a valid pattern at any SIL stage
 // resulting in a block-type phi. In later SIL stages, the optimizer may form
@@ -231,6 +241,12 @@ AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
     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:
@@ -248,6 +264,14 @@ AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
       // 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>(address)))
+        return AccessedStorage(address, AccessedStorage::Unidentified);
+      return AccessedStorage();
+
     // A block argument may be a box value projected out of
     // switch_enum. Address-type block arguments are not allowed.
     case ValueKind::SILPHIArgument:
@@ -276,6 +300,12 @@ AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
       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:
@@ -297,8 +327,8 @@ AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
       continue;
 
     // Access to a Builtin.RawPointer. Treat this like the inductive cases
-    // above because some RawPointer's originate from identified locations. See
-    // the special case for global addressors, which return RawPointer above.
+    // 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
@@ -320,11 +350,10 @@ AccessedStorage swift::findAccessedStorage(SILValue sourceAddr) {
       address = cast<SingleValueInstruction>(address)->getOperand(0);
       continue;
 
-    // Subobject projections.
+    // Address-to-address subobject projections.
     case ValueKind::StructElementAddrInst:
     case ValueKind::TupleElementAddrInst:
     case ValueKind::UncheckedTakeEnumDataAddrInst:
-    case ValueKind::RefTailAddrInst:
     case ValueKind::TailAddrInst:
     case ValueKind::IndexAddrInst:
       address = cast<SingleValueInstruction>(address)->getOperand(0);

test/Interpreter/enforce_exclusive_access.swift

@@ -3,7 +3,6 @@
 //
 // RUN: %target-run %t/a.out
 // REQUIRES: executable_test
-// REQUIRES: rdar41660554
 
 // Tests for traps at run time when enforcing exclusive access.
 
@@ -306,7 +305,7 @@ ExclusiveAccessTestSuite.test("SequentialKeyPathWritesDontOverlap") {
 
   c[keyPath: getF] = 7
   c[keyPath: getF] = 8 // no-trap
-  c[keyPath: getF] += c[keyPath: getF] + 1 // no-trap
+  c[keyPath: getF] += c[keyPath: getF] // no-trap
 }
 
 ExclusiveAccessTestSuite.test("KeyPathInoutKeyPathWriteClassStoredProp")

test/SILOptimizer/exclusivity_static_diagnostics.sil

@@ -1106,3 +1106,31 @@ bb0(%0 : $UnsafePointer<Int32>, %1 : $*Int32):
   %v = tuple ()
   return %v : $()
 }
+
+// <rdar://problem/41660554> Swift CI (macOS release master, OSS): SIL verification failed: Unknown formal access pattern: storage
+// Test access marker verification of a KeyPath projection with nested @inout access after inlining.
+sil @takeInoutInt : $@convention(thin) (@inout Int) -> ()
+
+// The compiler intrinsic _projectXXXKeyPath returns a tuple of
+// UnsafePointer and Optional AnyObject. After inlining, the unsafe
+// pointer may appear to originate from anywhere, including a phi.
+// There's currently no way to tell that the UnsafePointer originated
+// from a KeyPath projection. This pattern could occur with any
+// addressor. In either case, the nested access is valid but
+// unidentified. Addressors that require enforcement must start a
+// dynamic access within the addressor itself, before returning an
+// UnsafePointer.
+sil @testNestedKeypathAccess : $@convention(thin) (@guaranteed (UnsafeMutablePointer<Int>, Optional<AnyObject>)) -> () {
+bb0(%0 : $(UnsafeMutablePointer<Int>, Optional<AnyObject>)):
+  %up = tuple_extract %0 : $(UnsafeMutablePointer<Int>, Optional<AnyObject>), 0
+  %o = tuple_extract %0 : $(UnsafeMutablePointer<Int>, Optional<AnyObject>), 1
+  %p = struct_extract %up : $UnsafeMutablePointer<Int>, #UnsafeMutablePointer._rawValue
+  %adr = pointer_to_address %p : $Builtin.RawPointer to [strict] $*Int
+  %dep = mark_dependence %adr : $*Int on %o : $Optional<AnyObject>
+  %access = begin_access [modify] [static] %dep : $*Int
+  %f = function_ref @takeInoutInt : $@convention(thin) (@inout Int) -> ()
+  %call = apply %f(%access) : $@convention(thin) (@inout Int) -> ()
+  end_access %access : $*Int
+  %v = tuple ()
+  return %v : $()
+}