[Exclusivity] Enforce exclusive access for class offsets in KeyPaths

devincoughlin · devincoughlin · commit d1757fce3925 · 2018-03-31T11:55:15.000-07:00
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
diff --git a/include/swift/AST/Builtins.def b/include/swift/AST/Builtins.def
@@ -294,6 +294,27 @@ BUILTIN_SIL_OPERATION(Gep, "gep", Integer)
 /// allocated element type 'E'.
 BUILTIN_SIL_OPERATION(GetTailAddr, "getTailAddr", Integer)
 
+/// performInstantaneousReadAccess(Builtin.RawPointer, T.Type) -> ()
+/// Begin and then immediately end a read access to the given raw pointer,
+/// which will be treated as an address of type 'T'.
+BUILTIN_SIL_OPERATION(PerformInstantaneousReadAccess,
+                      "performInstantaneousReadAccess", Special)
+
+/// beginUnpairedModifyAccess(Builtin.RawPointer, Builtin.RawPointer,
+///                           T.Type) -> ()
+/// Begins but does not end a 'modify' access to the first raw pointer argument.
+/// The second raw pointer must be a pointer to an UnsafeValueBuffer, which
+/// will be used by the runtime to record the access. The lifetime of the
+/// value buffer must be longer than that of the access itself. The accessed
+/// address will be treated as having type 'T'.
+BUILTIN_SIL_OPERATION(BeginUnpairedModifyAccess, "beginUnpairedModifyAccess",
+                      Special)
+
+/// endUnpairedAccess(Builtin.RawPointer) -> ()
+/// Ends an in-progress unpaired access. The raw pointer argument must be
+/// be a pointer to an UnsafeValueBuffer that records an in progress access.
+BUILTIN_SIL_OPERATION(EndUnpairedAccess, "endUnpairedAccess", Special)
+
 /// condfail(Int1) -> ()
 /// Triggers a runtime failure if the condition is true.
 BUILTIN_SIL_OPERATION(CondFail, "condfail", Special)
diff --git a/lib/AST/Builtins.cpp b/lib/AST/Builtins.cpp
@@ -738,6 +738,31 @@ static ValueDecl *getGetTailAddrOperation(ASTContext &Context, Identifier Id,
   return builder.build(Id);
 }
 
