[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 3/6] target: [tcg] Add generic translation fr
From: |
Lluís Vilanova |
Subject: |
Re: [Qemu-devel] [PATCH v5 3/6] target: [tcg] Add generic translation framework |
Date: |
Mon, 16 Jan 2017 16:41:22 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
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