Re: [PATCH 1/2] gdbstub: Assert len read from each register

[Peter Xu <peterx@redhat.com>]

On Thu, Apr 09, 2020 at 07:51:49PM +0200, Philippe Mathieu-Daudé wrote:
> On 4/9/20 6:49 PM, Peter Xu wrote:
> > This can expose the issue earlier on which register returned the wrong
> > result.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   gdbstub.c | 11 ++++++++---
> >   1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/gdbstub.c b/gdbstub.c
> > index 171e150950..f763545e81 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -911,17 +911,22 @@ static int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
> >       CPUClass *cc = CPU_GET_CLASS(cpu);
> >       CPUArchState *env = cpu->env_ptr;
> >       GDBRegisterState *r;
> > +    int len = 0, orig_len = buf->len;
> >       if (reg < cc->gdb_num_core_regs) {
> > -        return cc->gdb_read_register(cpu, buf, reg);
> > +        len = cc->gdb_read_register(cpu, buf, reg);
> 
> This change the code flow. We could add ...:

I didn't expect the "if" and "for" would collapse each other, but yeah
that could still be better.

Thanks,

> 
>            goto out;
> 
> >       }
> 
> ... or use else?
> >       for (r = cpu->gdb_regs; r; r = r->next) {
> >           if (r->base_reg <= reg && reg < r->base_reg + r->num_regs) {
> > -            return r->get_reg(env, buf, reg - r->base_reg);
> > +            len = r->get_reg(env, buf, reg - r->base_reg);
> > +            break;
> >           }
> >       }
> > -    return 0;
> > +
> 
>   out:
> 
> > +    assert(len == buf->len - orig_len);
> > +
> > +    return len;
> >   }
> >   static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
> > 
> 

-- 
Peter Xu