[Lldb-commits] [PATCH]race condition calling PushProcessIOHandler
Shawn Best
sbest at blueshiftinc.com
Sat Aug 2 11:45:31 PDT 2014
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> 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> wrote:
> >
> > oops, with the attachment this time.
> >
> >
> > On Thu, Jul 31, 2014 at 2:56 PM, Shawn Best <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>
> 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>
> 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>
> 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>> 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>
> > >
> > > 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>. Keep up to date with CSR on our technical
> > > blog, www.csr.com/blog <http://www.csr.com/blog>, CSR people
> > > blog, www.csr.com/people <http://www.csr.com/people>, YouTube,
> > > 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>, or follow us
> > > on Twitter at 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>.
> > > _______________________________________________
> > > 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_04.diff>_______________________________________________
> > > lldb-commits mailing list
> > > 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/20140802/848922bc/attachment.html>
-------------- next part --------------
diff --git a/include/lldb/Target/Process.h b/include/lldb/Target/Process.h
index 658bc75..d4a3736 100644
--- a/include/lldb/Target/Process.h
+++ b/include/lldb/Target/Process.h
@@ -2654,6 +2654,25 @@ public:
bool wait_always = true,
Listener *hijack_listener = NULL);
+
+ //--------------------------------------------------------------------------------------
+ /// Waits for the process state to be running within a given usec timeout.
+ ///
+ /// The main purpose of this is to implement an interlock waiting for HandlePrivateEvent
+ /// to push an IOHandler.
+ ///
+ /// @param[in] timeout_usec
+ /// The maximum time length to wait for the process to transition to the
+ /// eStateRunning state, specified in microseconds.
+ ///
+ /// @return
+ /// true if successfully signalled that process started and IOHandler pushes, false
+ /// if it timed out.
+ //--------------------------------------------------------------------------------------
+ bool
+ SyncIOHandler (uint64_t timeout_usec);
+
+
lldb::StateType
WaitForStateChangedEvents (const TimeValue *timeout,
lldb::EventSP &event_sp,
@@ -3031,6 +3050,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 3534df3..59c25f2 100644
--- a/source/Commands/CommandObjectProcess.cpp
+++ b/source/Commands/CommandObjectProcess.cpp
@@ -766,8 +766,14 @@ protected:
process->GetThreadList().GetThreadAtIndex(idx)->SetResumeState (eStateRunning, override_suspend);
}
}
-
+
Error error(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 (error.Success())
{
result.AppendMessageWithFormat ("Process %" PRIu64 " resuming\n", process->GetID());
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 7e09e7e..570db1b 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_usec)
+{
+ 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_usec);
+
+ 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 "): FAIL", __FUNCTION__, GetID (), timeout_usec);
+ 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)
@@ -3878,9 +3907,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..d24e5e9 100644
--- a/source/Target/Target.cpp
+++ b/source/Target/Target.cpp
@@ -2420,7 +2420,12 @@ Target::Launch (Listener &listener, ProcessLaunchInfo &launch_info)
m_process_sp->RestoreProcessEvents ();
error = m_process_sp->PrivateResume();
-
+
+ // 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 (error.Success())
{
if (synchronous_execution)
More information about the lldb-commits
mailing list