Merge pull request #16051 from atrick/exclusive-key-path-fix

[Exclusivity] Enforce exclusive access for class offsets in KeyPaths

Author: atrick

include/swift/Runtime/Exclusivity.h

@@ -58,6 +58,19 @@ void swift_endAccess(ValueBuffer *buffer);
 SWIFT_RUNTIME_EXPORT
 bool _swift_disableExclusivityChecking;
 
+#ifndef NDEBUG
+
+/// Dump all accesses currently tracked by the runtime.
+///
+/// This is a debug routine that is intended to be used from the debugger and is
+/// compiled out when asserts are disabled. The intention is that it allows one
+/// to dump the access state to easily see if/when exclusivity violations will
+/// happen. This eases debugging.
+SWIFT_RUNTIME_EXPORT
+void swift_dumpTrackedAccesses();
+
+#endif
+
 } // end namespace swift
 
 #endif

include/swift/Strings.h

@@ -13,6 +13,9 @@
 #ifndef SWIFT_STRINGS_H
 #define SWIFT_STRINGS_H
 
+#include "swift/Basic/LLVM.h"
+#include "llvm/ADT/StringRef.h"
+
 namespace swift {
 
 /// The extension for serialized modules.
@@ -89,6 +92,10 @@ constexpr static const char BUILTIN_TYPE_NAME_VEC[] = "Builtin.Vec";
 constexpr static const char BUILTIN_TYPE_NAME_SILTOKEN[] = "Builtin.SILToken";
 /// The name of the Builtin type for Word
 constexpr static const char BUILTIN_TYPE_NAME_WORD[] = "Builtin.Word";
+
+constexpr static StringLiteral OPTIMIZE_SIL_PRESERVE_EXCLUSIVITY =
+    "optimize.sil.preserve_exclusivity";
+
 } // end namespace swift
 
 #endif // SWIFT_STRINGS_H

lib/IRGen/GenDecl.cpp

