[Qemu-devel] Re: [PATCH v5 16/18] gdbstub: x86: Refactor register access

[Jan Kiszka]

Anthony Liguori wrote:
> Jan Kiszka wrote:
>>          return 10;
>> -    } else if (n >= CPU_NB_REGS + 24) {
>> -        n -= CPU_NB_REGS + 24;
>> -        if (n < CPU_NB_REGS) {
>> -            stq_p(mem_buf, env->xmm_regs[n].XMM_Q(0));
>> -            stq_p(mem_buf + 8, env->xmm_regs[n].XMM_Q(1));
>> -            return 16;
>> -        } else if (n == CPU_NB_REGS) {
>> -            GET_REG32(env->mxcsr);
>> -        } +    } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS +
>> CPU_NB_REGS) {
>> +        n -= IDX_XMM_REGS;
>> +        stq_p(mem_buf, env->xmm_regs[n].XMM_Q(0));
>> +        stq_p(mem_buf + 8, env->xmm_regs[n].XMM_Q(1));
>> +        return 16;
>>   
> 
> I think you have too much going on in this patch to review it properly. 
> It's not immediately obvious to me that this change results in identical
> code.

I would say this is not only because of the changes...

> 
>>      } else {
>> -        n -= CPU_NB_REGS;
>>          switch (n) {
>> -        case 0: GET_REGL(env->eip);
>> -        case 1: GET_REG32(env->eflags);
>> -        case 2: GET_REG32(env->segs[R_CS].selector);
>> -        case 3: GET_REG32(env->segs[R_SS].selector);
>> -        case 4: GET_REG32(env->segs[R_DS].selector);
>> -        case 5: GET_REG32(env->segs[R_ES].selector);
>> -        case 6: GET_REG32(env->segs[R_FS].selector);
>> -        case 7: GET_REG32(env->segs[R_GS].selector);
>> -        /* 8...15 x87 regs.  */
>> -        case 16: GET_REG32(env->fpuc);
>> -        case 17: GET_REG32((env->fpus & ~0x3800) | (env->fpstt & 0x7)
>> << 11);
>> -        case 18: GET_REG32(0); /* ftag */
>> -        case 19: GET_REG32(0); /* fiseg */
>> -        case 20: GET_REG32(0); /* fioff */
>> -        case 21: GET_REG32(0); /* foseg */
>> -        case 22: GET_REG32(0); /* fooff */
>> -        case 23: GET_REG32(0); /* fop */
>> -        /* 24+ xmm regs.  */
>> +        case IDX_IP_REG:    GET_REGL(env->eip);
>> +        case IDX_FLAGS_REG: GET_REG32(env->eflags);
>> +
>> +        case IDX_SEG_REGS:     GET_REG32(env->segs[R_CS].selector);
>> +        case IDX_SEG_REGS + 1: GET_REG32(env->segs[R_SS].selector);
>> +        case IDX_SEG_REGS + 2: GET_REG32(env->segs[R_DS].selector);
>> +        case IDX_SEG_REGS + 3: GET_REG32(env->segs[R_ES].selector);
>> +        case IDX_SEG_REGS + 4: GET_REG32(env->segs[R_FS].selector);
>> +        case IDX_SEG_REGS + 5: GET_REG32(env->segs[R_GS].selector);
>> +
>> +        case IDX_FP_REGS + 8:  GET_REG32(env->fpuc);
>> +        case IDX_FP_REGS + 9:  GET_REG32((env->fpus & ~0x3800) |
>> +                                         (env->fpstt & 0x7) << 11);
>> +        case IDX_FP_REGS + 10: GET_REG32(0); /* ftag */
>> +        case IDX_FP_REGS + 11: GET_REG32(0); /* fiseg */
>> +        case IDX_FP_REGS + 12: GET_REG32(0); /* fioff */
>> +        case IDX_FP_REGS + 13: GET_REG32(0); /* foseg */
>> +        case IDX_FP_REGS + 14: GET_REG32(0); /* fooff */
>> +        case IDX_FP_REGS + 15: GET_REG32(0); /* fop */
>> +
>> +        case IDX_MXCSR_REG: GET_REG32(env->mxcsr);
>>          }
>>      }
>>      return 0;
>>  }
>>  
>> -static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf,
>> int i)
>> +static int cpu_gdb_write_register(CPUState *env, uint8_t *mem_buf,
>> int n)
>>   
> 
> Is this rename really necessary?

It improves consistency of this code, readability ("Why was it called
'i' here, but 'n' above?").

> 
>>  #define LOAD_SEG(index, sreg)\
>>              tmp = ldl_p(mem_buf);\
>>              if (tmp != env->segs[sreg].selector)\
>> -                cpu_x86_load_seg(env, sreg, tmp);
>> +                cpu_x86_load_seg(env, sreg, tmp);\
>> +            return 4
>>  #else
>>  /* FIXME: Honor segment registers.  Needs to avoid raising an exception
>>     when the selector is invalid.  */
>> -#define LOAD_SEG(index, sreg) do {} while(0)
>> +#define LOAD_SEG(index, sreg) return 4
>>  #endif
>>   
> 
> I don't like the idea of hiding a return in a LOAD_SEG thing.  You would
> expect for these cases to fall through unless you look at the macro
> definition.

The macro fortunately dies with the next patch. So you may argue I
should leave that part alone and simply remove it later...

BTW, there are more of such macros. Changing them would have caused more
churn than I felt like I should cause.

> 
> Code cleanup patches are good, but please try and split them in such a
> way that they are easier to review.  Things like variable renames or
> introductions of #define's should be completely mechanical.  If you want
> to reswizzle code, please separate that into a separate patch.  That
> keeps the later smaller which requires a more fine review.

Well, this is always a trade-off: Leave the code as-is, just add the
functionality that I need right now, or also attempt to clean up what
caused confusing or nagged me otherwise. But the latter can easily cost
much more than the former. Up to a certain point I agree with your aim,
but up from a certain point of split up I would have to fall back to the
first approach due to time constraints.

So please let me know what you think this one should be split up into.

Jan

signature.asc
Description: OpenPGP digital signature