[v2,2/2] gdbstub: Fix vCont behaviour

Commit Message

Pedro Alves Oct. 28, 2016, 2:01 p.m. UTC

On 10/28/2016 02:35 PM, Claudio Imbrenda wrote:
> On 27/10/16 13:40, Pedro Alves wrote:
>> I'm not a qemu gdbstub expert, but FYI, seeing this reminded me to push
>> to gdb's master a (getting old) gdb patch that clarifies how vCont actions
>> should be interpreted:
>>
>>  https://sourceware.org/ml/gdb-patches/2016-02/msg00493.html
>>
>> It's already live at:
>>
>>  https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html
>>
>> (The "already running" case is for non-stop mode, which qemu
>> probably doesn't support.)
> 
> From the new specifications I seem to understand that if I specify a
> default as first action, then no further actions will be considered,
> since it matches all threads?

Right.

> 
> e.g. vCont;c;s:1  will ignore the s:1 because the c was specified
> earlier and it matches all threads, and therefore it's the leftmost
> match? do I understand correctly?

Yes.

Note that GDB never ever sent a vCont like that.  It's always put
the default action last.  So in practice, it doesn't change
any behavior.  OTOH, stubs can be simpler, given there's no need to
special-case the default action.

(Also, with this definition we can safely break a big vCont packet
in multiple smaller ones, in non-stop mode:

 vCont;s:1;c == vCont;s:1 + vCont;c
 vCont;c;s:1 == vCont;c + vCont;s:1
 vCont;s:1;s:2;s:3;s:3;s:4;...s:N;c == vCont;s:1;s:2;s:3 + vCont;s:4;...s:N;c

 etc.
)

> 
> Or does the default only match any thread without an explicit match?
> (which is how I interpreted the old spec)

Maybe I should just remove all mentions of a "default" from the text?

Like:

~~~
For each inferior thread, the leftmost action with a matching
thread-id is applied. Threads that don’t match any action remain in
their current state. Thread IDs are specified using the syntax
described in thread-id syntax. If multiprocess extensions (see
multiprocess extensions) are supported, actions can be specified to
match all threads in a process by using the ‘ppid.-1’ form of the
thread-id. An action with no thread-id is called the default action
and matches all threads. Specifying multiple default actions is an
error; specifying no actions is also an error.
~~~

To this:

~~~
For each inferior thread, the leftmost action with a matching
thread-id is applied. Threads that don’t match any action remain in
their current state. Thread IDs are specified using the syntax
described in thread-id syntax. If multiprocess extensions (see
multiprocess extensions) are supported, actions can be specified to
match all threads in a process by using the ‘ppid.-1’ form of the
thread-id. An action with no thread-id matches all threads. Specifying
no actions is an error.
~~~

Would that help ?

Thanks,
Pedro Alves

Comments

Claudio Imbrenda Oct. 28, 2016, 2:25 p.m. UTC | #1

On 28/10/16 16:01, Pedro Alves wrote:
>> From the new specifications I seem to understand that if I specify a
>> default as first action, then no further actions will be considered,
>> since it matches all threads?
> 
> Right.

ok, that's the way I understand the new specs, which is not how I had
understood the old specs (hence my questions), so I think your
documentation patch was really needed.

> Note that GDB never ever sent a vCont like that.  It's always put
> the default action last.  So in practice, it doesn't change
> any behavior.  OTOH, stubs can be simpler, given there's no need to
> special-case the default action.

ok, I admit I never looked deeply into what GDB sends :)

> (Also, with this definition we can safely break a big vCont packet
> in multiple smaller ones, in non-stop mode:
> 
>  vCont;s:1;c == vCont;s:1 + vCont;c
>  vCont;c;s:1 == vCont;c + vCont;s:1
>  vCont;s:1;s:2;s:3;s:3;s:4;...s:N;c == vCont;s:1;s:2;s:3 + vCont;s:4;...s:N;c

that makes sense, although I never looked into it since Qemu doesn't
support non-stop mode

>> Or does the default only match any thread without an explicit match?
>> (which is how I interpreted the old spec)
> 
> Maybe I should just remove all mentions of a "default" from the text?

[..]

> Like:
> 
> diff --git c/gdb/doc/gdb.texinfo w/gdb/doc/gdb.texinfo
> index d636a16..df548dc 100644
> --- c/gdb/doc/gdb.texinfo
> +++ w/gdb/doc/gdb.texinfo
> @@ -35538,9 +35538,8 @@ syntax described in @ref{thread-id syntax}.  If multiprocess
>  extensions (@pxref{multiprocess extensions}) are supported, actions
>  can be specified to match all threads in a process by using the
>  @samp{p@var{pid}.-1} form of the @var{thread-id}.  An action with no
> -@var{thread-id} is called the default action and matches all threads.
> -Specifying multiple default actions is an error; specifying no actions
> -is also an error.
> +@var{thread-id} matches all threads.  Specifying no actions is an
> +error.

> Would that help ?

that makes it even more clear (and simple).
On the other hand, now expressing more than one default is not
considered an error any longer (although the extra defaults serve no
purpose); in the end it wouldn't break backwards compatibility, so why not.

Basically a "default" e.g. "s" is the same as specifying "s:-1". This
does make the implementation of the stub easier.


thanks a lot for your prompt and detailed reply!

Pedro Alves Nov. 2, 2016, 2:50 p.m. UTC | #2

Closing the loop here:

On 10/28/2016 03:25 PM, Claudio Imbrenda wrote:

>> Maybe I should just remove all mentions of a "default" from the text?

...

>> Would that help ?
> 
> that makes it even more clear (and simple).
> On the other hand, now expressing more than one default is not
> considered an error any longer (although the extra defaults serve no
> purpose); in the end it wouldn't break backwards compatibility, so why not.
> 
> Basically a "default" e.g. "s" is the same as specifying "s:-1". This
> does make the implementation of the stub easier.

That gdb doc simplification is in master now, and live at the same url.

> thanks a lot for your prompt and detailed reply!

Np!  Thank you too.

Patch

diff --git c/gdb/doc/gdb.texinfo w/gdb/doc/gdb.texinfo
index d636a16..df548dc 100644
--- c/gdb/doc/gdb.texinfo
+++ w/gdb/doc/gdb.texinfo
@@ -35538,9 +35538,8 @@  syntax described in @ref{thread-id syntax}.  If multiprocess
 extensions (@pxref{multiprocess extensions}) are supported, actions
 can be specified to match all threads in a process by using the
 @samp{p@var{pid}.-1} form of the @var{thread-id}.  An action with no
-@var{thread-id} is called the default action and matches all threads.
-Specifying multiple default actions is an error; specifying no actions
-is also an error.
+@var{thread-id} matches all threads.  Specifying no actions is an
+error.

I.e., go from this: