Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2

[Pavel Labath via lldb-commits]

labath added a subscriber: labath.
labath added a comment.

OK, i see. Thanks for the explanation. This may actually be some
windows specific thing then, as I remember zachary mentioning they
have some flakyness issues there.

BTW, this has sped up the LLDB test suite nearly 2x, so thanks a lot
for that. :)

pl


Repository:
  rL LLVM

http://reviews.llvm.org/D21296



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

Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2

[Pavel Labath via lldb-commits]

OK, i see. Thanks for the explanation. This may actually be some
windows specific thing then, as I remember zachary mentioning they
have some flakyness issues there.

BTW, this has sped up the LLDB test suite nearly 2x, so thanks a lot
for that. :)

pl


On 14 June 2016 at 17:13, Cameron  wrote:
> cameron314 added a comment.
>
> Thanks everyone :-)
>
> Ah, yeah, sorry if I gave the wrong impression, but that comment is not 
> specific to Linux (in fact, I've only seen it once, on Windows). At one point 
> the debugger had entered ControlPrivateStateThread on one thread to stop it, 
> seen that the thread was already in an invalid state (it was), and assumed 
> that meant that the thread was already exiting and did a join without sending 
> the stop. But the state thread somehow wasn't on its way out yet, it was 
> stuck waiting for a control event first (this is the part that I'm not sure 
> should be possible, but empirically is). This caused a deadlock. So I changed 
> my patch to always send the event if the thread is joinable, not just if its 
> state is valid, and left that comment to explain why this must remain so.
>
>
> http://reviews.llvm.org/D21296
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2

[Cameron via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL272682: [lldb] Fixed race conditions on private state thread 
exit (authored by cameron314).

Changed prior to commit:
  http://reviews.llvm.org/D21296?vs=60536=60693#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D21296

Files:
  lldb/trunk/include/lldb/Target/Process.h
  lldb/trunk/source/Target/Process.cpp

Index: lldb/trunk/source/Target/Process.cpp
===
--- lldb/trunk/source/Target/Process.cpp
+++ lldb/trunk/source/Target/Process.cpp
@@ -4088,7 +4088,7 @@
 void
 Process::StopPrivateStateThread ()
 {
-if (PrivateStateThreadIsValid ())
+if (m_private_state_thread.IsJoinable ())
 ControlPrivateStateThread (eBroadcastInternalStateControlStop);
 else
 {
@@ -4110,21 +4110,23 @@
 if (log)
 log->Printf ("Process::%s (signal = %d)", __FUNCTION__, signal);
 
-// Signal the private state thread. First we should copy this is case the
-// thread starts exiting since the private state thread will NULL this out
-// when it exits
+// Signal the private state thread
+if (m_private_state_thread.IsJoinable())
 {
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
+// Broadcast the event.
+// It is important to do this outside of the if below, because
+// it's possible that the thread state is invalid but that the
+// thread is waiting on a control event instead of simply being
+// on its way out (this should not happen, but it apparently can).
+if (log)
+log->Printf ("Sending control event of type: %d.", signal);
+std::shared_ptr event_receipt_sp(new EventDataReceipt());
+m_private_state_control_broadcaster.BroadcastEvent(signal, event_receipt_sp);
+
+// Wait for the event receipt or for the private state thread to exit
+bool receipt_received = false;
+if (PrivateStateThreadIsValid())
 {
-if (log)
-log->Printf ("Sending control event of type: %d.", signal);
-// Send the control event and wait for the receipt or for the private state
-// thread to exit
-std::shared_ptr event_receipt_sp(new EventDataReceipt());
-m_private_state_control_broadcaster.BroadcastEvent(signal, event_receipt_sp);
-
-bool receipt_received = false;
 while (!receipt_received)
 {
 bool timed_out = false;
@@ -4137,23 +4139,24 @@
 if (!receipt_received)
 {
 // Check if the private state thread is still around. If it isn't then we are done waiting
-if (!m_private_state_thread.IsJoinable())
-break; // Private state thread exited, we are done
+if (!PrivateStateThreadIsValid())
+break; // Private state thread exited or is exiting, we are done
 }
 }
-
-if (signal == eBroadcastInternalStateControlStop)
-{
-thread_result_t result = NULL;
-private_state_thread.Join();
-}
 }
-else
+
+if (signal == eBroadcastInternalStateControlStop)
 {
-if (log)
-log->Printf ("Private state thread already dead, no need to signal it to stop.");
+thread_result_t result = NULL;
+m_private_state_thread.Join();
+m_private_state_thread.Reset();
 }
 }
+else
+{
+if (log)
+log->Printf("Private state thread already dead, no need to signal it to stop.");
+}
 }
 
 void
@@ -4446,7 +4449,6 @@
 // try to change it on the way out.
 if (!is_secondary_thread)
 m_public_run_lock.SetStopped();
-m_private_state_thread.Reset();
 return NULL;
 }
 
Index: lldb/trunk/include/lldb/Target/Process.h
===
--- lldb/trunk/include/lldb/Target/Process.h
+++ lldb/trunk/include/lldb/Target/Process.h
@@ -3309,9 +3309,13 @@
 bool
 PrivateStateThreadIsValid () const
 {
-return m_private_state_thread.IsJoinable();
+lldb::StateType state = m_private_state.GetValue();
+return state != lldb::eStateInvalid &&
+   state != lldb::eStateDetached &&
+   state != lldb::eStateExited &&
+   m_private_state_thread.IsJoinable();
 }
-
+
 void
 ForceNextEventDelivery()
 {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2

[Pavel Labath via lldb-commits]

labath accepted this revision.
labath added a comment.

Seems to run fine on linux now. Thanks for investigating this. We'll monitor 
the buildbots and let you know if anything bad happens. ;)

BTW. your comment in ControlPrivateStateThread seems to indicate that the linux 
behavior is inconsistent/unexpected in some way. Do you think it would be worth 
filing a bug about that?


http://reviews.llvm.org/D21296



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

Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2

[Greg Clayton via lldb-commits]

clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

Looks good if Jim is happy.


http://reviews.llvm.org/D21296



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

Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2

[Jim Ingham via lldb-commits]

jingham accepted this revision.
jingham added a comment.

This looks okay to me.


http://reviews.llvm.org/D21296



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

Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2

[Greg Clayton via lldb-commits]

clayborg added a comment.

Ok, as long as Jim agrees, then I will give it the go ahead.


http://reviews.llvm.org/D21296



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

Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2

[Cameron via lldb-commits]

cameron314 added a comment.

@clayborg: Thanks for having a look! I've added Jim Ingham as a reviewer. 
@jingham, I'd appreciate if you could take a few minutes to look this over.

Right, I'd seen the backup/restore of the thread. As far as I can tell it 
should still work; the code in `ControlPrivateStateThread` has no idea it's 
working with a temporary thread, just as it didn't know before (in fact, if you 
look carefully at the code in the present tip of the trunk, a recent change 
seems to have introduced a mix of using both `private_state_thread` and 
`m_private_state_thread`, probably by accident). `m_private_state_thread` 
cannot be reset to the backup during a control event, since the first thing 
that's done before restoring the backup thread is to stop the temporary thread.


http://reviews.llvm.org/D21296



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

Re: [Lldb-commits] [PATCH] D21296: [lldb] Fixed race condition on private state thread exit, take 2

[Greg Clayton via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

This looks like it would work for normal operation. I am not sure it will work 
when an extra private state thread is spun up. In certain circumstances we need 
to create more private state threads. This happens when you have an expression 
that is run from the private state thread. Since the private state thread is 
what controls the actual process, we can't run an expression from the current 
private state thread, so we spin up new ones. The current code is doing some 
tricky things to deal with this, and that was part of the reason 
Process::ControlPrivateStateThread() was making a copy of the current value of 
"m_private_state_thread" into a local variable named "private_state_thread":

  HostThread private_state_thread(m_private_state_thread);

See the code in "Process::RunThreadPlan()" around the code:

  if (m_private_state_thread.EqualsThread(Host::GetCurrentThread()))

The new loop as written in Process::ControlPrivateStateThread() could end up 
using m_private_state_thread with differing contents in the "if 
(m_private_state_thread.IsJoinable())" if statement. Jim Ingham made these 
changes, so we should add him to the reviewer list. I am going to mark as 
"Request Changes" so we can address any such issues, but Jim should chime in on 
this before we proceed.

The way this normally happens is an expression is being run and while handling 
the expression on the private state thread, we need to run another expression 
(like a call to "mmap" in the debugged process so we can allocate memory), so 
we need to spin up another private state thread to handle the needed starts and 
stops. Only one of these threads will be actively grabbing events at a single 
time, so your patch might just work, but I want to get some more eyes on this 
to be sure.

Please add Jim Ingham as a reviewer.


http://reviews.llvm.org/D21296



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