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

[Alex Bennée]

Damien Hedde <address@hidden> writes:

> 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 ?

Yes - I suspect this would have been broken if used by a big-endian
system. gdbstub generally (SVE excepted) wants things in target order.

>
>>      }
>>      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;

Good catch.

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

Fixed.


-- 
Alex Bennée