[Lldb-commits] [lldb] [lldb] Make ProcessRunLock recursive on the read side per-thread (PR #201685)

[Med Ismail Bennani via lldb-commits]

================
@@ -51,6 +57,66 @@ bool ProcessRunLock::SetStopped() {
   return was_running;
 }
 
-} // namespace lldb_private
+#endif // !_WIN32
+
+/// Per-thread map of which ProcessRunLocks this thread already holds, with
+/// a recursion count. ProcessRunLocker uses this to detect re-entrant
+/// acquisitions and bump the count instead of recursively acquiring the
+/// underlying rwlock — recursive shared acquisition is either unsafe or
+/// implementation-defined, and on a write-preferring rwlock would deadlock
+/// against a queued writer.
+static llvm::DenseMap<ProcessRunLock *, uint32_t> &GetHeldLocks() {
+  static thread_local llvm::DenseMap<ProcessRunLock *, uint32_t> s_held;
----------------
medismailben wrote:

Tried this and it doesn't quite hold up — `single-std::atomic<thread::id>` can only record one holder, but pthread_rwlock_t in read mode can have many concurrent ones.

Concretely: `m_public_run_lock` regularly has multiple readers. The lldb command interpreter, the DAP request handler, and any test/SB API client can all enter SB API simultaneously, all succeed at `pthread_rwlock_rdlock` (rwlocks allow concurrent readers), and now we have N legitimate holders for one slot. Whichever thread wins the `compare_and_swap` becomes "the holder"; the others are real readers that the bookkeeping has no record of.

When one of those unrecorded readers later re-enters (e.g., it dispatches a frame-provider Python callback that calls back into SB API), its TryLock checks `m_holder == self`, sees the other thread's id, falls through to the real `rdlock`, and we're back to the original deadlock — a queued writer (PST SetStopped) blocks the inner reader exactly as before.

To track multiple concurrent readers we need a container, not a scalar. A `DenseMap<thread::id, uint32_t>` per `ProcessRunLock` is what landed: same colocation property as your suggestion (state lives in the lock, not in TLS), but actually large enough to hold all current holders. Cost is one mutex + a hash lookup per `TryLock`/`Unlock` — comparable to the global-TLS version, just relocated.

I do agree the colocation is the right framing — that's the change from the prior version. It just turned out the data structure had to grow with it.

https://github.com/llvm/llvm-project/pull/201685