On Sat, Dec 12, 2020 at 11:00:03AM +0100, Claudio Fontana wrote:
> On 12/11/20 9:02 PM, Eduardo Habkost wrote:
> > On Fri, Dec 11, 2020 at 07:51:54PM +0100, Claudio Fontana wrote:
> >> On 12/11/20 7:26 PM, Philippe Mathieu-Daudé wrote:
> >>> On 12/11/20 7:22 PM, Richard Henderson wrote:
> >>>> On 12/11/20 12:15 PM, Claudio Fontana wrote:
> >>>>> Should I return this file to the original state (without the extra
> >>>>> #includes that pretend it to be a standalone header file,
> >>>>> and call it
> >>>>>
> >>>>> tcg-cpu-ops.h.inc
> >>>>>
> >>>>> ?
> >>>>
> >>>> If this header can work with qemu/typedefs.h, then no, because the
> >>>> circularity
> >>>> has been resolved. Otherwise, yes.
> >>>
> >>> My editor got confused with TranslationBlock, which is why I asked
> >>> to include its declaration.
> >>>
> >>> Easier to forward-declare TranslationBlock in qemu/typedefs.h?
> >>>
> >>> Regards,
> >>>
> >>> Phil.
> >>>
> >>
> >> Hello Philippe,
> >>
> >> ok you propose to move the existing fwd declaration of TranslationBlock
> >> from cpu.h to qemu/typedefs.h .
> >
> > It seems simpler to just add a
> >
> > typedef struct TranslationBlock TranslationBlock;
> >
> > line to tcg-cpu-ops.h.
> >
> > Or, an even simpler solution: just use `struct TranslationBlock`
> > instead of `TranslationBlock` in the declarations being moved to
> > tcg-cpu-ops.h.
> >
> > We don't need to move declarations to typedefs.h anymore, because
> > now the compilers we support don't warn about typedef
> > redefinitions:
> > https://lore.kernel.org/qemu-devel/20200914134636.gz1618...@habkost.net/
> >
> >
> >>
> >> And what about #include "exec/memattrs.h"?
> >>
> >> I assume you propose to put struct MemTxAttrs there as a fwd declaration
> >> too,
> >
> > This can't be done, because MemTxAttrs can't be an incomplete
> > type in the code you are moving (the methods get a MemTxAttrs
> > value, not a pointer).
>
>
>
> I'm confused now on what we are trying to do: if we want the
> file to be a "proper header" or just a TCG-ops-only convenience
> split of cpu.h.
Personally, I don't see the point of creating a new header if
it's not a proper header.
>
> I thought that we were only solving a highlighting issue in some editor
> (Philippe),
> and I wonder if these changes in qemu/typedef.h help with that?
>
> I tried adding both to qemu/typedef.h, and since cpu.h is the only user of
> the file, and it already includes memattrs.h, everything is fine.
>
> But here maybe you are proposing to make it a regular header, and include
> this instead of just hw/core/cpu.h in the targets?
>
> I am thinking whether it is the case to scrap this whole mess, make TCGCPUOps
> a pointer in CPUClass, and in the targets say for example:
>
> #include "tcg-cpu-ops.h"
>
> ...
>
> +static struct TCGCPUOps cris_tcg_ops = {
> + .initialize = cris_initialize_tcg,
> +};
> +
> static void cris_cpu_class_init(ObjectClass *oc, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(oc);
> @@ -284,7 +292,7 @@ static void cris_cpu_class_init(ObjectClass *oc, void
> *data)
> cc->gdb_stop_before_watchpoint = true;
>
> cc->disas_set_info = cris_disas_set_info;
> - cc->tcg_ops.initialize = cris_initialize_tcg;
> + cc->tcg_ops = &cris_tcg_ops;
> }
>
>
> What do you all think of this?
Making tcg_ops a pointer may make a lot of sense, but (as
mentioned in my reply to v12) I'm worried by the scope of this
series growing too much.
I suggest doing this refactor in smaller steps, to let us focus
in a single issue at a time. Instead of splitting the struct and
creating a new header file in a single patch, you can first
create the new struct in the same header, and worry about moving
it to a separate header later (in the same series, or in another
series).
--
Eduardo
