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

[Richard Henderson <richard.henderson@linaro.org>]

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~