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

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

[Lucien Murray-Pitts]

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 ;
     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_THREADS ;
+            pid = 1 ;
+            tid = 1 ;
+        } 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;
-- 
2.17.2

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

[Eric Blake]

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

On 1/30/19 10:48 PM, Lucien Murray-Pitts wrote:
> The result is that vCont now does not recognise the case where no process/thread is provided after the action.

Thanks for re-sending; this one came through a lot nicer!  Welcome to
the community; while a first submission can always be a bit daunting,
the maintainers generally will help fix things up as you refine your
process for making future submissions run more smoothly.

My next suggestion: wrap your commit message body around 65-70 columns,
rather than using long lines. While it may sound archaic, many
developers still use 80-column terminals, and 'git log' adds
indentation; long lines become difficult to read if you don't manually
wrap them.

> 
> 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(-)

I'll let someone more familiar with gdbstub review the actual patch
content for correctness, but I'll do a quick pass for style:

> 
> 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 ;

No space before ;

>      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

Grammar suggestion: s/In the case/When/, and use a trailing '.' to end
the sentence.

> +         * Alternatively vCont;c;s:p1.1 is a possible, but meaningless format,
> +         * And in the else the "vCont;c:p1.1;... format is supported.

s/And in the else/otherwise/

> +         */
> +        if (*p == '\0' || *p == ';') {
> +            vcontThreadType = GDB_ALL_THREADS ;
> +            pid = 1 ;
> +            tid = 1 ;

Again, no space before ;

> +        } 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;
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

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

[Luc Michel]

[-- 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 --]

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

[Lucien Anti-Spam]

 Hi Luc,
Great feedback from both you and Eric - im blown away, I dont thinktheres a single 1byte ascii character left unturned there :)My coding style snuck in erywhere, as did my Scottish English!
    On Friday, February 1, 2019, 10:33:18 PM GMT+9, Luc Michel <luc.michel@greensocs.com> wrote: > > +    GDBThreadIdKind vcontThreadType ;
> The coding style ... CODING_STYLE ...
Ah I didnt pick up on this whilst reading the Wiki for coding style, thank you!
> > +            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.
I was in a quandary here, and I did see your previous email too.However, I looked at how the call to read_thread_id() behaved and copied over the same behavior as if the command was vCont;c:-1which I understand to mean continue all threads?
So maybe we should look at altering read_thread_id() which sets pid=1 when thereis no "ppid." part to the thread-id description, and if you set "tid=-1" then it performs a GDB_ALL_THREAD
>From the vCont spec I also got;"p-1 ... means all processes""An action with no thread-id matches all threads."
"tid=-1 ... matches all threads"
But it is not very clear what happens if you omit ppid and supply tid as -1.
What do you think?
Cheers,

Luc

  

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

[Luc Michel]

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

Hi,

On 2/1/19 3:25 PM, Lucien Anti-Spam wrote:
> Hi Luc,
> 
> Great feedback from both you and Eric - im blown away, I dont think
> theres a single 1byte ascii character left unturned there :)
> My coding style snuck in erywhere, as did my Scottish English!
> 
> On Friday, February 1, 2019, 10:33:18 PM GMT+9, Luc Michel
> <luc.michel@greensocs.com> wrote:
>> > +    GDBThreadIdKind vcontThreadType ;
>> The coding style ... CODING_STYLE ...
> Ah I didnt pick up on this whilst reading the Wiki for coding style,
> thank you!
> 
>> > +            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.
> 
> I was in a quandary here, and I did see your previous email too.
> However, I looked at how the call to read_thread_id() behaved and
>  copied over the same behavior as if the command was vCont;c:-1
> which I understand to mean continue all threads?
This is where the spec lacks details. The doc 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).

Otherwise, we can go for GDB_ALL_THREAD and take the PID of the first
attached CPU. I'm not sure sure which one is better...

Thanks.

Luc

> 
> So maybe we should look at altering read_thread_id() which sets pid=1
> when there
> is no "ppid." part to the thread-id description, and if you set "tid=-1"
> then it performs a GDB_ALL_THREAD
> 
> From the vCont spec I also got;
> "p-1 ... means all processes"
> "An action with no thread-id matches all threads."
> "tid=-1 ... matches all threads"
> 
> But it is not very clear what happens if you omit ppid and supply tid as -1.
> 
> What do you think?
> 
> Cheers,
> 
> Luc
> 
> 


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

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

[Lucien Murray-Pitts]

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).

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

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.

 * 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;...).
+         */
+        if (*p == '\0' || *p == ';') {
+            /* unclear behavior in RSP spec, assume GDB_ALL_PROCESSES is ok */
+            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;
-- 
2.20.1

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

[Luc Michel]

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

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

[Lucien Anti-Spam]

This fixes a regression in rsp packet vCont due to 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 )
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.cindex 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 ;     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_THREADS ;+            pid = 1 ;+            tid = 1 ;+        } 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;-- 2.17.2

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

[Eric Blake]

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

On 1/30/19 6:41 PM, Lucien Anti-Spam via Qemu-devel wrote:
> This fixes a regression in rsp packet vCont due to recently added multiprocess support. (Short commit hash: e40e520).
> 

Your mailer completely botched the message (wrong line endings?)  Can
you please resend, preferably using git send-email, or at a minimum
attaching git format-patch output rather than inline pasting, so that we
stand a chance of seeing what you actually intended?

> 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.cindex 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 ;     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_THREADS ;+            pid = 1 ;+            tid = 1 ;+        } 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;-- 2.17.2
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

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

[Lucien Anti-Spam]

 

    On Thursday, January 31, 2019, 12:47:23 PM GMT+9, Eric Blake <eblake@redhat.com> wrote:  
 
 On 1/30/19 6:41 PM, Lucien Anti-Spam via Qemu-devel wrote:
> > This fixes a regression in rsp packet vCont due to recently added multiprocess support. (Short commit hash: e40e520).
> > 
> Your mailer completely botched the message (wrong line endings?)  Can ...
Sorry about that - none of my linux machines have smtp access and my workaround mangled it.I am trying to work out another way around that now.