| Message ID | 20200409164954.36902-3-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gdbstub: Unbreak i386 with gdb remote | expand |
On 4/9/20 6:49 PM, 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. You are correct. Fixes: a010bdbe719 ("extend GByteArray to read register helpers") Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Same problem in m68k_fpu_gdb_get_reg(). TODO for 5.1, rename mem_buf -> array. > > Cc: Alex Bennée <alex.bennee@linaro.org> > Cc: 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; >
Hi, Phil, On Thu, Apr 09, 2020 at 07:21:04PM +0200, Philippe Mathieu-Daudé wrote: > On 4/9/20 6:49 PM, 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. > > You are correct. > > Fixes: a010bdbe719 ("extend GByteArray to read register helpers") Oh I forgot to paste the fix line. However, is it b7b8756a9c ("target/i386: use gdb_get_reg helpers", 2020-03-17) instead? > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Thanks! > > Same problem in m68k_fpu_gdb_get_reg(). > > TODO for 5.1, rename mem_buf -> array.
On 4/9/20 8:01 PM, Peter Xu wrote: > Hi, Phil, > > On Thu, Apr 09, 2020 at 07:21:04PM +0200, Philippe Mathieu-Daudé wrote: >> On 4/9/20 6:49 PM, 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. >> >> You are correct. >> >> Fixes: a010bdbe719 ("extend GByteArray to read register helpers") > > Oh I forgot to paste the fix line. However, is it b7b8756a9c > ("target/i386: use gdb_get_reg helpers", 2020-03-17) instead? b7b8756a9c is correct, at that time the codebase was using the correct API. the next commit updated the API but missed to update the lines you are fixed. So I think "fixes a010bdbe719" is correct. > >> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Thanks! > >> >> Same problem in m68k_fpu_gdb_get_reg(). >> >> TODO for 5.1, rename mem_buf -> array. >
Peter Xu <peterx@redhat.com> writes: > 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> > Signed-off-by: Peter Xu <peterx@redhat.com> Queued to for-5.0/more-random-fixes, thanks. > --- > 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;
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;
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> Signed-off-by: Peter Xu <peterx@redhat.com> --- target/i386/gdbstub.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)