Re: [PATCH 3/5] gdbstub: reject unsupported flags in handle_set_qemu_sstep

["Alex Bennée" <alex.bennee@linaro.org>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> From: Maxim Levitsky <mlevitsk@redhat.com>
>
> handle_query_qemu_sstepbits is reporting NOIRQ and NOTIMER bits
> even if they are not supported (as is the case with record/replay).
> Instead, store the supported singlestep flags and reject
> any unsupported bits in handle_set_qemu_sstep.  This removes
> the need for the get_sstep_flags() wrapper.
>
> While at it, move the variables in GDBState, instead of using
> global variables.
>
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> [Extracted from Maxim's patch into a separate commit. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  gdbstub.c | 73 +++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 47 insertions(+), 26 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 23baaef40e..960b9fbcd0 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -368,27 +368,10 @@ typedef struct GDBState {
>      gdb_syscall_complete_cb current_syscall_cb;
>      GString *str_buf;
>      GByteArray *mem_buf;
> +    int sstep_flags;
> +    int supported_sstep_flags;
>  } GDBState;
>  
> -/* By default use no IRQs and no timers while single stepping so as to
> - * make single stepping like an ICE HW step.
> - */
> -static int sstep_flags = SSTEP_ENABLE|SSTEP_NOIRQ|SSTEP_NOTIMER;
> -
> -/* Retrieves flags for single step mode. */
> -static int get_sstep_flags(void)
> -{
> -    /*
> -     * In replay mode all events written into the log should be replayed.
> -     * That is why NOIRQ flag is removed in this mode.
> -     */
> -    if (replay_mode != REPLAY_MODE_NONE) {
> -        return SSTEP_ENABLE;
> -    } else {
> -        return sstep_flags;
> -    }
> -}
> -
>  static GDBState gdbserver_state;
>  
>  static void init_gdbserver_state(void)
> @@ -399,6 +382,24 @@ static void init_gdbserver_state(void)
>      gdbserver_state.str_buf = g_string_new(NULL);
>      gdbserver_state.mem_buf = g_byte_array_sized_new(MAX_PACKET_LENGTH);
>      gdbserver_state.last_packet = g_byte_array_sized_new(MAX_PACKET_LENGTH + 4);
> +
> +    /*
> +     * In replay mode all events written into the log should be replayed.
> +     * That is why NOIRQ flag is removed in this mode.

I guess if we were being complete we could say something like:

  In replay mode all events will come from the log and can't be
  suppressed otherwise we would break determinism. However as those
  events are tied to the number of executed instructions we won't see
  them occurring every time we single step. 

> +     */
> +    if (replay_mode != REPLAY_MODE_NONE) {
> +        gdbserver_state.supported_sstep_flags = SSTEP_ENABLE;
> +    } else {
> +        gdbserver_state.supported_sstep_flags =
> +            SSTEP_ENABLE | SSTEP_NOIRQ | SSTEP_NOTIMER;
> +    }
> +
> +    /*
> +     * By default use no IRQs and no timers while single stepping so as to
> +     * make single stepping like an ICE HW step.
> +     */
> +    gdbserver_state.sstep_flags = gdbserver_state.supported_sstep_flags;
> +
>  }
>  
>  #ifndef CONFIG_USER_ONLY
> @@ -505,7 +506,7 @@ static int gdb_continue_partial(char *newstates)
>      CPU_FOREACH(cpu) {
>          if (newstates[cpu->cpu_index] == 's') {
>              trace_gdbstub_op_stepping(cpu->cpu_index);
> -            cpu_single_step(cpu, sstep_flags);
> +            cpu_single_step(cpu, gdbserver_state.sstep_flags);
>          }
>      }
>      gdbserver_state.running_state = 1;
> @@ -524,7 +525,7 @@ static int gdb_continue_partial(char *newstates)
>                  break; /* nothing to do here */
>              case 's':
>                  trace_gdbstub_op_stepping(cpu->cpu_index);
> -                cpu_single_step(cpu, get_sstep_flags());
> +                cpu_single_step(cpu, gdbserver_state.sstep_flags);
>                  cpu_resume(cpu);
>                  flag = 1;
>                  break;
> @@ -1883,7 +1884,7 @@ static void handle_step(GArray *params, void *user_ctx)
>          gdb_set_cpu_pc((target_ulong)get_param(params, 0)->val_ull);
>      }
>  
> -    cpu_single_step(gdbserver_state.c_cpu, get_sstep_flags());
> +    cpu_single_step(gdbserver_state.c_cpu, gdbserver_state.sstep_flags);
>      gdb_continue();
>  }
>  
> @@ -2017,24 +2018,44 @@ static void handle_v_commands(GArray *params, void *user_ctx)
>  
>  static void handle_query_qemu_sstepbits(GArray *params, void *user_ctx)
>  {
> -    g_string_printf(gdbserver_state.str_buf, "ENABLE=%x,NOIRQ=%x,NOTIMER=%x",
> -                    SSTEP_ENABLE, SSTEP_NOIRQ, SSTEP_NOTIMER);
> +    g_string_printf(gdbserver_state.str_buf, "ENABLE=%x", SSTEP_ENABLE);
> +
> +    if (gdbserver_state.supported_sstep_flags & SSTEP_NOIRQ) {
> +        g_string_append_printf(gdbserver_state.str_buf, ",NOIRQ=%x",
> +                               SSTEP_NOIRQ);
> +    }
> +
> +    if (gdbserver_state.supported_sstep_flags & SSTEP_NOTIMER) {
> +        g_string_append_printf(gdbserver_state.str_buf, ",NOTIMER=%x",
> +                               SSTEP_NOTIMER);
> +    }
> +
>      put_strbuf();
>  }
>  
>  static void handle_set_qemu_sstep(GArray *params, void *user_ctx)
>  {
> +    int new_sstep_flags;
> +
>      if (!params->len) {
>          return;
>      }
>  
> -    sstep_flags = get_param(params, 0)->val_ul;
> +    new_sstep_flags = get_param(params, 0)->val_ul;
> +
> +    if (new_sstep_flags  & ~gdbserver_state.supported_sstep_flags) {
> +        put_packet("E22");
> +        return;
> +    }
> +
> +    gdbserver_state.sstep_flags = new_sstep_flags;
>      put_packet("OK");
>  }
>  
>  static void handle_query_qemu_sstep(GArray *params, void *user_ctx)
>  {
> -    g_string_printf(gdbserver_state.str_buf, "0x%x", sstep_flags);
> +    g_string_printf(gdbserver_state.str_buf, "0x%x",
> +                    gdbserver_state.sstep_flags);
>      put_strbuf();
>  }

Otherwise LGTM:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée