diff mbox series

gdbstub: fixes cases where wrong threads were reported to GDB on SIGINT

Message ID 20230623181256.2596-1-dark.ryu.550@gmail.com
State New
Headers show
Series gdbstub: fixes cases where wrong threads were reported to GDB on SIGINT | expand

Commit Message

Matheus Branco Borella June 23, 2023, 6:12 p.m. UTC 
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1725

This fix is implemented by having the vCont handler set the value of
`gdbserver_state.c_cpu` if any threads are to be resumed. The specific CPU
is picked arbitrarily from the ones to be resumed, but it should be okay, as all
GDB cares about is that it is a resumed thread.

Keep in mind that because this patch overwrites `c_cpu`, it breaks cases where
$vCont is used together with $Hc, so there might be more work to be done here.
It might also be the case that it breaking this, specifically, isn't of
consequence, seeing as single stepping with $vCont already overwrites `c_cpu`
anyway, so you could say the implementation already behaves oddly as far as
mixing $vCont and $Hc is concerned.
---
 gdbstub/gdbstub.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

Comments

Alex Bennée June 27, 2023, 10:39 a.m. UTC | #1 
Matheus Branco Borella <dark.ryu.550@gmail.com> writes:

> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1725
>
> This fix is implemented by having the vCont handler set the value of
> `gdbserver_state.c_cpu` if any threads are to be resumed. The specific CPU
> is picked arbitrarily from the ones to be resumed, but it should be okay, as all
> GDB cares about is that it is a resumed thread.
>
> Keep in mind that because this patch overwrites `c_cpu`, it breaks cases where
> $vCont is used together with $Hc, so there might be more work to be
> done here.

That doesn't sound good. Is that a possible case or an invalid one
because we shouldn't see gdbs using both?

> It might also be the case that it breaking this, specifically, isn't of
> consequence, seeing as single stepping with $vCont already overwrites `c_cpu`
> anyway, so you could say the implementation already behaves oddly as far as
> mixing $vCont and $Hc is concerned.

It would be nice to have some unit tests for this behaviour to defend
it. See the various tests in tests/tcg that call $(GDB_SCRIPT) for
examples.

BTW you are missing a Signed-off-by: tag which we will need to take a
patch submission. See:

  https://qemu.readthedocs.io/en/latest/devel/submitting-a-patch.html


> ---
>  gdbstub/gdbstub.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index be18568d0a..4f7ac5ddfe 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -595,6 +595,15 @@ static int gdb_handle_vcont(const char *p)
>       *  or incorrect parameters passed.
>       */
>      res = 0;
> +    
> +    /* 
> +     * target_count and last_target keep track of how many CPUs we are going to
> +     * step or resume, and a pointer to the state structure of one of them, 
> +     * respectivelly
> +     */
> +    int target_count = 0;
> +    CPUState *last_target = NULL;
> +
>      while (*p) {
>          if (*p++ != ';') {
>              res = -ENOTSUP;
> @@ -639,8 +648,10 @@ static int gdb_handle_vcont(const char *p)
>              while (cpu) {
>                  if (newstates[cpu->cpu_index] == 1) {
>                      newstates[cpu->cpu_index] = cur_action;
> -                }
>  
> +                    target_count++;
> +                    last_target = cpu;
> +                }
>                  cpu = gdb_next_attached_cpu(cpu);
>              }
>              break;
> @@ -657,6 +668,9 @@ static int gdb_handle_vcont(const char *p)
>              while (cpu) {
>                  if (newstates[cpu->cpu_index] == 1) {
>                      newstates[cpu->cpu_index] = cur_action;
> +                    
> +                    target_count++;
> +                    last_target = cpu;
>                  }
>  
>                  cpu = gdb_next_cpu_in_process(cpu);
> @@ -675,10 +689,25 @@ static int gdb_handle_vcont(const char *p)
>              /* only use if no previous match occourred */
>              if (newstates[cpu->cpu_index] == 1) {
>                  newstates[cpu->cpu_index] = cur_action;
> +
> +                target_count++;
> +                last_target = cpu;
>              }
>              break;
>          }
>      }
> +
> +    /* 
> +     * if we're about to resume a specific set of CPUs/threads, make it so that 
> +     * in case execution gets interrupted, we can send GDB a stop reply with a
> +     * correct value. it doesn't really matter which CPU we tell GDB the signal 
> +     * happened in (VM pauses stop all of them anyway), so long as it is one of
> +     * the ones we resumed/single stepped here.
> +     */
> +    if (target_count > 0) {
> +        gdbserver_state.c_cpu = last_target;
> +    }
> +
>      gdbserver_state.signal = signal;
>      gdb_continue_partial(newstates);

Looks reasonable at first glance but I would like some tests.
diff mbox series

Patch

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index be18568d0a..4f7ac5ddfe 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -595,6 +595,15 @@  static int gdb_handle_vcont(const char *p)
      *  or incorrect parameters passed.
      */
     res = 0;
+    
+    /* 
+     * target_count and last_target keep track of how many CPUs we are going to
+     * step or resume, and a pointer to the state structure of one of them, 
+     * respectivelly
+     */
+    int target_count = 0;
+    CPUState *last_target = NULL;
+
     while (*p) {
         if (*p++ != ';') {
             res = -ENOTSUP;
@@ -639,8 +648,10 @@  static int gdb_handle_vcont(const char *p)
             while (cpu) {
                 if (newstates[cpu->cpu_index] == 1) {
                     newstates[cpu->cpu_index] = cur_action;
-                }
 
+                    target_count++;
+                    last_target = cpu;
+                }
                 cpu = gdb_next_attached_cpu(cpu);
             }
             break;
@@ -657,6 +668,9 @@  static int gdb_handle_vcont(const char *p)
             while (cpu) {
                 if (newstates[cpu->cpu_index] == 1) {
                     newstates[cpu->cpu_index] = cur_action;
+                    
+                    target_count++;
+                    last_target = cpu;
                 }
 
                 cpu = gdb_next_cpu_in_process(cpu);
@@ -675,10 +689,25 @@  static int gdb_handle_vcont(const char *p)
             /* only use if no previous match occourred */
             if (newstates[cpu->cpu_index] == 1) {
                 newstates[cpu->cpu_index] = cur_action;
+
+                target_count++;
+                last_target = cpu;
             }
             break;
         }
     }
+
+    /* 
+     * if we're about to resume a specific set of CPUs/threads, make it so that 
+     * in case execution gets interrupted, we can send GDB a stop reply with a
+     * correct value. it doesn't really matter which CPU we tell GDB the signal 
+     * happened in (VM pauses stop all of them anyway), so long as it is one of
+     * the ones we resumed/single stepped here.
+     */
+    if (target_count > 0) {
+        gdbserver_state.c_cpu = last_target;
+    }
+
     gdbserver_state.signal = signal;
     gdb_continue_partial(newstates);