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