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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
