Re: [PATCH 05/22] plugins: Move function pointer in qemu_plugin_dyn_cb

[Pierrick Bouvier]

On 3/20/24 01:30, Richard Henderson wrote:
> On 3/19/24 03:18, Pierrick Bouvier wrote:
> > On 3/16/24 05:57, Richard Henderson wrote:
> > > The out-of-line function pointer is mutually exclusive
> > > with inline expansion, so move it into the union.
> > > Wrap the pointer in a structure named 'regular' to match
> > > PLUGIN_CB_REGULAR.
> > >
> > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > > ---
> > >    include/qemu/plugin.h  | 4 +++-
> > >    accel/tcg/plugin-gen.c | 4 ++--
> > >    plugins/core.c         | 8 ++++----
> > >    3 files changed, 9 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
> > > index 12a96cea2a..143262dca8 100644
> > > --- a/include/qemu/plugin.h
> > > +++ b/include/qemu/plugin.h
> > > @@ -84,13 +84,15 @@ enum plugin_dyn_cb_subtype {
> > >     * instance of a callback to be called upon the execution of a particular TB.
> > >     */
> > >    struct qemu_plugin_dyn_cb {
> > > -    union qemu_plugin_cb_sig f;
> > >        void *userp;
> > >        enum plugin_dyn_cb_subtype type;
> > >        /* @rw applies to mem callbacks only (both regular and inline) */
> > >        enum qemu_plugin_mem_rw rw;
> > >        /* fields specific to each dyn_cb type go here */
> > >        union {
> > > +        struct {
> > > +            union qemu_plugin_cb_sig f;
> > > +        } regular;
> > >            struct {
> > >                qemu_plugin_u64 entry;
> > >                enum qemu_plugin_op op;
> >
> > While we are cleaning this, maybe this could be only a union (moving rw and 
> > userp to
> > fields), and only type + union would be used.
> > Even if we duplicate userp in regular, and mem_cb, it would be much more 
> > readable.
> > For instance, userp is never used by inline operations.
>
> I was wondering about this.  But I was also thinking about your follow-on patch 
> set to add
> conditional calls: do we want a multiplicity of union members, or will we want 
> separate
> bits and pieces that can be strung together?
>
> E.g.
>
>       TCGCond cond;  /* ALWAYS, or compare entry vs imm. */
>
>       /* PLUGIN_CB_REGULAR_COND or PLUGIN_CB_INLINE_* */
>       qemu_plugin_u64 entry;
>       uint64_t imm;
>
>       /* PLUGIN_CB_REGULAR_* */
>       union qemu_plugin_cb_sig f;
>       void *userp;

In an ideal world:

struct regular_cb {
   // full data for regular cb
};

struct conditional_cb {
   // full data for cond cb
};

struct inline_op {
   // full data for inline op
};

...

and

struct qemu_plugin_dyn_cb {
   enum plugin_dyn_cb_subtype type;
   union {
       struct regular_cb regular;
       struct conditional_cb conditional;
       struct inline_op inline;
   };
};

It's the same as describing the structs directly inside union, but at 
least you can duplicate fields without thinking too much. In terms of 
memory layout it does not change anything, the upper bound is still the 
largest struct, whether you share fields or not.

In short, an algebraic data type. Hope it's not a bad word around here :)

> r~