[lldb-dev] gdb-remote lldb prompt race condition

Todd Fiala tfiala at google.com
Fri Aug 22 09:05:04 PDT 2014

FWIW I am planning on having Shawn Best look at doing this when he gets


On Thu, Aug 21, 2014 at 10:59 PM, Matthew Gardiner <mg11 at csr.com> wrote:

> Ok,
> Thanks for this, Greg. I'm not in a position to look at this right now,
> but wanted to at least point out some of my observations. Hopefully someone
> can pick this up sometime soon.
> thanks
> Matt
> Greg Clayton wrote:
>> On Aug 21, 2014, at 12:27 AM, Matthew Gardiner <mg11 at csr.com> wrote:
>>> Hi Greg,
>>> If that is the case, then the main thread (i.e. the one running
>>> Debugger::ExecuteIOHandler) needs to be blocked until the event arrives.
>> The command line command "process connect" needs to synchronize better.
>> It currently isn't and this is causing the problem. All we know is that we
>> are executing a "gdb-remote 1234" command which turns into "process connect
>> connect://localhost:1234" and then the command execution returns
>> immediately and then the io handler gets the next command as it should. We
>> should add code to commands that we know might needs some syncing to make
>> sure they don't violate things.
>>  Why?
>>> Because with the existing design once the user has entered their
>>> "gdb-remote" command, and completed the connect call,  the main thread goes
>>> back to the IOHandlerEditline::Run() loop, sees that IsActive() is true,
>>> prints the next prompt, etc.. When I debugged this I didn't see any call to
>>> Deactivate or SetIsDone, which would have made IsActive return false. (And
>>> then the async DefaultEventHandler awakes and it's output "Process 1
>>> stopped" splats over the prompt).
>> No other IOHandler gets pushed, so the command interpreter is active and
>> it will just get the next command as soon as it can. Most commands do
>> something, then complete and then we are ready for more. There are a few
>> that needs some extra syncing because they cause a launch/attach/connect
>> and we usually get a response right away. There are others that might be
>> immediate and might be async "thread step-XXX" for example. The step might
>> complete and it might not. There are others that are purely async "process
>> continue".
>> To complicate this, if we attach to a process, then there is no process
>> IO handler to push and the command line will always be active.
>>  If the code is changed so that the edit line IOHandler's IsActive
>>> returns false, while an asynchronous event is happening, then I think that
>>> the main thread would spin, since the reader_sp->Run() function below:
>>> void
>>> Debugger::ExecuteIOHanders()
>>> {
>>>     while (1)
>>>     {
>>>         IOHandlerSP reader_sp(m_input_reader_stack.Top());
>>>         if (!reader_sp)
>>>             break;
>>>         reader_sp->Activate();
>>>         reader_sp->Run();
>>>         reader_sp->Deactivate();
>>> would immediately return. That's why I'm thinking the main thread
>>> probably should block until the last issued command has completed.
>> So this is the job of each command. Most commands complete right way.
>> It sounds like we might want to introduce a formal synchronization to the
>> IOHandler and each command would need to change it from the default "no
>> sync required".
>> We would need two things:
>> - no sync required (default)
>> - sync with timeout for commands that usually complete quickly, but if
>> they don't we need to get back to commands
>>  Out of interest, I did research your "If someone is grabbing the event
>>> manually by hijacking events" point. But when stopped state is detected
>>> (i.e. the reply to ?) in GDBRemote and Broadcaster::PrivateBroadcastEvent
>>> is called, there is no hijacking_listener. Indeed the execution path is
>>> that as indicated by the -->
>>>     if (hijacking_listener)
>>>     {
>>>         if (unique && hijacking_listener->PeekAtNextEventForBroadcasterWithType
>>> (this, event_type))
>>>             return;
>>>         hijacking_listener->AddEvent (event_sp);
>>>     }
>>>     else
>>>     {
>>>         collection::iterator pos, end = m_listeners.end();
>>>         // Iterate through all listener/mask pairs
>>>         for (pos = m_listeners.begin(); pos != end; ++pos)
>>>         {
>>>             // If the listener's mask matches any bits that we just set,
>>> then
>>>             // put the new event on its event queue.
>>>             if (event_type & pos->second)
>>>             {
>>>                 if (unique && pos->first->PeekAtNextEventForBroadcasterWithType
>>> (this, event_type))
>>>                     continue;
>>> ---->         pos->first->AddEvent (event_sp);
>>> So my contention is that in the case of gdb-connect the initial stop
>>> state should either be delivered/printed sychronously in the
>>> Process::ConnectRemote (i.e. in the mainthread context) or that the main
>>> thread should block until either the event arrives, or for some other
>>> reason the command last issued by the user is deemed to be "complete".
>>>  Agreed. "process connect" should be doing better synchronization.
>> We also need a better way to deliver async output to the IOHandler stack.
>> Right the only place that tries to handle this is in the Debugger class
>> where it handles process events. This is where the tricky code of popping
>> the process IOHandler lives. What should happen is we should have a
>> function like:
>> Debugger::AsyncIO (...)
>> and any async output (from the process' stdout/stderr, or an
>> eStateStopped event with thread state and frame and reason for stop) should
>> be delivered through this.
>> The top IOHandler would then get this delivered to it in a thread safe
>> way and it could then hide itself (for the command interpreter remove the
>> "(lldb) " prompt and any command you are typing, display the output, then
>> refresh the prompt and command, and continue. I believe this will solve all
>> of our issues (using Debugger::AyncIO(...)).
>> Greg
>>   To report this email as spam click https://www.mailcontrol.com/
>> sr/MZbqvYs5QwJvpeaetUwhCQ== .
> 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. Keep up to date with CSR on
> our technical blog, www.csr.com/blog, CSR people blog, www.csr.com/people,
> YouTube, www.youtube.com/user/CSRplc, Facebook,
> www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at
> www.twitter.com/CSR_plc.
> New for 2014, you can now access the wide range of products powered by
> aptX at www.aptx.com.
> _______________________________________________
> lldb-dev mailing list
> lldb-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev

Todd Fiala | Software Engineer | tfiala at google.com | 650-943-3180
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20140822/d174a163/attachment.html>

More information about the lldb-dev mailing list