Re: [PATCH 01/10] modules: Provide macros making it easier to identify module exports

[Christophe de Dinechin <dinechin@redhat.com>]

On 2020-06-29 at 12:13 CEST, Claudio Fontana wrote...
> Hello Christophe,
>
> On 6/26/20 6:42 PM, Christophe de Dinechin wrote:
>> In order to facilitate the move of large chunks of functionality to
>> load modules, it is simpler to create a wrapper with the same name
>> that simply relays the implementation. For efficiency, this is
>> typically done using inline functions in the header for the
>> corresponding functionality. In that case, we rename the actual
>> implementation by appending _implementation to its name. This makes it
>> easier to select which function you want to put a breakpoint on.
>>
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>>  include/qemu/module.h | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/include/qemu/module.h b/include/qemu/module.h
>> index 011ae1ae76..1922a0293c 100644
>> --- a/include/qemu/module.h
>> +++ b/include/qemu/module.h
>> @@ -39,6 +39,30 @@ static void __attribute__((constructor)) do_qemu_init_ ## function(void)    \
>>  }
>>  #endif
>>
>> +#ifdef CONFIG_MODULES
>> +/* Identify which functions are replaced by a callback stub */
>> +#ifdef MODULE_STUBS
>> +#define MODIFACE(Ret,Name,Args)                                         \
>> +    Ret (*Name)Args;                                                    \
>> +    extern Ret Name##_implementation Args
>> +#else /* !MODULE_STUBS */
>> +#define MODIFACE(Ret,Name,Args)                                         \
>> +    extern Ret (*Name)Args;                                             \
>> +    extern Ret Name##_implementation Args
>> +#endif /* MODULE_STUBS */
>> +
>> +#define MODIMPL(Ret,Name,Args)                                          \
>> +    static void __attribute__((constructor)) Name##_register(void)      \
>> +    {                                                                   \
>> +        Name = Name##_implementation;                                   \
>> +    }                                                                   \
>> +    Ret Name##_implementation Args
>> +#else /* !CONFIG_MODULES */
>> +/* When not using a module, such functions are called directly */
>> +#define MODIFACE(Ret,Name,Args)         Ret Name Args
>> +#define MODIMPL(Ret,Name,Args)          Ret Name Args
>> +#endif /* CONFIG_MODULES */
>> +
>>  typedef enum {
>>      MODULE_INIT_MIGRATION,
>>      MODULE_INIT_BLOCK,
>>
>
> Just as background, I am interested in all modules-related work, because of my long term plan to have target-specific modules as well:
>
>  https://lists.gnu.org/archive/html/qemu-devel/2020-05/msg04628.html
>
> I am not 100% clear on what is the goal and expected usage of this
> preprocessor code, despite the commit message, maybe you could clarify a
> bit with more verbosity?

Well, so far, the preference seems to be to go through a more verbose
approach with an explicit table of functions.

What the preprocessor code did was:

- If you build without modules, nothing changes, you get a direct call

- If you build with modules:

 + In the DSO, foo is replaced with foo_implementation
 + Elsewhere, foo is replaced with a function pointer also called foo.
 + The implementation adds constructor code that sets foo to point to foo_implementation


>
> Additionally if you happen to be interested, maybe you know already or
> could think about what this could mean for target-specific modules, which
> will require some improvements to the modules "subsystem"(?) as well.

So far, I've only integrated Gerd's workaround for target-specific
modules. Some additional mechanics is needed to name target-specific
modules, e.g. put them in some target directory.

>
> In my experimentation I didn't have to do this preprocessor work, instead
> I had to fine tune a bit the makefile support (rules.mak and makefiles) to
> be able to accomodate for (even large) modules in target/ as well.

It's probably because the modules you were dealing with already had the
required indirection and module_init calls, i.e. they were only invoked
using QOM already.

The mechanism I was proposing is to quickly add the indirection for
qemu functionality that does not have such indirect calls yet.

The consensus so far seems to be that the syntax I proposed is not nice, and
that it's better to make it more explicit through a table and indirect
calls, even if that means changing the call sites.

>
> Thanks!
>
> CLaudio


--
Cheers,
Christophe de Dinechin (IRC c3d)