Re: [Qemu-devel] [PATCH] Fix for RSP vCont packet

[Luc Michel <luc.michel@greensocs.com>]

Hi Lucien,

When you do a re-roll of your patch, you should:
  - Send it as a new thread (don't reply to an existing one)
  - Add the re-roll count ([PATCH vX] in the subject, easily done using
the -v option of git format-patch).

On 3/4/19 8:39 AM, Lucien Murray-Pitts wrote:
> This fixes a regression in rsp packet vCont, this was due to the
> recently added multiprocess support. (Short commit hash: e40e520).
> 
> The result is that vCont now does not recognise the case where
> no process/thread is provided after the action.
> 
> This may not show up with GDB, but using Lauterbach Trace32,
> and Hexrays IDA Pro this issue is immediately seen.
> 
> The response is a "$#00" empty packet, showing it is unsupported packet.
> 
> This is defined in the RSP document as "An action with no thread-id
> matches all threads."
> (https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html#vCont-packet)
> 
> This is where the document lacks details. It says that the vCont packet
> is used to "Resume the inferior, specifying different actions for each
> thread."
> 
> So it seems to target only one inferior (i.e. one process).
> 
> When multiprocess extension is in use, this packet must be supported,
> and used in place of 'Hc' (which modifies s->c_cpu) and other action
> packets (s, S, ...) which are deprecated.
> 
> So the question is: when the vCont packet does not specifies a
> thread-id, what _process_ we should choose?
> 
> Going for all process is probably ok because:
>   - we don't have to bother choosing a PID,
>   - it will cover the mono-process case correctly (main use-case of this
> form of vCont packet I think).
Huum this looks like a copy/paste of my text. That's ok but I would have
appreciated an acknowledgment.

> 
> The alternative would be can go for GDB_ALL_THREAD and take the PID of the
> first attached CPU.  But may lead to problems, so I have chosen the
> GDB_ALL_PROCESSES.>
> A request for improved documentation has been made at;
> https://sourceware.org/bugzilla/show_bug.cgi?id=24177
That's nice!

> 
> Thus, for now, the valid vCont packets now are as below.
> However, parsing is still not very strict and many other invalid arguments
> formats are possible but they are not expected from standard debuggers.
I'm not sure what you mean by "invalid" here. From the protocol PoV, the
syntax is valid. If the first action applies to all processes/threads,
then the next actions will simply have no effect.

If you agree with me then I suggest that you remove those examples below
to avoid confusions.
> 
>  * vCont;c/s
>  ** Step/Continue all threads
> 
>  * vCont;c/s:[pX.]Y
>  ** Step/Continue optional process X, thread Y
> 
>  * vCont;C##/S##:[pX.]Y
>  ** Step/Continue with signal ## on optional process X, thread Y
> 
>  * If X or Y are -1 then it applies the action to all processes/threads.
> 
> Signed-off-by: Lucien Murray-Pitts <lucienmp_antispam@yahoo.com>
> ---
> Third attempt, taking greatly valued input on formatting.
> Also includes a change to the way kind is handled for no-action vcont
> ---
>  gdbstub.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index a4be63f6eb..f8680f7ee8 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1145,6 +1145,7 @@ static int is_query_packet(const char *p, const char *query, char separator)
>   */
>  static int gdb_handle_vcont(GDBState *s, const char *p)
>  {
> +    GDBThreadIdKind kind;
>      int res, signal = 0;
>      char cur_action;
>      char *newstates;
> @@ -1194,12 +1195,24 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
>              goto out;
>          }
>  
> -        if (*p++ != ':') {
> +        /*
> +         * When we have vCont;c or vCont;s - action is on all threads
> +         * Alternatively action maybe followed by a ';' and more specifiers
> +         * such as vCont;c;s:p1.1 is a possible, but meaningless format.
> +         * Otherwise if the action is followed by a ':' then
> +         * the pPID.TID format is expected (for example "vCont;c:p1.1;...).
> +         */
Same as my comment above, maybe you can remove this comment, and add
some precisions to your comment bellow:
> +        if (*p == '\0' || *p == ';') {> +            /* unclear behavior in RSP spec, assume GDB_ALL_PROCESSES
is ok */
  /* No thread specifier, action is on "all threads". The specification
   * is unclear regarding the process to act on. We choose all
   * processes.
   */
> +            kind = GDB_ALL_PROCESSES;
> +        } else if (*p++ == ':') {
> +            kind = read_thread_id(p, &p, &pid, &tid);
> +        } else {
>              res = -ENOTSUP;
>              goto out;
>          }
>  
> -        switch (read_thread_id(p, &p, &pid, &tid)) {
> +        switch (kind) {
>          case GDB_READ_THREAD_ERR:
>              res = -EINVAL;
>              goto out;
> 

With those changes:
Reviewed-by: Luc Michel <luc.michel@greensocs.com>

Thanks,
Luc