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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D154271#4466170 , @bulbazord wrote:

> Making `m_lock_count`'s type into `std::atomic` 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 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

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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

In D154271#4466339 , @JDevlieghere 
wrote:

> In D154271#4466170 , @bulbazord 
> wrote:
>
>> Making `m_lock_count`'s type into `std::atomic` 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.

@bulbazord I understand the confusion regarding `OperatingSystem` but it's used 
for "OS Thread Plugins", a way to mock a thread using python script, so it does 
change the `ThreadList`.


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

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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 536540.
mib edited the summary of this revision.
mib added a comment.

Use `std::atomic::fetch_{add, sub}` to address @JDevlieghere feedback.


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

https://reviews.llvm.org/D154271

Files:
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/source/Target/Process.cpp


Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2467,6 +2467,7 @@
 }
 
 void Process::LoadOperatingSystemPlugin(bool flush) {
+  std::lock_guard guard(m_thread_mutex);
   if (flush)
 m_thread_list.Clear();
   m_os_up.reset(OperatingSystem::FindPlugin(this, nullptr));
Index: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -380,12 +380,22 @@
 
   uint32_t IsExecutingPython() const { return m_lock_count > 0; }
 
-  uint32_t IncrementLockCount() { return ++m_lock_count; }
+  uint32_t IncrementLockCount() {
+// std::atomic::fetch_add is an atomic post-increment operation so we need
+// to add 1 to its return value.
+return m_lock_count.fetch_add(1, std::memory_order_relaxed) + 1;
+  }
 
   uint32_t DecrementLockCount() {
-if (m_lock_count > 0)
-  --m_lock_count;
-return m_lock_count;
+// std::atomic::fetch_sub is an atomic post-increment operation so we need
+// to subtract 1 to its return value.
+uint32_t count = m_lock_count.fetch_sub(1, std::memory_order_relaxed) - 1;
+if (static_cast(count) < 0) {
+  // Handle underflow here & reset count to zero.
+  count = 0;
+  m_lock_count.store(count, std::memory_order_relaxed);
+}
+return count;
   }
 
   enum ActiveIOHandler {
@@ -421,7 +431,7 @@
   bool m_session_is_active;
   bool m_pty_secondary_is_open;
   bool m_valid_session;
-  uint32_t m_lock_count;
+  std::atomic m_lock_count;
   PyThreadState *m_command_thread_state;
 };
 


Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2467,6 +2467,7 @@
 }
 
 void Process::LoadOperatingSystemPlugin(bool flush) {
+  std::lock_guard guard(m_thread_mutex);
   if (flush)
 m_thread_list.Clear();
   m_os_up.reset(OperatingSystem::FindPlugin(this, nullptr));
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -380,12 +380,22 @@
 
   uint32_t IsExecutingPython() const { return m_lock_count > 0; }
 
-  uint32_t IncrementLockCount() { return ++m_lock_count; }
+  uint32_t IncrementLockCount() {
+// std::atomic::fetch_add is an atomic post-increment operation so we need
+// to add 1 to its return value.
+return m_lock_count.fetch_add(1, std::memory_order_relaxed) + 1;
+  }
 
   uint32_t DecrementLockCount() {
-if (m_lock_count > 0)
-  --m_lock_count;
-return m_lock_count;
+// std::atomic::fetch_sub is an atomic post-increment operation so we need
+// to subtract 1 to its return value.
+uint32_t count = m_lock_count.fetch_sub(1, std::memory_order_relaxed) - 1;
+if (static_cast(count) < 0) {
+  // Handle underflow here & reset count to zero.
+  count = 0;
+  m_lock_count.store(count, std::memory_order_relaxed);
+}
+return count;
   }
 
   enum ActiveIOHandler {
@@ -421,7 +431,7 @@
   bool m_session_is_active;
   bool m_pty_secondary_is_open;
   bool m_valid_session;
-  uint32_t m_lock_count;
+  std::atomic m_lock_count;
   PyThreadState *m_command_thread_state;
 };
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 536541.
mib added a comment.

Fix typo


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

https://reviews.llvm.org/D154271

Files:
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/source/Target/Process.cpp


Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2467,6 +2467,7 @@
 }
 
 void Process::LoadOperatingSystemPlugin(bool flush) {
+  std::lock_guard guard(m_thread_mutex);
   if (flush)
 m_thread_list.Clear();
   m_os_up.reset(OperatingSystem::FindPlugin(this, nullptr));
Index: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -380,12 +380,22 @@
 
   uint32_t IsExecutingPython() const { return m_lock_count > 0; }
 
-  uint32_t IncrementLockCount() { return ++m_lock_count; }
+  uint32_t IncrementLockCount() {
+// std::atomic::fetch_add is an atomic post-increment operation so we need
+// to add 1 to its return value.
+return m_lock_count.fetch_add(1, std::memory_order_relaxed) + 1;
+  }
 
   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 to its return value.
+uint32_t count = m_lock_count.fetch_sub(1, std::memory_order_relaxed) - 1;
+if (static_cast(count) < 0) {
+  // Handle underflow here & reset count to zero.
+  count = 0;
+  m_lock_count.store(count, std::memory_order_relaxed);
+}
+return count;
   }
 
   enum ActiveIOHandler {
@@ -421,7 +431,7 @@
   bool m_session_is_active;
   bool m_pty_secondary_is_open;
   bool m_valid_session;
-  uint32_t m_lock_count;
+  std::atomic m_lock_count;
   PyThreadState *m_command_thread_state;
 };
 


Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2467,6 +2467,7 @@
 }
 
 void Process::LoadOperatingSystemPlugin(bool flush) {
+  std::lock_guard guard(m_thread_mutex);
   if (flush)
 m_thread_list.Clear();
   m_os_up.reset(OperatingSystem::FindPlugin(this, nullptr));
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -380,12 +380,22 @@
 
   uint32_t IsExecutingPython() const { return m_lock_count > 0; }
 
-  uint32_t IncrementLockCount() { return ++m_lock_count; }
+  uint32_t IncrementLockCount() {
+// std::atomic::fetch_add is an atomic post-increment operation so we need
+// to add 1 to its return value.
+return m_lock_count.fetch_add(1, std::memory_order_relaxed) + 1;
+  }
 
   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 to its return value.
+uint32_t count = m_lock_count.fetch_sub(1, std::memory_order_relaxed) - 1;
+if (static_cast(count) < 0) {
+  // Handle underflow here & reset count to zero.
+  count = 0;
+  m_lock_count.store(count, std::memory_order_relaxed);
+}
+return count;
   }
 
   enum ActiveIOHandler {
@@ -421,7 +431,7 @@
   bool m_session_is_active;
   bool m_pty_secondary_is_open;
   bool m_valid_session;
-  uint32_t m_lock_count;
+  std::atomic m_lock_count;
   PyThreadState *m_command_thread_state;
 };
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 536542.
mib added a subscriber: jasonmolenda.
mib added a comment.

