Artificially target-dependend compiles

Artificially target-dependend compiles

[Markus Armbruster]

Some .c need to be compiled per target.  build-system.rst calls these
target-dependent.

Consider hw/acpi/vmgenid.c.  hw/acpi/meson.build has

    acpi_ss.add(when: 'CONFIG_ACPI_VMGENID', if_true: files('vmgenid.c'))

and

    softmmu_ss.add_all(when: 'CONFIG_ACPI', if_true: acpi_ss)

softmmu_ss is target-independent, and so is hw/acpi/vmgenid.c.

The C macro CONFIG_ACPI_VMGENID is actually target-dependent;
config-poison.h has

    #pragma GCC poison CONFIG_ACPI_VMGENID

This feels odd until you think about it some.  Meson's
CONFIG_ACPI_VMGENID means "we need to compile hw/acpi/vmgenid.c (because
at least one target we're building needs it)".  We have no use for a C
macro with this meaning.  We sometimes want "*this* target needs it".
That's C macro CONFIG_ACPI_VMGENID.  It's unused, as far as I can tell.
Similar C macros exist that are used.


QMP command query-vm-generation-id exists regardless of either
CONFIG_ACPI_VMGENID.  For targets with CONFIG_ACPI_VMGENID, we link the
real handler from hw/acpi/vmgenid.c.  For other targets, we link the
stub from stubs/vmgenid.c, which always fails.

