[Lldb-commits] [PATCH]race condition calling PushProcessIOHandler
Greg Clayton
gclayton at apple.com
Wed Jul 30 18:34:22 PDT 2014
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
More information about the lldb-commits
mailing list