[Lldb-commits] [PATCH]race condition calling PushProcessIOHandler
Shawn Best
sbest at blueshiftinc.com
Fri Aug 8 19:19:08 PDT 2014
Hi Matt,
Yeah that would probably be a good idea, particularly with a longer
timeout. I have attached a revised patch:
- changed units of timeout passed to SyncIOHandler() from us to ms. I
think ms are a more appropriate unit for this kind of timeout. This will
have the effect of increasing the timeout from 2ms to 2sec.
- moved the sync point to be inside "if (error.Success())" to prevent case
of an error causing the main thread to block for 2s.
Shawn.
On Fri, Aug 8, 2014 at 1:35 AM, Matthew Gardiner <mg11 at csr.com> wrote:
> Hi Shawn,
>
> In the patch should not the call to SyncIOHandler in Target.cpp and
> CommandObjectProcess.cpp be inside the
>
> if (error.Success())
> {
>
> ?
>
> My thinking that is, if the resume operation reports a failure then,
> presumably the running event won't be delivered and we won't PushIOHandler,
> and flag the condition to unblock the waiting thread. So in such a
> situation the main thread would be unnecessarily blocked.
>
> What are your thoughts?
>
> Matt
>
>
>
> Shawn Best wrote:
>
>> Thanks for the feedback Greg. I have attached a revised patch based on
>> your suggestions.
>>
>> - renamed m_pushed_IOHandler to m_iohandler_sync
>>
>> - got rid of method ClearPushedIOHandlerSync() and make calls directly to
>> m_iohandler_sync.SetValue(false..)
>>
>> - renamed WaitForPushedIOHandlerSync() to SyncIOHandler()
>>
>> - only wait in case where m_process_input_reader != NULL
>>
>> I put the calls to reset the sync flag in both PrivateEventThread (after
>> it has seen a public stop), and after the call to SyncIOHandler() is
>> completed. As far as I can tell it should work fine, but my preference is
>> normally to reset the flag immediately before using it instead of relying
>> on it set to false. Having it internal does clean up the code though.
>>
>> I think the last suggestion of moving the sync point to
>> Debugger::HandleProcessEvent() would defeat the purpose of the patch since
>> it is called from the EventHandler thread. The race condition is the main
>> thread returning up the call stack to start another round of commandIO
>> handling before PushProcessIOHandler() gets called.
>>
>>
>> On Fri, Aug 1, 2014 at 10:39 AM, Greg Clayton <gclayton at apple.com
>> <mailto:gclayton at apple.com>> wrote:
>>
>> Comments:
>>
>> Why should anyone outside of lldb_private::Process have to call
>> ClearPushedIOHandlerSync() manually? Can we get rid of this
>> function and just have Process.cpp do it at the right times by
>> directly calling m_pushed_IOHandler.SetValue(false,
>> eBroadcastNever);?
>>
>> If so then the following fixes apply:
>> 1 - remove Process::ClearPushedIOHandlerSync() since it will be
>> done internally within process.
>> 2 - rename m_pushed_IOHandler to m_iohandler_sync
>> 3 - rename WaitForPushedIOHandlerSync() to SyncIOHandler
>>
>> Other general fixes:
>> 1 - the WaitForPushedIOHandlerSync should do nothing if there is
>> no process IOHandler (no stdio or we attached to a process. This
>> can be done by testing m_process_input_reader with "if
>> (m_process_input_reader) { ... }"
>>
>> I would also like the fix this sync issue by not having to have
>> every command add a call to
>> process->WaitForPushedIOHandlerSync(...). Can't we sync this in
>> the Debugger::HandleProcessEvent()?
>>
>> > On Jul 31, 2014, at 2:57 PM, Shawn Best <sbest at blueshiftinc.com
>> <mailto:sbest at blueshiftinc.com>> wrote:
>> >
>> > oops, with the attachment this time.
>> >
>> >
>> > On Thu, Jul 31, 2014 at 2:56 PM, Shawn Best
>> <sbest at blueshiftinc.com <mailto:sbest at blueshiftinc.com>> wrote:
>> > Thanks everyone for the feedback. I have reworked the patch to
>> use Predicate <bool>, it reads much cleaner now.
>> >
>> > Shawn.
>> >
>> > On Wed, Jul 30, 2014 at 6:34 PM, Greg Clayton
>> <gclayton at apple.com <mailto:gclayton at apple.com>> wrote:
>> > You will want to use a Predicate<bool> here in stead of what you
>> have since it is exactly what we use a predicate for. The following:
>> >
>> > + bool m_process_running_sync; // used with
>> WaitForProcessRunning() synchronization
>> > + std::condition_variable m_condition_process_running; //
>> used with WaitForProcessRunning() synchronization
>> > + std::mutex m_mutex_process_running; // used with
>> WaitForProcessRunning() synchronization
>> >
>> > Is exactly what the Predicate class does: protect a value with a
>> mutex and condition.
>> >
>> > The above code should be replaced with:
>> >
>> > Predicate<bool> m_process_running_sync;
>> >
>> > The API on Predicate should do what you want. See the header
>> file at "lldb/Host/Predicate.h" and also look for other places
>> that use this class to wait for a value to be equal to another
>> value, or wait for a value to not be equal to something.
>> >
>> > Let me know when you have a patch that uses Predicate and we
>> will look at that.
>> >
>> > Greg
>> >
>> > > On Jul 30, 2014, at 4:03 PM, Shawn Best
>> <sbest at blueshiftinc.com <mailto:sbest at blueshiftinc.com>> wrote:
>> > >
>> > > I have reworked the patch to use std::condition_variable.
>> This particular sync mechanism was new to me, I hope I used it
>> correctly. Is it portable across all target platforms/compilers?
>> I tested on linux and OSX.
>> > >
>> > > The timeout is pretty small (1ms) but seems ample based on the
>> measurements I made.
>> > >
>> > >
>> > > On Tue, Jul 29, 2014 at 9:58 PM, Matthew Gardiner
>> <mg11 at csr.com <mailto:mg11 at csr.com>> wrote:
>> > > Cool, let us know how you get on!
>> > > Matt
>> > >
>> > > Shawn Best wrote:
>> > > Thanks for the feedback guys.
>> > >
>> > > Studying the code, I had figured going with a straight int
>> would in practice be most efficient and not run into
>> multi-threaded problems, even if initially appearing a bit risky.
>> I will rework it to use a std::condition_variable. That will be
>> more robust and readable.
>> > >
>> > > Shawn.
>> > >
>> > > On 7/29/2014 10:53 AM, Zachary Turner wrote:
>> > > Even better would be an std::condition_variable
>> > >
>> > >
>> > > On Mon, Jul 28, 2014 at 10:30 PM, Matthew Gardiner
>> <mg11 at csr.com <mailto:mg11 at csr.com> <mailto:mg11 at csr.com
>>
>> <mailto:mg11 at csr.com>>> wrote:
>> > >
>> > > Hi Shawn,
>> > >
>> > > I use 64-bit linux and I see this issue a lot. It usually
>> > > manifests itself as the prompt just not being printed (or
>> perhaps
>> > > it just gets overwritten) - regardless - I invoke a
>> command, and
>> > > I don't see an (lldb) prompt when I should. So I'm well
>> pleased
>> > > that you are looking at this!
>> > >
>> > > Would it not be more robust to use a semaphore than usleep to
>> > > synchronise the problematic threads?
>> > >
>> > > Although I've not looked too deeply into this particular
>> issue,
>> > > whenever I've seen similar races, I found that it's almost
>> > > impossible to pick the right value when using a sleep
>> command. A
>> > > semaphore, though, should always ensure the waiting thread
>> will
>> > > wake precisely.
>> > >
>> > > I'd be happy to help to test such a fix.
>> > >
>> > > Matt
>> > >
>> > >
>> > > Shawn Best wrote:
>> > >
>> > > Hi,
>> > >
>> > > I have attached a patch which addresses 3 related race
>> > > conditions that cause the command line (lldb) prompt
>> to get
>> > > displayed inappropriately and make it appear it is not
>> > > working correctly. This issue can be seen on linux and
>> > > FreeBSD. I can also artificailly induce the problem
>> on OSX.
>> > >
>> > > The issue happens when the command handler (in the main
>> > > thread) issues a command such as run, step or continue.
>> > > After the command finishes initiating its action, it
>> returns
>> > > up the call stack and goes back into the main command loop
>> > > waiting for user input. Simultaneously, as the inferior
>> > > process starts up, the MonitorChildProcess thread picks up
>> > > the change and posts to the PrivateEvent thread.
>> > > HandePrivateEvent() then calls PushProcessIOHandler()
>> which
>> > > will disable the command IO handler and give the inferior
>> > > control of the TTY. To observe this on OSX, put a
>> > >
>> > > usleep(100);
>> > >
>> > > immediately prior the PushProcessIOHandler() in
>> > > HandlePrivateEvent.
>> > >
>> > >
>> > > My proposed solution is that after a 'run', 'step', or
>> > > 'continue' command, insert a synchronization point and
>> wait
>> > > until HandlePrivateEvent knows the inferior process is
>> > > running and has pushed the IO handler. One context switch
>> > > (<100us) is usually all the time it takes on my
>> machine. As
>> > > an additional safety, I have a timeout (currently 1ms)
>> so it
>> > > will never hang the main thread.
>> > >
>> > > Any thoughts, or suggestions would be appreciated.
>> > >
>> > > Regards,
>> > > Shawn.
>> > >
>> > >
>> > > To report this email as spam click here
>> > > <https://www.mailcontrol.com/sr/MZbqvYs5QwJvpeaetUwhCQ==
>> >.
>> > >
>> > >
>> > >
>> > > _______________________________________________
>> > > lldb-commits mailing list
>> > > lldb-commits at cs.uiuc.edu <mailto:lldb-commits at cs.uiuc.edu>
>> <mailto:lldb-commits at cs.uiuc.edu <mailto:lldb-commits at cs.uiuc.edu>>
>>
>> > >
>> > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>> > >
>> > >
>> > >
>> > >
>> > > Member of the CSR plc group of companies. CSR plc
>> registered in
>> > > England and Wales, registered number 4187346, registered
>> office
>> > > Churchill House, Cambridge Business Park, Cowley Road,
>> Cambridge,
>> > > CB4 0WZ, United Kingdom
>> > > More information can be found at www.csr.com
>> <http://www.csr.com>
>> > > <http://www.csr.com>. Keep up to date with CSR on our
>> technical
>> > > blog, www.csr.com/blog <http://www.csr.com/blog>
>> <http://www.csr.com/blog>, CSR people
>> > > blog, www.csr.com/people <http://www.csr.com/people>
>> <http://www.csr.com/people>, YouTube,
>> > > www.youtube.com/user/CSRplc
>> <http://www.youtube.com/user/CSRplc>
>> <http://www.youtube.com/user/CSRplc>,
>> > > Facebook, www.facebook.com/pages/CSR/191038434253534
>> <http://www.facebook.com/pages/CSR/191038434253534>
>> > > <http://www.facebook.com/pages/CSR/191038434253534>, or
>> follow us
>> > > on Twitter at www.twitter.com/CSR_plc
>> <http://www.twitter.com/CSR_plc>
>> > > <http://www.twitter.com/CSR_plc>.
>> > >
>> > > New for 2014, you can now access the wide range of products
>> > > powered by aptX at www.aptx.com <http://www.aptx.com>
>> <http://www.aptx.com>.
>> > > _______________________________________________
>> > > lldb-commits mailing list
>> > > lldb-commits at cs.uiuc.edu <mailto:lldb-commits at cs.uiuc.edu>
>> <mailto:lldb-commits at cs.uiuc.edu <mailto:lldb-commits at cs.uiuc.edu>>
>>
>> > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>> > >
>> > >
>> > >
>> > >
>> > >
>> > >
>> <sbest_iohandler_race_rev_04.diff>_________________________
>> ______________________
>> > > lldb-commits mailing list
>> > > lldb-commits at cs.uiuc.edu <mailto:lldb-commits at cs.uiuc.edu>
>> > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>> >
>> >
>> >
>> > <sbest_iohandler_race_rev_05.diff>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140808/e2f0a4a1/attachment.html>
-------------- next part --------------
diff --git a/include/lldb/Target/Process.h b/include/lldb/Target/Process.h
index 11666f9..95ebf26 100644
--- a/include/lldb/Target/Process.h
+++ b/include/lldb/Target/Process.h
@@ -2660,6 +2660,25 @@ public:
bool wait_always = true,
Listener *hijack_listener = NULL);
+
+ //--------------------------------------------------------------------------------------
+ /// Waits for the process state to be running within a given msec timeout.
+ ///
+ /// The main purpose of this is to implement an interlock waiting for HandlePrivateEvent
+ /// to push an IOHandler.
+ ///
+ /// @param[in] timeout_msec
+ /// The maximum time length to wait for the process to transition to the
+ /// eStateRunning state, specified in milliseconds.
+ ///
+ /// @return
+ /// true if successfully signalled that process started and IOHandler pushes, false
+ /// if it timed out.
+ //--------------------------------------------------------------------------------------
+ bool
+ SyncIOHandler (uint64_t timeout_msec);
+
+
lldb::StateType
WaitForStateChangedEvents (const TimeValue *timeout,
lldb::EventSP &event_sp,
@@ -3037,6 +3056,7 @@ protected:
std::string m_stderr_data;
Mutex m_profile_data_comm_mutex;
std::vector<std::string> m_profile_data;
+ Predicate<bool> m_iohandler_sync;
MemoryCache m_memory_cache;
AllocatedMemoryCache m_allocated_memory_cache;
bool m_should_detach; /// Should we detach if the process object goes away with an explicit call to Kill or Detach?
diff --git a/source/Commands/CommandObjectProcess.cpp b/source/Commands/CommandObjectProcess.cpp
index 5dde4dd..5678ae4 100644
--- a/source/Commands/CommandObjectProcess.cpp
+++ b/source/Commands/CommandObjectProcess.cpp
@@ -773,10 +773,16 @@ protected:
process->GetThreadList().GetThreadAtIndex(idx)->SetResumeState (eStateRunning, override_suspend);
}
}
-
+
Error error(process->Resume());
+
if (error.Success())
{
+ // There is a race condition where this thread will return up the call stack to the main command
+ // handler and show an (lldb) prompt before HandlePrivateEvent (from PrivateStateThread) has
+ // a chance to call PushProcessIOHandler().
+ process->SyncIOHandler(2000);
+
result.AppendMessageWithFormat ("Process %" PRIu64 " resuming\n", process->GetID());
if (synchronous_execution)
{
diff --git a/source/Commands/CommandObjectThread.cpp b/source/Commands/CommandObjectThread.cpp
index c58e9e8..e7a8652 100644
--- a/source/Commands/CommandObjectThread.cpp
+++ b/source/Commands/CommandObjectThread.cpp
@@ -624,7 +624,11 @@ protected:
process->GetThreadList().SetSelectedThreadByID (thread->GetID());
process->Resume ();
-
+
+ // There is a race condition where this thread will return up the call stack to the main command handler
+ // and show an (lldb) prompt before HandlePrivateEvent (from PrivateStateThread) has
+ // a chance to call PushProcessIOHandler().
+ process->SyncIOHandler(2000);
if (synchronous_execution)
{
diff --git a/source/Target/Process.cpp b/source/Target/Process.cpp
index b4e88bd..147fe00 100644
--- a/source/Target/Process.cpp
+++ b/source/Target/Process.cpp
@@ -691,6 +691,7 @@ Process::Process(Target &target, Listener &listener) :
m_stderr_data (),
m_profile_data_comm_mutex (Mutex::eMutexTypeRecursive),
m_profile_data (),
+ m_iohandler_sync (false),
m_memory_cache (*this),
m_allocated_memory_cache (*this),
m_should_detach (false),
@@ -889,6 +890,34 @@ Process::GetNextEvent (EventSP &event_sp)
return state;
}
+bool
+Process::SyncIOHandler (uint64_t timeout_msec)
+{
+ bool timed_out = false;
+
+ // don't sync (potentially context switch) in case where there is no process IO
+ if (m_process_input_reader)
+ {
+ TimeValue timeout = TimeValue::Now();
+ timeout.OffsetWithMicroSeconds(timeout_msec*1000);
+
+ m_iohandler_sync.WaitForValueEqualTo(true, &timeout, &timed_out);
+
+ Log *log(lldb_private::GetLogIfAllCategoriesSet (LIBLLDB_LOG_PROCESS));
+ if(log)
+ {
+ if(timed_out)
+ log->Printf ("Process::%s pid %" PRIu64 " (timeout=%" PRIu64 "ms): FAIL", __FUNCTION__, GetID (), timeout_msec);
+ else
+ log->Printf ("Process::%s pid %" PRIu64 ": SUCCESS", __FUNCTION__, GetID ());
+ }
+
+ // reset sync one-shot so it will be ready for next time
+ m_iohandler_sync.SetValue(false, eBroadcastNever);
+ }
+
+ return !timed_out;
+}
StateType
Process::WaitForProcessToStop (const TimeValue *timeout, lldb::EventSP *event_sp_ptr, bool wait_always, Listener *hijack_listener)
@@ -3888,9 +3917,11 @@ Process::HandlePrivateEvent (EventSP &event_sp)
// as this means the curses GUI is in use...
if (!GetTarget().GetDebugger().IsForwardingEvents())
PushProcessIOHandler ();
+ m_iohandler_sync.SetValue(true, eBroadcastAlways);
}
else if (StateIsStoppedState(new_state, false))
{
+ m_iohandler_sync.SetValue(false, eBroadcastNever);
if (!Process::ProcessEventData::GetRestartedFromEvent(event_sp.get()))
{
// If the lldb_private::Debugger is handling the events, we don't
diff --git a/source/Target/Target.cpp b/source/Target/Target.cpp
index f959010..e3fd790 100644
--- a/source/Target/Target.cpp
+++ b/source/Target/Target.cpp
@@ -2420,9 +2420,14 @@ Target::Launch (Listener &listener, ProcessLaunchInfo &launch_info)
m_process_sp->RestoreProcessEvents ();
error = m_process_sp->PrivateResume();
-
+
if (error.Success())
{
+ // there is a race condition where this thread will return up the call stack to the main command
+ // handler and show an (lldb) prompt before HandlePrivateEvent (from PrivateStateThread) has
+ // a chance to call PushProcessIOHandler()
+ m_process_sp->SyncIOHandler(2000);
+
if (synchronous_execution)
{
state = m_process_sp->WaitForProcessToStop (NULL, NULL, true, hijack_listener_sp.get());
More information about the lldb-commits
mailing list