Re: [PATCH v3 08/20] gdbstub: extend GByteArray to read register helpers

[Damien Hedde]

Hi Alex,

On 12/11/19 6:05 PM, Alex Bennée wrote:
> Instead of passing a pointer to memory now just extend the GByteArray
> to all the read register helpers. They can then safely append their
> data through the normal way. We don't bother with this abstraction for
> write registers as we have already ensured the buffer being copied
> from is the correct size.
> 
> Signed-off-by: Alex Bennée <address@hidden>

[...]

> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 0ac950d6c71..6476245e789 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -47,30 +47,27 @@ static bool get_phys_addr_lpae(CPUARMState *env, 
> target_ulong address,
>  
>  static void switch_mode(CPUARMState *env, int mode);
>  
> -static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
> +static int vfp_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg)
>  {
>      int nregs;
>  
>      /* VFP data registers are always little-endian.  */
>      nregs = arm_feature(env, ARM_FEATURE_VFP3) ? 32 : 16;
>      if (reg < nregs) {
> -        stq_le_p(buf, *aa32_vfp_dreg(env, reg));
> -        return 8;
> +        return gdb_get_reg64(buf, *aa32_vfp_dreg(env, reg));

It was a little-endian version, you've put a target-endian version.
Is that what you meant ?

>      }
>      if (arm_feature(env, ARM_FEATURE_NEON)) {
>          /* Aliases for Q regs.  */
>          nregs += 16;
>          if (reg < nregs) {
>              uint64_t *q = aa32_vfp_qreg(env, reg - 32);
> -            stq_le_p(buf, q[0]);
> -            stq_le_p(buf + 8, q[1]);
> -            return 16;
> +            return gdb_get_reg128(buf, q[0], q[1]);

Ditto here.

>          }
>      }
>      switch (reg - nregs) {
> -    case 0: stl_p(buf, env->vfp.xregs[ARM_VFP_FPSID]); return 4;
> -    case 1: stl_p(buf, vfp_get_fpscr(env)); return 4;
> -    case 2: stl_p(buf, env->vfp.xregs[ARM_VFP_FPEXC]); return 4;
> +    case 0: return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPSID]); break;
> +    case 1: return gdb_get_reg32(buf, vfp_get_fpscr(env)); break;
> +    case 2: return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPEXC]); break;
>      }
>      return 0;
>  }
> @@ -101,7 +98,7 @@ static int vfp_gdb_set_reg(CPUARMState *env, uint8_t *buf, 
> int reg)
>      return 0;
>  }
>  
> -static int aarch64_fpu_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
> +static int aarch64_fpu_gdb_get_reg(CPUARMState *env, GByteArray *buf, int 
> reg)
>  {
>      switch (reg) {
>      case 0 ... 31:
> @@ -204,7 +201,7 @@ static void write_raw_cp_reg(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>      }
>  }
>  
> -static int arm_gdb_get_sysreg(CPUARMState *env, uint8_t *buf, int reg)
> +static int arm_gdb_get_sysreg(CPUARMState *env, GByteArray *buf, int reg)
>  {
>      ARMCPU *cpu = env_archcpu(env);
>      const ARMCPRegInfo *ri;

[...]

> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index 823759c92e7..6f08021cc22 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -114,10 +114,11 @@ void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t 
> *mem_buf, int len)
>   * the FP regs zero size when talking to a newer gdb.
>   */
>  
> -int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
> +int ppc_cpu_gdb_read_register(CPUState *cs, GByteArray *buf, int n)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *env = &cpu->env;
> +    uint8_t *mem_buf;
>      int r = ppc_gdb_register_len(n);
>  
>      if (!r) {
> @@ -126,17 +127,17 @@ int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t 
> *mem_buf, int n)
>  
>      if (n < 32) {
>          /* gprs */
> -        gdb_get_regl(mem_buf, env->gpr[n]);
> +        gdb_get_regl(buf, env->gpr[n]);
>      } else if (n < 64) {
>          /* fprs */
> -        stfq_p(mem_buf, *cpu_fpr_ptr(env, n - 32));
> +        gdb_get_reg64(buf, *cpu_fpr_ptr(env, n - 32));
>      } else {
>          switch (n) {
>          case 64:
> -            gdb_get_regl(mem_buf, env->nip);
> +            gdb_get_regl(buf, env->nip);
>              break;
>          case 65:
> -            gdb_get_regl(mem_buf, env->msr);
> +            gdb_get_regl(buf, env->msr);
>              break;
>          case 66:
>              {
> @@ -145,31 +146,33 @@ int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t 
> *mem_buf, int n)
>                  for (i = 0; i < 8; i++) {
>                      cr |= env->crf[i] << (32 - ((i + 1) * 4));
>                  }
> -                gdb_get_reg32(mem_buf, cr);
> +                gdb_get_reg32(buf, cr);
>                  break;
>              }
>          case 67:
> -            gdb_get_regl(mem_buf, env->lr);
> +            gdb_get_regl(buf, env->lr);
>              break;
>          case 68:
> -            gdb_get_regl(mem_buf, env->ctr);
> +            gdb_get_regl(buf, env->ctr);
>              break;
>          case 69:
> -            gdb_get_reg32(mem_buf, env->xer);
> +            gdb_get_reg32(buf, env->xer);
>              break;
>          case 70:
> -            gdb_get_reg32(mem_buf, env->fpscr);
> +            gdb_get_reg32(buf, env->fpscr);
>              break;
>          }
>      }
> +    mem_buf = buf->data - r;

