[Exclusivity] Enforce exclusive access for class offsets in KeyPaths

Add dynamic enforcement of exclusive access when a KeyPath directly accesses a final
stored property on an instance of a class. For read-only projections, this begins and ends
the access immediately. For mutable projections, this uses the ClassHolder to perform
a long-term access that lasts as long as the lifetime of the ClassHolder.

rdar://problem/31972680

Author: devincoughlin

stdlib/public/core/KeyPath.swift

@@ -626,22 +626,73 @@ internal enum KeyPathComponent: Hashable {
 }
 
 // A class that maintains ownership of another object while a mutable projection
-// into it is underway.
+// into it is underway. The lifetime of the instance of this class is also used
+// to begin and end exclusive 'modify' access to the projected address.
 @_fixed_layout // FIXME(sil-serialize-all)
 @usableFromInline // FIXME(sil-serialize-all)
-internal final class ClassHolder {
+internal final class ClassHolder<ProjectionType> {
+
+  /// The type of the scratch record passed to the runtime to record
+  /// accesses to guarantee exlcusive access.
+  internal typealias AccessRecord = Builtin.UnsafeValueBuffer
+
   @usableFromInline // FIXME(sil-serialize-all)
-  internal let previous: AnyObject?
+  internal var previous: AnyObject?
   @usableFromInline // FIXME(sil-serialize-all)
-  internal let instance: AnyObject
+  internal var instance: AnyObject
 
   @inlinable // FIXME(sil-serialize-all)
   internal init(previous: AnyObject?, instance: AnyObject) {
     self.previous = previous
     self.instance = instance
   }
+
   @inlinable // FIXME(sil-serialize-all)
-  deinit {}
+  @usableFromInline // FIXME(sil-serialize-all)
+  internal final class func _create(
+      previous: AnyObject?,
+      instance: AnyObject,
+      accessingAddress address: UnsafeRawPointer,
+      type: ProjectionType.Type
+  ) -> ClassHolder {
+
+    // Tail allocate the UnsafeValueBuffer used as the AccessRecord.
+    // This avoids a second heap allocation since there is no source-level way to
+    // initialize a Builtin.UnsafeValueBuffer type and thus we cannot have a
+    // stored property of that type.
+    let holder: ClassHolder = Builtin.allocWithTailElems_1(self,
+                                                          1._builtinWordValue,
+                                                          AccessRecord.self)
+
+    // Initialize the ClassHolder's instance variables. This is done via
+    // withUnsafeMutablePointer(to:) because the instance was just allocated with
+    // allocWithTailElems_1 and so we need to make sure to use an initialization
+    // rather than an assignment.
+    withUnsafeMutablePointer(to: &holder.previous) {
+      $0.initialize(to: previous)
+    }
+
+    withUnsafeMutablePointer(to: &holder.instance) {
+      $0.initialize(to: instance)
+    }
+
+    let accessRecordPtr = Builtin.projectTailElems(holder, AccessRecord.self)
+
+    // Begin a 'modify' access to the address. This access is ended in
+    // ClassHolder's deinitializer.
+    Builtin.beginUnpairedModifyAccess(address._rawValue, accessRecordPtr, type)
+
+    return holder
+  }
+
+  @inlinable // FIXME(sil-serialize-all)
+  @usableFromInline // FIXME(sil-serialize-all)
+  deinit {
+    let accessRecordPtr = Builtin.projectTailElems(self, AccessRecord.self)
+
+    // Ends the access begun in _create().
+    Builtin.endUnpairedAccess(accessRecordPtr)
+  }
 }
 
 // A class that triggers writeback to a pointer when destroyed.
@@ -1185,7 +1236,15 @@ internal struct RawKeyPathComponent {
       let baseObj = unsafeBitCast(base, to: AnyObject.self)
       let basePtr = UnsafeRawPointer(Builtin.bridgeToRawPointer(baseObj))
       defer { _fixLifetime(baseObj) }
-      return .continue(basePtr.advanced(by: offset)
+
+      let offsetAddress = basePtr.advanced(by: offset)
+
+      // Perform an instaneous record access on the address in order to
+      // ensure that the read will not conflict with an already in-progress
+      // 'modify' access.
+      Builtin.performInstantaneousReadAccess(offsetAddress._rawValue,
+                                             NewValue.self)
+      return .continue(offsetAddress
         .assumingMemoryBound(to: NewValue.self)
         .pointee)
 
@@ -1244,12 +1303,16 @@ internal struct RawKeyPathComponent {
       // AnyObject memory can alias any class reference memory, so we can
       // assume type here
       let object = base.assumingMemoryBound(to: AnyObject.self).pointee
-      // The base ought to be kept alive for the duration of the derived access
-      keepAlive = keepAlive == nil
-        ? object
-        : ClassHolder(previous: keepAlive, instance: object)
-      return UnsafeRawPointer(Builtin.bridgeToRawPointer(object))
+      let offsetAddress = UnsafeRawPointer(Builtin.bridgeToRawPointer(object))
             .advanced(by: offset)
+
+      // Keep the  base alive for the duration of the derived access and also
+      // enforce exclusive access to the address.
+      keepAlive = ClassHolder._create(previous: keepAlive, instance: object,
+                                      accessingAddress: offsetAddress,
+                                      type: NewValue.self)
+
+      return offsetAddress
 
     case .mutatingGetSet(id: _, get: let rawGet, set: let rawSet,
                          argument: let argument):

test/Interpreter/enforce_exclusive_access.swift

@@ -241,26 +241,95 @@ ExclusiveAccessTestSuite.test("InoutWriteEscapeWrite")
 }
 
 class ClassWithStoredProperty {
-  var f = 7
+  final var f = 7
 }
 
-// FIXME: This should trap with a modify/modify violation at run time.
-ExclusiveAccessTestSuite.test("KeyPathClassStoredProp")
+ExclusiveAccessTestSuite.test("KeyPathInoutDirectWriteClassStoredProp")
   .skip(.custom(
     { _isFastAssertConfiguration() },
     reason: "this trap is not guaranteed to happen in -Ounchecked"))
-//  .crashOutputMatches("Previous access (a modification) started at")
-//  .crashOutputMatches("Current access (a modification) started at")
+  .crashOutputMatches("Previous access (a modification) started at")
+  .crashOutputMatches("Current access (a modification) started at")
+  .code
+{
+  let getF = \ClassWithStoredProperty.f
+  let c = ClassWithStoredProperty()
+
+  expectCrashLater()
+  modifyAndPerform(&c[keyPath: getF]) {
+    c.f = 12
+  }
+}
+
+ExclusiveAccessTestSuite.test("KeyPathInoutDirectReadClassStoredProp")
+  .skip(.custom(
+    { _isFastAssertConfiguration() },
+    reason: "this trap is not guaranteed to happen in -Ounchecked"))
+  .crashOutputMatches("Previous access (a modification) started at")
+  .crashOutputMatches("Current access (a read) started at")
   .code
 {
   let getF = \ClassWithStoredProperty.f
   let c = ClassWithStoredProperty()
 
-//  expectCrashLater()
+  expectCrashLater()
+  modifyAndPerform(&c[keyPath: getF]) {
+    let x = c.f
+    _blackHole(x)
+  }
+}
+
+// Unlike inout accesses, read-only inout-to-pointer conversions on key paths for
+// final stored-properties do not perform a long-term read access. Instead, they
+// materialize a location on the stack, perform an instantaneous read
+// from the storage indicated by the key path and write the read value to the
+// stack location. The stack location is then passed as the pointer for the
+// inout-to-pointer conversion.
+//
+// This means that there is no conflict between a read-only inout-to-pointer
+// conversion of the key path location for a call and an access to the
+// the same location within the call.
+ExclusiveAccessTestSuite.test("KeyPathReadOnlyInoutToPtrDirectWriteClassStoredProp") {
+  let getF = \ClassWithStoredProperty.f
+  let c = ClassWithStoredProperty()
+
+  // This performs an instantaneous read
+  readAndPerform(&c[keyPath: getF]) {
+    c.f = 12 // no-trap
+  }
+}
+
+ExclusiveAccessTestSuite.test("SequentialKeyPathWritesDontOverlap") {
+  let getF = \ClassWithStoredProperty.f
+  let c = ClassWithStoredProperty()
+
+  c[keyPath: getF] = 7
+  c[keyPath: getF] = 8 // no-trap
+  c[keyPath: getF] += c[keyPath: getF] + 1 // no-trap
+}
+
+// This does not trap, for now, because the standard library (and thus KeyPath) is
+// compiled in Swift 3 mode and we currently log rather than trap in Swift mode.
+ExclusiveAccessTestSuite.test("KeyPathInoutKeyPathWriteClassStoredProp")
+{
+  let getF = \ClassWithStoredProperty.f
+  let c = ClassWithStoredProperty()
+
   modifyAndPerform(&c[keyPath: getF]) {
     c[keyPath: getF] = 12
   }
 }
 
+// This does not currently trap because the standard library is compiled in Swift 3 mode,
+// which logs.
+ExclusiveAccessTestSuite.test("KeyPathInoutKeyPathReadClassStoredProp") {
+  let getF = \ClassWithStoredProperty.f
+  let c = ClassWithStoredProperty()
+
+  modifyAndPerform(&c[keyPath: getF]) {
+    let y = c[keyPath: getF]
+    _blackHole(y)
+  }
+}
 
 runAllTests()

test/Interpreter/enforce_exclusive_access_backtrace.swift

@@ -17,16 +17,61 @@ func readAndPerform<T>(_ _: UnsafePointer<T>, closure: () ->()) {
   closure()
 }
 
+func writeAndPerform<T>(_ _: UnsafeMutablePointer<T>, closure: () ->()) {
+  closure()
+}
+
 var globalX = X()
 withUnsafePointer(to: &globalX) { _ = fputs(String(format: "globalX: 0x%lx\n", Int(bitPattern: $0)), stderr) }
 // CHECK: globalX: [[ADDR:0x.*]]
 
+fputs("Global Access\n", stderr);
 readAndPerform(&globalX) {
   globalX = X()
+  // CHECK-LABEL: Global Access
   // CHECK: Simultaneous accesses to [[ADDR]], but modification requires exclusive access.
   // CHECK: Previous access (a read) started at a.out`main + {{.*}} (0x{{.*}}).
   // CHECK: Current access (a modification) started at:
   // CHECK: a.out {{.*}} closure
   // CHECK: a.out {{.*}} readAndPerform
   // CHECK: a.out {{.*}} main
 }
+
+class C {
+  final var f = 7
+}
+
+var c = C()
+withUnsafePointer(to: &c.f) { _ = fputs(String(format: "c.f: 0x%lx\n", Int(bitPattern: $0)), stderr) }
+// CHECK: c.f: [[C_F_ADDR:0x.*]]
+
+// Key path accesses are performed in the Standard Library. The Standard Library
+// is currently compiled in Swift 3 mode and the compiler currently logs conflicts
+// (rather than trapping on them) code compiled in Swift 3 mode. For this reason
+// conflicts where the second conflicting access is via a key path will log rather than
+// trap.
+
+fputs("Overlapping Key Path Write Accesses\n", stderr);
+writeAndPerform(&c[keyPath: \C.f]) {
+  c[keyPath: \C.f] = 8
+  // CHECK-LABEL: Overlapping Key Path Write Accesses
+  // CHECK: Simultaneous accesses to [[C_F_ADDR]], but modification requires exclusive access.
+  // CHECK: Previous access (a modification)
+  // CHECK: Current access (a modification)
+  // CHECK: a.out {{.*}} closure
+  // CHECK: a.out {{.*}} writeAndPerform
+  // CHECK: a.out {{.*}} main
+}
+
+fputs("Overlapping Key Path Write Access and Key Path Read Access\n", stderr);
+writeAndPerform(&c[keyPath: \C.f]) {
+  let x = c[keyPath: \C.f]
+  _blackHole(x)
+  // CHECK-LABEL: Overlapping Key Path Write Access and Key Path Read Access
+  // CHECK: Simultaneous accesses to [[C_F_ADDR]], but modification requires exclusive access.
+  // CHECK: Previous access (a modification)
+  // CHECK: Current access (a read)
+  // CHECK: a.out {{.*}} closure
+  // CHECK: a.out {{.*}} writeAndPerform
+  // CHECK: a.out {{.*}} main
+}

test/SILGen/builtins.swift

@@ -370,31 +370,26 @@ func getTailAddr<T1, T2>(start: Builtin.RawPointer, i: Builtin.Word, ty1: T1.Typ
 }
 
 // CHECK-LABEL: sil hidden @$S8builtins25beginUnpairedModifyAccess{{[_0-9a-zA-Z]*}}F
-// CHECK: [[P2A_ADDR:%.*]] = pointer_to_address %0
-// CHECK: [[P2A_SCRATCH:%.*]] = pointer_to_address %1
-// CHECK: begin_unpaired_access [modify] [dynamic] [[P2A_ADDR]] : $*T1, [[P2A_SCRATCH]] : $*Builtin.UnsafeValueBuffer
-// CHECK: [[RESULT:%.*]] = tuple ()
-// CHECK: [[RETURN:%.*]] = tuple ()
-// CHECK: return [[RETURN]] : $()
-// CHECK: } // end sil function '$S8builtins25beginUnpairedModifyAccess{{[_0-9a-zA-Z]*}}F'
 func beginUnpairedModifyAccess<T1>(address: Builtin.RawPointer, scratch: Builtin.RawPointer, ty1: T1.Type) {
-  Builtin.beginUnpairedModifyAccess(address, scratch, ty1);
-}
+  // CHECK: [[P2A_ADDR:%.*]] = pointer_to_address %0
+  // CHECK: [[P2A_SCRATCH:%.*]] = pointer_to_address %1
+  // CHECK: begin_unpaired_access [modify] [dynamic] [[P2A_ADDR]] : $*T1, [[P2A_SCRATCH]] : $*Builtin.UnsafeValueBuffer
+  // CHECK: [[RESULT:%.*]] = tuple ()
+  // CHECK: [[RETURN:%.*]] = tuple ()
+  // CHECK: return [[RETURN]] : $()
 
-// CHECK-LABEL: sil hidden @$S8builtins17endUnpairedAccess{{[_0-9a-zA-Z]*}}F : $@convention(thin) (Builtin.RawPointer) -> () {
-// CHECK: [[P2A_ADDR:%.*]] = pointer_to_address %0
-// CHECK: end_unpaired_access [dynamic] [[P2A_ADDR]]
-// CHECK: } // end sil function '$S8builtins17endUnpairedAccess{{[_0-9a-zA-Z]*}}F'
-func endUnpairedAccess(address: Builtin.RawPointer) {
-  Builtin.endUnpairedAccess(address)
+  Builtin.beginUnpairedModifyAccess(address, scratch, ty1);
 }
 
 // CHECK-LABEL: sil hidden @$S8builtins30performInstantaneousReadAccess{{[_0-9a-zA-Z]*}}F
-// CHECK: [[P2A_ADDR:%.*]] = pointer_to_address %0
-// CHECK: [[ACCESS:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[P2A_ADDR]] : $*T1
-// CHECK-NEXT: end_access [[ACCESS]] : $*T1
-// CHECK: } // end sil function '$S8builtins30performInstantaneousReadAccess{{[_0-9a-zA-Z]*}}F'
 func performInstantaneousReadAccess<T1>(address: Builtin.RawPointer, scratch: Builtin.RawPointer, ty1: T1.Type) {
+  // CHECK: [[P2A_ADDR:%.*]] = pointer_to_address %0
+  // CHECK: [[ACCESS:%.*]] = begin_access [read] [dynamic] [no_nested_conflict] [[P2A_ADDR]] : $*T1
+  // CHECK-NEXT: end_access [[ACCESS]] : $*T1
+  // CHECK: [[RESULT:%.*]] = tuple ()
+  // CHECK: [[RETURN:%.*]] = tuple ()
+  // CHECK: return [[RETURN]] : $()
+
   Builtin.performInstantaneousReadAccess(address, ty1);
 }