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