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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added inline comments.
This revision now requires changes to proceed.


================
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;
+  }
----------------
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


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h:390-398
+    // 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;
+      m_lock_count.store(count, std::memory_order_relaxed);
----------------
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. 


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

https://reviews.llvm.org/D154271