On 6/10/21 1:06 PM, Markus Armbruster wrote: > 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.
I agree with your nose :) >>> 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? Good question. I suppose for historical reasons? Copy/pasting of another older include/sysemu/ files? Recently I saw these headers received more love, such better #ifdef'ry to allow code elision. I'll defer that to Stefan. Thanks for the review, Phil.