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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

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.

> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
> index 8028786c7b..c56f104aee 100644
> --- a/accel/tcg/plugin-gen.c
> +++ b/accel/tcg/plugin-gen.c
> @@ -431,7 +431,7 @@ static TCGOp *append_udata_cb(const struct qemu_plugin_dyn_cb *cb,
>       }
>   
>       /* call */
> -    op = copy_call(&begin_op, op, cb->f.vcpu_udata, cb_idx);
> +    op = copy_call(&begin_op, op, cb->regular.f.vcpu_udata, cb_idx);
>   
>       return op;
>   }
> @@ -479,7 +479,7 @@ static TCGOp *append_mem_cb(const struct qemu_plugin_dyn_cb *cb,
>   
>       if (type == PLUGIN_GEN_CB_MEM) {
>           /* call */
> -        op = copy_call(&begin_op, op, cb->f.vcpu_udata, cb_idx);
> +        op = copy_call(&begin_op, op, cb->regular.f.vcpu_udata, cb_idx);
>       }
>   
>       return op;
> diff --git a/plugins/core.c b/plugins/core.c
> index 4487cb7c48..837c373690 100644
> --- a/plugins/core.c
> +++ b/plugins/core.c
> @@ -342,7 +342,7 @@ void plugin_register_dyn_cb__udata(GArray **arr,
>   
>       dyn_cb->userp = udata;
>       /* Note flags are discarded as unused. */
> -    dyn_cb->f.vcpu_udata = cb;
> +    dyn_cb->regular.f.vcpu_udata = cb;
>       dyn_cb->type = PLUGIN_CB_REGULAR;
>   }
>   
> @@ -359,7 +359,7 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
>       /* Note flags are discarded as unused. */
>       dyn_cb->type = PLUGIN_CB_REGULAR;
>       dyn_cb->rw = rw;
> -    dyn_cb->f.generic = cb;
> +    dyn_cb->regular.f.vcpu_mem = cb;
>   }
>   
>   /*
> @@ -511,8 +511,8 @@ void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
>           }
>           switch (cb->type) {
>           case PLUGIN_CB_REGULAR:
> -            cb->f.vcpu_mem(cpu->cpu_index, make_plugin_meminfo(oi, rw),
> -                           vaddr, cb->userp);
> +            cb->regular.f.vcpu_mem(cpu->cpu_index, make_plugin_meminfo(oi, rw),
> +                                   vaddr, cb->userp);
>               break;
>           case PLUGIN_CB_INLINE:
>               exec_inline_op(cb, cpu->cpu_index);