Re: [PATCH RESEND v5 12/26] gdbstub: Use GDBFeature for GDBRegisterState

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

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> Simplify GDBRegisterState by replacing num_regs and xml members with
> one member that points to GDBFeature.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  gdbstub/gdbstub.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index b62002bc34..ee6b8b98c8 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -47,10 +47,9 @@
>  
>  typedef struct GDBRegisterState {
>      int base_reg;
> -    int num_regs;
>      gdb_get_reg_cb get_reg;
>      gdb_set_reg_cb set_reg;
> -    const char *xml;
> +    const GDBFeature *feature;
>      struct GDBRegisterState *next;
>  } GDBRegisterState;
>  
> @@ -390,7 +389,7 @@ static const char *get_feature_xml(const char *p, const char **newp,
>              pstrcat(buf, buf_sz, "\"/>");
>              for (r = cpu->gdb_regs; r; r = r->next) {
>                  pstrcat(buf, buf_sz, "<xi:include href=\"");
> -                pstrcat(buf, buf_sz, r->xml);
> +                pstrcat(buf, buf_sz, r->feature->xmlname);
>                  pstrcat(buf, buf_sz, "\"/>");
>              }

I should note I've done some cleaning up of the XML code in the gdbstub
so you'll probably want to re-base once the PR has gone in.


>              pstrcat(buf, buf_sz, "</target>");
> @@ -497,7 +496,7 @@ static int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
>      }
>  
>      for (r = cpu->gdb_regs; r; r = r->next) {
> -        if (r->base_reg <= reg && reg < r->base_reg + r->num_regs) {
> +        if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
>              return r->get_reg(env, buf, reg - r->base_reg);
>          }
>      }
> @@ -515,7 +514,7 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
>      }
>  
>      for (r = cpu->gdb_regs; r; r = r->next) {
> -        if (r->base_reg <= reg && reg < r->base_reg + r->num_regs) {
> +        if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
>              return r->set_reg(env, mem_buf, reg - r->base_reg);
>          }
>      }
> @@ -538,17 +537,16 @@ void gdb_register_coprocessor(CPUState *cpu,
>      p = &cpu->gdb_regs;
>      while (*p) {
>          /* Check for duplicates.  */
> -        if (strcmp((*p)->xml, feature->xmlname) == 0)
> +        if ((*p)->feature == feature)
>              return;
>          p = &(*p)->next;
>      }
>  
>      s = g_new0(GDBRegisterState, 1);
>      s->base_reg = cpu->gdb_num_regs;
> -    s->num_regs = feature->num_regs;
>      s->get_reg = get_reg;
>      s->set_reg = set_reg;
> -    s->xml = feature->xml;
> +    s->feature = feature;
>  
>      /* Add to end of list.  */
>      cpu->gdb_num_regs += feature->num_regs;

Otherwise:

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

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro