Remember a PC in the exclusivity-tracking set and report it during failures.

rjmccall · rjmccall · commit e01c31a17c8b · 2017-05-10T14:44:56.000-04:00
diff --git a/include/swift/Runtime/Exclusivity.h b/include/swift/Runtime/Exclusivity.h
@@ -28,9 +28,13 @@ struct ValueBuffer;
 ///
 /// The buffer is opaque scratch space that the runtime may use for
 /// the duration of the access.
+///
+/// The PC argument is an instruction pointer to associate with the start
+/// of the access.  If it is null, the return address of the call to
+/// swift_beginAccess will be used.
 SWIFT_RUNTIME_EXPORT
 void swift_beginAccess(void *pointer, ValueBuffer *buffer,
-                       ExclusivityFlags flags);
+                       ExclusivityFlags flags, void *pc);
 
 /// Stop dynamically tracking an access.
 SWIFT_RUNTIME_EXPORT
diff --git a/include/swift/Runtime/RuntimeFunctions.def b/include/swift/Runtime/RuntimeFunctions.def
@@ -1092,7 +1092,7 @@ FUNCTION(RegisterTypeMetadataRecords,
 // void swift_beginAccess(void *pointer, ValueBuffer *scratch, size_t flags);
 FUNCTION(BeginAccess, swift_beginAccess, DefaultCC,
          RETURNS(VoidTy),
-         ARGS(Int8PtrTy, getFixedBufferTy()->getPointerTo(), SizeTy),
+         ARGS(Int8PtrTy, getFixedBufferTy()->getPointerTo(), SizeTy, Int8PtrTy),
          ATTRS(NoUnwind))
 
 // void swift_endAccess(ValueBuffer *scratch);
diff --git a/lib/IRGen/IRGenSIL.cpp b/lib/IRGen/IRGenSIL.cpp
@@ -3992,8 +3992,9 @@ void IRGenSILFunction::visitBeginAccessInst(BeginAccessInst *access) {
       Builder.CreateBitCast(addr.getAddress(), IGM.Int8PtrTy);
     llvm::Value *flags =
       llvm::ConstantInt::get(IGM.SizeTy, uint64_t(getExclusivityFlags(access)));
+    llvm::Value *pc = llvm::ConstantPointerNull::get(IGM.Int8PtrTy);
     auto call = Builder.CreateCall(IGM.getBeginAccessFn(),
-                                   { pointer, scratch, flags });
+                                   { pointer, scratch, flags, pc });
     call->setDoesNotThrow();
 
     setLoweredDynamicallyEnforcedAddress(access, addr, scratch);
@@ -4003,6 +4004,11 @@ void IRGenSILFunction::visitBeginAccessInst(BeginAccessInst *access) {
   llvm_unreachable("bad access enforcement");
 }
 
+static bool hasBeenInlined(BeginUnpairedAccessInst *access) {
+  // Check to see if the buffer is defined locally.
+  return isa<AllocStackInst>(access->getBuffer());
+}
+
 void IRGenSILFunction::visitBeginUnpairedAccessInst(
                                               BeginUnpairedAccessInst *access) {
   Address addr = getLoweredAddress(access->getSource());
@@ -4022,8 +4028,26 @@ void IRGenSILFunction::visitBeginUnpairedAccessInst(
       Builder.CreateBitCast(addr.getAddress(), IGM.Int8PtrTy);
     llvm::Value *flags =
       llvm::ConstantInt::get(IGM.SizeTy, uint64_t(getExclusivityFlags(access)));
+
+    // Compute the effective PC of the access.
+    // Since begin_unpaired_access is designed for materializeForSet, our
+    // heuristic here is as well: we've either been inlined, in which case
+    // we should use the current PC (i.e. pass null), or we haven't,
+    // in which case we should use the caller, which is generally ok because
+    // materializeForSet can't usually be thunked.
+    llvm::Value *pc;
+    if (hasBeenInlined(access)) {
+      pc = llvm::ConstantPointerNull::get(IGM.Int8PtrTy);
+    } else {
+      auto retAddrFn =
+        llvm::Intrinsic::getDeclaration(IGM.getModule(),
+                                        llvm::Intrinsic::returnaddress);
+      pc = Builder.CreateCall(retAddrFn,
+                              { llvm::ConstantInt::get(IGM.Int32Ty, 0) });
+    }
+
     auto call = Builder.CreateCall(IGM.getBeginAccessFn(),
-                                   { pointer, scratch, flags });
+                                   { pointer, scratch, flags, pc });
     call->setDoesNotThrow();
     return;
   }
diff --git a/stdlib/public/runtime/Exclusivity.cpp b/stdlib/public/runtime/Exclusivity.cpp
@@ -41,6 +41,18 @@
 #include <stdio.h>
 #endif
 
+// Pick a return-address strategy
+#if __GNUC__
+#define get_return_address() __builtin_return_address(0)
+#elif _MSC_VER
+#include <intrin.h>
+#define get_return_address() _ReturnAddress()
+#else
+#error missing implementation for get_return_address
+#define get_return_address() ((void*) 0)
+#endif
+
+
 using namespace swift;
 
 bool swift::_swift_disableExclusivityChecking = false;
@@ -58,6 +70,7 @@ namespace {
 /// A single access that we're tracking.
 struct Access {
   void *Pointer;
+  void *PC;
   uintptr_t NextAndAction;
 
   enum : uintptr_t {
@@ -78,8 +91,10 @@ struct Access {
     return ExclusivityFlags(NextAndAction & ActionMask);
   }
 
-  void initialize(void *pointer, Access *next, ExclusivityFlags action) {
+  void initialize(void *pc, void *pointer, Access *next,
+                  ExclusivityFlags action) {
     Pointer = pointer;
+    PC = pc;
     NextAndAction = reinterpret_cast<uintptr_t>(next) | uintptr_t(action);
   }
 };
@@ -94,7 +109,7 @@ class AccessSet {
 public:
   constexpr AccessSet() {}
 
-  void insert(Access *access, void *pointer, ExclusivityFlags flags) {
+  void insert(Access *access, void *pc, void *pointer, ExclusivityFlags flags) {
     auto action = getAccessAction(flags);
 
     for (Access *cur = Head; cur != nullptr; cur = cur->getNext()) {
@@ -110,13 +125,15 @@ class AccessSet {
       // Otherwise, it's a conflict.
       // TODO: try to recover source-location information from the return
       // address.
-      fatalError(0, "%s/%s access conflict detected on address %p, aborting\n",
-                 getAccessName(action), getAccessName(cur->getAccessAction()),
+      fatalError(0, "%s(%p) / %s(%p) access conflict detected on address %p,"
+                 " aborting\n",
+                 getAccessName(cur->getAccessAction()), cur->PC,
+                 getAccessName(action), pc,
                  pointer);
     }
 
     // Insert to the front of the array so that remove tends to find it faster.
-    access->initialize(pointer, Head, action);
+    access->initialize(pc, pointer, Head, action);
     Head = access;
   }
 
@@ -214,7 +231,7 @@ static AccessSet &getAccessSet(void *pointer) {
 /// This may cause a runtime failure if an incompatible access is
 /// already underway.
 void swift::swift_beginAccess(void *pointer, ValueBuffer *buffer,
-                              ExclusivityFlags flags) {
+                              ExclusivityFlags flags, void *pc) {
   assert(pointer && "beginning an access on a null pointer?");
 
   Access *access = reinterpret_cast<Access*>(buffer);
@@ -226,7 +243,9 @@ void swift::swift_beginAccess(void *pointer, ValueBuffer *buffer,
     return;
   }
 
-  getAccessSet(pointer).insert(access, pointer, flags);
+  if (!pc) pc = get_return_address();
+
+  getAccessSet(pointer).insert(access, pc, pointer, flags);
 }
 
 /// End tracking a dynamic access.
diff --git a/test/IRGen/access_markers.sil b/test/IRGen/access_markers.sil
@@ -27,7 +27,7 @@ bb0(%0 : $A):
   // CHECK-NEXT: [[T0:%.*]] = bitcast [[BUFFER]]* [[SCRATCH1]] to i8*
   // CHECK-NEXT: call void @llvm.lifetime.start(i64 {{.*}}, i8* [[T0]])
   // CHECK-NEXT: [[T1:%.*]] = bitcast [[INT]]* [[PROPERTY]] to i8*
-  // CHECK-NEXT: call void @swift_beginAccess(i8* [[T1]], [[BUFFER]]* [[SCRATCH1]], [[SIZE]] 1)
+  // CHECK-NEXT: call void @swift_beginAccess(i8* [[T1]], [[BUFFER]]* [[SCRATCH1]], [[SIZE]] 1, i8* null)
   %3 = begin_access [modify] [dynamic] %2 : $*Int
 
   // CHECK-NEXT: call void @swift_endAccess([[BUFFER]]* [[SCRATCH1]])
@@ -38,7 +38,7 @@ bb0(%0 : $A):
   // CHECK-NEXT: [[T0:%.*]] = bitcast [[BUFFER]]* [[SCRATCH2]] to i8*
   // CHECK-NEXT: call void @llvm.lifetime.start(i64 {{.*}}, i8* [[T0]])
   // CHECK-NEXT: [[T1:%.*]] = bitcast [[INT]]* [[PROPERTY]] to i8*
-  // CHECK-NEXT: call void @swift_beginAccess(i8* [[T1]], [[BUFFER]]* [[SCRATCH2]], [[SIZE]] 0)
+  // CHECK-NEXT: call void @swift_beginAccess(i8* [[T1]], [[BUFFER]]* [[SCRATCH2]], [[SIZE]] 0, i8* null)
   %5 = begin_access [read] [dynamic] %2 : $*Int
 
   // CHECK-NEXT: call void @swift_endAccess([[BUFFER]]* [[SCRATCH2]])
@@ -60,14 +60,14 @@ bb0(%0 : $A):
   %2 = ref_element_addr %0 : $A, #A.property
 
   // CHECK-NEXT: [[T1:%.*]] = bitcast [[INT]]* [[PROPERTY]] to i8*
-  // CHECK-NEXT: call void @swift_beginAccess(i8* [[T1]], [[BUFFER]]* [[SCRATCH]], [[SIZE]] 1)
+  // CHECK-NEXT: call void @swift_beginAccess(i8* [[T1]], [[BUFFER]]* [[SCRATCH]], [[SIZE]] 1, i8* null)
   %3 = begin_unpaired_access [modify] [dynamic] %2 : $*Int, %1 : $*Builtin.UnsafeValueBuffer
 
   // CHECK-NEXT: call void @swift_endAccess([[BUFFER]]* [[SCRATCH]])
   %4 = end_unpaired_access [dynamic] %1 : $*Builtin.UnsafeValueBuffer
 
   // CHECK-NEXT: [[T1:%.*]] = bitcast [[INT]]* [[PROPERTY]] to i8*
-  // CHECK-NEXT: call void @swift_beginAccess(i8* [[T1]], [[BUFFER]]* [[SCRATCH]], [[SIZE]] 0)
+  // CHECK-NEXT: call void @swift_beginAccess(i8* [[T1]], [[BUFFER]]* [[SCRATCH]], [[SIZE]] 0, i8* null)
   %5 = begin_unpaired_access [read] [dynamic] %2 : $*Int, %1 : $*Builtin.UnsafeValueBuffer
 
   // CHECK-NEXT: call void @swift_endAccess([[BUFFER]]* [[SCRATCH]])
@@ -79,6 +79,32 @@ bb0(%0 : $A):
   return %20 : $()
 }
 
+// CHECK-LABEL: define {{.*}}void @testUnpairedExternal(
+sil @testUnpairedExternal : $(@guaranteed A, @inout Builtin.UnsafeValueBuffer) -> () {
+bb0(%0 : $A, %1 : $*Builtin.UnsafeValueBuffer):
+  // CHECK:      [[PROPERTY:%.*]] = getelementptr inbounds [[C]], [[C]]* %0, i32 0, i32 1
+  %2 = ref_element_addr %0 : $A, #A.property
+
+  // CHECK-NEXT: [[T1:%.*]] = bitcast [[INT]]* [[PROPERTY]] to i8*
+  // CHECK-NEXT: [[PC:%.*]] = call i8* @llvm.returnaddress(i32 0)
+  // CHECK-NEXT: call void @swift_beginAccess(i8* [[T1]], [[BUFFER]]* [[SCRATCH:%1]], [[SIZE]] 1, i8* [[PC]])
+  %3 = begin_unpaired_access [modify] [dynamic] %2 : $*Int, %1 : $*Builtin.UnsafeValueBuffer
+
+  // CHECK-NEXT: call void @swift_endAccess([[BUFFER]]* [[SCRATCH]])
+  %4 = end_unpaired_access [dynamic] %1 : $*Builtin.UnsafeValueBuffer
+
+  // CHECK-NEXT: [[T1:%.*]] = bitcast [[INT]]* [[PROPERTY]] to i8*
+  // CHECK-NEXT: [[PC:%.*]] = call i8* @llvm.returnaddress(i32 0)
+  // CHECK-NEXT: call void @swift_beginAccess(i8* [[T1]], [[BUFFER]]* [[SCRATCH]], [[SIZE]] 0, i8* [[PC]])
+  %5 = begin_unpaired_access [read] [dynamic] %2 : $*Int, %1 : $*Builtin.UnsafeValueBuffer
+
+  // CHECK-NEXT: call void @swift_endAccess([[BUFFER]]* [[SCRATCH]])
+  %6 = end_unpaired_access [dynamic] %1 : $*Builtin.UnsafeValueBuffer
+
+  %20 = tuple ()
+  return %20 : $()
+}
+
 // rdar://31964550
 // Just check that this doesn't crash.
 sil @testCopyAddr : $(@guaranteed A) -> () {
diff --git a/test/IRGen/exclusivity.sil b/test/IRGen/exclusivity.sil
@@ -24,7 +24,7 @@ bb0(%0 : $A):
   // CHECK: [[T0:%.*]] = bitcast [[BUFFER_T]]* [[SCRATCH0]] to i8*
   // CHECK: call void @llvm.lifetime.start(i64 -1, i8* [[T0]])
   // CHECK: [[T0:%.*]] = bitcast [[INT:%TSi]]* [[PROP]] to i8*
-  // CHECK: call void @swift_beginAccess(i8* [[T0]], [[BUFFER_T]]* [[SCRATCH0]], [[SIZE_T:i32|i64]] 0)
+  // CHECK: call void @swift_beginAccess(i8* [[T0]], [[BUFFER_T]]* [[SCRATCH0]], [[SIZE_T:i32|i64]] 0, i8* null)
   %4 = begin_access [read] [dynamic] %3 : $*Int
 
   // CHECK: [[T0:%.*]] = getelementptr inbounds [[INT]], [[INT]]* %1, i32 0, i32 0
@@ -44,7 +44,7 @@ bb0(%0 : $A):
   // CHECK: [[T0:%.*]] = bitcast [[BUFFER_T]]* [[SCRATCH1]] to i8*
   // CHECK: call void @llvm.lifetime.start(i64 -1, i8* [[T0]])
   // CHECK: [[T0:%.*]] = bitcast [[INT:%TSi]]* [[PROP]] to i8*
-  // CHECK: call void @swift_beginAccess(i8* [[T0]], [[BUFFER_T]]* [[SCRATCH1]], [[SIZE_T]] 1)
+  // CHECK: call void @swift_beginAccess(i8* [[T0]], [[BUFFER_T]]* [[SCRATCH1]], [[SIZE_T]] 1, i8* null)
   %12 = begin_access [modify] [dynamic] %11 : $*Int
 
   // CHECK: call {{.*}} @changeInt([[INT]]*{{.*}} [[PROP]])
