<p dir="ltr">Just coming back to the world of LLDB. Looks like lots of healthy discussion on this one!</p>
<div class="gmail_quote">On Aug 10, 2014 10:30 PM, "Matthew Gardiner" <<a href="mailto:mg11@csr.com">mg11@csr.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks Shawn,<br>
<br>
If Greg agrees that we need the SyncIOHandler() invocation in the Command object(s) (not the HandlePrivateEvent code) then I'll submit the patch.<br>
<br>
Greg?<br>
<br>
<br>
Shawn Best wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Matt,<br>
<br>
Yeah that would probably be a good idea, particularly with a longer timeout. I have attached a revised patch:<br>
<br>
- 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.<br>
<br>
- moved the sync point to be inside "if (error.Success())" to prevent case of an error causing the main thread to block for 2s.<br>
<br>
Shawn.<br>
<br>
<br>
On Fri, Aug 8, 2014 at 1:35 AM, Matthew Gardiner <<a href="mailto:mg11@csr.com" target="_blank">mg11@csr.com</a> <mailto:<a href="mailto:mg11@csr.com" target="_blank">mg11@csr.com</a>>> wrote:<br>
<br>
Hi Shawn,<br>
<br>
In the patch should not the call to SyncIOHandler in Target.cpp<br>
and CommandObjectProcess.cpp be inside the<br>
<br>
if (error.Success())<br>
{<br>
<br>
?<br>
<br>
My thinking that is, if the resume operation reports a failure<br>
then, presumably the running event won't be delivered and we won't<br>
PushIOHandler, and flag the condition to unblock the waiting<br>
thread. So in such a situation the main thread would be<br>
unnecessarily blocked.<br>
<br>
What are your thoughts?<br>
<br>
Matt<br>
<br>
<br>
<br>
Shawn Best wrote:<br>
<br>
Thanks for the feedback Greg. I have attached a revised patch<br>
based on your suggestions.<br>
<br>
- renamed m_pushed_IOHandler to m_iohandler_sync<br>
<br>
- got rid of method ClearPushedIOHandlerSync() and make calls<br>
directly to m_iohandler_sync.SetValue(<u></u>false..)<br>
<br>
- renamed WaitForPushedIOHandlerSync() to SyncIOHandler()<br>
<br>
- only wait in case where m_process_input_reader != NULL<br>
<br>
I put the calls to reset the sync flag in both<br>
PrivateEventThread (after it has seen a public stop), and<br>
after the call to SyncIOHandler() is completed. As far as I<br>
can tell it should work fine, but my preference is normally to<br>
reset the flag immediately before using it instead of relying<br>
on it set to false. Having it internal does clean up the code<br>
though.<br>
<br>
I think the last suggestion of moving the sync point to<br>
Debugger::HandleProcessEvent() would defeat the purpose of the<br>
patch since it is called from the EventHandler thread. The<br>
race condition is the main thread returning up the call stack<br>
to start another round of commandIO handling before<br>
PushProcessIOHandler() gets called.<br>
<br>
<br>
On Fri, Aug 1, 2014 at 10:39 AM, Greg Clayton<br>
<<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a> <mailto:<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a>><br>
<mailto:<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a> <mailto:<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a>>>> wrote:<br>
<br>
Comments:<br>
<br>
Why should anyone outside of lldb_private::Process have to<br>
call<br>
ClearPushedIOHandlerSync() manually? Can we get rid of this<br>
function and just have Process.cpp do it at the right times by<br>
directly calling m_pushed_IOHandler.SetValue(<u></u>false,<br>
eBroadcastNever);?<br>
<br>
If so then the following fixes apply:<br>
1 - remove Process::<u></u>ClearPushedIOHandlerSync() since it<br>
will be<br>
done internally within process.<br>
2 - rename m_pushed_IOHandler to m_iohandler_sync<br>
3 - rename WaitForPushedIOHandlerSync() to SyncIOHandler<br>
<br>
Other general fixes:<br>
1 - the WaitForPushedIOHandlerSync should do nothing if<br>
there is<br>
no process IOHandler (no stdio or we attached to a<br>
process. This<br>
can be done by testing m_process_input_reader with "if<br>
(m_process_input_reader) { ... }"<br>
<br>
I would also like the fix this sync issue by not having to<br>
have<br>
every command add a call to<br>
process-><u></u>WaitForPushedIOHandlerSync(...<u></u>). Can't we sync<br>
this in<br>
the Debugger::HandleProcessEvent()<u></u>?<br>
<br>
> On Jul 31, 2014, at 2:57 PM, Shawn Best<br>
<<a href="mailto:sbest@blueshiftinc.com" target="_blank">sbest@blueshiftinc.com</a> <mailto:<a href="mailto:sbest@blueshiftinc.com" target="_blank">sbest@blueshiftinc.com</a><u></u>><br>
<mailto:<a href="mailto:sbest@blueshiftinc.com" target="_blank">sbest@blueshiftinc.com</a><br>
<mailto:<a href="mailto:sbest@blueshiftinc.com" target="_blank">sbest@blueshiftinc.com</a><u></u>>>> wrote:<br>
><br>
> oops, with the attachment this time.<br>
><br>
><br>
> On Thu, Jul 31, 2014 at 2:56 PM, Shawn Best<br>
<<a href="mailto:sbest@blueshiftinc.com" target="_blank">sbest@blueshiftinc.com</a> <mailto:<a href="mailto:sbest@blueshiftinc.com" target="_blank">sbest@blueshiftinc.com</a><u></u>><br>
<mailto:<a href="mailto:sbest@blueshiftinc.com" target="_blank">sbest@blueshiftinc.com</a><br>
<mailto:<a href="mailto:sbest@blueshiftinc.com" target="_blank">sbest@blueshiftinc.com</a><u></u>>>> wrote:<br>
> Thanks everyone for the feedback. I have reworked the<br>
patch to<br>
use Predicate <bool>, it reads much cleaner now.<br>
><br>
> Shawn.<br>
><br>
> On Wed, Jul 30, 2014 at 6:34 PM, Greg Clayton<br>
<<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a> <mailto:<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a>><br>
<mailto:<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a> <mailto:<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a>>>> wrote:<br>
> You will want to use a Predicate<bool> here in stead of<br>
what you<br>
have since it is exactly what we use a predicate for. The<br>
following:<br>
><br>
> + bool m_process_running_sync; // used with<br>
WaitForProcessRunning() synchronization<br>
> + std::condition_variable<br>
m_condition_process_running; //<br>
used with WaitForProcessRunning() synchronization<br>
> + std::mutex m_mutex_process_running; // used with<br>
WaitForProcessRunning() synchronization<br>
><br>
> Is exactly what the Predicate class does: protect a<br>
value with a<br>
mutex and condition.<br>
><br>
> The above code should be replaced with:<br>
><br>
> Predicate<bool> m_process_running_sync;<br>
><br>
> The API on Predicate should do what you want. See the header<br>
file at "lldb/Host/Predicate.h" and also look for other places<br>
that use this class to wait for a value to be equal to another<br>
value, or wait for a value to not be equal to something.<br>
><br>
> Let me know when you have a patch that uses Predicate and we<br>
will look at that.<br>
><br>
> Greg<br>
><br>
> > On Jul 30, 2014, at 4:03 PM, Shawn Best<br>
<<a href="mailto:sbest@blueshiftinc.com" target="_blank">sbest@blueshiftinc.com</a> <mailto:<a href="mailto:sbest@blueshiftinc.com" target="_blank">sbest@blueshiftinc.com</a><u></u>><br>
<mailto:<a href="mailto:sbest@blueshiftinc.com" target="_blank">sbest@blueshiftinc.com</a><br>
<mailto:<a href="mailto:sbest@blueshiftinc.com" target="_blank">sbest@blueshiftinc.com</a><u></u>>>> wrote:<br>
> ><br>
> > I have reworked the patch to use std::condition_variable.<br>
This particular sync mechanism was new to me, I hope I<br>
used it<br>
correctly. Is it portable across all target<br>
platforms/compilers?<br>
I tested on linux and OSX.<br>
> ><br>
> > The timeout is pretty small (1ms) but seems ample<br>
based on the<br>
measurements I made.<br>
> ><br>
> ><br>
> > On Tue, Jul 29, 2014 at 9:58 PM, Matthew Gardiner<br>
<<a href="mailto:mg11@csr.com" target="_blank">mg11@csr.com</a> <mailto:<a href="mailto:mg11@csr.com" target="_blank">mg11@csr.com</a>> <mailto:<a href="mailto:mg11@csr.com" target="_blank">mg11@csr.com</a><br>
<mailto:<a href="mailto:mg11@csr.com" target="_blank">mg11@csr.com</a>>>> wrote:<br>
> > Cool, let us know how you get on!<br>
> > Matt<br>
> ><br>
> > Shawn Best wrote:<br>
> > Thanks for the feedback guys.<br>
> ><br>
> > Studying the code, I had figured going with a straight int<br>
would in practice be most efficient and not run into<br>
multi-threaded problems, even if initially appearing a bit<br>
risky.<br>
I will rework it to use a std::condition_variable. That<br>
will be<br>
more robust and readable.<br>
> ><br>
> > Shawn.<br>
> ><br>
> > On 7/29/2014 10:53 AM, Zachary Turner wrote:<br>
> > Even better would be an std::condition_variable<br>
> ><br>
> ><br>
> > On Mon, Jul 28, 2014 at 10:30 PM, Matthew Gardiner<br>
<<a href="mailto:mg11@csr.com" target="_blank">mg11@csr.com</a> <mailto:<a href="mailto:mg11@csr.com" target="_blank">mg11@csr.com</a>> <mailto:<a href="mailto:mg11@csr.com" target="_blank">mg11@csr.com</a><br>
<mailto:<a href="mailto:mg11@csr.com" target="_blank">mg11@csr.com</a>>> <mailto:<a href="mailto:mg11@csr.com" target="_blank">mg11@csr.com</a> <mailto:<a href="mailto:mg11@csr.com" target="_blank">mg11@csr.com</a>><br>
<br>
<mailto:<a href="mailto:mg11@csr.com" target="_blank">mg11@csr.com</a> <mailto:<a href="mailto:mg11@csr.com" target="_blank">mg11@csr.com</a>>>>> wrote:<br>
> ><br>
> > Hi Shawn,<br>
> ><br>
> > I use 64-bit linux and I see this issue a lot. It<br>
usually<br>
> > manifests itself as the prompt just not being<br>
printed (or<br>
perhaps<br>
> > it just gets overwritten) - regardless - I invoke a<br>
command, and<br>
> > I don't see an (lldb) prompt when I should. So I'm<br>
well<br>
pleased<br>
> > that you are looking at this!<br>
> ><br>
> > Would it not be more robust to use a semaphore<br>
than usleep to<br>
> > synchronise the problematic threads?<br>
> ><br>
> > Although I've not looked too deeply into this<br>
particular<br>
issue,<br>
> > whenever I've seen similar races, I found that<br>
it's almost<br>
> > impossible to pick the right value when using a sleep<br>
command. A<br>
> > semaphore, though, should always ensure the<br>
waiting thread<br>
will<br>
> > wake precisely.<br>
> ><br>
> > I'd be happy to help to test such a fix.<br>
> ><br>
> > Matt<br>
> ><br>
> ><br>
> > Shawn Best wrote:<br>
> ><br>
> > Hi,<br>
> ><br>
> > I have attached a patch which addresses 3<br>
related race<br>
> > conditions that cause the command line (lldb)<br>
prompt<br>
to get<br>
> > displayed inappropriately and make it appear<br>
it is not<br>
> > working correctly. This issue can be seen on<br>
linux and<br>
> > FreeBSD. I can also artificailly induce the<br>
problem<br>
on OSX.<br>
> ><br>
> > The issue happens when the command handler (in<br>
the main<br>
> > thread) issues a command such as run, step or<br>
continue.<br>
> > After the command finishes initiating its<br>
action, it<br>
returns<br>
> > up the call stack and goes back into the main<br>
command loop<br>
> > waiting for user input. Simultaneously, as<br>
the inferior<br>
> > process starts up, the MonitorChildProcess<br>
thread picks up<br>
> > the change and posts to the PrivateEvent thread.<br>
> > HandePrivateEvent() then calls<br>
PushProcessIOHandler()<br>
which<br>
> > will disable the command IO handler and give<br>
the inferior<br>
> > control of the TTY. To observe this on OSX, put a<br>
> ><br>
> > usleep(100);<br>
> ><br>
> > immediately prior the PushProcessIOHandler() in<br>
> > HandlePrivateEvent.<br>
> ><br>
> ><br>
> > My proposed solution is that after a 'run',<br>
'step', or<br>
> > 'continue' command, insert a synchronization<br>
point and<br>
wait<br>
> > until HandlePrivateEvent knows the inferior<br>
process is<br>
> > running and has pushed the IO handler. One<br>
context switch<br>
> > (<100us) is usually all the time it takes on my<br>
machine. As<br>
> > an additional safety, I have a timeout<br>
(currently 1ms)<br>
so it<br>
> > will never hang the main thread.<br>
> ><br>
> > Any thoughts, or suggestions would be appreciated.<br>
> ><br>
> > Regards,<br>
> > Shawn.<br>
> ><br>
> ><br>
> > To report this email as spam click here<br>
> > <<a href="https://www.mailcontrol.com/sr/MZbqvYs5QwJvpeaetUwhCQ==" target="_blank">https://www.mailcontrol.com/<u></u>sr/MZbqvYs5QwJvpeaetUwhCQ==</a>>.<br>
> ><br>
> ><br>
> ><br>
> > ______________________________<u></u>_________________<br>
> > lldb-commits mailing list<br>
> > <a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.edu</a><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a>><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a>>><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a>><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a>>>><br>
<br>
> ><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/lldb-commits</a><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > Member of the CSR plc group of companies. CSR plc<br>
registered in<br>
> > England and Wales, registered number 4187346,<br>
registered<br>
office<br>
> > Churchill House, Cambridge Business Park, Cowley Road,<br>
Cambridge,<br>
> > CB4 0WZ, United Kingdom<br>
> > More information can be found at <a href="http://www.csr.com" target="_blank">www.csr.com</a><br>
<<a href="http://www.csr.com" target="_blank">http://www.csr.com</a>><br>
<<a href="http://www.csr.com" target="_blank">http://www.csr.com</a>><br>
> > <<a href="http://www.csr.com" target="_blank">http://www.csr.com</a>>. Keep up to date with CSR on our<br>
technical<br>
> > blog, <a href="http://www.csr.com/blog" target="_blank">www.csr.com/blog</a> <<a href="http://www.csr.com/blog" target="_blank">http://www.csr.com/blog</a>><br>
<<a href="http://www.csr.com/blog" target="_blank">http://www.csr.com/blog</a>><br>
<<a href="http://www.csr.com/blog" target="_blank">http://www.csr.com/blog</a>>, CSR people<br>
> > blog, <a href="http://www.csr.com/people" target="_blank">www.csr.com/people</a><br>
<<a href="http://www.csr.com/people" target="_blank">http://www.csr.com/people</a>> <<a href="http://www.csr.com/people" target="_blank">http://www.csr.com/people</a>><br>
<<a href="http://www.csr.com/people" target="_blank">http://www.csr.com/people</a>>, YouTube,<br>
> > <a href="http://www.youtube.com/user/CSRplc" target="_blank">www.youtube.com/user/CSRplc</a><br>
<<a href="http://www.youtube.com/user/CSRplc" target="_blank">http://www.youtube.com/user/<u></u>CSRplc</a>><br>
<<a href="http://www.youtube.com/user/CSRplc" target="_blank">http://www.youtube.com/user/<u></u>CSRplc</a>><br>
<<a href="http://www.youtube.com/user/CSRplc" target="_blank">http://www.youtube.com/user/<u></u>CSRplc</a>>,<br>
> > Facebook,<br>
<a href="http://www.facebook.com/pages/CSR/191038434253534" target="_blank">www.facebook.com/pages/CSR/<u></u>191038434253534</a><br>
<<a href="http://www.facebook.com/pages/CSR/191038434253534" target="_blank">http://www.facebook.com/<u></u>pages/CSR/191038434253534</a>><br>
<<a href="http://www.facebook.com/pages/CSR/191038434253534" target="_blank">http://www.facebook.com/<u></u>pages/CSR/191038434253534</a>><br>
> > <<a href="http://www.facebook.com/pages/CSR/191038434253534" target="_blank">http://www.facebook.com/<u></u>pages/CSR/191038434253534</a>>, or<br>
follow us<br>
> > on Twitter at <a href="http://www.twitter.com/CSR_plc" target="_blank">www.twitter.com/CSR_plc</a><br>
<<a href="http://www.twitter.com/CSR_plc" target="_blank">http://www.twitter.com/CSR_<u></u>plc</a>><br>
<<a href="http://www.twitter.com/CSR_plc" target="_blank">http://www.twitter.com/CSR_<u></u>plc</a>><br>
> > <<a href="http://www.twitter.com/CSR_plc" target="_blank">http://www.twitter.com/CSR_<u></u>plc</a>>.<br>
> ><br>
> > New for 2014, you can now access the wide range of<br>
products<br>
> > powered by aptX at <a href="http://www.aptx.com" target="_blank">www.aptx.com</a><br>
<<a href="http://www.aptx.com" target="_blank">http://www.aptx.com</a>> <<a href="http://www.aptx.com" target="_blank">http://www.aptx.com</a>><br>
<<a href="http://www.aptx.com" target="_blank">http://www.aptx.com</a>>.<br>
> > ______________________________<u></u>_________________<br>
> > lldb-commits mailing list<br>
> > <a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.edu</a><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a>><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a>>><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a>><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a>>>><br>
<br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/lldb-commits</a><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
<sbest_iohandler_race_rev_04.<u></u>diff>_________________________<u></u>______________________<br>
> > lldb-commits mailing list<br>
> > <a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.edu</a><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a>><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a><br>
<mailto:<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.<u></u>edu</a>>><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/lldb-commits</a><br>
><br>
><br>
><br>
> <sbest_iohandler_race_rev_05.<u></u>diff><br>
<br>
<br>
<br>
<br>
</blockquote>
<br>
______________________________<u></u>_________________<br>
lldb-commits mailing list<br>
<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/lldb-commits</a><br>
</blockquote></div>