@@ -2310,11 +2310,19 @@ llvm::Function *IRGenModule::getAddrOfSILFunction(SILFunction *f,
 
   LinkInfo link = LinkInfo::get(*this, entity, forDefinition);
 
-  if (f->getInlineStrategy() == NoInline) {
+  switch (f->getInlineStrategy()) {
+  case NoInline:
     attrs = attrs.addAttribute(signature.getType()->getContext(),
                                llvm::AttributeList::FunctionIndex,
                                llvm::Attribute::NoInline);
+    break;
+  case AlwaysInline:
+    // FIXME: We do not currently transfer AlwaysInline since doing so results
+    // in test failures, which must be investigated first.
+  case InlineDefault:
+    break;
   }
+
   if (isReadOnlyFunction(f)) {
     attrs = attrs.addAttribute(signature.getType()->getContext(),
                                llvm::AttributeList::FunctionIndex,

lib/SILGen/SILGenBuiltin.cpp

@@ -574,10 +574,11 @@ static ManagedValue emitBuiltinBeginUnpairedModifyAccess(SILGenFunction &SGF,
   SILType valueBufferTy =
       SGF.getLoweredType(SGF.getASTContext().TheUnsafeValueBufferType);
 
-  SILValue buffer = SGF.B.createPointerToAddress(loc, args[1].getUnmanagedValue(),
-                                                 valueBufferTy.getAddressType(),
-                                                 /*strict*/ true,
-                                                 /*invariant*/ false);
+  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);
@@ -586,11 +587,10 @@ static ManagedValue emitBuiltinBeginUnpairedModifyAccess(SILGenFunction &SGF,
 }
 
 /// Specialized emitter for Builtin.performInstantaneousReadAccess
-static ManagedValue emitBuiltinPerformInstantaneousReadAccess(SILGenFunction &SGF,
-                                                       SILLocation loc,
-                                                       SubstitutionList substitutions,
-                                                       ArrayRef<ManagedValue> args,
-                                                       SGFContext C) {
+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 &&
@@ -603,13 +603,21 @@ static ManagedValue emitBuiltinPerformInstantaneousReadAccess(SILGenFunction &SG
                                                /*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);
+  SILType valueBufferTy =
+    SGF.getLoweredType(SGF.getASTContext().TheUnsafeValueBufferType);
+  SILValue unusedBuffer = SGF.emitTemporaryAllocation(loc, valueBufferTy);
+
+  // Begin an "unscoped" read access. No nested conflict is possible because
+  // the compiler should generate the actual read for the KeyPath expression
+  // immediately after the call to this builtin, which forms the address of
+  // that real access. When noNestedConflict=true, no EndUnpairedAccess should
+  // be emitted.
+  //
+  // Unpaired access is necessary because a BeginAccess/EndAccess pair with no
+  // use will be trivially optimized away.
+  SGF.B.createBeginUnpairedAccess(loc, addr, unusedBuffer, SILAccessKind::Read,
+                                  SILAccessEnforcement::Dynamic,
+                                  /*noNestedConflict*/ true);
 
   return ManagedValue::forUnmanaged(SGF.emitEmptyTuple(loc));
 }

lib/SILOptimizer/Mandatory/AccessMarkerElimination.cpp

@@ -19,12 +19,19 @@
 /// test cases through the pipeline and exercising SIL verification before all
 /// passes support access markers.
 ///
+/// This must only run before inlining _semantic calls. If we inline and drop
+/// the @_semantics("optimize.sil.preserve_exclusivity") attribute, the inlined
+/// markers will be eliminated, but the noninlined markers will not. This would
+/// result in inconsistent begin/end_unpaired_access resulting in unpredictable,
+/// potentially catastrophic runtime behavior.
+///
 //===----------------------------------------------------------------------===//
 
 #define DEBUG_TYPE "access-marker-elim"
 #include "swift/Basic/Range.h"
 #include "swift/SIL/SILFunction.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
+#include "swift/Strings.h"
 #include "llvm/Support/CommandLine.h"
 
 using namespace swift;
@@ -167,6 +174,12 @@ struct AccessMarkerEliminationPass : SILModuleTransform {
   void run() override {
     auto &M = *getModule();
     for (auto &F : M) {
+      if (F.hasSemanticsAttr(OPTIMIZE_SIL_PRESERVE_EXCLUSIVITY)) {
+        DEBUG(llvm::dbgs() << "Skipping " << F.getName() << ". Found "
+                           << OPTIMIZE_SIL_PRESERVE_EXCLUSIVITY << " tag!\n");
+        continue;
+      }
+
       bool removedAny = AccessMarkerElimination(&F).stripMarkers();
 
       // Only invalidate analyses if we removed some markers.

lib/SILOptimizer/Mandatory/IRGenPrepare.cpp

@@ -20,12 +20,14 @@
 ///
 //===----------------------------------------------------------------------===//
 
-#include "swift/SILOptimizer/PassManager/Passes.h"
+#define DEBUG_TYPE "sil-cleanup"
 #include "swift/SIL/SILFunction.h"
 #include "swift/SIL/SILInstruction.h"
 #include "swift/SIL/SILModule.h"
-#include "swift/SILOptimizer/Utils/Local.h"
+#include "swift/SILOptimizer/PassManager/Passes.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
+#include "swift/SILOptimizer/Utils/Local.h"
+#include "swift/Strings.h"
 
 using namespace swift;
 
@@ -68,10 +70,12 @@ namespace {
 
 class IRGenPrepare : public SILFunctionTransform {
   void run() override {
-    bool shouldInvalidate = cleanFunction(*getFunction());
-    if (!shouldInvalidate)
-      return;
-    invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
+    SILFunction *F = getFunction();
+
+    bool shouldInvalidate = cleanFunction(*F);
+
+    if (shouldInvalidate)
+      invalidateAnalysis(SILAnalysis::InvalidationKind::Instructions);
   }
 };
 

stdlib/public/core/KeyPath.swift

@@ -625,23 +625,107 @@ internal enum KeyPathComponent: Hashable {
   }
 }
 
+// _semantics("optimize.sil.preserve_exclusivity" forces the compiler to
+// generate access markers regardless of the current build settings. This way,
+// user code that accesses keypaths are properly enforced even if the standard
+// library has exclusivity checking internally disabled. The semantic attribute
+// must be consistently applied to both the begin and end unpaired access
+// markers, otherwise the runtime will fail catastrophically and unpredictably.
+// This relies on Swift module serialization occurring before these _semantic
+// function are inlined, since inlining would unpredictably cause the attribute
+// to be dropped.
+//
+// An exclusivity violation will report the caller's stack frame location.
+// Runtime diagnostics will be improved by inlining this function. However, this
+// cannot be marked @transparent because it can't be inlined prior to
+// serialization.
+@inlinable
+@inline(__always)
+@_semantics("optimize.sil.preserve_exclusivity")
+func beginAccessHelper<T>(_ address: Builtin.RawPointer, _ accessRecordPtr: Builtin.RawPointer, _ type: T.Type) {
+  Builtin.beginUnpairedModifyAccess(address, accessRecordPtr, type)
+}
+
+@inlinable
+@inline(__always)
+@_semantics("optimize.sil.preserve_exclusivity")
+func endAccessHelper(_ accessRecordPtr: Builtin.RawPointer) {
+  Builtin.endUnpairedAccess(accessRecordPtr)
+}
+
+@inlinable
+@inline(__always)
+@_semantics("optimize.sil.preserve_exclusivity")
+func instantaneousAccessHelper<T>(_ address: Builtin.RawPointer, _ type: T.Type) {
+  Builtin.performInstantaneousReadAccess(address, T.self)
+}
+
 // 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 {}
+  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.
+    beginAccessHelper(address._rawValue, accessRecordPtr, type)
+
+    return holder
+  }
+
+  @inlinable // FIXME(sil-serialize-all)
+  deinit {
+    let accessRecordPtr = Builtin.projectTailElems(self, AccessRecord.self)
+
+    // Ends the access begun in _create().
+    endAccessHelper(accessRecordPtr)
+  }
 }
 
 // A class that triggers writeback to a pointer when destroyed.
@@ -1185,7 +1269,14 @@ 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.
+      instantaneousAccessHelper(offsetAddress._rawValue, NewValue.self)
+      return .continue(offsetAddress
         .assumingMemoryBound(to: NewValue.self)
         .pointee)
 
@@ -1244,12 +1335,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):

stdlib/public/runtime/Exclusivity.cpp

@@ -260,6 +260,16 @@ class AccessSet {
 
     swift_runtime_unreachable("access not found in set");
   }
+
+#ifndef NDEBUG
+  /// Only available with asserts. Intended to be used with
+  /// swift_dumpTrackedAccess().
+  void forEach(std::function<void (Access *)> action) {
+    for (auto *iter = Head; iter != nullptr; iter = iter->getNext()) {
+      action(iter);
+    }
+  }
+#endif
 };
 
 } // end anonymous namespace
@@ -348,3 +358,17 @@ void swift::swift_endAccess(ValueBuffer *buffer) {
 
   getAccessSet().remove(access);
 }
+
+#ifndef NDEBUG
+
+// Dump the accesses that are currently being tracked by the runtime.
+//
+// This is only intended to be used in the debugger.
+void swift::swift_dumpTrackedAccesses() {
+  getAccessSet().forEach([](Access *a) {
+      fprintf(stderr, "Access. Pointer: %p. PC: %p. AccessAction: %s",
+              a->Pointer, a->PC, getAccessName(a->getAccessAction()));
+  });
+}
+
+#endif