Fix major typo, thanks @jasonmolenda ;)


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

https://reviews.llvm.org/D154271

Files:
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
  lldb/source/Target/Process.cpp


Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2467,6 +2467,7 @@
 }
 
 void Process::LoadOperatingSystemPlugin(bool flush) {
+  std::lock_guard guard(m_thread_mutex);
   if (flush)
 m_thread_list.Clear();
   m_os_up.reset(OperatingSystem::FindPlugin(this, nullptr));
Index: 
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -380,12 +380,22 @@
 
   uint32_t IsExecutingPython() const { return m_lock_count > 0; }
 
-  uint32_t IncrementLockCount() { return ++m_lock_count; }
+  uint32_t IncrementLockCount() {
+// std::atomic::fetch_add is an atomic post-increment operation so we need
+// to add 1 to its return value.
+return m_lock_count.fetch_add(1, std::memory_order_relaxed) + 1;
+  }
 
   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(count) < 0) {
+  // Handle underflow here & reset count to zero.
+  count = 0;
+  m_lock_count.store(count, std::memory_order_relaxed);
+}
+return count;
   }
 
   enum ActiveIOHandler {
@@ -421,7 +431,7 @@
   bool m_session_is_active;
   bool m_pty_secondary_is_open;
   bool m_valid_session;
-  uint32_t m_lock_count;
+  std::atomic m_lock_count;
   PyThreadState *m_command_thread_state;
 };
 


Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2467,6 +2467,7 @@
 }
 
 void Process::LoadOperatingSystemPlugin(bool flush) {
+  std::lock_guard guard(m_thread_mutex);
   if (flush)
 m_thread_list.Clear();
   m_os_up.reset(OperatingSystem::FindPlugin(this, nullptr));
Index: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
===
--- lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
+++ lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h
@@ -380,12 +380,22 @@
 
   uint32_t IsExecutingPython() const { return m_lock_count > 0; }
 
-  uint32_t IncrementLockCount() { return ++m_lock_count; }
+  uint32_t IncrementLockCount() {
+// std::atomic::fetch_add is an atomic post-increment operation so we need
+// to add 1 to its return value.
+return m_lock_count.fetch_add(1, std::memory_order_relaxed) + 1;
+  }
 
   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(count) < 0) {
+  // Handle underflow here & reset count to zero.
+  count = 0;
+  m_lock_count.store(count, std::memory_order_relaxed);
+}
+return count;
   }
 
   enum ActiveIOHandler {
@@ -421,7 +431,7 @@
   bool m_session_is_active;
   bool m_pty_secondary_is_open;
   bool m_valid_session;
-  uint32_t m_lock_count;
+  std::atomic m_lock_count;
   PyThreadState *m_command_thread_state;
 };
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[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(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