[lldb-dev] gdb-remote lldb prompt race condition
Matthew Gardiner
mg11 at csr.com
Mon Aug 25 22:29:13 PDT 2014
That would be good - thanks for the update, Todd!
Matt
Todd Fiala wrote:
> FWIW I am planning on having Shawn Best look at doing this when he
> gets back.
>
> -Todd
>
>
> On Thu, Aug 21, 2014 at 10:59 PM, Matthew Gardiner <mg11 at csr.com
> <mailto: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 <mailto: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 <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-dev mailing list
> lldb-dev at cs.uiuc.edu <mailto:lldb-dev at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
>
>
>
>
> --
> Todd Fiala | Software Engineer | tfiala at google.com
> <mailto:tfiala at google.com> | 650-943-3180
>
>
More information about the lldb-dev
mailing list