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