[Qemu-devel] Proper use of unnest-vars (was: [PATCH v5 00/18] qapi: add #if pre-processor conditions to generated code (part 3))

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 2/14/19 9:24 AM, Markus Armbruster wrote:
>> Diff from v4:
>> 
>
>> +++ b/qapi/Makefile.objs
>> @@ -18,8 +18,9 @@ util-obj-y += $(QAPI_COMMON_MODULES:%=qapi-visit-%.o)
>>  util-obj-y += qapi-emit-events.o
>>  util-obj-y += $(QAPI_COMMON_MODULES:%=qapi-events-%.o)
>>  
>> -common-obj-y += $(QAPI_COMMON_MODULES:%=qapi-commands-%.o)
>> +common-obj-y = $(QAPI_COMMON_MODULES:%=qapi-commands-%.o)
>
> In other situations, use of = instead of += has resulted in inadvertent
> omission of files. Should we just always use +=, rather than having to
> remember to audit if the use of = is not overriding earlier lines?

There's quite some magic at work around here, which today I understand
better than last week (and probably worse than next week), but maybe not
well enough to get things quite right.  I'm cc'ing Paolo and Fam for
advice.

The Makefile.objs get included via the magical unnest-vars function.
Note that the file names in qapi/Makefile.objs are all relative to
qapi/, like:

    util-obj-y += qapi-emit-events.o

If we simply included this into the top level Makefile, this would be
wrong.  We'd have to say

    util-obj-y += qapi/qapi-emit-events.o

We don't, because unnest-vars takes care of adding the prefix.  How?
Let me show you with the help of this tracing patch:

diff --git a/Makefile.target b/Makefile.target
index d6ce549388..b2218011ef 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -167,7 +167,9 @@ GENERATED_FILES += hmp-commands.h hmp-commands-info.h
 
 endif # CONFIG_SOFTMMU
 
+$(info @target/ before $(obj-y))
 dummy := $(call unnest-vars,,obj-y)
+$(info @target/ after $(obj-y))
 all-obj-y := $(obj-y)
 
 target-obj-y :=
diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
index 87e4df1660..1789811769 100644
--- a/qapi/Makefile.objs
+++ b/qapi/Makefile.objs
@@ -1,3 +1,4 @@
+$(info @qapi/ initial $(obj-y))
 util-obj-y = qapi-visit-core.o qapi-dealloc-visitor.o qobject-input-visitor.o
 util-obj-y += qobject-output-visitor.o qmp-registry.o qmp-dispatch.o
 util-obj-y += string-input-visitor.o string-output-visitor.o
@@ -29,3 +30,4 @@ obj-y += $(QAPI_TARGET_MODULES:%=qapi-events-%.o)
 obj-y += qapi-events.o
 obj-y += $(QAPI_TARGET_MODULES:%=qapi-commands-%.o)
 obj-y += qapi-commands.o
+$(info @qapi/ final $(obj-y))

Output for me with V=1:

    make: Entering directory '/work/armbru/qemu/bld-x86'
[Eliding stuff that's redundant for the points I'm trying to make...]
    make[1]: Entering directory '/work/armbru/qemu/bld-x86/x86_64-softmmu'
    @target/ before exec.o accel/ tcg/tcg.o tcg/tcg-op.o tcg/tcg-op-vec.o 
tcg/tcg-op-gvec.o tcg/tcg-common.o tcg/optimize.o fpu/softfloat.o target/i386/ 
disas.o gdbstub-xml.o arch_init.o cpus.o monitor.o gdbstub.o balloon.o ioport.o 
numa.o qtest.o hw/ qapi/ memory.o memory_mapping.o dump.o win_dump.o 
migration/ram.o hw/i386/

This is right before $(call unnest-vars,,obj-y).  Note $(obj-y) is
non-blank.

$(call unnest-vars,,obj-y) now includes dir/Makefile.objs for all dir/
in $(obj-y).  In particular, it includes qapi/Makefile.objs.  Make
output continues:

    @qapi/ initial 

Now $(obj-y) is blank!  That's because unnest-vars makes it blank, so it
can more easily add the prefix later.

    @qapi/ final qapi-introspect.o qapi-types-target.o qapi-types.o 
qapi-visit-target.o qapi-visit.o qapi-events-target.o qapi-events.o 
qapi-commands-target.o qapi-commands.o

Makefile.objs did its thing.  Now unnest-vars works its magic:

    @target/ after exec.o tcg/tcg.o tcg/tcg-op.o tcg/tcg-op-vec.o 
tcg/tcg-op-gvec.o tcg/tcg-common.o tcg/optimize.o fpu/softfloat.o disas.o 
gdbstub-xml.o arch_init.o cpus.o monitor.o gdbstub.o balloon.o ioport.o numa.o 
qtest.o memory.o memory_mapping.o dump.o win_dump.o migration/ram.o 
accel/accel.o accel/kvm/kvm-all.o accel/stubs/hax-stub.o accel/stubs/hvf-stub.o 
accel/stubs/whpx-stub.o accel/tcg/tcg-all.o accel/tcg/cputlb.o 
accel/tcg/tcg-runtime.o accel/tcg/tcg-runtime-gvec.o accel/tcg/cpu-exec.o 
accel/tcg/cpu-exec-common.o accel/tcg/translate-all.o accel/tcg/translator.o 
hw/9pfs/virtio-9p-device.o hw/block/virtio-blk.o 
hw/block/dataplane/virtio-blk.o hw/block/dataplane/xen-block.o 
hw/char/virtio-serial-bus.o hw/display/vga.o hw/display/virtio-gpu.o 
hw/display/virtio-gpu-3d.o hw/display/virtio-gpu-pci.o hw/display/virtio-vga.o 
hw/hyperv/hyperv.o hw/hyperv/hyperv_testdev.o hw/intc/apic.o 
hw/intc/apic_common.o hw/intc/ioapic.o hw/isa/lpc_ich9.o hw/misc/ivshmem.o 
hw/misc/pvpanic.o hw/net/virtio-net.o hw/net/vhost_net.o hw/rdma/rdma_utils.o 
hw/rdma/rdma_backend.o hw/rdma/rdma_rm.o hw/rdma/vmw/pvrdma_dev_ring.o 
hw/rdma/vmw/pvrdma_cmd.o hw/rdma/vmw/pvrdma_qp_ops.o hw/rdma/vmw/pvrdma_main.o 
hw/scsi/virtio-scsi.o hw/scsi/virtio-scsi-dataplane.o 
hw/scsi/vhost-scsi-common.o hw/scsi/vhost-scsi.o hw/timer/mc146818rtc.o 
hw/tpm/tpm_ppi.o hw/vfio/common.o hw/vfio/spapr.o hw/vfio/pci.o 
hw/vfio/pci-quirks.o hw/vfio/display.o hw/virtio/virtio.o 
hw/virtio/virtio-balloon.o hw/virtio/virtio-crypto.o 
hw/virtio/virtio-crypto-pci.o hw/virtio/vhost.o hw/virtio/vhost-backend.o 
hw/virtio/vhost-user.o hw/virtio/vhost-vsock.o hw/virtio/vhost-vsock-pci.o 
hw/virtio/vhost-scsi-pci.o hw/virtio/virtio-input-host-pci.o 
hw/virtio/virtio-input-pci.o hw/virtio/virtio-rng-pci.o 
hw/virtio/virtio-balloon-pci.o hw/virtio/virtio-9p-pci.o 
hw/virtio/virtio-scsi-pci.o hw/virtio/virtio-blk-pci.o 
hw/virtio/virtio-net-pci.o hw/virtio/virtio-serial-pci.o 
hw/xen/xen-host-pci-device.o hw/xen/xen_pt.o hw/xen/xen_pt_config_init.o 
hw/xen/xen_pt_graphics.o hw/xen/xen_pt_msi.o hw/xen/xen_pt_load_rom.o 
hw/i386/multiboot.o hw/i386/pc.o hw/i386/pc_piix.o hw/i386/pc_q35.o 
hw/i386/pc_sysfw.o hw/i386/x86-iommu.o hw/i386/intel_iommu.o 
hw/i386/x86-iommu.o hw/i386/amd_iommu.o hw/i386/vmport.o hw/i386/vmmouse.o 
hw/i386/kvmvapic.o hw/i386/acpi-build.o hw/i386/../xenpv/xen_machine_pv.o 
hw/i386/kvm/clock.o hw/i386/kvm/apic.o hw/i386/kvm/i8259.o hw/i386/kvm/ioapic.o 
hw/i386/kvm/i8254.o hw/i386/xen/xen_platform.o hw/i386/xen/xen_apic.o 
hw/i386/xen/xen_pvdevice.o hw/i386/xen/xen-hvm.o hw/i386/xen/xen-mapcache.o 
qapi/qapi-introspect.o qapi/qapi-types-target.o qapi/qapi-types.o 
qapi/qapi-visit-target.o qapi/qapi-visit.o qapi/qapi-events-target.o 
qapi/qapi-events.o qapi/qapi-commands-target.o qapi/qapi-commands.o 
target/i386/helper.o target/i386/cpu.o target/i386/gdbstub.o 
target/i386/xsave_helper.o target/i386/translate.o target/i386/bpt_helper.o 
target/i386/cc_helper.o target/i386/excp_helper.o target/i386/fpu_helper.o 
target/i386/int_helper.o target/i386/mem_helper.o target/i386/misc_helper.o 
target/i386/mpx_helper.o target/i386/seg_helper.o target/i386/smm_helper.o 
target/i386/svm_helper.o target/i386/machine.o 
target/i386/arch_memory_mapping.o target/i386/arch_dump.o target/i386/monitor.o 
target/i386/kvm.o target/i386/hyperv.o target/i386/sev.o

This is

* All the non-directory names: exec.o ... migration/ram.o

* Stuff from directories: accel/ hw/ hw/i386/ qapi/ target/i386/.  Note
  that directories are reordered.

You can see how unnest-vars replaces each directory in $(obj-y) by the
value of $(obj-y) set in that directory's Makefile.objs, prefixed with
the directory.

The key insight to address Eric's concern is unnest-vars empties out
$(obj-y) before it includes a Makefile.objs.  Therefore, my "obj-y =" in
qapi/Makefile.objs can't clobber anything.

Having convinced you of this proposition, I'm now going to convince you
of its negation ;-P

A few lines down in Makefile.target, we have

    target-obj-y :=
    block-obj-y :=
    common-obj-y :=
    chardev-obj-y :=
    slirp-obj-y :=
    include $(SRC_PATH)/Makefile.objs
    dummy := $(call unnest-vars,,target-obj-y)
    target-obj-y-save := $(target-obj-y)
    dummy := $(call unnest-vars,.., \
                   block-obj-y \
                   block-obj-m \
                   chardev-obj-y \
                   crypto-obj-y \
                   crypto-aes-obj-y \
                   qom-obj-y \
                   io-obj-y \
                   common-obj-y \
                   common-obj-m \
                   slirp-obj-y)
    target-obj-y := $(target-obj-y-save)
    all-obj-y += $(common-obj-y)
    all-obj-y += $(target-obj-y)
    all-obj-y += $(qom-obj-y)
    all-obj-$(CONFIG_SOFTMMU) += $(block-obj-y) $(chardev-obj-y)
    all-obj-$(CONFIG_USER_ONLY) += $(crypto-aes-obj-y)
    all-obj-$(CONFIG_SOFTMMU) += $(crypto-obj-y)
    all-obj-$(CONFIG_SOFTMMU) += $(io-obj-y)
    all-obj-$(CONFIG_SOFTMMU) += $(slirp-obj-y)

$(call unnest-vars,,target-obj-y) is not interesting for us, as
$(target-obj-y) doesn't contain qapi/.  However, $(common-obj-y) does,
and the other $(call unnest-vars, ...) includes Makefile.objs again:

    @qapi/ initial exec.o tcg/tcg.o tcg/tcg-op.o tcg/tcg-op-vec.o 
tcg/tcg-op-gvec.o tcg/tcg-common.o tcg/optimize.o fpu/softfloat.o disas.o 
gdbstub-xml.o arch_init.o cpus.o monitor.o gdbstub.o balloon.o ioport.o numa.o 
qtest.o memory.o memory_mapping.o dump.o win_dump.o migration/ram.o 
accel/accel.o accel/kvm/kvm-all.o accel/stubs/hax-stub.o accel/stubs/hvf-stub.o 
accel/stubs/whpx-stub.o accel/tcg/tcg-all.o accel/tcg/cputlb.o 
accel/tcg/tcg-runtime.o accel/tcg/tcg-runtime-gvec.o accel/tcg/cpu-exec.o 
accel/tcg/cpu-exec-common.o accel/tcg/translate-all.o accel/tcg/translator.o 
hw/9pfs/virtio-9p-device.o hw/block/virtio-blk.o 
hw/block/dataplane/virtio-blk.o hw/block/dataplane/xen-block.o 
hw/char/virtio-serial-bus.o hw/display/vga.o hw/display/virtio-gpu.o 
hw/display/virtio-gpu-3d.o hw/display/virtio-gpu-pci.o hw/display/virtio-vga.o 
hw/hyperv/hyperv.o hw/hyperv/hyperv_testdev.o hw/intc/apic.o 
hw/intc/apic_common.o hw/intc/ioapic.o hw/isa/lpc_ich9.o hw/misc/ivshmem.o 
hw/misc/pvpanic.o hw/net/virtio-net.o hw/net/vhost_net.o hw/rdma/rdma_utils.o 
hw/rdma/rdma_backend.o hw/rdma/rdma_rm.o hw/rdma/vmw/pvrdma_dev_ring.o 
hw/rdma/vmw/pvrdma_cmd.o hw/rdma/vmw/pvrdma_qp_ops.o hw/rdma/vmw/pvrdma_main.o 
hw/scsi/virtio-scsi.o hw/scsi/virtio-scsi-dataplane.o 
hw/scsi/vhost-scsi-common.o hw/scsi/vhost-scsi.o hw/timer/mc146818rtc.o 
hw/tpm/tpm_ppi.o hw/vfio/common.o hw/vfio/spapr.o hw/vfio/pci.o 
hw/vfio/pci-quirks.o hw/vfio/display.o hw/virtio/virtio.o 
hw/virtio/virtio-balloon.o hw/virtio/virtio-crypto.o 
hw/virtio/virtio-crypto-pci.o hw/virtio/vhost.o hw/virtio/vhost-backend.o 
hw/virtio/vhost-user.o hw/virtio/vhost-vsock.o hw/virtio/vhost-vsock-pci.o 
hw/virtio/vhost-scsi-pci.o hw/virtio/virtio-input-host-pci.o 
hw/virtio/virtio-input-pci.o hw/virtio/virtio-rng-pci.o 
hw/virtio/virtio-balloon-pci.o hw/virtio/virtio-9p-pci.o 
hw/virtio/virtio-scsi-pci.o hw/virtio/virtio-blk-pci.o 
hw/virtio/virtio-net-pci.o hw/virtio/virtio-serial-pci.o 
hw/xen/xen-host-pci-device.o hw/xen/xen_pt.o hw/xen/xen_pt_config_init.o 
hw/xen/xen_pt_graphics.o hw/xen/xen_pt_msi.o hw/xen/xen_pt_load_rom.o 
hw/i386/multiboot.o hw/i386/pc.o hw/i386/pc_piix.o hw/i386/pc_q35.o 
hw/i386/pc_sysfw.o hw/i386/x86-iommu.o hw/i386/intel_iommu.o 
hw/i386/x86-iommu.o hw/i386/amd_iommu.o hw/i386/vmport.o hw/i386/vmmouse.o 
hw/i386/kvmvapic.o hw/i386/acpi-build.o hw/i386/../xenpv/xen_machine_pv.o 
hw/i386/kvm/clock.o hw/i386/kvm/apic.o hw/i386/kvm/i8259.o hw/i386/kvm/ioapic.o 
hw/i386/kvm/i8254.o hw/i386/xen/xen_platform.o hw/i386/xen/xen_apic.o 
hw/i386/xen/xen_pvdevice.o hw/i386/xen/xen-hvm.o hw/i386/xen/xen-mapcache.o 
qapi/qapi-introspect.o qapi/qapi-types-target.o qapi/qapi-types.o 
qapi/qapi-visit-target.o qapi/qapi-visit.o qapi/qapi-events-target.o 
qapi/qapi-events.o qapi/qapi-commands-target.o qapi/qapi-commands.o 
target/i386/helper.o target/i386/cpu.o target/i386/gdbstub.o 
target/i386/xsave_helper.o target/i386/translate.o target/i386/bpt_helper.o 
target/i386/cc_helper.o target/i386/excp_helper.o target/i386/fpu_helper.o 
target/i386/int_helper.o target/i386/mem_helper.o target/i386/misc_helper.o 
target/i386/mpx_helper.o target/i386/seg_helper.o target/i386/smm_helper.o 
target/i386/svm_helper.o target/i386/machine.o 
target/i386/arch_memory_mapping.o target/i386/arch_dump.o target/i386/monitor.o 
target/i386/kvm.o target/i386/hyperv.o target/i386/sev.o 9pfs/ acpi/ adc/ 
audio/ block/ bt/ char/ cpu/ display/ dma/ gpio/ hyperv/ i2c/ ide/ input/ intc/ 
ipack/ ipmi/ isa/ misc/ net/ rdma/ nvram/ pci/ pci-bridge/ pci-host/ pcmcia/ 
scsi/ sd/ ssi/ timer/ tpm/ usb/ vfio/ virtio/ watchdog/ xen/ mem/ smbios/ core/ 
9pfs/ acpi/ adc/ audio/ block/ bt/ char/ cpu/ display/ dma/ gpio/ hyperv/ i2c/ 
ide/ input/ intc/ ipack/ ipmi/ isa/ misc/ net/ rdma/ nvram/ pci/ pci-bridge/ 
pci-host/ pcmcia/ scsi/ sd/ ssi/ timer/ tpm/ usb/ vfio/ virtio/ watchdog/ xen/ 
mem/ smbios/ core/ virtio-9p-device.o virtio-blk.o dataplane/ 
virtio-serial-bus.o vga.o virtio-gpu.o virtio-gpu-3d.o virtio-gpu-pci.o 
virtio-vga.o hyperv.o hyperv_testdev.o apic.o apic_common.o ioapic.o lpc_ich9.o 
ivshmem.o pvpanic.o virtio-net.o vhost_net.o rdma_utils.o rdma_backend.o 
rdma_rm.o vmw/pvrdma_dev_ring.o vmw/pvrdma_cmd.o vmw/pvrdma_qp_ops.o 
vmw/pvrdma_main.o virtio-scsi.o virtio-scsi-dataplane.o vhost-scsi-common.o 
vhost-scsi.o mc146818rtc.o tpm_ppi.o common.o spapr.o pci.o pci-quirks.o 
display.o virtio.o virtio-balloon.o virtio-crypto.o virtio-crypto-pci.o vhost.o 
vhost-backend.o vhost-user.o vhost-vsock.o vhost-vsock-pci.o vhost-scsi-pci.o 
virtio-input-host-pci.o virtio-input-pci.o virtio-rng-pci.o 
virtio-balloon-pci.o virtio-9p-pci.o virtio-scsi-pci.o virtio-blk-pci.o 
virtio-net-pci.o virtio-serial-pci.o xen-host-pci-device.o xen_pt.o 
xen_pt_config_init.o xen_pt_graphics.o xen_pt_msi.o xen_pt_load_rom.o

This time, $(obj-y) is very much not blank, and...

    @qapi/ final qapi-introspect.o qapi-types-target.o qapi-types.o 
qapi-visit-target.o qapi-visit.o qapi-events-target.o qapi-events.o 
qapi-commands-target.o qapi-commands.o
    [Trailing make output elided]

... qapi/Makefile.obj-y *does* clobber it.  Oww.

How come this works anyway?

Consider what would happen if qapi/Makefile.objs used += as suggested by
Eric.  We'd end up with

    obj-y = exec.o [...] xen_pt_load_rom.o qapi-introspect.o [...] 
qapi-commands.o

which is just as wrong: the file names from qapi/Makefile.objs lack
their qapi/ prefix.

It works because $(obj-y) is not used!

Perhaps unnest-vars could be more hygienic.  But that's not my immediate
concern.  All I want to know right now is whether I should refrain from
= and := in Makefile.objs.  Paolo, Fam?

>> +obj-y = qapi-introspect.o
>
> and again
>
>>  obj-y += $(QAPI_TARGET_MODULES:%=qapi-types-%.o)
>>  obj-y += qapi-types.o
>>  obj-y += $(QAPI_TARGET_MODULES:%=qapi-visit-%.o)
>> @@ -28,4 +29,3 @@ obj-y += $(QAPI_TARGET_MODULES:%=qapi-events-%.o)
>>  obj-y += qapi-events.o
>>  obj-y += $(QAPI_TARGET_MODULES:%=qapi-commands-%.o)
>>  obj-y += qapi-commands.o
>> -obj-y += qapi-introspect.o
>>