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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 5259 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]