Re: [PATCH v1 13/28] target/i386: use gdb_get_reg helpers

[Philippe Mathieu-Daudé]

On 3/17/20 10:53 AM, Philippe Mathieu-Daudé wrote:
> On 3/16/20 6:21 PM, Alex Bennée wrote:
> > This is cleaner than poking memory directly and will make later
> > clean-ups easier.
> >
> > Signed-off-by: Alex Bennée <address@hidden>
> >
> > ---
> > v7
> >    - remove stray space
> >    - fixup the floatx80 set/get routines
> > ---
> >   target/i386/gdbstub.c | 27 +++++++++++----------------
> >   1 file changed, 11 insertions(+), 16 deletions(-)
> >
> > diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
> > index 572ead641ca..e4d8cb66c00 100644
> > --- a/target/i386/gdbstub.c
> > +++ b/target/i386/gdbstub.c
> > @@ -98,26 +98,22 @@ int x86_cpu_gdb_read_register(CPUState *cs, 
> > uint8_t *mem_buf, int n)
> >                   return gdb_get_reg64(mem_buf,
> >                                        env->regs[gpr_map[n]] & 
> > 0xffffffffUL);
> >               } else {
> > -                memset(mem_buf, 0, sizeof(target_ulong));
> > -                return sizeof(target_ulong);
> > +                return gdb_get_regl(mem_buf, 0);
> >               }
> >           } else {
> >               return gdb_get_reg32(mem_buf, env->regs[gpr_map32[n]]);
> >           }
> >       } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
> > -#ifdef USE_X86LDOUBLE
> > -        /* FIXME: byteswap float values - after fixing fpregs layout. */
> > -        memcpy(mem_buf, &env->fpregs[n - IDX_FP_REGS], 10);
> > -#else
> > -        memset(mem_buf, 0, 10);
> > -#endif
> > -        return 10;
> > +        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));
> > +        return len;
> >       } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
> >           n -= IDX_XMM_REGS;
> >           if (n < CPU_NB_REGS32 || TARGET_LONG_BITS == 64) {
> > -            stq_p(mem_buf, env->xmm_regs[n].ZMM_Q(0));
> > -            stq_p(mem_buf + 8, env->xmm_regs[n].ZMM_Q(1));
> > -            return 16;
> > +            return gdb_get_reg128(mem_buf,
> > +                                  env->xmm_regs[n].ZMM_Q(0),
> > +                                  env->xmm_regs[n].ZMM_Q(1));
> >           }
> >       } else {
> >           switch (n) {
> > @@ -290,10 +286,9 @@ int x86_cpu_gdb_write_register(CPUState *cs, 
> > uint8_t *mem_buf, int n)
> >               return 4;
> >           }
> >       } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
> > -#ifdef USE_X86LDOUBLE
> > -        /* FIXME: byteswap float values - after fixing fpregs layout. */
> > -        memcpy(&env->fpregs[n - IDX_FP_REGS], mem_buf, 10);
> > -#endif
> > +        floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
> > +        fp->low = le64_to_cpu(* (uint64_t *) mem_buf);
> > +        fp->high = le16_to_cpu(* (uint16_t *) (mem_buf + 8));

Note, checkpatch complains for both lines:
ERROR: space prohibited after that '*' (ctx:BxW)

> >           return 10;
> >       } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
> >           n -= IDX_XMM_REGS;
>
> I'd prefer both USE_X86LDOUBLE hunks in a separate patch, anyway:
>
> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>