Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
cameron314 added a comment. Hmm, that's not good. No, I don't have a Linux machine set up. I can set up a VM but if you already have the dumps/logs it would be faster to start with those. Thanks! Repository: rL LLVM http://reviews.llvm.org/D19122 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
labath added a subscriber: labath. labath added a comment. Hi, I have reverted this as it was causing a number of failures on the linux build bot http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/13585. I think we'll need to investigate them before putting this in. Do you have the ability to run the linux test suite? If not, I can send you some logs and core files from the build machine... Repository: rL LLVM http://reviews.llvm.org/D19122 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
This revision was automatically updated to reflect the committed changes.
Closed by commit rL266733: LLDB: Fixed two race conditions when stopping
private state thread (authored by mamai).
Changed prior to commit:
http://reviews.llvm.org/D19122?vs=54098&id=54185#toc
Repository:
rL LLVM
http://reviews.llvm.org/D19122
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
@@ -4112,11 +4112,8 @@
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
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
+// Signal the private state thread
+if (PrivateStateThreadIsValid())
{
TimeValue timeout_time;
bool timed_out;
@@ -4134,7 +4131,7 @@
{
if (timed_out)
{
-Error error = private_state_thread.Cancel();
+Error error = m_private_state_thread.Cancel();
if (log)
log->Printf ("Timed out responding to the control event,
cancel got error: \"%s\".", error.AsCString());
}
@@ -4145,7 +4142,7 @@
}
thread_result_t result = NULL;
-private_state_thread.Join(&result);
+m_private_state_thread.Join(&result);
m_private_state_thread.Reset();
}
}
@@ -4449,7 +4446,6 @@
if (!is_secondary_thread)
m_public_run_lock.SetStopped();
m_private_state_control_wait.SetValue (true, eBroadcastAlways);
-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
@@ -3310,7 +3310,10 @@
bool
PrivateStateThreadIsValid () const
{
-return m_private_state_thread.IsJoinable();
+lldb::StateType state = m_private_state.GetValue();
+return state != lldb::eStateDetached &&
+ state != lldb::eStateExited &&
+ m_private_state_thread.IsJoinable();
}
void
Index: lldb/trunk/source/Target/Process.cpp
===
--- lldb/trunk/source/Target/Process.cpp
+++ lldb/trunk/source/Target/Process.cpp
@@ -4112,11 +4112,8 @@
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
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
+// Signal the private state thread
+if (PrivateStateThreadIsValid())
{
TimeValue timeout_time;
bool timed_out;
@@ -4134,7 +4131,7 @@
{
if (timed_out)
{
-Error error = private_state_thread.Cancel();
+Error error = m_private_state_thread.Cancel();
if (log)
log->Printf ("Timed out responding to the control event, cancel got error: \"%s\".", error.AsCString());
}
@@ -4145,7 +4142,7 @@
}
thread_result_t result = NULL;
-private_state_thread.Join(&result);
+m_private_state_thread.Join(&result);
m_private_state_thread.Reset();
}
}
@@ -4449,7 +4446,6 @@
if (!is_secondary_thread)
m_public_run_lock.SetStopped();
m_private_state_control_wait.SetValue (true, eBroadcastAlways);
-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
@@ -3310,7 +3310,10 @@
bool
PrivateStateThreadIsValid () const
{
-return m_private_state_thread.IsJoinable();
+lldb::StateType state = m_private_state.GetValue();
+return state != lldb::eStateDetached &&
+ state != lldb::eStateExited &&
+ m_private_state_thread.IsJoinable();
}
void
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good. http://reviews.llvm.org/D19122 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
cameron314 updated this revision to Diff 54098.
cameron314 added a comment.
Oops, uploaded wrong diff a minute ago. Here's the one with the full fix.
http://reviews.llvm.org/D19122
Files:
include/lldb/Target/Process.h
source/Target/Process.cpp
Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -4112,11 +4112,8 @@
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
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
+// Signal the private state thread
+if (PrivateStateThreadIsValid())
{
TimeValue timeout_time;
bool timed_out;
@@ -4134,7 +4131,7 @@
{
if (timed_out)
{
-Error error = private_state_thread.Cancel();
+Error error = m_private_state_thread.Cancel();
if (log)
log->Printf ("Timed out responding to the control event,
cancel got error: \"%s\".", error.AsCString());
}
@@ -4145,7 +4142,7 @@
}
thread_result_t result = NULL;
-private_state_thread.Join(&result);
+m_private_state_thread.Join(&result);
m_private_state_thread.Reset();
}
}
@@ -4449,7 +4446,6 @@
if (!is_secondary_thread)
m_public_run_lock.SetStopped();
m_private_state_control_wait.SetValue (true, eBroadcastAlways);
-m_private_state_thread.Reset();
return NULL;
}
Index: include/lldb/Target/Process.h
===
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -3310,7 +3310,10 @@
bool
PrivateStateThreadIsValid () const
{
-return m_private_state_thread.IsJoinable();
+lldb::StateType state = m_private_state.GetValue();
+return state != lldb::eStateDetached &&
+ state != lldb::eStateExited &&
+ m_private_state_thread.IsJoinable();
}
void
Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -4112,11 +4112,8 @@
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
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
+// Signal the private state thread
+if (PrivateStateThreadIsValid())
{
TimeValue timeout_time;
bool timed_out;
@@ -4134,7 +4131,7 @@
{
if (timed_out)
{
-Error error = private_state_thread.Cancel();
+Error error = m_private_state_thread.Cancel();
if (log)
log->Printf ("Timed out responding to the control event, cancel got error: \"%s\".", error.AsCString());
}
@@ -4145,7 +4142,7 @@
}
thread_result_t result = NULL;
-private_state_thread.Join(&result);
+m_private_state_thread.Join(&result);
m_private_state_thread.Reset();
}
}
@@ -4449,7 +4446,6 @@
if (!is_secondary_thread)
m_public_run_lock.SetStopped();
m_private_state_control_wait.SetValue (true, eBroadcastAlways);
-m_private_state_thread.Reset();
return NULL;
}
Index: include/lldb/Target/Process.h
===
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -3310,7 +3310,10 @@
bool
PrivateStateThreadIsValid () const
{
-return m_private_state_thread.IsJoinable();
+lldb::StateType state = m_private_state.GetValue();
+return state != lldb::eStateDetached &&
+ state != lldb::eStateExited &&
+ m_private_state_thread.IsJoinable();
}
void
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
cameron314 updated the summary for this revision.
cameron314 removed rL LLVM as the repository for this revision.
cameron314 updated this revision to Diff 54095.
cameron314 added a comment.
I've updated the diff to include a fix for the race between Detach and Stop.
This has been working nicely for us without any problems for the last few days.
I looked again at putting back the wrapper for the m_private_state_thread, but
it really doesn't make sense to me. Anything that can go wrong without the
wrapper can still happen with the wrapper.
http://reviews.llvm.org/D19122
Files:
include/lldb/Target/Process.h
source/Target/Process.cpp
Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -4112,11 +4112,8 @@
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
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
+// Signal the private state thread
+if (m_private_state_thread.IsJoinable())
{
TimeValue timeout_time;
bool timed_out;
@@ -4134,7 +4131,7 @@
{
if (timed_out)
{
-Error error = private_state_thread.Cancel();
+Error error = m_private_state_thread.Cancel();
if (log)
log->Printf ("Timed out responding to the control event,
cancel got error: \"%s\".", error.AsCString());
}
@@ -4145,7 +4142,7 @@
}
thread_result_t result = NULL;
-private_state_thread.Join(&result);
+m_private_state_thread.Join(&result);
m_private_state_thread.Reset();
}
}
@@ -4449,7 +4446,6 @@
if (!is_secondary_thread)
m_public_run_lock.SetStopped();
m_private_state_control_wait.SetValue (true, eBroadcastAlways);
-m_private_state_thread.Reset();
return NULL;
}
Index: include/lldb/Target/Process.h
===
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -3310,7 +3310,10 @@
bool
PrivateStateThreadIsValid () const
{
-return m_private_state_thread.IsJoinable();
+lldb::StateType state = m_private_state.GetValue();
+return state != lldb::eStateDetached &&
+ state != lldb::eStateExited &&
+ m_private_state_thread.IsJoinable();
}
void
Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -4112,11 +4112,8 @@
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
-HostThread private_state_thread(m_private_state_thread);
-if (private_state_thread.IsJoinable())
+// Signal the private state thread
+if (m_private_state_thread.IsJoinable())
{
TimeValue timeout_time;
bool timed_out;
@@ -4134,7 +4131,7 @@
{
if (timed_out)
{
-Error error = private_state_thread.Cancel();
+Error error = m_private_state_thread.Cancel();
if (log)
log->Printf ("Timed out responding to the control event, cancel got error: \"%s\".", error.AsCString());
}
@@ -4145,7 +4142,7 @@
}
thread_result_t result = NULL;
-private_state_thread.Join(&result);
+m_private_state_thread.Join(&result);
m_private_state_thread.Reset();
}
}
@@ -4449,7 +4446,6 @@
if (!is_secondary_thread)
m_public_run_lock.SetStopped();
m_private_state_control_wait.SetValue (true, eBroadcastAlways);
-m_private_state_thread.Reset();
return NULL;
}
Index: include/lldb/Target/Process.h
===
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -3310,7 +3310,10 @@
bool
PrivateStateThreadIsValid () const
{
-return m_private_state_thread.IsJoinable();
+lldb::StateType state = m_private_state.GetValue();
+return state != lldb::eStateDetached &&
+ state != lldb::eStateExited &&
+ m_private_state_thread.IsJoinable();
}
void
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
cameron314 added a comment.
Ooh, that might work. But when ControlProvateStateThread resets
m_private_state_control_wait to false there's still a race between that and the
thread exiting. It could then be set back to false even after the thread has
exited (this is even likely for a detach).
I tried a different approach, changing `PrivateStateThreadIsValid` to the
following. I'm not sure it's the right thing to do, but it certainly fixes the
race (and thus timeout) on our platform:
bool
PrivateStateThreadIsValid () const
{
lldb::StateType state = m_private_state.GetValue();
return state != lldb::eStateDetached &&
state != lldb::eStateExited &&
m_private_state_thread.IsJoinable();
}
Repository:
rL LLVM
http://reviews.llvm.org/D19122
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
clayborg added a comment.
> I think the real bug is in StopPrivateStateThread, which only sends the stop
> state if the thread is still joinable; since I removed the Reset, it is of
> course still joinable after it exits. But even with the Reset (which we agree
> shouldn't be there), there is still a race between the time the detach signal
> is sent and when the thread sees it and exits. It seems once a detach or stop
> is sent, no further detaches/stops can be sent, regardless of what the thread
> is doing (since it will exit as soon as it sees the first one). So there
> should be a flag to that effect somewhere checked in
> ControlPrivateStateThread. Probably all the calls to `IsJoinable()` should be
> replaced with calls to `PrivateStateThreadIsValid`, which should be updated
> to check that flag.
>
> I don't see a nice way to add this flag for each private state thread,
> though, instead of for the process object as a whole. But maybe that's all
> that's necessary?
You can just check the value of m_private_state_control_wait:
void
Process::ControlPrivateStateThread (uint32_t 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
HostThread private_state_thread(m_private_state_thread);
if (private_state_thread.IsJoinable() &&
m_private_state_control_wait.GetValue() == false)
{
m_private_state_control_wait is always false if the private state thread is
running. It is set to true immediately after sending a control and syncing with
the sender and then it is set back to false. It is set to true again when the
thread exits.
Repository:
rL LLVM
http://reviews.llvm.org/D19122
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
cameron314 added a comment. OK, I'll pare down the patch to just that line then. I found the reason for the time out. At the end of our debug session, we destroy the Target, which destroys the process. Process::Destroy in turn calls Process::Detach, which calls DoDetach just before calling StopPrivateStateThread (which times out). Our platform's process object (modeled after the GDB remote one) sets the thread state to eStateDetached in its DoDetach implementation; and the private state thread exits as soon as it sees that state, meaning it's no longer around handle the eBroadcastInternalStateControlStop state event from the StopPrivateStateThread just after. I think the real bug is in StopPrivateStateThread, which only sends the stop state if the thread is still joinable; since I removed the Reset, it is of course still joinable after it exits. But even with the Reset (which we agree shouldn't be there), there is still a race between the time the detach signal is sent and when the thread sees it and exits. It seems once a detach or stop is sent, no further detaches/stops can be sent, regardless of what the thread is doing (since it will exit as soon as it sees the first one). So there should be a flag to that effect somewhere checked in ControlPrivateStateThread. Probably all the calls to `IsJoinable()` should be replaced with calls to `PrivateStateThreadIsValid`, which should be updated to check that flag. I don't see a nice way to add this flag for each private state thread, though, instead of for the process object as a whole. But maybe that's all that's necessary? Repository: rL LLVM http://reviews.llvm.org/D19122 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
jingham added a subscriber: jingham. jingham added a comment. At the Command & SB API level, we restrict access to the process from multiple threads with the run lock. But internally it is currently more of a gentleman's agreement not to do this. I don't think it would ever be useful to try to make calling functions in the target (which is the job of RunThreadPlan) work concurrently. Rather we should have some higher level permission baton that you have to have to do anything that would run the target, and then let only one thread at a time do this work. That would make it impossible for RunThreadPlan to get run on multiple threads for the same process. So while it wouldn't hurt to protect the m_private_state_thread variable with locks, that's not really the right level to do this, and if you ever let two threads try to run function calls in the target simultaneously, racy-ness in the m_private_state_thread would be the least of your problems... Jim Repository: rL LLVM http://reviews.llvm.org/D19122 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
At the Command & SB API level, we restrict access to the process from multiple threads with the run lock. But internally it is currently more of a gentleman's agreement not to do this. I don't think it would ever be useful to try to make calling functions in the target (which is the job of RunThreadPlan) work concurrently. Rather we should have some higher level permission baton that you have to have to do anything that would run the target, and then let only one thread at a time do this work. That would make it impossible for RunThreadPlan to get run on multiple threads for the same process. So while it wouldn't hurt to protect the m_private_state_thread variable with locks, that's not really the right level to do this, and if you ever let two threads try to run function calls in the target simultaneously, racy-ness in the m_private_state_thread would be the least of your problems... Jim > On Apr 14, 2016, at 3:44 PM, Cameron wrote: > > cameron314 added a comment. > > I read the same docs :D This is the important part: > >> If multiple threads of execution access the same shared_ptr without >> synchronization and any of those accesses uses a non-const member function >> of shared_ptr then a data race will occur; the shared_ptr overloads of >> atomic functions can be used to prevent the data race. > > > Copying the shared pointer and accessing its pointee through it is thread > safe (copying a wrapper object technically isn't, but it will boil down to > the same thing, so let's leave that aside). But copying it while the pointer > is being reassigned (`operator=`) on a different thread is //not// thread > safe. > > To answer your question, yes, absolutely, we can just remove that one call to > Reset. But if the copy is still necessary, that means a lot of existing code > (including that copy) is also racy and should be reviewed (though this would > make sense to do in a separate patch). > > > Repository: > rL LLVM > > http://reviews.llvm.org/D19122 > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
clayborg added a comment. agreed Repository: rL LLVM http://reviews.llvm.org/D19122 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
cameron314 added a comment. I read the same docs :D This is the important part: > If multiple threads of execution access the same shared_ptr without > synchronization and any of those accesses uses a non-const member function of > shared_ptr then a data race will occur; the shared_ptr overloads of atomic > functions can be used to prevent the data race. Copying the shared pointer and accessing its pointee through it is thread safe (copying a wrapper object technically isn't, but it will boil down to the same thing, so let's leave that aside). But copying it while the pointer is being reassigned (`operator=`) on a different thread is //not// thread safe. To answer your question, yes, absolutely, we can just remove that one call to Reset. But if the copy is still necessary, that means a lot of existing code (including that copy) is also racy and should be reviewed (though this would make sense to do in a separate patch). Repository: rL LLVM http://reviews.llvm.org/D19122 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
clayborg added a comment. We should figure out what is going wrong on your system though and figure out why we need to call Cancel() as well. Repository: rL LLVM http://reviews.llvm.org/D19122 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
clayborg added a comment. from the C++ docs: > All member functions (including copy constructor and copy assignment) can be > called by multiple threads on different instances of shared_ptr without > additional synchronization even if these instances are copies and share > ownership of the same object. If multiple threads of execution access the > same shared_ptr without synchronization and any of those accesses uses a > non-const member function of shared_ptr then a data race will occur; the > shared_ptr overloads of atomic functions can be used to prevent the data race. So we might not need to do anything. So back to my original comment: can we just remove all changes except the "m_private_state_thread.Reset();" in Process::RunPrivateStateThread()? Repository: rL LLVM http://reviews.llvm.org/D19122 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
cameron314 added a comment. Hmm, interesting. Yes, it should not be timing out in the first place. That's likely a bug on our side, still to be determined. I'm looking into it. Regardless, it led us to find and fix this race :-) I think it still makes sense for LLDB to do a `Cancel()` as a last-ditch effort on timing out. But `Cancel()` is fundamentally very dangerous, so then again, maybe not. Can the code that spins up the extra thread (RunThreadPlan) run on a different thread than ControlPrivateStateThread runs on? I was under the (perhaps entirely false!) impression that the Process object was used by only one thread at a time (with the exception of the private state thread(s), of course, but that thread shouldn't touch its own control variables). If that's not the case, then looking at the RunThreadPlan code, there's lots more races on m_private_state_thread alone (e.g. calling EqualsThread on it while it could be assigned from another thread). And even just creating a copy is not thread safe, for example, since it could be reassigned from another thread at the same time... you'd need `m_private_state_thread` to be wrap its shared pointer with `std::atomic`. Repository: rL LLVM http://reviews.llvm.org/D19122 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
clayborg added a comment. HostThread::Reset() is a bad function and should not be directly used by anyone other that Join() or Detach(). Just clearing the thread ID and setting the result to zero is bad and means we will leak a thread if no one else joins it. If someone else already called Detach() on the thread, then Reset() is ok. Also calling m_private_state_thread.Cancel() is equally bad. The real question is why is the cancel timing out? I agree that the call to "m_private_state_thread.Reset();" should be removed from Process::RunPrivateStateThread(). But we should look at why we need to call Cancel in the first place. We should never need to cancel our own thread unless something really bad has gone on. So it seems like the real bug here is bad thread synchronization leading to issues. I don't like the fact that we are not copying the thread anymore. We have code that sometimes needs to spin up an extra private state thread and I wouldn't want the code that changes the m_private_state_thread after backing up an older one, cause this function to call IsJoinable(), Cancel() and Join() on different objects. So it seems like the only needed fix here is to not call "m_private_state_thread.Reset();" in Process::RunPrivateStateThread(), all other changes can be removed? Repository: rL LLVM http://reviews.llvm.org/D19122 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
cameron314 added a comment. Note also that using a copy of the instance variable is no different from using the instance variable directly, here, since as you say they both have a shared pointer to the same underlying object which is manipulated through either of the two to the same effect. Repository: rL LLVM http://reviews.llvm.org/D19122 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
cameron314 added inline comments.
Comment at: source/Target/Process.cpp:4116
@@ -4120,1 +4115,3 @@
+// Signal the private state thread
+if (m_private_state_thread.IsJoinable())
{
clayborg wrote:
> If you are going to do IsJoinable(), Cancel(), and Join() then this is still
> racy in that someone else could do the same thing on another thread. So this
> code should probably take a mutex to protect:
>
> ```
> Mutex::Locker locker(m_private_state_thread.GetMutex());
> if (m_private_state_thread.IsJoinable())
> ```
>
> This would need to live in HostNativeThreadBase because HostThread contains a
> shared pointer to a HostNativeThreadBase and HostThread can be copied.
>
I removed the call to `Reset` from the other thread (line 4452 below). It
didn't make sense for a thread to reset its own `HostThread` since it's not its
own owner, the Process is.
So now only the `Process` is allowed to control the `HostThread` it owns, and
there's no races and thus no need for a mutex.
Repository:
rL LLVM
http://reviews.llvm.org/D19122
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D19122: LLDB: Fixed race condition on timeout when stopping private state thread
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
If you use the instance variable directly, we will need a mutex inside the
HostNativeThreadBase class to protect against multi-threaded access. I believe
the reason it was being copied before was in case the Reset was called on
another thread. So we might want a mutex in HostNativeThreadBase that gets
exposed via HostThread. See inlined comments.
Comment at: source/Target/Process.cpp:4116
@@ -4120,1 +4115,3 @@
+// Signal the private state thread
+if (m_private_state_thread.IsJoinable())
{
If you are going to do IsJoinable(), Cancel(), and Join() then this is still
racy in that someone else could do the same thing on another thread. So this
code should probably take a mutex to protect:
```
Mutex::Locker locker(m_private_state_thread.GetMutex());
if (m_private_state_thread.IsJoinable())
```
This would need to live in HostNativeThreadBase because HostThread contains a
shared pointer to a HostNativeThreadBase and HostThread can be copied.
Repository:
rL LLVM
http://reviews.llvm.org/D19122
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
