Merge pull request #9431 from rjmccall/dynamic-exclusivity-refinements

Dynamic exclusivity refinements

Author: rjmccall

include/swift/ABI/MetadataValues.h

@@ -570,12 +570,28 @@ class FieldType {
 enum class ExclusivityFlags : uintptr_t {
   Read             = 0x0,
   Modify           = 0x1,
-  ActionMask       = 0x1
+  // Leave space for other actions.
+  // Don't rely on ActionMask in stable ABI.
+  ActionMask       = 0x1,
+
+  // Downgrade exclusivity failures to a warning.
+  WarningOnly      = 0x10
 };
+static inline ExclusivityFlags operator|(ExclusivityFlags lhs,
+                                         ExclusivityFlags rhs) {
+  return ExclusivityFlags(uintptr_t(lhs) | uintptr_t(rhs));
+}
+static inline ExclusivityFlags &operator|=(ExclusivityFlags &lhs,
+                                           ExclusivityFlags rhs) {
+  return (lhs = (lhs | rhs));
+}
 static inline ExclusivityFlags getAccessAction(ExclusivityFlags flags) {
   return ExclusivityFlags(uintptr_t(flags)
                         & uintptr_t(ExclusivityFlags::ActionMask));
 }
+static inline bool isWarningOnly(ExclusivityFlags flags) {
+  return uintptr_t(flags) & uintptr_t(ExclusivityFlags::WarningOnly);
+}
 
 } // end namespace swift
 

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

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);

lib/IRGen/IRGenSIL.cpp

@@ -3952,7 +3952,7 @@ void IRGenSILFunction::visitProjectBoxInst(swift::ProjectBoxInst *i) {
   }
 }
 
