[Lldb-commits] [PATCH]race condition calling PushProcessIOHandler
Matthew Gardiner
mg11 at csr.com
Mon Aug 11 23:19:12 PDT 2014
Hey Todd,
Welcome back! So how much do you see these (lldb) prompt issues? Myself
and Shawn witness the prompt disappearing frequently when doing
continue, interrupt, step and such.
I also see issues where upon the prompt appears when it should not (e.g.
"process launch"), see:
http://lists.cs.uiuc.edu/pipermail/lldb-dev/2014-August/004733.html
I was wondering if you could run Shawn's patch over on OSX, then I think
can submit it, if all is well. I set you as reviewer of the patch:
http://reviews.llvm.org/D4863
Later
Matt
Todd Fiala wrote:
>
> Just coming back to the world of LLDB. Looks like lots of healthy
> discussion on this one!
>
> On Aug 10, 2014 10:30 PM, "Matthew Gardiner" <mg11 at csr.com
> <mailto:mg11 at csr.com>> wrote:
>
> 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> <mailto: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>>
> <mailto: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>>
> <mailto: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>>
> <mailto: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>>
> <mailto: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>>
> <mailto: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>>
> <mailto: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>>>
> <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 <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>>>
> <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
> <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>
> > > <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>
> <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>
> <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>
> <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>
> > >
> <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>
> > > <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>
> <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>>>
> <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
> <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>>
> <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_05.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
>
More information about the lldb-commits
mailing list