[Lldb-commits] [PATCH]race condition calling PushProcessIOHandler
Todd Fiala
tfiala at google.com
Mon Aug 11 08:32:11 PDT 2014
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> 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>> 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>
>>
>>
>>
>>
>>
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140811/9a50afee/attachment.html>
More information about the lldb-commits
mailing list