[PATCH] D45586: Prevent deadlock in OS Plugins

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: clayborg, jingham, labath.

When performing a synchronous resume, the API mutex is held until the
process is stopped. This is fine, except for when the OS plugins are processing
an event before the main thread is aware of it, in which case we end up with a
deadlock because in the internal thread we acquire a resource lock first, and
then wait for the API lock, while in the main thread we do the opposite, we
already hold the API mutex but are now waiting for the event mutex to handle
the event.

This patch fixes this by relaxing the need for the API lock in the OS plugins.
We can get away with this because we now this code is executed in the main
thread. As stated in the comment above, we just want to ensure nobody else
messes with the API while we're making a change. In theory it's possible that
the main thread would release the lock while we're executing the function, but
prevent this would require a more structural solution (which we want, but do
not have today).

The same workaround was already present, but this patch generalizes it to the
whole file.

This will allow me to re-land r329891.


https://reviews.llvm.org/D45586

Files:
  source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp


Index: source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
===================================================================
--- source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
+++ source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
@@ -156,19 +156,20 @@
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OS));
 
-  // First thing we have to do is to try to get the API lock, and the run lock.
-  // We're going to change the thread content of the process, and we're going
-  // to use python, which requires the API lock to do it.
+  // First thing we have to do is to try to get the API lock, and the
+  // interpreter lock. We're going to change the thread content of the process,
+  // and we're going to use python, which requires the API lock to do it. We
+  // need the interpreter lock to make sure thread_info_dict stays alive.
   //
   // If someone already has the API lock, that is ok, we just want to avoid
   // external code from making new API calls while this call is happening.
   //
   // This is a recursive lock so we can grant it to any Python code called on
   // the stack below us.
   Target &target = m_process->GetTarget();
-  std::unique_lock<std::recursive_mutex> lock(target.GetAPIMutex(),
-                                              std::defer_lock);
-  lock.try_lock();
+  std::unique_lock<std::recursive_mutex> api_lock(target.GetAPIMutex(),
+                                                  std::defer_lock);
+  api_lock.try_lock();
 
   if (log)
     log->Printf("OperatingSystemPython::UpdateThreadList() fetching thread "
@@ -301,20 +302,24 @@
   if (!IsOperatingSystemPluginThread(thread->shared_from_this()))
     return reg_ctx_sp;
 
-  // First thing we have to do is get the API lock, and the run lock.  We're
-  // going to change the thread
-  // content of the process, and we're going to use python, which requires the
-  // API lock to do it.
-  // So get & hold that.  This is a recursive lock so we can grant it to any
-  // Python code called on the stack below us.
+  // First thing we have to do is to try to get the API lock, and the
+  // interpreter lock. We're going to change the thread content of the process,
+  // and we're going to use python, which requires the API lock to do it. We
+  // need the interpreter lock to make sure thread_info_dict stays alive.
+  //
+  // If someone already has the API lock, that is ok, we just want to avoid
+  // external code from making new API calls while this call is happening.
+  //
+  // This is a recursive lock so we can grant it to any Python code called on
+  // the stack below us.
   Target &target = m_process->GetTarget();
-  std::lock_guard<std::recursive_mutex> guard(target.GetAPIMutex());
+  std::unique_lock<std::recursive_mutex> api_lock(target.GetAPIMutex(),
+                                                  std::defer_lock);
+  api_lock.try_lock();
+  auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD));
 
-  auto lock =
-      m_interpreter
-          ->AcquireInterpreterLock(); // to make sure python objects stays alive
   if (reg_data_addr != LLDB_INVALID_ADDRESS) {
     // The registers data is in contiguous memory, just create the register
     // context using the address provided
@@ -383,18 +388,21 @@
                 tid, context);
 
   if (m_interpreter && m_python_object_sp) {
-    // First thing we have to do is get the API lock, and the run lock.  We're
-    // going to change the thread
-    // content of the process, and we're going to use python, which requires the
-    // API lock to do it.
-    // So get & hold that.  This is a recursive lock so we can grant it to any
-    // Python code called on the stack below us.
+    // First thing we have to do is to try to get the API lock, and the
+    // interpreter lock. We're going to change the thread content of the
+    // process, and we're going to use python, which requires the API lock to
+    // do it. We need the interpreter lock to make sure thread_info_dict stays
+    // alive.
+    //
+    // If someone already has the API lock, that is ok, we just want to avoid
+    // external code from making new API calls while this call is happening.
+    //
+    // This is a recursive lock so we can grant it to any Python code called on
+    // the stack below us.
     Target &target = m_process->GetTarget();
     std::lock_guard<std::recursive_mutex> guard(target.GetAPIMutex());
+    auto interpreter_lock = m_interpreter->AcquireInterpreterLock();
 
-    auto lock = m_interpreter->AcquireInterpreterLock(); // to make sure
-                                                         // thread_info_dict
-                                                         // stays alive
     StructuredData::DictionarySP thread_info_dict =
         m_interpreter->OSPlugin_CreateThread(m_python_object_sp, tid, context);
     std::vector<bool> core_used_map;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D45586.142238.patch
Type: text/x-patch
Size: 5009 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180412/7e1ef310/attachment.bin>