[Lldb-commits] [PATCH] D154271: [lldb] Fix data race when interacting with python scripts

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D154271#4466170 <https://reviews.llvm.org/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.



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h:424
   bool m_valid_session;
-  uint32_t m_lock_count;
+  std::atomic<uint32_t> m_lock_count;
   PyThreadState *m_command_thread_state;
----------------
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. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154271/new/

https://reviews.llvm.org/D154271