This works, but it's not introspectable.  To make it introspectable,
we'd have to make the command conditional.  Naive attempt:

    diff --git a/qapi/machine.json b/qapi/machine.json
    index 17794ef681..e554cac53d 100644
    --- a/qapi/machine.json
    +++ b/qapi/machine.json
    @@ -264,7 +264,8 @@
     #
     # Since: 2.9
     ##
    -{ 'struct': 'GuidInfo', 'data': {'guid': 'str'} }
    +{ 'struct': 'GuidInfo', 'data': {'guid': 'str'},
    +  'if': 'CONFIG_ACPI_VMGENID' }

     ##
     # @query-vm-generation-id:
    @@ -273,7 +274,8 @@
     #
     # Since: 2.9
     ##
    -{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' }
    +{ 'command': 'query-vm-generation-id', 'returns': 'GuidInfo',
    +  'if': 'CONFIG_ACPI_VMGENID' }

     ##
     # @system_reset:

No go, because CONFIG_ACPI_VMGENID can only be used in target-dependent
code.

Moving these definitions to machine-target.json moves the generated C
from qapi/qapi-*-machine.[ch] to qapi/qapi-*-machine-target.[ch], where
CONFIG_ACPI_VMGENID is okay.  It also makes qmp_query_vm_generation_id()
target-dependent: it needs qapi/qapi-commands-machine-target.h.

The target-dependence is completely artificial: compiling
hw/acpi/vmgenid.c just once is as fine as it ever was.

You might challenge the utility of making this one introspectable.  I'm
not going to debate; it's just a random example.  The problem exists
unless you can demonstrate that introspection is useless for *all*
commands with similar target-dependent stubbery.


Have you seen similar artificial target-dependence elsewhere?

Any smart ideas on avoiding it?

Re: Artificially target-dependend compiles

[Paolo Bonzini]

On 11/5/21 14:45, Markus Armbruster wrote:
> Moving these definitions to machine-target.json moves the generated C
> from qapi/qapi-*-machine.[ch] to qapi/qapi-*-machine-target.[ch], where
> CONFIG_ACPI_VMGENID is okay.  It also makes qmp_query_vm_generation_id()
> target-dependent: it needs qapi/qapi-commands-machine-target.h.

If I understand correctly, the problem that 
qapi-commands-machine-target.h is target-dependent, because it uses 
"#ifdef CONFIG_ACPI_VMGENID" around the prototype?

On one hand, the "#ifdef" is unnecessary: the prototype does not depend 
on anything target-specific.  Removing it will avoid the 
target-dependence.  On the other hand, the "#ifdef" has a defensive 
purpose, in that an unnecessary definition (such as the one currently in 
the stub) will fail due to the implicit definition of 
qmp_query_vm_generation_id().

> Have you seen similar artificial target-dependence elsewhere?

I can't think of a specific example, but it does ring some bells.

Paolo

Re: Artificially target-dependend compiles

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 11/5/21 14:45, Markus Armbruster wrote:
>> Moving these definitions to machine-target.json moves the generated C
>> from qapi/qapi-*-machine.[ch] to qapi/qapi-*-machine-target.[ch], where
>> CONFIG_ACPI_VMGENID is okay.  It also makes qmp_query_vm_generation_id()
>> target-dependent: it needs qapi/qapi-commands-machine-target.h.
>
> If I understand correctly, the problem that
> qapi-commands-machine-target.h is target-dependent, because it uses 
> "#ifdef CONFIG_ACPI_VMGENID" around the prototype?

Around the prototype and struct GuidInfo.

> On one hand, the "#ifdef" is unnecessary: the prototype does not
> depend on anything target-specific.  Removing it will avoid the 
> target-dependence.  On the other hand, the "#ifdef" has a defensive
> purpose, in that an unnecessary definition (such as the one currently
> in the stub) will fail due to the implicit definition of 
> qmp_query_vm_generation_id().

Also, it immediately flags uses of qmp_query_vm_generation_id() and
struct GuidInfo.  Without it, the linker still catches most, but not all
uses.

>> Have you seen similar artificial target-dependence elsewhere?
>
> I can't think of a specific example, but it does ring some bells.

I just ran into an instance that may be clearer.

The "rocker" device is target-independent (hw/net/meson.build adds it to
softmmu_ss), but linked only for selected targets (hw/net/Kconfig has
depends on PCI && MSI_NONBROKEN).

This makes our build machinery put CONFIG_ROCKER in
$TARGET-softmmu-config-devices.h, and poison it in config-poison.h.
Feels uncalled for.

Re: Artificially target-dependend compiles

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

[...]

> I just ran into an instance that may be clearer.
>
> The "rocker" device is target-independent (hw/net/meson.build adds it to
> softmmu_ss), but linked only for selected targets (hw/net/Kconfig has
> depends on PCI && MSI_NONBROKEN).
>
> This makes our build machinery put CONFIG_ROCKER in
> $TARGET-softmmu-config-devices.h, and poison it in config-poison.h.
> Feels uncalled for.

Hmm, maybe not.

Our build process links the rocker stuff for selected targets.

The QAPI schema provides rocker definitions unconditionally.
QAPI-generated rocker code gets linked for all targets.  The
command handlers resolve to the real ones when on the selected targets,
else to stubs.

This works and is fairly simple.  We link a bit of useless code
(QAPI-generated and stubs).  query-qmp-schema can't tell us whether
rocker is present, which is sad, but there's a work-around:
qom-list-types.

We may still run into cases where we really want query-qmp-schema to
tell, say because there is no easy work-around.

Making the QAPI schema definitions properly conditional does the trick,
but makes code artificially target-dependent, slowing down the build.
It can also lead to extra #ifdeffery, because now useless code doesn't
compile anymore.

Simply not poisoning the CONFIG_FOO when the FOO code is actually
target-independent avoids the target-dependency, but also messes up
introspection: new the FOO stuff is present for all targets when *any*
of them has it.  This cure feels worse than the disease.

Needs more thought.

Re: Artificially target-dependend compiles

[Paolo Bonzini]

On 11/8/21 09:09, Markus Armbruster wrote:
> 
> Simply not poisoning the CONFIG_FOO when the FOO code is actually
> target-independent avoids the target-dependency, but also messes up
> introspection: new the FOO stuff is present for all targets when*any*
> of them has it.  This cure feels worse than the disease.

Yeah, seems like a case of perfect being the enemy of good.  Full of 
corner cases and harder to get right.

Paolo

Re: Artificially target-dependend compiles

[Thomas Huth]

On 08/11/2021 09.09, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> [...]
> 
>> I just ran into an instance that may be clearer.
>>
>> The "rocker" device is target-independent (hw/net/meson.build adds it to
>> softmmu_ss), but linked only for selected targets (hw/net/Kconfig has
>> depends on PCI && MSI_NONBROKEN).
>>
>> This makes our build machinery put CONFIG_ROCKER in
>> $TARGET-softmmu-config-devices.h, and poison it in config-poison.h.
>> Feels uncalled for.
> 
> Hmm, maybe not.
> 
> Our build process links the rocker stuff for selected targets.
> 
> The QAPI schema provides rocker definitions unconditionally.
> QAPI-generated rocker code gets linked for all targets.  The
> command handlers resolve to the real ones when on the selected targets,
> else to stubs.
> 
> This works and is fairly simple.  We link a bit of useless code
> (QAPI-generated and stubs).  query-qmp-schema can't tell us whether
> rocker is present, which is sad, but there's a work-around:
> qom-list-types.
> 
> We may still run into cases where we really want query-qmp-schema to
> tell, say because there is no easy work-around.
> 
> Making the QAPI schema definitions properly conditional does the trick,
> but makes code artificially target-dependent, slowing down the build.
> It can also lead to extra #ifdeffery, because now useless code doesn't
> compile anymore.
> 
> Simply not poisoning the CONFIG_FOO when the FOO code is actually
> target-independent avoids the target-dependency, but also messes up
> introspection: new the FOO stuff is present for all targets when *any*
> of them has it.  This cure feels worse than the disease.
> 
> Needs more thought.

Hmm, we used to have a config-all-devices.mak file in the past (see commit 
a98006bc798169e which removed it), maybe we could re-introduce something 
similar again, but producing a config-all.h header file instead? So that 
this header file contains switches like CONFIG_ANY_ACPI_VMGENID and 
CONFIG_ANY_ROCKER that are set if any of the targets uses the device ... and 
these switches would not get poisoned in common code... ?

  Thomas

Re: Artificially target-dependend compiles

[Paolo Bonzini]

On 11/8/21 16:38, Thomas Huth wrote:
> 
> Hmm, we used to have a config-all-devices.mak file in the past (see 
> commit a98006bc798169e which removed it), maybe we could re-introduce 
> something similar again, but producing a config-all.h header file 
> instead? So that this header file contains switches like 
> CONFIG_ANY_ACPI_VMGENID and CONFIG_ANY_ROCKER that are set if any of the 
> targets uses the device ... and these switches would not get poisoned in 
> common code... ?

That would work, however the schema would still not provide any more 
information than it currently does.

Paolo

Re: Artificially target-dependend compiles

[Thomas Huth]

On 08/11/2021 17.23, Paolo Bonzini wrote:
> On 11/8/21 16:38, Thomas Huth wrote:
>>
>> Hmm, we used to have a config-all-devices.mak file in the past (see commit 
>> a98006bc798169e which removed it), maybe we could re-introduce something 
>> similar again, but producing a config-all.h header file instead? So that 
>> this header file contains switches like CONFIG_ANY_ACPI_VMGENID and 
>> CONFIG_ANY_ROCKER that are set if any of the targets uses the device ... 
>> and these switches would not get poisoned in common code... ?
> 
> That would work, however the schema would still not provide any more 
> information than it currently does.

Yeah, it's of limited use - you would only get a difference if none of the 
targets provided a feature (say if you did not build the x86 and arm 
targets, CONFIG_ANY_ACPI_VMGENID would likely not be set) ... not too 
useful, I guess.

  Thomas