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 <m...@csr.com
<mailto:m...@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
<m...@csr.com <mailto:m...@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@cs.uiuc.edu <mailto:lldb-dev@cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
--
Todd Fiala | Software Engineer | tfi...@google.com
<mailto:tfi...@google.com> | 650-943-3180
_______________________________________________
lldb-dev mailing list
lldb-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
