[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

In D53412#1450182 , @clayborg wrote:

> All of these are in SBProcess. No need to change everywhere else, I just see 
> a ton of duplicated code here in the SBProcess.cpp. If we contain those in a 
> small struct/class, then we can easily make changes as needed without 50 
> diffs in this file each time. So no need to fix all the call sites in this 
> patch, just the ones in SBProcess.cpp


Ah, that makes more sense :-) Sorry I misinterpreted your suggestion.

I can add a RAII lock-helper struct in SBProcess, but the order that locks are 
taken within it must not be modified without changing *all* the functions that 
take both locks (both directly and indirectly), which includes a significant 
portion of the SB API (and not just SBProcess).


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In general any call that is doing anything through the public API needs to take 
the target API mutex. It is ok for quick accessors or other things to not do 
so, but anything that would need to keep the process in its current state, like 
asking a frame for a register value, should take the API mutex to ensure one 
thread doesn't say "run" and another says "what is the PC from frame 0 of 
thread 1".


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Added Jim Ingham in case he wants to chime in here.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

All of these are in SBProcess. No need to change everywhere else, I just see a 
ton of duplicated code here in the SBProcess.cpp. If we contain those in a 
small struct/class, then we can easily make changes as needed without 50 diffs 
in this file each time. So no need to fix all the call sites in this patch, 
just the ones in SBProcess.cpp


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

There's dozens of places that take the API mutex without taking the process 
mutex. Take `Kill` for example: It needs to take the API mutex, but cannot take 
the run lock since it will be taken by the private state thread. Another 
example is `HandleCommand`, which takes the API mutex but has no process that 
it could ask to lock the API mutex for it.

On the flip side, all the `SBFrame` methods lock the process run lock but not 
the API mutex. And so on.

I just really don't think this can be refactored in a useful way without 
rewriting the way all SB locks are taken, which is almost impossible to do at 
this point.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D53412#1446252 , @cameron314 wrote:

> @clayborg, I'm not sure how that would work. There's many places that lock 
> the process run lock without locking the target API mutex, and vice versa.


Add an argument to the ProcessLocker constructor maybe?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

@clayborg, I'm not sure how that would work. There's many places that lock the 
process run lock without locking the target API mutex, and vice versa.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Or better yet, create a structure that everyone must use and have the locking 
exist in the class itself:

  struct ProcessLocker {
std::lock_guard guard;
Process::StopLocker stop_locker;
  
void ProcessLocker(Process ) {
   ...
}
  };




Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.
Herald added a project: LLDB.

Anyone?
We still have this patch applied on our recently-rebased fork with no 
problems...


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D53412



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

Ping?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53412



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 added a comment.

I suppose we could but there's a few places outside of SBProcess that also use 
the run lock and API mutex; personally, I prefer it to be explicit which mutex 
is being taken first.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53412



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Since this is duplicated in so many spots, can we make a helper function to do 
this to ensure no one gets it wrong. We might be able to pass in the "guard" 
and "stop_locker" as reference variables and modify them in the helper function.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53412



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D53412: [lldb] Fixed deadlock when SBProcess is Kill()ed and inspected

[Cameron via Phabricator via lldb-commits]

cameron314 created this revision.
cameron314 added reviewers: zturner, clayborg.
Herald added a subscriber: lldb-commits.

This patch changes the order that mutexes are acquired in SBProcess such that 
the target API mutex is now always acquired before the public run lock mutex is 
acquired.

This fixes a deadlock in LLDB when calling `SBProcess::Kill()` while calling 
e.g. `SBProcess::GetThreadByID()`: `Kill` takes the target API mutex, then 
waits for the private state thread to exit, which tries to lock the public run 
lock mutex, but it's already being held by `GetThreadByID`, which is waiting in 
turn for the target API mutex.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53412

Files:
  source/API/SBProcess.cpp

Index: source/API/SBProcess.cpp
===
--- source/API/SBProcess.cpp
+++ source/API/SBProcess.cpp
@@ -197,11 +197,10 @@
   uint32_t num_threads = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-Process::StopLocker stop_locker;
