Damien Hedde <damien.he...@greensocs.com> 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 <alex.ben...@linaro.org>
>
> [...]
>
>> 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