+static ValueDecl *getBeginUnpairedAccessOperation(ASTContext &Context,
+                                                  Identifier Id) {
+  BuiltinGenericSignatureBuilder builder(Context);
+  builder.addParameter(makeConcrete(Context.TheRawPointerType));
+  builder.addParameter(makeConcrete(Context.TheRawPointerType));
+  builder.addParameter(makeMetatype(makeGenericParam(0)));
+  builder.setResult(makeConcrete(Context.TheEmptyTupleType));
+  return builder.build(Id);
+}
+
+static ValueDecl *
+getPerformInstantaneousReadAccessOperation(ASTContext &Context,
+                                           Identifier Id) {
+  BuiltinGenericSignatureBuilder builder(Context);
+  builder.addParameter(makeConcrete(Context.TheRawPointerType));
+  builder.addParameter(makeMetatype(makeGenericParam(0)));
+  builder.setResult(makeConcrete(Context.TheEmptyTupleType));
+  return builder.build(Id);
+}
+
+static ValueDecl *getEndUnpairedAccessOperation(ASTContext &Context,
+                                                Identifier Id) {
+  return getBuiltinFunction(Id, { Context.TheRawPointerType },
+                                Context.TheEmptyTupleType);
+}
 static ValueDecl *getSizeOrAlignOfOperation(ASTContext &Context,
                                             Identifier Id) {
   BuiltinGenericSignatureBuilder builder(Context);
@@ -1644,6 +1669,18 @@ ValueDecl *swift::getBuiltinValueDecl(ASTContext &Context, Identifier Id) {
     if (Types.size() != 1) return nullptr;
     return getGetTailAddrOperation(Context, Id, Types[0]);
 
+  case BuiltinValueKind::PerformInstantaneousReadAccess:
+    if (!Types.empty()) return nullptr;
+      return getPerformInstantaneousReadAccessOperation(Context, Id);
+
+  case BuiltinValueKind::BeginUnpairedModifyAccess:
+    if (!Types.empty()) return nullptr;
+    return getBeginUnpairedAccessOperation(Context, Id);
+
+  case BuiltinValueKind::EndUnpairedAccess:
+    if (!Types.empty()) return nullptr;
+    return getEndUnpairedAccessOperation(Context, Id);
+
 #define BUILTIN(id, name, Attrs)
 #define BUILTIN_BINARY_OPERATION(id, name, attrs, overload)  case BuiltinValueKind::id:
 #include "swift/AST/Builtins.def"
diff --git a/lib/SIL/SILOwnershipVerifier.cpp b/lib/SIL/SILOwnershipVerifier.cpp
@@ -1431,6 +1431,9 @@ BUILTINS_THAT_SHOULD_HAVE_BEEN_LOWERED_TO_SILINSTS(AddressOf)
 BUILTINS_THAT_SHOULD_HAVE_BEEN_LOWERED_TO_SILINSTS(GepRaw)
 BUILTINS_THAT_SHOULD_HAVE_BEEN_LOWERED_TO_SILINSTS(Gep)
 BUILTINS_THAT_SHOULD_HAVE_BEEN_LOWERED_TO_SILINSTS(GetTailAddr)
+BUILTINS_THAT_SHOULD_HAVE_BEEN_LOWERED_TO_SILINSTS(PerformInstantaneousReadAccess)
+BUILTINS_THAT_SHOULD_HAVE_BEEN_LOWERED_TO_SILINSTS(BeginUnpairedModifyAccess)
+BUILTINS_THAT_SHOULD_HAVE_BEEN_LOWERED_TO_SILINSTS(EndUnpairedAccess)
 BUILTINS_THAT_SHOULD_HAVE_BEEN_LOWERED_TO_SILINSTS(CondFail)
 BUILTINS_THAT_SHOULD_HAVE_BEEN_LOWERED_TO_SILINSTS(FixLifetime)
 BUILTINS_THAT_SHOULD_HAVE_BEEN_LOWERED_TO_SILINSTS(IsUnique)
diff --git a/lib/SIL/ValueOwnershipKindClassifier.cpp b/lib/SIL/ValueOwnershipKindClassifier.cpp
@@ -475,6 +475,9 @@ CONSTANT_OWNERSHIP_BUILTIN(Trivial, AddressOf)
 CONSTANT_OWNERSHIP_BUILTIN(Trivial, GepRaw)
 CONSTANT_OWNERSHIP_BUILTIN(Trivial, Gep)
 CONSTANT_OWNERSHIP_BUILTIN(Trivial, GetTailAddr)
+CONSTANT_OWNERSHIP_BUILTIN(Trivial, PerformInstantaneousReadAccess)
+CONSTANT_OWNERSHIP_BUILTIN(Trivial, BeginUnpairedModifyAccess)
+CONSTANT_OWNERSHIP_BUILTIN(Trivial, EndUnpairedAccess)
 CONSTANT_OWNERSHIP_BUILTIN(Trivial, OnFastPath)
 CONSTANT_OWNERSHIP_BUILTIN(Trivial, IsUnique)
 CONSTANT_OWNERSHIP_BUILTIN(Trivial, IsUniqueOrPinned)
diff --git a/lib/SILGen/SILGenBuiltin.cpp b/lib/SILGen/SILGenBuiltin.cpp
@@ -517,6 +517,90 @@ static ManagedValue emitBuiltinGetTailAddr(SILGenFunction &SGF,
   return ManagedValue::forUnmanaged(addr);
 }
 
+/// Specialized emitter for Builtin.beginUnpairedModifyAccess.
+static ManagedValue emitBuiltinBeginUnpairedModifyAccess(SILGenFunction &SGF,
+                                                    SILLocation loc,
+                                           SubstitutionList substitutions,
+                                           ArrayRef<ManagedValue> args,
+                                           SGFContext C) {
+  assert(substitutions.size() == 1 &&
+        "Builtin.beginUnpairedModifyAccess should have one substitution");
+  assert(args.size() == 3 &&
+         "beginUnpairedModifyAccess should be given three arguments");
+
+  SILType elemTy = SGF.getLoweredType(substitutions[0].getReplacement());
+  SILValue addr = SGF.B.createPointerToAddress(loc, args[0].getUnmanagedValue(),
+                                               elemTy.getAddressType(),
+                                               /*strict*/ true,
+                                               /*invariant*/ false);
+
+  SILType valueBufferTy =
+      SGF.getLoweredType(SGF.getASTContext().TheUnsafeValueBufferType);
+
+  SILValue buffer = SGF.B.createPointerToAddress(loc, args[1].getUnmanagedValue(),
+                                                 valueBufferTy.getAddressType(),
+                                                 /*strict*/ true,
+                                                 /*invariant*/ false);
+  SGF.B.createBeginUnpairedAccess(loc, addr, buffer, SILAccessKind::Modify,
+                                  SILAccessEnforcement::Dynamic,
+                                  /*noNestedConflict*/ false);
+
+  return ManagedValue::forUnmanaged(SGF.emitEmptyTuple(loc));
+}
+
+/// Specialized emitter for Builtin.performInstantaneousReadAccess
+static ManagedValue emitBuiltinPerformInstantaneousReadAccess(SILGenFunction &SGF,
+                                                       SILLocation loc,
+                                                       SubstitutionList substitutions,
+                                                       ArrayRef<ManagedValue> args,
+                                                       SGFContext C) {
+  assert(substitutions.size() == 1 &&
+         "Builtin.performInstantaneousReadAccess should have one substitution");
+  assert(args.size() == 2 &&
+         "Builtin.performInstantaneousReadAccess should be given "
+         "two arguments");
+
+  SILType elemTy = SGF.getLoweredType(substitutions[0].getReplacement());
+  SILValue addr = SGF.B.createPointerToAddress(loc, args[0].getUnmanagedValue(),
+                                               elemTy.getAddressType(),
+                                               /*strict*/ true,
+                                               /*invariant*/ false);
+
+  // Begin and then immediately end a read access. No nested conflict is
+  // possible because there are no instructions between the begin and the
+  // end of the access.
+  SILValue access = SGF.B.createBeginAccess(loc, addr, SILAccessKind::Read,
+                                            SILAccessEnforcement::Dynamic,
+                                            /*noNestedConflict*/ true);
+  SGF.B.createEndAccess(loc, access, /*aborted*/ false);
+
+  return ManagedValue::forUnmanaged(SGF.emitEmptyTuple(loc));
+}
+
+/// Specialized emitter for Builtin.endUnpairedAccessModifyAccess.
+static ManagedValue emitBuiltinEndUnpairedAccess(SILGenFunction &SGF,
+                                                    SILLocation loc,
+                                           SubstitutionList substitutions,
+                                           ArrayRef<ManagedValue> args,
+                                           SGFContext C) {
+  assert(substitutions.size() == 0 &&
+        "Builtin.endUnpairedAccess should have no substitutions");
+  assert(args.size() == 1 &&
+         "endUnpairedAccess should be given one argument");
+
+  SILType valueBufferTy =
+      SGF.getLoweredType(SGF.getASTContext().TheUnsafeValueBufferType);
+
+  SILValue buffer = SGF.B.createPointerToAddress(loc, args[0].getUnmanagedValue(),
+                                                 valueBufferTy.getAddressType(),
+                                                 /*strict*/ true,
+                                                 /*invariant*/ false);
+  SGF.B.createEndUnpairedAccess(loc, buffer, SILAccessEnforcement::Dynamic,
+                                /*aborted*/ false);
+
+  return ManagedValue::forUnmanaged(SGF.emitEmptyTuple(loc));
+}
+
 /// Specialized emitter for Builtin.condfail.
 static ManagedValue emitBuiltinCondFail(SILGenFunction &SGF,
                                         SILLocation loc,
diff --git a/stdlib/public/core/KeyPath.swift b/stdlib/public/core/KeyPath.swift
@@ -644,24 +644,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)
   @usableFromInline // FIXME(sil-serialize-all)
-  internal init(previous: AnyObject?, instance: AnyObject) {
-    self.previous = previous
-    self.instance = instance
+  internal init() {
+    _sanityCheckFailure("use _create(...)")
   }
+
   @inlinable // FIXME(sil-serialize-all)
   @usableFromInline // FIXME(sil-serialize-all)
-  deinit {}
+  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.
@@ -1253,7 +1302,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)
 
@@ -1313,12 +1370,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):
diff --git a/test/Interpreter/enforce_exclusive_access.swift b/test/Interpreter/enforce_exclusive_access.swift
diff --git a/test/Interpreter/enforce_exclusive_access_backtrace.swift b/test/Interpreter/enforce_exclusive_access_backtrace.swift
diff --git a/test/SILGen/builtins.swift b/test/SILGen/builtins.swift
