Support exclusivity enforcement of KeyPath read-only access.

Use begin_unpaired_access [no_nested_conflict] for
Builtin.performInstantaneousReadAccess. This can't be optimized away
and is the proper marker to use when the access scope is unknown.

Drop the requirement that
_semantics("optimize.sil.preserve_exclusivity") be @inline(never). We
actually want theses inlined into user code. Verify that the
@_semantic functions are not inlined or otherwise tampered with prior
to serialization.

Make *no* change to propagate @inline(__always) into LLVM. This no longer has
any relationship to this PR and can be investigated seperately.

Author: atrick

lib/IRGen/GenDecl.cpp

@@ -2317,7 +2317,8 @@ llvm::Function *IRGenModule::getAddrOfSILFunction(SILFunction *f,
                                llvm::Attribute::NoInline);
     break;
   case AlwaysInline:
-    // We do not transfer AlwaysInline since we get weird test failures.
+    // FIXME: We do not currently transfer AlwaysInline since doing so results
+    // in test failures, which must be investigated first.
   case InlineDefault:
     break;
   }

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,6 +19,12 @@
 /// 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"

lib/SILOptimizer/Mandatory/IRGenPrepare.cpp

@@ -72,14 +72,6 @@ class IRGenPrepare : public SILFunctionTransform {
   void run() override {
     SILFunction *F = getFunction();
 
-    if (F->hasSemanticsAttr(OPTIMIZE_SIL_PRESERVE_EXCLUSIVITY)) {
-      assert(F->getInlineStrategy() == NoInline &&
-             "All OPTIMIZE_SIL_PRESERVE_EXCLUSIVITY functions should have "
-             "no-inline");
-      // We always want to inline these at the llvm level.
-      F->setInlineStrategy(AlwaysInline);
-    }
-
     bool shouldInvalidate = cleanFunction(*F);
 
     if (shouldInvalidate)

stdlib/public/core/KeyPath.swift

@@ -625,6 +625,41 @@ 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. The lifetime of the instance of this class is also used
 // to begin and end exclusive 'modify' access to the projected address.
@@ -648,7 +683,6 @@ internal final class ClassHolder<ProjectionType> {
   }
 
   @inlinable // FIXME(sil-serialize-all)
-  @usableFromInline // FIXME(sil-serialize-all)
   internal final class func _create(
       previous: AnyObject?,
       instance: AnyObject,
@@ -680,18 +714,17 @@ internal final class ClassHolder<ProjectionType> {
 
     // Begin a 'modify' access to the address. This access is ended in
     // ClassHolder's deinitializer.
-    Builtin.beginUnpairedModifyAccess(address._rawValue, accessRecordPtr, type)
+    beginAccessHelper(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)
+    endAccessHelper(accessRecordPtr)
   }
 }
 
@@ -1242,8 +1275,7 @@ internal struct RawKeyPathComponent {
       // 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)
+      instantaneousAccessHelper(offsetAddress._rawValue, NewValue.self)
       return .continue(offsetAddress
         .assumingMemoryBound(to: NewValue.self)
         .pointee)

test/IRGen/preserve_exclusivity.swift

@@ -1,12 +1,10 @@
-// RUN: %target-swift-frontend -parse-stdlib -parse-stdlib -emit-ir -Onone %s | %FileCheck --check-prefix=IR-Onone %s
-
-// We check separately that:
+// RUN: %target-swift-frontend -parse-stdlib -emit-ir -Onone %s | %FileCheck --check-prefix=IR-Onone %s
 //
-// 1. We properly emit our special functions as inline always.
-// 2. end-to-end we inline the access markers.
+// Check that access markers in @_semantics("optimize.sil.preserve_exclusivity") functions generate runtime calls.
 
-// RUN: %target-swift-frontend -parse-stdlib -Xllvm -sil-disable-pass=FunctionSignatureOpts -Xllvm -sil-disable-pass=GenericSpecializer -parse-stdlib -emit-ir -O -disable-llvm-optzns %s | %FileCheck --check-prefix=IR-Osil %s
-// RUN: %target-swift-frontend -parse-stdlib -Xllvm -sil-disable-pass=FunctionSignatureOpts -Xllvm -sil-disable-pass=GenericSpecializer -parse-stdlib -emit-ir -O %s | %FileCheck --check-prefix=IR-Ollvm %s
+// RUN: %target-swift-frontend -parse-stdlib -Xllvm -sil-disable-pass=FunctionSignatureOpts -Xllvm -sil-disable-pass=GenericSpecializer -emit-ir -O %s | %FileCheck --check-prefix=IR-O %s
+//
+// Check that the -O pipeline preserves the runtime calls for @_semantics("optimize.sil.preserve_exclusivity") functions.
 
 @_silgen_name("marker1")
 func marker1() -> ()
@@ -20,83 +18,119 @@ func marker3() -> ()
 @_silgen_name("marker4")
 func marker4() -> ()
 
-// IR-Onone: define swiftcc void @"$S20preserve_exclusivity10beginNoOptyyBp_BpxmtlF"(i8*, i8*, %swift.type*, %swift.type* %T1)
+@_silgen_name("marker5")
+func marker5() -> ()
+
+@_silgen_name("marker6")
+func marker6() -> ()
+
+// IR-Onone-LABEL: define {{.*}}swiftcc void @"$S20preserve_exclusivity10beginNoOptyyBp_BpxmtlF"(i8*, i8*, %swift.type*, %swift.type* %T1)
 // IR-Onone: call void @swift_beginAccess
 // IR-Onone-NEXT: ret void
 
-// IR-Osil: define swiftcc void @"$S20preserve_exclusivity10beginNoOptyyBp_BpxmtlF"(i8*, i8*, %swift.type*, %swift.type* %T1) [[ATTR:#[0-9][0-9]*]] {
-// IR-Osil:   call void @swift_beginAccess
-// IR-Osil-NEXT: ret void
+// IR-O-LABEL: define {{.*}}swiftcc void @"$S20preserve_exclusivity10beginNoOptyyBp_BpxmtlF"(i8*, i8*, %swift.type*{{.*}}, %swift.type*{{.*}} %T1)
+// IR-O:   call void @swift_beginAccess
+// IR-O-NEXT: ret void
 
-@inline(never)
 @_semantics("optimize.sil.preserve_exclusivity")
 public func beginNoOpt<T1>(_ address: Builtin.RawPointer, _ scratch: Builtin.RawPointer, _ ty1: T1.Type) {
   marker1()
   Builtin.beginUnpairedModifyAccess(address, scratch, ty1);
 }
 
-// IR-Onone: define swiftcc void @"$S20preserve_exclusivity8endNoOptyyBpF"(i8*)
+// IR-Onone-LABEL: define {{.*}}swiftcc void @"$S20preserve_exclusivity8endNoOptyyBpF"(i8*)
 // IR-Onone: call void @swift_endAccess
 // IR-Onone-NEXT: ret void
 
-// IR-Osil: define swiftcc void @"$S20preserve_exclusivity8endNoOptyyBpF"(i8*) [[ATTR]]
-// IR-Osil:   call void @swift_endAccess
-// IR-Osil-NEXT: ret void
-@inline(never)
+// IR-O-LABEL: define {{.*}}swiftcc void @"$S20preserve_exclusivity8endNoOptyyBpF"(i8*)
+// IR-O:   call void @swift_endAccess
+// IR-O-NEXT: ret void
 @_semantics("optimize.sil.preserve_exclusivity")
 public func endNoOpt(_ address: Builtin.RawPointer) {
   marker2()
   Builtin.endUnpairedAccess(address)
 }
 
-class Klass {}
+// IR-Onone-LABEL: define {{.*}}swiftcc void @"$S20preserve_exclusivity9readNoOptyyBp_xmtlF"(i8*, %swift.type*, %swift.type* %T1)
+// IR-Onone: call void @swift_beginAccess
+// IR-Onone: ret void
+
+// IR-O-LABEL: define {{.*}}swiftcc void @"$S20preserve_exclusivity9readNoOptyyBp_xmtlF"(i8*, %swift.type*{{.*}}, %swift.type*{{.*}} %T1)
+// IR-O:   call void @swift_beginAccess
+// IR-O: ret void
+@_semantics("optimize.sil.preserve_exclusivity")
+public func readNoOpt<T1>(_ address: Builtin.RawPointer, _ ty1: T1.Type) {
+  marker3()
+  Builtin.performInstantaneousReadAccess(address, ty1);
+}
 
 // Make sure testNoOpt properly inlines in our functions.
 //
-// IR-Ollvm: define swiftcc void @"$S20preserve_exclusivity9testNoOptyyBpF"(i8*)
-// IR-Ollvm: call swiftcc void @marker1
-// IR-Ollvm: call void @swift_beginAccess
-// IR-Ollvm: call swiftcc void @marker2
-// IR-Ollvm: call void @swift_endAccess
-// IR-Ollvm-NEXT: ret void
+// IR-O-LABEL: define {{.*}}swiftcc void @"$S20preserve_exclusivity9testNoOptyyBpF"(i8*)
+// IR-O: call swiftcc void @marker1
+// IR-O: call void @swift_beginAccess
+// IR-O: call swiftcc void @marker2
+// IR-O: call void @swift_endAccess
+// IR-O: call swiftcc void @marker3
+// IR-O: call void @swift_beginAccess
+// IR-O: ret void
 public func testNoOpt(_ k1: Builtin.RawPointer) {
   beginNoOpt(k1, k1, Builtin.RawPointer.self)
   endNoOpt(k1)
+  readNoOpt(k1, Builtin.RawPointer.self)
 }
 
-// IR-Onone: define swiftcc void @"$S20preserve_exclusivity8beginOptyyBp_BpxmtlF"(i8*, i8*, %swift.type*, %swift.type* %T1)
+// IR-Onone-LABEL: define {{.*}}swiftcc void @"$S20preserve_exclusivity8beginOptyyBp_BpxmtlF"(i8*, i8*, %swift.type*, %swift.type* %T1)
 // IR-Onone: call void @swift_beginAccess
 // IR-Onone-NEXT: ret void
 
-// IR-Osil: define swiftcc void @"$S20preserve_exclusivity8beginOptyyBp_BpxmtlF"(i8*, i8*, %swift.type*, %swift.type* %T1)
-// IR-Osil-NEXT: entry
-// IR-Osil-NEXT: call swiftcc void @marker3
-// IR-Osil-NEXT: ret void
+// IR-O-LABEL: define {{.*}}swiftcc void @"$S20preserve_exclusivity8beginOptyyBp_BpxmtlF"(i8*{{.*}}, i8*{{.*}}, %swift.type*{{.*}}, %swift.type*{{.*}} %T1)
+// IR-O-NEXT: entry
+// IR-O-NEXT: call swiftcc void @marker4
+// IR-O-NEXT: ret void
 
-@inline(never)
 public func beginOpt<T1>(_ address: Builtin.RawPointer, _ scratch: Builtin.RawPointer, _ ty1: T1.Type) {
-  marker3()
+  marker4()
   Builtin.beginUnpairedModifyAccess(address, scratch, ty1);
 }
 
-// IR-Onone: define swiftcc void @"$S20preserve_exclusivity6endOptyyBpF"(i8*)
+// IR-Onone-LABEL: define {{.*}}swiftcc void @"$S20preserve_exclusivity6endOptyyBpF"(i8*)
 // IR-Onone: call void @swift_endAccess
 // IR-Onone-NEXT: ret void
 
-// IR-Osil: define swiftcc void @"$S20preserve_exclusivity6endOptyyBpF"(i8*)
-// IR-Osil-NEXT: entry
-// IR-Osil-NEXT: call swiftcc void @marker4
-// IR-Osil-NEXT: ret void
+// IR-O-LABEL: define {{.*}}swiftcc void @"$S20preserve_exclusivity6endOptyyBpF"(i8*{{.*}})
+// IR-O-NEXT: entry
+// IR-O-NEXT: call swiftcc void @marker5
+// IR-O-NEXT: ret void
 
-@inline(never)
 public func endOpt(_ address: Builtin.RawPointer) {
-  marker4()
+  marker5()
   Builtin.endUnpairedAccess(address)
 }
 
+// IR-Onone-LABEL: define {{.*}}swiftcc void @"$S20preserve_exclusivity7readOptyyBp_xmtlF"(i8*, %swift.type*, %swift.type* %T1)
+// IR-Onone: call void @swift_beginAccess
+// IR-Onone: ret void
+
+// IR-O-LABEL: define {{.*}}swiftcc void @"$S20preserve_exclusivity7readOptyyBp_xmtlF"(i8*{{.*}}, %swift.type*{{.*}}, %swift.type*{{.*}} %T1)
+// IR-O-NEXT: entry
+// IR-O-NEXT: call swiftcc void @marker6
+// IR-O-NEXT: ret void
+
+public func readOpt<T1>(_ address: Builtin.RawPointer, _ ty1: T1.Type) {
+  marker6()
+  Builtin.performInstantaneousReadAccess(address, ty1);
+}
+
+// Make sure testOpt properly inlines in our functions.
+//
+// IR-O-LABEL: define {{.*}}swiftcc void @"$S20preserve_exclusivity7testOptyyBpF"(i8*{{.*}})
+// IR-O: call swiftcc void @marker4
+// IR-O: call swiftcc void @marker5
+// IR-O: call swiftcc void @marker6
+// IR-O: ret void
 public func testOpt(_ k1: Builtin.RawPointer) {
   beginOpt(k1, k1, Builtin.RawPointer.self)
   endOpt(k1)
+  readOpt(k1, Builtin.RawPointer.self)
 }
-
-// IR-Osil: attributes [[ATTR]] = { alwaysinline

test/SILGen/builtins.swift

@@ -384,8 +384,9 @@ func beginUnpairedModifyAccess<T1>(address: Builtin.RawPointer, scratch: Builtin
 // CHECK-LABEL: sil hidden @$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: [[SCRATCH:%.*]] = alloc_stack $Builtin.UnsafeValueBuffer
+  // CHECK: begin_unpaired_access [read] [dynamic] [no_nested_conflict] [[P2A_ADDR]] : $*T1, [[SCRATCH]] : $*Builtin.UnsafeValueBuffer
+  // CHECK-NOT: end_{{.*}}access
   // CHECK: [[RESULT:%.*]] = tuple ()
   // CHECK: [[RETURN:%.*]] = tuple ()
   // CHECK: return [[RETURN]] : $()