[lldb] Fix data race when interacting with python scripts

[mib]

Summary

This patch should fix some data races when a python script (i.e. a
Scripted Process) has a nested call to another python script (i.e. a
OperatingSystem Plugin), which can cause concurrent writes to the python
lock count.

This patch also fixes a data race happening when resetting the operating
system unique pointer.

To address these issues, both accesses is guarded by a mutex.

rdar://109413039

Signed-off-by: Med Ismail Bennani <[e-mail]>

[JDevlieghere]

Does this have to be a recursive mutex? Can we simplify this by using a std::atomic<unsigned> instead?

[mib]

Address @JDevlieghere comment.

[bulbazord]

Making m_lock_count's type into std::atomic<uint32_t> makes sense to me, but I'm a little confused about why Process::LoadOperatingSystemPlugin is guarded by acquiring m_thread_mutex. My (admittedly limited) understanding of that is that it's a mutex that the Process holds for the ThreadList to manage concurrent modifications to the thread list. Is loading an Operating System plugin related to modifying the ThreadList? If not, perhaps it would be better served by having its own mutex?

[JDevlieghere]

In D154271#4466170, @bulbazord wrote:

Making m_lock_count's type into std::atomic<uint32_t> makes sense to me, but I'm a little confused about why Process::LoadOperatingSystemPlugin is guarded by acquiring m_thread_mutex. My (admittedly limited) understanding of that is that it's a mutex that the Process holds for the ThreadList to manage concurrent modifications to the thread list. Is loading an Operating System plugin related to modifying the ThreadList? If not, perhaps it would be better served by having its own mutex?

Yes, the OS plugins are named somewhat confusingly, but their purpose is to allow an operating systems (e.g. XNU) to specify threads when doing system level debugging, where otherwise we'd have just one thread per core.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h

434

I was looking at how m_lock_count is used. IsExecutingPython` is loading the value once so that's fine. IncrementLockCount is using operator++ so that's fine too. The only problem is DecrementLockCount, which is loading the value multiple times:

uint32_t DecrementLockCount() {
  if (m_lock_count > 0)
    --m_lock_count;
  return m_lock_count;
}

We can either drop the "safe" behavior and implement DecrementLockCount as --m_lock_count or we'll have to use a (hopefully non-recursive) mutex after al.

[mib]

In D154271#4466339, @JDevlieghere wrote:

In D154271#4466170, @bulbazord wrote:

Making m_lock_count's type into std::atomic<uint32_t> makes sense to me, but I'm a little confused about why Process::LoadOperatingSystemPlugin is guarded by acquiring m_thread_mutex. My (admittedly limited) understanding of that is that it's a mutex that the Process holds for the ThreadList to manage concurrent modifications to the thread list. Is loading an Operating System plugin related to modifying the ThreadList? If not, perhaps it would be better served by having its own mutex?

Yes, the OS plugins are named somewhat confusingly, but their purpose is to allow an operating systems (e.g. XNU) to specify threads when doing system level debugging, where otherwise we'd have just one thread per core.

@bulbazord I understand the confusion regarding OperatingSystem but it's used for "OS Thread Plugins", a way to mock a thread using python script, so it does change the ThreadList.

[mib]

Use std::atomic<T>::fetch_{add, sub} to address @JDevlieghere feedback.

[mib]

Fix typo

[mib]

Fix major typo, thanks @jasonmolenda ;)

[JDevlieghere]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h

386

Sorry if my previous comment wasn't clear. std::atomic provides these operators so the old code was fine: https://en.cppreference.com/w/cpp/atomic/atomic/operator_arith

390–399

What you do here isn't safe. Nothing is preventing the atomic value from being modified between the fetch_sub and the store and there is no atomic operation that does a less-than-compare-and-store. I don't think you can keep the current behavior with just an atomic.

[mib]

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h

386

Makes sense, we shouldn't touch the implementation of IncrementLockCount.

389–399

You're right, but I think we should at least keep the guardrails to prevent returning an underflow value and return 0 instead.

[mib]

Make DecrementLockCount actually thread-safe and revert IncrementLockCount to its original implementation.

[mib]

After chatting with Jonas, it turns out that my previous implementation didn't actually address the underflow issue using atomics, so we have to revert to using a std::mutex when incrementing/decrementing the python lock counter.

[JDevlieghere]

LGTM modulo one more lock.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h

381

We should probably take the lock here as well. It won't be super meaningful (the value can change after the function returns) but I think TSan will complain about the potential race.

[mib]

Address @JDevlieghere last comment.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h

public:
 
   static void AddToSysPath(AddLocation location, std::string path);
 
   bool EnterSession(uint16_t on_entry_flags, lldb::FileSP in, lldb::FileSP out,
                     lldb::FileSP err);
 
   void LeaveSession();
 
   uint32_t IsExecutingPython() const { return m_lock_count > 0; }
 
-  uint32_t IncrementLockCount() { return ++m_lock_count; }
+  uint32_t IncrementLockCount() {
+    // std::atomic::fetch_add is an atomic post-increment operation so we need
+    // to add 1 to its return value.
+    return m_lock_count.fetch_add(1, std::memory_order_relaxed) + 1;
+  }
 
   uint32_t DecrementLockCount() {
-    if (m_lock_count > 0)
-      --m_lock_count;
-    return m_lock_count;
+    // std::atomic::fetch_sub is an atomic post-increment operation so we need
+    // to subtract 1 to its return value.
+    uint32_t count = m_lock_count.fetch_sub(1, std::memory_order_relaxed) - 1;
+    if (static_cast<int32_t>(count) < 0) {
+      // Handle underflow here & reset count to zero.
+      count = 0;
+      m_lock_count.store(count, std::memory_order_relaxed);
+    }
+    return count;
   }
 
   enum ActiveIOHandler {
     eIOHandlerNone,
     eIOHandlerBreakpoint,
     eIOHandlerWatchpoint
   };
 
   python::PythonModule &GetMainModule();
public:
   python::PythonDictionary m_sys_module_dict;
   python::PythonObject m_run_one_line_function;
   python::PythonObject m_run_one_line_str_global;
   std::string m_dictionary_name;
   ActiveIOHandler m_active_io_handler;
   bool m_session_is_active;
   bool m_pty_secondary_is_open;
   bool m_valid_session;
-  uint32_t m_lock_count;
+  std::atomic<uint32_t> m_lock_count;
   PyThreadState *m_command_thread_state;
 };
 
 class IOHandlerPythonInterpreter : public IOHandler {
 public:
   IOHandlerPythonInterpreter(Debugger &debugger,
                              ScriptInterpreterPythonImpl *python)
       : IOHandler(debugger, IOHandler::Type::PythonInterpreter),

lldb/source/Target/Process.cpp

while (true) {
 
     if (event_sp)
       HandlePrivateEvent(event_sp);
   }
   return state;
 }
 
 void Process::LoadOperatingSystemPlugin(bool flush) {
+  std::lock_guard<std::recursive_mutex> guard(m_thread_mutex);
   if (flush)
     m_thread_list.Clear();
   m_os_up.reset(OperatingSystem::FindPlugin(this, nullptr));
   if (flush)
     Flush();
 }
 
 Status Process::Launch(ProcessLaunchInfo &launch_info) {