Re: [PATCH 3/5] target/i386/gdbstub: Factor out gdb_get_reg() and gdb_write_reg()

[Richard Henderson <richard.henderson@linaro.org>]

On 8/2/24 02:59, Ilya Leoshkevich wrote:
> @@ -248,6 +253,22 @@ static int x86_cpu_gdb_load_seg(X86CPU *cpu, X86Seg sreg, uint8_t *mem_buf)
>       return 4;
>   }
>   
> +static int gdb_write_reg(CPUX86State *env, uint8_t *mem_buf, target_ulong *val)
> +{
> +    if (TARGET_LONG_BITS == 64) {
> +        if (env->hflags & HF_CS64_MASK) {
> +            *val = ldq_p(mem_buf);
> +        } else {
> +            *val = ldq_p(mem_buf) & 0xffffffffUL;
> +        }
> +        return 8;
> +    } else {
> +        *val &= ~0xffffffffUL;
> +        *val |= (uint32_t)ldl_p(mem_buf);
> +        return 4;
> +    }
> +}
> +
>   int x86_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>   {
>       X86CPU *cpu = X86_CPU(cs);
> @@ -288,18 +309,7 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>       } else {
>           switch (n) {
>           case IDX_IP_REG:
> -            if (TARGET_LONG_BITS == 64) {
> -                if (env->hflags & HF_CS64_MASK) {
> -                    env->eip = ldq_p(mem_buf);
> -                } else {
> -                    env->eip = ldq_p(mem_buf) & 0xffffffffUL;
> -                }
> -                return 8;
> -            } else {
> -                env->eip &= ~0xffffffffUL;
> -                env->eip |= (uint32_t)ldl_p(mem_buf);
> -                return 4;
> -            }
> +            return gdb_write_reg(env, mem_buf, &env->eip);

Existing bug, but the insert in the !(TARGET_LONG_BITS == 64) case is silly.
Because TARGET_LONG_BITS == 32, target_ulong eip is 32-bits, so the "insert" doesn't 
really insert anything.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~