diff mbox series

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

Message ID 20200409164954.36902-2-peterx@redhat.com (mailing list archive)
State New, archived
Headers show
Series gdbstub: Unbreak i386 with gdb remote | expand

Commit Message

Peter Xu April 9, 2020, 4:49 p.m. UTC 
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(-)

Comments

Philippe Mathieu-Daudé April 9, 2020, 5:51 p.m. UTC | #1 
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 ...:

            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 April 9, 2020, 6:12 p.m. UTC | #2 
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)
> > 
>
diff mbox series

Patch

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);
     }
 
     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;
+
+    assert(len == buf->len - orig_len);
+
+    return len;
 }
 
 static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)