Re: attach_command_post_wait oddity

[Doug Evans <dje at google dot com>]

On Wed, Jul 30, 2014 at 7:08 AM, Pedro Alves <palves@redhat.com> wrote:
> On 07/30/2014 12:10 AM, Doug Evans wrote:
>> Hi.
>>
>> I don't understand this snippet from attach_command_post_wait:
>> [this is the !async_exec case, i.e. no "&"]
>>
>>       /* In all-stop, by definition, all threads have to be already
>>        stopped at this point.  In non-stop, however, although the
>>        selected thread is stopped, others may still be executing.
>>        Be sure to explicitly stop all threads of the process.  This
>>        should have no effect on already stopped threads.  */
>>       if (non_stop)
>>       target_stop (pid_to_ptid (inferior->pid));
>>
>> All target_stop does is (essentially) send SIGSTOP, it doesn't wait
>> for threads to stop.
>>
>> So who does?
>
> Nothing does, the event loop handles the other threads' stops as
> asynchronous stops, similar to "interrupt -a".   Currently,
> you're given the prompt immediately (one thread stopped), and then the
> other threads report stops asynchronously.

I see.  Wouldn't that break user expectations when a command is used
without "&"?
When I read "!async_exec" to me that means "& was not specified", and
thus it is (IMO) an error to give the user the prompt before all
threads have been stopped.  I see that currently attach with no & on
extended-remote linux doesn't wait for the other threads to stop -
isn't that a bug?  What if I want to script it and be able to assume
that after "attach 1234" (no &) the next command in my script can
execute without worrying whether the previous command (the attach) has
completed?  As a user I certainly don't expect to see all those
"Thread stopped" notices appear after my attach command has apparently
"completed" (ie. I've been given the prompt).

I'm working on a "wait" command to let one use "&" in scripts better,
but in the above example there is no "&".

>
>>
>> The above code is immediately followed by:
>>
>>       /* Tell the user/frontend where we're stopped.  */
>>       normal_stop ();
>>       if (deprecated_attach_hook)
>>       deprecated_attach_hook ();
>>
>> and that's it.  So there's no waiting for the inferior to stop here.
>> Either the call to target_stop is redundant (and can be deleted), or
>> there's missing code to wait for threads to stop, or I'm missing something.
>>
>> After some research, I think the *only* case where the above target_stop
>> call *could* do something useful
>> is via this code in
>> remote.c:process_stop_reply:
>>
>>       remote_notice_new_inferior (ptid, 0);
>>
>> [I wrote out a long winded explanation of why I think that's true,
>> but in the end I think it might be quicker to just convince yourself of that.]
>
> We need it for "attach" too.  There's no requirement that target_attach finds
> threads stopped (see description of vAttach in the manual).

The wording in the manual isn't stellar.
Even if a target supports attaching and leaving threads running, it
doesn't specify when that will happen (ie. when threads will be left
running and when they will not).

> And, in attach_command, when we do:
>
>         /* The user requested an `attach', so stop all threads of this
>            inferior.  */
>         target_stop (pid_to_ptid (ptid_get_pid (inferior_ptid)));
>
> we may not know yet the whole set of threads in the process.

The comment is misleading alright.  Need to fix that. :-)

> We may
> only learn about them when we see the first stop, and manage to
> query the list (such as with with libthread_db, for example, which
> only happens in post_create_inferior, when we fetch the list of
> shared libraries).  Now, with native debugging on Linux, we happen to
> discover threads stopped already, but that's not the case with
> all targets.  For those that leave threads running on attach, if
> we don't stop them explicitly here too, and we've only discovered
> them after target_attach, we'll leave them running free by mistake.

Interestingly, on remote linux if I disable that call then threads are
still stopped with "attach" (no "&"). [linux gdbserver has already
attached to threads by the time it processes the earlier stop request]
Plus in the with-& case, if I disable the call to resume threads in
non-stop (proceed_after_attach), they are still running after "attach
&". [linux gdbserver attaches and leaves threads running]
This complexity must be needed for other targets though.

btw, another source of confusion here is that & has two meanings:
1) are threads stopped or running after the attach?
2) is the prompt returned to the user immediately or after the command
completes?
In the "leave threads running" case it doesn't make much sense to
provide the choice of whether to wait for the command to complete (it
could be useful but I'm not worried about it here).  The "leave
threads stopped" case could arguably be a case where having a choice
of waiting for the command to complete or not could be useful.  I
don't have a strong opinion on providing the choice.  I think the
current behaviour of remote attaching without the & is broken though.

I can appreciate that even if one did try to wait for all threads to
stop, that's problematic as the last thing a thread could have done is
spawn a new thread (which we'd then have to wait for) and the last
thing this new thread could have done is spawn a new thread, and so
on.
We *could* augment vAttach to specify whether to leave threads running
(the default could remain as it is today: we don't know whether the
remote client will or will not leave them running).   We can still
improve the linux case (and others where we can augment vAttach - or
add a new command).

[btw, data point: we employ a heuristic to find threads in linux
gdbserver (we iterate a couple of times in case a new thread appears)
because if we miss a thread and set a breakpoint ... boom ... that
thread crashes on a SIGTRAP (because it wasn't PTRACE_ATTACH'd)]