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

[Richard Henderson]

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;


r~