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é <philmd@redhat.com>
---
RFC: No clue about QAPI...
Tested with GCC. If the compiler is picky we could use the 'unused'
function attribute.
---
 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)
 {
     Visitor *v;
 
-- 
2.31.1

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é <philmd@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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Eric Blake <eblake@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é <philmd@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.

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.



diff --git a/qapi/tpm.json b/qapi/tpm.json
index 09332e6f99..e74c881ea6 100644
--- a/qapi/tpm.json
+++ b/qapi/tpm.json
@@ -17,7 +17,9 @@
 #
 # Since: 1.5
 ##
-{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb', 'tpm-spapr' ] }
+{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb', 'tpm-spapr' ],
+  'if': 'defined(CONFIG_TPM)' }
+
 ##
 # @query-tpm-models:
 #
@@ -47,7 +49,8 @@
 #
 # Since: 1.5
 ##
-{ 'enum': 'TpmType', 'data': [ 'passthrough', 'emulator' ] }
+{ 'enum': 'TpmType', 'data': [ 'passthrough', 'emulator' ],
+  'if': 'defined(CONFIG_TPM)' }
 
 ##
 # @query-tpm-types:
@@ -124,7 +127,8 @@
 { 'struct': 'TPMInfo',
   'data': {'id': 'str',
            'model': 'TpmModel',
-           'options': 'TpmTypeOptions' } }
+           'options': 'TpmTypeOptions' },
+  'if': 'defined(CONFIG_TPM)' }
 
 ##
 # @query-tpm:

On 6/10/21 11:33 AM, Markus Armbruster wrote:
> Eric Blake <eblake@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é <philmd@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 :)

> 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é.

> diff --git a/qapi/tpm.json b/qapi/tpm.json
> index 09332e6f99..e74c881ea6 100644
> --- a/qapi/tpm.json
> +++ b/qapi/tpm.json
> @@ -17,7 +17,9 @@
>  #
>  # Since: 1.5
>  ##
> -{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb', 'tpm-spapr' ] }
> +{ 'enum': 'TpmModel', 'data': [ 'tpm-tis', 'tpm-crb', 'tpm-spapr' ],
> +  'if': 'defined(CONFIG_TPM)' }
> +
>  ##
>  # @query-tpm-models:
>  #
> @@ -47,7 +49,8 @@
>  #
>  # Since: 1.5
>  ##
> -{ 'enum': 'TpmType', 'data': [ 'passthrough', 'emulator' ] }
> +{ 'enum': 'TpmType', 'data': [ 'passthrough', 'emulator' ],
> +  'if': 'defined(CONFIG_TPM)' }
>  
>  ##
>  # @query-tpm-types:
> @@ -124,7 +127,8 @@
>  { 'struct': 'TPMInfo',
>    'data': {'id': 'str',
>             'model': 'TpmModel',
> -           'options': 'TpmTypeOptions' } }
> +           'options': 'TpmTypeOptions' },
> +  'if': 'defined(CONFIG_TPM)' }
>  
>  ##
>  # @query-tpm:
> 

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 6/10/21 11:33 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@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é <philmd@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?

[...]

On 6/10/21 1:06 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> On 6/10/21 11:33 AM, Markus Armbruster wrote:
>>> Eric Blake <eblake@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é <philmd@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.

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 6/10/21 1:06 PM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> 
>>> On 6/10/21 11:33 AM, Markus Armbruster wrote:
>>>> Eric Blake <eblake@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é <philmd@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.

Stefan, would you be willing to look into this?

On 6/11/21 4:02 AM, Markus Armbruster wrote:
> Stefan, would you be willing to look into this?
>
Have a look at the 3 topmost patches: 
https://github.com/stefanberger/qemu-tpm/commits/tpm-eliminate-if-not-config-tpm