Re: [Qemu-devel] [PATCH v5 3/6] target: [tcg] Add generic translation framework

["Lluís Vilanova" <vilanova@ac.upc.edu>]

Richard Henderson writes:

> On 12/28/2016 08:28 AM, Lluís Vilanova wrote:
>> +typedef enum DisasJumpType {
>> +    DJ_NEXT,
>> +    DJ_TOO_MANY,
>> +    DJ_TARGET,
>> +} DisasJumpType;

> I wonder if enums like DJ_TARGET_{0..N} wouldn't be better, rather than doing
> addition in the target-specific names.

I'm not sure. ARM has 12 target-specific flags (while x86 only has 2), and
adding so many "unspecified" values to the generic enum does not seem right to
me.

I you feel strongly against the current state, I'll change it; re-defining
existing enum values could, for example, have benefits in switch/case ranges and
compiler warnings.


>> +typedef struct DisasContextBase {
>> +    TranslationBlock *tb;
>> +    bool singlestep_enabled;
>> +    target_ulong pc_first;
>> +    target_ulong pc_next;
>> +    DisasJumpType jmp_type;
>> +    unsigned int num_insns;
>> +} DisasContextBase;

> Sort the bool to the end to minimize padding.

Done.


>> +/* Get first breakpoint matching a PC */
>> +static inline CPUBreakpoint *cpu_breakpoint_get(CPUState *cpu, vaddr pc,
>> +                                                CPUBreakpoint *bp)
>> +{
>> +    if (likely(bp == NULL)) {
>> +        if (unlikely(!QTAILQ_EMPTY(&cpu->breakpoints))) {
>> +            QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
>> +                if (bp->pc == pc) {
>> +                    return bp;
>> +                }
>> +            }
>> +        }
>> +    } else {
>> +        QTAILQ_FOREACH_CONTINUE(bp, entry) {
>> +            if (bp->pc == pc) {
>> +                return bp;
>> +            }
>> +        }
>> +    }
>> +    return NULL;
>> +}

> Any reason not to put the QTAILQ_FOREACH directly into gen_intermediate_code,
> rather than indirect it like this?  I don't see this abstraction as an
> improvement.

I thought this belonged here to maintain encapsulation of how breakpoint lists
are implemented (just as we already have insert/remove/test functions right
above this one).

Having it as a separate function (wherever it is) also helps readability. Again,
if you feel strongly against it, I can move that function into the translate
template header.


Thanks,
  Lluis