[Lldb-commits] [PATCH]race condition calling PushProcessIOHandler
Matthew Gardiner
mg11 at csr.com
Fri Aug 1 06:41:47 PDT 2014
Hi Shawn,
I've *just* started looking at this. A few initial observations:
1. When I had a paused native target, when I repeatedly stepped the
prompt always appeared as expected. Good!
2. However, when I attached to a stopped target over gdb-remote, the
prompt was not there. Did you expect that? Or is that out of the scope
of your investigation. I wondered whether such a fix should work for
that too. See below:
(lldb) target create ~/my-dir/i64-hello.elf
Current executable set to '~/my-dir/i64-hello.elf' (x86_64).
(lldb) gdb-remote 7779
Process 18048 stopped
* thread #1: tid = 18048, 0x0000003675a011f0, name = 'i64-hello.elf',
stop reason = signal SIGSTOP
frame #0: 0x0000003675a011f0
-> 0x3675a011f0: movq %rsp, %rdi
0x3675a011f3: callq 0x3675a046e0
0x3675a011f8: movq %rax, %r12
0x3675a011fb: movl 0x21eb97(%rip), %eax
(I expected a prompt here - since the target is stopped)
3. Also what's the significance of the 2000 literal? Is it a timeout?
Since we expect to wake on a signal, we could make this bigger.
I'll have a better look on monday.
thanks
Matt
Shawn Best 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>> 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>> 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>> 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>> 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>>> 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>>
> >
> > 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>. Keep up to date with CSR on our
> technical
> > blog, 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>, YouTube,
> > 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>, or
> follow us
> > on Twitter at 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>.
> > _______________________________________________
> > 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_04.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