[Lldb-commits] [PATCH]race condition calling PushProcessIOHandler
Matthew Gardiner
mg11 at csr.com
Sun Aug 10 22:29:34 PDT 2014
Thanks Shawn,
If Greg agrees that we need the SyncIOHandler() invocation in the
Command object(s) (not the HandlePrivateEvent code) then I'll submit the
patch.
Greg?
Shawn Best wrote:
> 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
> <mailto: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>
> <mailto: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>
> <mailto: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>
> <mailto: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>
> <mailto: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>
> <mailto: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> <mailto: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>> <mailto: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>>
> <mailto: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>
> > > <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>
> <http://www.csr.com/blog>, CSR people
> > > blog, www.csr.com/people
> <http://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>
> <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>
> > >
> <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>
> > > <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>
> <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>>
> <mailto: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>
> <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_05.diff>
>
>
>
>
More information about the lldb-commits
mailing list