Should it not be something more like this ?
mem_buf = buf->data + buf->len - r;

There seem to be the same issue below for every
ppc_maybe_bswap_register() call.

>      ppc_maybe_bswap_register(env, mem_buf, r);
>      return r;
>  }
>  
> -int ppc_cpu_gdb_read_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
> +int ppc_cpu_gdb_read_register_apple(CPUState *cs, GByteArray *buf, int n)
>  {
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *env = &cpu->env;
> +    uint8_t *mem_buf;
>      int r = ppc_gdb_register_len_apple(n);
>  
>      if (!r) {
> @@ -178,21 +181,21 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, 
> uint8_t *mem_buf, int n)
>  
>      if (n < 32) {
>          /* gprs */
> -        gdb_get_reg64(mem_buf, env->gpr[n]);
> +        gdb_get_reg64(buf, env->gpr[n]);
>      } else if (n < 64) {
>          /* fprs */
> -        stfq_p(mem_buf, *cpu_fpr_ptr(env, n - 32));
> +        gdb_get_reg64(buf, *cpu_fpr_ptr(env, n - 32));
>      } else if (n < 96) {
>          /* Altivec */
> -        stq_p(mem_buf, n - 64);
> -        stq_p(mem_buf + 8, 0);
> +        gdb_get_reg64(buf, n - 64);
> +        gdb_get_reg64(buf, 0);
>      } else {
>          switch (n) {
>          case 64 + 32:
> -            gdb_get_reg64(mem_buf, env->nip);
> +            gdb_get_reg64(buf, env->nip);
>              break;
>          case 65 + 32:
> -            gdb_get_reg64(mem_buf, env->msr);
> +            gdb_get_reg64(buf, env->msr);
>              break;
>          case 66 + 32:
>              {
> @@ -201,23 +204,24 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, 
> uint8_t *mem_buf, int n)
>                  for (i = 0; i < 8; i++) {
>                      cr |= env->crf[i] << (32 - ((i + 1) * 4));
>                  }
> -                gdb_get_reg32(mem_buf, cr);
> +                gdb_get_reg32(buf, cr);
>                  break;
>              }
>          case 67 + 32:
> -            gdb_get_reg64(mem_buf, env->lr);
> +            gdb_get_reg64(buf, env->lr);
>              break;
>          case 68 + 32:
> -            gdb_get_reg64(mem_buf, env->ctr);
> +            gdb_get_reg64(buf, env->ctr);
>              break;
>          case 69 + 32:
> -            gdb_get_reg32(mem_buf, env->xer);
> +            gdb_get_reg32(buf, env->xer);
>              break;
>          case 70 + 32:
> -            gdb_get_reg64(mem_buf, env->fpscr);
> +            gdb_get_reg64(buf, env->fpscr);
>              break;
>          }
>      }
> +    mem_buf = buf->data - r;
>      ppc_maybe_bswap_register(env, mem_buf, r);
>      return r;
>  }
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index ba726dec4d0..154f876e44c 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -9587,7 +9587,7 @@ static int gdb_find_spr_idx(CPUPPCState *env, int n)
>      return -1;
>  }
>  
> -static int gdb_get_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_spr_reg(CPUPPCState *env, GByteArray *buf, int n)
>  {
>      int reg;
>      int len;
> @@ -9598,8 +9598,8 @@ static int gdb_get_spr_reg(CPUPPCState *env, uint8_t 
> *mem_buf, int n)
>      }
>  
>      len = TARGET_LONG_SIZE;
> -    stn_p(mem_buf, len, env->spr[reg]);
> -    ppc_maybe_bswap_register(env, mem_buf, len);
> +    gdb_get_regl(buf, env->spr[reg]);
> +    ppc_maybe_bswap_register(env, buf->data - len, len);
>      return len;
>  }
>  
> @@ -9621,15 +9621,18 @@ static int gdb_set_spr_reg(CPUPPCState *env, uint8_t 
> *mem_buf, int n)
>  }
>  #endif
>  
> -static int gdb_get_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_float_reg(CPUPPCState *env, GByteArray *buf, int n)
>  {
> +    uint8_t *mem_buf;
>      if (n < 32) {
> -        stfq_p(mem_buf, *cpu_fpr_ptr(env, n));
> +        gdb_get_reg64(buf, *cpu_fpr_ptr(env, n));
> +        mem_buf = buf->data - 8;
>          ppc_maybe_bswap_register(env, mem_buf, 8);
>          return 8;
>      }
>      if (n == 32) {
> -        stl_p(mem_buf, env->fpscr);
> +        gdb_get_reg32(buf, env->fpscr);
> +        mem_buf = buf->data - 4;
>          ppc_maybe_bswap_register(env, mem_buf, 4);
>          return 4;
>      }
> @@ -9651,28 +9654,31 @@ static int gdb_set_float_reg(CPUPPCState *env, 
> uint8_t *mem_buf, int n)
>      return 0;
>  }
>  
> -static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_avr_reg(CPUPPCState *env, GByteArray *buf, int n)
>  {
> +    uint8_t *mem_buf;
> +
>      if (n < 32) {
>          ppc_avr_t *avr = cpu_avr_ptr(env, n);
>          if (!avr_need_swap(env)) {
> -            stq_p(mem_buf, avr->u64[0]);
> -            stq_p(mem_buf + 8, avr->u64[1]);
> +            gdb_get_reg128(buf, avr->u64[0] , avr->u64[1]);
>          } else {
> -            stq_p(mem_buf, avr->u64[1]);
> -            stq_p(mem_buf + 8, avr->u64[0]);
> +            gdb_get_reg128(buf, avr->u64[1] , avr->u64[0]);
>          }
> +        mem_buf = buf->data - 16;
>          ppc_maybe_bswap_register(env, mem_buf, 8);
>          ppc_maybe_bswap_register(env, mem_buf + 8, 8);
>          return 16;
>      }
>      if (n == 32) {
> -        stl_p(mem_buf, helper_mfvscr(env));
> +        gdb_get_reg32(buf, helper_mfvscr(env));
> +        mem_buf = buf->data - 4;
>          ppc_maybe_bswap_register(env, mem_buf, 4);>          return 4;
>      }
>      if (n == 33) {
> -        stl_p(mem_buf, (uint32_t)env->spr[SPR_VRSAVE]);
> +        gdb_get_reg32(buf, (uint32_t)env->spr[SPR_VRSAVE]);
> +        mem_buf = buf->data - 4;
>          ppc_maybe_bswap_register(env, mem_buf, 4);
>          return 4;
>      }
> @@ -9707,25 +9713,25 @@ static int gdb_set_avr_reg(CPUPPCState *env, uint8_t 
> *mem_buf, int n)
>      return 0;
>  }
>  
> -static int gdb_get_spe_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_spe_reg(CPUPPCState *env, GByteArray *buf, int n)
>  {
>      if (n < 32) {
>  #if defined(TARGET_PPC64)
> -        stl_p(mem_buf, env->gpr[n] >> 32);
> -        ppc_maybe_bswap_register(env, mem_buf, 4);
> +        gdb_get_reg32(buf, env->gpr[n] >> 32);
> +        ppc_maybe_bswap_register(env, buf->data - 4, 4);
>  #else
> -        stl_p(mem_buf, env->gprh[n]);
> +        gdb_get_reg32(buf, env->gprh[n]);
>  #endif
>          return 4;
>      }
>      if (n == 32) {
> -        stq_p(mem_buf, env->spe_acc);
> -        ppc_maybe_bswap_register(env, mem_buf, 8);
> +        gdb_get_reg64(buf, env->spe_acc);
> +        ppc_maybe_bswap_register(env, buf->data - 8, 8);
>          return 8;
>      }
>      if (n == 33) {
> -        stl_p(mem_buf, env->spe_fscr);
> -        ppc_maybe_bswap_register(env, mem_buf, 4);
> +        gdb_get_reg32(buf, env->spe_fscr);
> +        ppc_maybe_bswap_register(env, buf->data - 4, 4);
>          return 4;
>      }
>      return 0;
> @@ -9760,11 +9766,11 @@ static int gdb_set_spe_reg(CPUPPCState *env, uint8_t 
> *mem_buf, int n)
>      return 0;
>  }
>  
> -static int gdb_get_vsx_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_vsx_reg(CPUPPCState *env, GByteArray *buf, int n)
>  {
>      if (n < 32) {
> -        stq_p(mem_buf, *cpu_vsrl_ptr(env, n));
> -        ppc_maybe_bswap_register(env, mem_buf, 8);
> +        gdb_get_reg64(buf, *cpu_vsrl_ptr(env, n));
> +        ppc_maybe_bswap_register(env, buf->data - 8, 8);
>          return 8;
>      }
>      return 0;

Otherwise, other files seem ok.

Regards,
--
Damien