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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h:386
+    // to add 1 to its return value.
+    return m_lock_count.fetch_add(1, std::memory_order_relaxed) + 1;
+  }
----------------
JDevlieghere wrote:
> 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
Makes sense, we shouldn't touch the implementation of `IncrementLockCount`.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h:389-398
   uint32_t DecrementLockCount() {
-    if (m_lock_count > 0)
-      --m_lock_count;
-    return m_lock_count;
+    // std::atomic::fetch_sub is an atomic post-decrement operation so we need
+    // to subtract 1 from 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;
----------------
You're right, but I think we should at least keep the guardrails to prevent returning an underflow value and return 0 instead.


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

https://reviews.llvm.org/D154271