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

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

[-- Attachment #1: Type: text/plain, Size: 2946 bytes --]

Hi Lucien,

On 1/31/19 5:48 AM, Lucien Murray-Pitts wrote:
> 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 )
> 
> Thus the valid vCont packets now are as below, however parsing is still not very strict.
>   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>
> ---
>  gdbstub.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index bfc7afb509..ce0dde2e24 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1169,6 +1169,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 vcontThreadType ;
The coding style for variable names is lower_case_with_underscores (see
CODING_STYLE). I think you can go with a simpler name like
GDBThreadIdKind kind;

>      int res, signal = 0;
>      char cur_action;
>      char *newstates;
> @@ -1218,12 +1219,23 @@ static int gdb_handle_vcont(GDBState *s, const char *p)
>              goto out;
>          }
>  
> -        if (*p++ != ':') {
> +        /*
> +         * In the case we have vCont;c or vCont;s - action is on all threads
> +         * Alternatively vCont;c;s:p1.1 is a possible, but meaningless format,
> +         * And in the else the "vCont;c:p1.1;... format is supported.
> +         */> +        if (*p == '\0' || *p == ';') {
> +            vcontThreadType = GDB_ALL_THREAD ;> +            pid = 1 ;
The spec is not clear but I would opt for GDB_ALL_PROCESSES instead of
GDB_ALL_THREAD here. pid = 1; is clearly wrong since you don't know if
this PID exists or is currently attached.

> +            tid = 1 ;
This one is not useful either (not used in the switch..case bellow).

Thanks

Luc

> +        } else if (*p++ == ':') {
> +            vcontThreadType = read_thread_id(p, &p, &pid, &tid) ;
> +        } else {
>              res = -ENOTSUP;
>              goto out;
>          }
>  
> -        switch (read_thread_id(p, &p, &pid, &tid)) {
> +        switch (vcontThreadType) {
>          case GDB_READ_THREAD_ERR:
>              res = -EINVAL;
>              goto out;
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]