[PATCH] D51127: [ORC] Lock SearchOrder independently from SessionLock to avoid deadlock when resolving relocations.

[Stefan Gränitz via Phabricator via llvm-commits]

sgraenitz created this revision.
sgraenitz added a reviewer: lhames.
Herald added a subscriber: llvm-commits.

Is it necessary to lock the entire session in order to resolve relocations or would it be sufficient to:
(1) Lock the collection of related dylibs.
(2) Visit each dylib individually with a lock for only this one instance.

The change here adds SearchOrderMutex and visitInSearchOrder() to achieve (1).
It does not implement (2), but instead just assumes that the individual dylibs remain unchanged during visitation. I started drafting (2), but it quickly became big and I stopped to avoid wasting time here.

One more note on the current approach:
IIUC ExecutionSessionBase::lookup() either resolves all symbols in all dylibs or returns an error. Wouldn't this mean that a single broken dylib could cause everything to fail?
Is there a fundamental reason for this?
Otherwise, for (2) it would be easy to return both, errors and symbols for each dylib, and degrade errors to warnings if all requested symbols could be resolved.
With your ErrorList implementation we would be perfectly prepared for it.


Repository:
  rL LLVM

https://reviews.llvm.org/D51127

Files:
  include/llvm/ExecutionEngine/Orc/Core.h
  lib/ExecutionEngine/Orc/Core.cpp
  lib/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.cpp


Index: lib/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.cpp
===================================================================
--- lib/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.cpp
+++ lib/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.cpp
@@ -30,7 +30,7 @@
     };
 
     auto InternedResult =
-        MR.getTargetJITDylib().withSearchOrderDo([&](const JITDylibList &JDs) {
+        MR.getTargetJITDylib().visitInSearchOrder([&](const JITDylibList &JDs) {
           return ES.lookup(JDs, InternedSymbols, RegisterDependencies, false);
         });
 
Index: lib/ExecutionEngine/Orc/Core.cpp
===================================================================
--- lib/ExecutionEngine/Orc/Core.cpp
+++ lib/ExecutionEngine/Orc/Core.cpp
@@ -1239,14 +1239,17 @@
   if (SearchThisJITDylibFirst && NewSearchOrder.front() != this)
     NewSearchOrder.insert(NewSearchOrder.begin(), this);
 
+  std::lock_guard<std::mutex> Lock(SearchOrderMutex);
   ES.runSessionLocked([&]() { SearchOrder = std::move(NewSearchOrder); });
 }
 
 void JITDylib::addToSearchOrder(JITDylib &JD) {
+  std::lock_guard<std::mutex> Lock(SearchOrderMutex);
   ES.runSessionLocked([&]() { SearchOrder.push_back(&JD); });
 }
 
 void JITDylib::replaceInSearchOrder(JITDylib &OldJD, JITDylib &NewJD) {
+  std::lock_guard<std::mutex> Lock(SearchOrderMutex);
   ES.runSessionLocked([&]() {
     auto I = std::find(SearchOrder.begin(), SearchOrder.end(), &OldJD);
 
@@ -1256,6 +1259,7 @@
 }
 
 void JITDylib::removeFromSearchOrder(JITDylib &JD) {
+  std::lock_guard<std::mutex> Lock(SearchOrderMutex);
   ES.runSessionLocked([&]() {
     auto I = std::find(SearchOrder.begin(), SearchOrder.end(), &JD);
     if (I != SearchOrder.end())
Index: include/llvm/ExecutionEngine/Orc/Core.h
===================================================================
--- include/llvm/ExecutionEngine/Orc/Core.h
+++ include/llvm/ExecutionEngine/Orc/Core.h
@@ -634,6 +634,15 @@
     return ES.runSessionLocked([&]() { return F(SearchOrder); });
   }
 
+  /// Visit related dylibs in search order. This locks a mutex, so SearchOrder
+  /// can't be modified during visitation. Session lock is NOT acquired.
+  template <typename Func>
+  auto visitInSearchOrder(Func &&F) const
+      -> decltype(F(std::declval<const JITDylibList &>())) {
+    std::lock_guard<std::mutex> Lock(SearchOrderMutex);
+    return F(SearchOrder);
+  }
+
   /// Define all symbols provided by the materialization unit to be part
   ///        of the given JITDylib.
   template <typename UniquePtrToMaterializationUnit>
@@ -753,6 +762,7 @@
   MaterializingInfosMap MaterializingInfos;
   FallbackDefinitionGeneratorFunction FallbackDefinitionGenerator;
   JITDylibList SearchOrder;
+  mutable std::mutex SearchOrderMutex;
 };
 
 /// An ExecutionSession represents a running JIT program.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D51127.162056.patch
Type: text/x-patch
Size: 2808 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180822/917611ed/attachment-0001.bin>