Richard Henderson writes: > On 03/09/2016 01:16 PM, Lluís Vilanova wrote: >> Richard Henderson writes: >> >>> On 03/09/2016 09:38 AM, Lluís Vilanova wrote: >>>> Hi, >>>> >>>> NOTE: I won't be throwing patches anytime soon, I just want to know if >>>> there's >>>> interest in this for the future. >>>> >>>> While adding events for tracing guest instructions, I've found that the >>>> per-target "gen_intermediate_code()" function is very similar but not >>>> exactly >>>> the same for each of the targets. This makes architecture-agnostic features >>>> harder to maintain across targets, specially when it comes to their >>>> relative >>>> order. >>>> >>>> So, would it be worth it if I generalized part of that code into an >>>> architecture-agnostic function that calls into target-specific hooks >>>> wherever it >>>> needs extending? There are many ways to do it that we can discuss later. >> >>> It's worth talking about, since I do believe it would make long-term >>> maintenance >>> across the targets easier. >> >>> These "target-specific hooks" probably ought not be "hooks" in the >>> traditional sense of attaching them to CPUState. I'd be more comfortable >>> with a >>> refactoring that used include files -- maybe .h or maybe .inc.c. If we do >>> the >>> normal sort of hook, then we've got to either expose DisasContext in places >>> we >>> shouldn't, or dynamically allocate it. Neither seems particularly >>> appealing. >> >> Great. I pondered about using QOM vs "template code", and I'm leaning towards >> the latter: >> >> * translate.c: >> >> struct DisasContextArch { >> DisasContext common; >> }; >> >> // implement "hooks" >> >> void gen_intermediate_code(CPUArchState *env, TranslationBlock *tb) >> { >> DisasContextArch dc; >> gen_intermediate_code_template(get_cpu(env), &dc.common, tb);
> Pointer down into "common" here... >> } >> >> * translate-template.h: >> >> struct DisasContext { /* ... */ }; >> >> void gen_intermediate_code_template(CPUState *cpu, DisasContext *dc, >> TranslationBlock *tb) >> { >> // init dc >> // arch-specific init dc hook >> >> // gen_icount, etc. >> while (true) { >> // generic processing code with calls to hooks > ... means you have to upcast to DisasContextArch here, or in the hooks > themselves. Yup, that was the intention. Hooks should then use "container_of()" to get their part. >> } >> } >> >> While initially simpler, the "template code" still feels a little dirty to >> me. With QOM, you could implement a DisasContextClass that provides the >> per-target hook pointers, which can be globally allocated once per target and >> pointed to by the DisasContext allocated in stack. >> >> I'm not sure about what you mean by exposing DisasContext in places it >> shouldn't >> be, though. > You either split DisasContextArch / DisasContext in "odd" ways, and then have > to > cast back and forth, or you have to expose the whole of DisasContextArch. It's > the latter that I considered inappropriate. Well, moving whatever is used in the generic code into DisasContext doesn't sound too odd as a first solution. > Alternately... can we broach the subject of C++? Honestly, it seems we work > too > hard sometimes to re-implement templates and classes in C. Whooo, I'd really *love* to switch to C++ just for templates and classes... But last time this was discussed, the idea wasn't met with much joy :) Cheers, Lluis