Re: [PATCH] gdbstub: fixes cases where wrong threads were reported to GDB on SIGINT

[Alex Bennée]

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.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro