[PATCH v2 0/2] gdbstub: Unbreak i386 with gdb remote

[PATCH v2 0/2] gdbstub: Unbreak i386 with gdb remote

[Peter Xu]

v2:
- use "goto" in patch 1 [Phil]
- pick some tags

Thanks,

Peter Xu (2):
  gdbstub: Assert len read from each register
  gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb

 gdbstub.c             | 12 +++++++++---
 target/i386/gdbstub.c |  2 +-
 2 files changed, 10 insertions(+), 4 deletions(-)

-- 
2.24.1

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

[Peter Xu]

This can expose the issue earlier on which register returned the wrong
result.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 gdbstub.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 171e150950..69656e1052 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -911,17 +911,23 @@ 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);
+        goto out;
     }
 
     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)
-- 
2.24.1

[PATCH v2 2/2] gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb

[Peter Xu]

We should only pass in gdb_get_reg16() with the GByteArray* object
itself, no need to shift.  Without this patch, gdb remote attach will
crash QEMU.

Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Fixes: b7b8756a9c ("target/i386: use gdb_get_reg helpers", 2020-03-17)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 target/i386/gdbstub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
index f3d23b614e..b98a99500a 100644
--- a/target/i386/gdbstub.c
+++ b/target/i386/gdbstub.c
@@ -106,7 +106,7 @@ int x86_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
         floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
         int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
-        len += gdb_get_reg16(mem_buf + len, cpu_to_le16(fp->high));
+        len += gdb_get_reg16(mem_buf, cpu_to_le16(fp->high));
         return len;
     } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
         n -= IDX_XMM_REGS;
-- 
2.24.1

Re: [PATCH v2 2/2] gdbstub: i386: Fix gdb_get_reg16() parameter to unbreak gdb

[Peter Xu]

On Thu, Apr 09, 2020 at 02:25:38PM -0400, Peter Xu wrote:
> We should only pass in gdb_get_reg16() with the GByteArray* object
> itself, no need to shift.  Without this patch, gdb remote attach will
> crash QEMU.
> 
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Fixes: b7b8756a9c ("target/i386: use gdb_get_reg helpers", 2020-03-17)

So I think this should still be:

Fixes: a010bdbe71 ("gdbstub: extend GByteArray to read register helpers")

As Phil & Laurent pointed out.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  target/i386/gdbstub.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
> index f3d23b614e..b98a99500a 100644
> --- a/target/i386/gdbstub.c
> +++ b/target/i386/gdbstub.c
> @@ -106,7 +106,7 @@ int x86_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>      } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
>          floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
>          int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
> -        len += gdb_get_reg16(mem_buf + len, cpu_to_le16(fp->high));
> +        len += gdb_get_reg16(mem_buf, cpu_to_le16(fp->high));
>          return len;
>      } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
>          n -= IDX_XMM_REGS;
> -- 
> 2.24.1
> 

-- 
Peter Xu