-static ExclusivityFlags getExclusivityFlags(SILAccessKind kind) {
+static ExclusivityFlags getExclusivityAction(SILAccessKind kind) {
   switch (kind) {
   case SILAccessKind::Read:
     return ExclusivityFlags::Read;
@@ -3965,9 +3965,20 @@ static ExclusivityFlags getExclusivityFlags(SILAccessKind kind) {
   llvm_unreachable("bad access kind");
 }
 
+static ExclusivityFlags getExclusivityFlags(SILModule &M,
+                                            SILAccessKind kind) {
+  auto flags = getExclusivityAction(kind);
+
+  // In old Swift compatibility modes, downgrade this to a warning.
+  if (M.getASTContext().LangOpts.isSwiftVersion3())
+    flags |= ExclusivityFlags::WarningOnly;
+
+  return flags;
+}
+
 template <class Inst>
 static ExclusivityFlags getExclusivityFlags(Inst *i) {
-  return getExclusivityFlags(i->getAccessKind());
+  return getExclusivityFlags(i->getModule(), i->getAccessKind());
 }
 
 void IRGenSILFunction::visitBeginAccessInst(BeginAccessInst *access) {
@@ -3992,8 +4003,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 +4015,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 +4039,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;
   }

stdlib/public/runtime/Exclusivity.cpp

@@ -19,6 +19,7 @@
 #include "swift/Runtime/Exclusivity.h"
 #include "swift/Runtime/Metadata.h"
 #include <memory>
+#include <stdio.h>
 
 // Pick an implementation strategy.
 #ifndef SWIFT_EXCLUSIVITY_USE_THREADLOCAL
@@ -41,6 +42,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;
@@ -49,40 +62,70 @@ static const char *getAccessName(ExclusivityFlags flags) {
   switch (flags) {
   case ExclusivityFlags::Read: return "read";
   case ExclusivityFlags::Modify: return "modify";
+  default: return "unknown";
+  }
+}
+
+static void reportExclusivityConflict(ExclusivityFlags oldAction, void *oldPC,
+                                      ExclusivityFlags newFlags, void *newPC,
+                                      void *pointer) {
+  // TODO: print something about where the pointer came from?
+  // TODO: if we do something more computationally intense here,
+  //   suppress warnings if the total warning count exceeds some limit
+
+  if (isWarningOnly(newFlags)) {
+    fprintf(stderr,
+            "WARNING: %s/%s access conflict detected on address %p\n"
+            "  first access started at PC=%p\n"
+            "  second access started at PC=%p\n",
+            getAccessName(oldAction),
+            getAccessName(getAccessAction(newFlags)),
+            pointer, oldPC, newPC);
+    return;
   }
-  return "unknown";
+
+  // TODO: try to recover source-location information from the return
+  // address.
+  fatalError(0,
+             "%s/%s access conflict detected on address %p, aborting\n"
+             "  first access started at PC=%p\n"
+             "  second access started at PC=%p\n",
+             getAccessName(oldAction),
+             getAccessName(getAccessAction(newFlags)),
+             pointer, oldPC, newPC);
 }
 
 namespace {
 
 /// A single access that we're tracking.
 struct Access {
   void *Pointer;
-  uintptr_t HereAndAction;
-  Access *Next;
+  void *PC;
+  uintptr_t NextAndAction;
 
   enum : uintptr_t {
     ActionMask = (uintptr_t)ExclusivityFlags::ActionMask,
-    HereMask = ~ActionMask
+    NextMask = ~ActionMask
   };
 
-  Access **getHere() const {
-    return reinterpret_cast<Access**>(HereAndAction & HereMask);
+  Access *getNext() const {
+    return reinterpret_cast<Access*>(NextAndAction & NextMask);
   }
 
-  void setHere(Access **newHere) {
-    HereAndAction = reinterpret_cast<uintptr_t>(newHere) | uintptr_t(getAccessAction());
+  void setNext(Access *next) {
+    NextAndAction =
+      reinterpret_cast<uintptr_t>(next) | (NextAndAction & NextMask);
   }
 
   ExclusivityFlags getAccessAction() const {
-    return ExclusivityFlags(HereAndAction & ActionMask);
+    return ExclusivityFlags(NextAndAction & ActionMask);
   }
 
-  void initialize(void *pointer, Access **here, ExclusivityFlags action) {
-    assert(*here == nullptr && "not inserting to end of list");
+  void initialize(void *pc, void *pointer, Access *next,
+                  ExclusivityFlags action) {
     Pointer = pointer;
-    HereAndAction = reinterpret_cast<uintptr_t>(here) | uintptr_t(action);
-    Next = nullptr;
+    PC = pc;
+    NextAndAction = reinterpret_cast<uintptr_t>(next) | uintptr_t(action);
   }
 };
 
@@ -96,11 +139,10 @@ 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);
 
-    Access **curP = &Head;
-    for (Access *cur = *curP; cur != nullptr; curP = &cur->Next, cur = *curP) {
+    for (Access *cur = Head; cur != nullptr; cur = cur->getNext()) {
       // Ignore accesses to different values.
       if (cur->Pointer != pointer)
         continue;
@@ -111,22 +153,34 @@ class AccessSet {
         continue;
 
       // 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()),
-                 pointer);
+      reportExclusivityConflict(cur->getAccessAction(), cur->PC,
+                                flags, pc, pointer);
+
+      // If we're only warning, don't report multiple conflicts.
+      break;
     }
 
-    access->initialize(pointer, curP, action);
-    *curP = access;
+    // Insert to the front of the array so that remove tends to find it faster.
+    access->initialize(pc, pointer, Head, action);
+    Head = access;
   }
 
-  static void remove(Access *access) {
-    Access **here = access->getHere();
-    *here = access->Next;
-    if (access->Next != nullptr)
-      access->Next->setHere(here);
+  void remove(Access *access) {
+    auto cur = Head;
+    // Fast path: stack discipline.
+    if (cur == access) {
+      Head = cur->getNext();
+      return;
+    }
+
+    for (Access *last = cur; cur != nullptr; last = cur, cur = cur->getNext()) {
+      if (last == access) {
+        last->setNext(cur->getNext());
+        return;
+      }
+    }
+
+    swift_runtime_unreachable("access not found in set");
   }
 };
 
@@ -205,7 +259,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);
@@ -217,7 +271,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.
@@ -231,5 +287,5 @@ void swift::swift_endAccess(ValueBuffer *buffer) {
     return;
   }
 
-  AccessSet::remove(access);
+  getAccessSet(pointer).remove(access);
 }