-
-const bool can_update = stop_locker.TryLock(_sp->GetRunLock());
 std::lock_guard guard(
 process_sp->GetTarget().GetAPIMutex());
+Process::StopLocker stop_locker;
+const bool can_update = stop_locker.TryLock(_sp->GetRunLock());
 num_threads = process_sp->GetThreadList().GetSize(can_update);
   }
 
@@ -457,10 +456,10 @@
   ThreadSP thread_sp;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-Process::StopLocker stop_locker;
-const bool can_update = stop_locker.TryLock(_sp->GetRunLock());
 std::lock_guard guard(
 process_sp->GetTarget().GetAPIMutex());
+Process::StopLocker stop_locker;
+const bool can_update = stop_locker.TryLock(_sp->GetRunLock());
 thread_sp = process_sp->GetThreadList().GetThreadAtIndex(index, can_update);
 sb_thread.SetThread(thread_sp);
   }
@@ -480,10 +479,10 @@
   uint32_t num_queues = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
+std::lock_guard guard(
+process_sp->GetTarget().GetAPIMutex());
 Process::StopLocker stop_locker;
 if (stop_locker.TryLock(_sp->GetRunLock())) {
-  std::lock_guard guard(
-  process_sp->GetTarget().GetAPIMutex());
   num_queues = process_sp->GetQueueList().GetSize();
 }
   }
@@ -502,10 +501,10 @@
   QueueSP queue_sp;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
+std::lock_guard guard(
+process_sp->GetTarget().GetAPIMutex());
 Process::StopLocker stop_locker;
 if (stop_locker.TryLock(_sp->GetRunLock())) {
-  std::lock_guard guard(
-  process_sp->GetTarget().GetAPIMutex());
   queue_sp = process_sp->GetQueueList().GetQueueAtIndex(index);
   sb_queue.SetQueue(queue_sp);
 }
@@ -816,10 +815,10 @@
   ThreadSP thread_sp;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-Process::StopLocker stop_locker;
-const bool can_update = stop_locker.TryLock(_sp->GetRunLock());
 std::lock_guard guard(
 process_sp->GetTarget().GetAPIMutex());
+Process::StopLocker stop_locker;
+const bool can_update = stop_locker.TryLock(_sp->GetRunLock());
 thread_sp = process_sp->GetThreadList().FindThreadByID(tid, can_update);
 sb_thread.SetThread(thread_sp);
   }
@@ -839,10 +838,10 @@
   ThreadSP thread_sp;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
-Process::StopLocker stop_locker;
-const bool can_update = stop_locker.TryLock(_sp->GetRunLock());
 std::lock_guard guard(
 process_sp->GetTarget().GetAPIMutex());
+Process::StopLocker stop_locker;
+const bool can_update = stop_locker.TryLock(_sp->GetRunLock());
 thread_sp =
 process_sp->GetThreadList().FindThreadByIndexID(index_id, can_update);
 sb_thread.SetThread(thread_sp);
@@ -959,10 +958,10 @@
 static_cast(sb_error.get()));
 
   if (process_sp) {
+std::lock_guard guard(
+process_sp->GetTarget().GetAPIMutex());
 Process::StopLocker stop_locker;
 if (stop_locker.TryLock(_sp->GetRunLock())) {
-  std::lock_guard guard(
-  process_sp->GetTarget().GetAPIMutex());
   bytes_read = process_sp->ReadMemory(addr, dst, dst_len, sb_error.ref());
 } else {
   if (log)
@@ -993,10 +992,10 @@
   size_t bytes_read = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
+std::lock_guard guard(
+process_sp->GetTarget().GetAPIMutex());
 Process::StopLocker stop_locker;
 if (stop_locker.TryLock(_sp->GetRunLock())) {
-  std::lock_guard guard(
-  process_sp->GetTarget().GetAPIMutex());
   bytes_read = process_sp->ReadCStringFromMemory(addr, (char *)buf, size,
  sb_error.ref());
 } else {
@@ -1018,10 +1017,10 @@
   uint64_t value = 0;
   ProcessSP process_sp(GetSP());
   if (process_sp) {
+std::lock_guard guard(
+process_sp->GetTarget().GetAPIMutex());
 Process::StopLocker stop_locker;
 if