Philippe Mathieu-Daudé <phi...@redhat.com> writes: > On 6/10/21 11:33 AM, Markus Armbruster wrote: >> Eric Blake <ebl...@redhat.com> writes: >> >>> On Wed, Jun 09, 2021 at 08:49:54PM +0200, Philippe Mathieu-Daudé wrote: >>>> In case we need to use QAPI types but no QAPI command / QAPI event >>>> actually use them, the generated qmp_marshal_output() function will >>>> trigger the compiler 'unused-function' warnings. >>>> To prevent that, emit these functions inlined: the compiler will >>>> ignore such unused functions. >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> >>>> --- >>>> RFC: No clue about QAPI... >>>> Tested with GCC. If the compiler is picky we could use the 'unused' >>>> function attribute. >>> >>> And I have no clue if clang will warn about an unused inline function. >>> Going with the compiler attribute seems safer and just as easy to do >>> in the same two-line change (remember, the "unused" attribute merely >>> means "suppress warnings if I don't use this", and not "warn me if I >>> use it in spite of calling it unused"). >>> >>>> --- >>>> scripts/qapi/commands.py | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py >>>> index 0e13d510547..bbed776a909 100644 >>>> --- a/scripts/qapi/commands.py >>>> +++ b/scripts/qapi/commands.py >>>> @@ -91,8 +91,8 @@ def gen_call(name: str, >>>> def gen_marshal_output(ret_type: QAPISchemaType) -> str: >>>> return mcgen(''' >>>> >>>> -static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, >>>> - QObject **ret_out, Error **errp) >>>> +static inline void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, >>>> + QObject **ret_out, Error **errp) >>> >>> On the other hand, the qapi generator is smart enough to only output >>> introspection data for qapi types that were actually used by a command >>> or event, so how is that working, and why is it not also being used to >>> elide the generation of unused qmp_marshal_output_FOO functions? This >>> is where I'll have to defer to Markus. >> >> This is a QAPI generator restriction. Let me explain. >> >> The qmp_marshal_output_T() are shared by all commands returning T. >> >> The commands may be conditional. The user is responsible for making T's >> 'if' the conjunction of the commands'. See the FIXME in commands.py. > > Yes, I noticed the FIXME: > > # FIXME: If T is a user-defined type, the user is responsible > # for making this work, i.e. to make T's condition the > # conjunction of the T-returning commands' conditions. If T > # is a built-in type, this isn't possible: the > # qmp_marshal_output_T() will be generated unconditionally. > > Using inline / unused attributes don't invalidate this :)
Generating the unused attribute lets us keep types unconditional even when the commands returning them are conditional (also takes care of the built-in case, where we cannot make the type conditional). However, conditional commands returning an unconditional type is a bit of a code smell. In this particular case, the smell seems to lead to a (minor) issue: too much TPM code is compiled even when CONFIG_TPM is off. With the attribute in place, we wouldn't have learned this. We may still find non-smelly instances of this pattern. Until then, I'm a bit reluctant to generate the attribute. >> If I do this for tpm.json (appended), then tpm.h misses TpmModel when >> CONFIG_TPM is off, and tpm_backend.h misses TpmType and TpmInfo. I >> suspect more TPM code needs to be guarded by CONFIG_TPM. > > Yes, this is what I did first, use the code below and add #ifdef'ry, > but the code becomes ugly and harder to maintain because the enums > are used in middle of a QOM interface structure: > > include/sysemu/tpm.h-37-struct TPMIfClass { > include/sysemu/tpm.h-38- InterfaceClass parent_class; > include/sysemu/tpm.h-39- > include/sysemu/tpm.h:40: enum TpmModel model; > include/sysemu/tpm.h-41- void (*request_completed)(TPMIf *obj, int ret); > include/sysemu/tpm.h-42- enum TPMVersion (*get_version)(TPMIf *obj); > include/sysemu/tpm.h-43-}; > include/sysemu/tpm.h-44- > > If you think using inline / unused attributes is not an option for > QAPI, then the #ifdef'ry isn't worth it and I'd prefer use v1 which > doesn't use conditional QAPI suggested by Marc-André. Ignorant question: why do we want to define QOM type "tpm-if" when CONFIG_TPM is off? [...]