Eric Blake <eblake@redhat.com> 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
>> 

On 15/02/19 08:53, Markus Armbruster wrote:
> 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?

It works because at this point obj-y is not used anymore, it is assigned
to all-obj-y a couple lines before:

	all-obj-y := $(obj-y)

As an aside, target-obj-y seems unnecessary to me.

> Perhaps unnest-vars could be more hygienic.

Macro hygiene and Make in the same sentence? (well, not sentence but
still...).

> 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?

No, there is no need for that.

Really the answer is that we are kind of pushing Makefiles to the limit
here.  We do get good expressiveness, but at the cost of hiding things
behind black magic.  In the end I think it's a net benefit, but the cost
does exist.

Paolo

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 15/02/19 08:53, Markus Armbruster wrote:
>> 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?
>
> It works because at this point obj-y is not used anymore, it is assigned
> to all-obj-y a couple lines before:
>
> 	all-obj-y := $(obj-y)

Confirms my findings.

> As an aside, target-obj-y seems unnecessary to me.

I have no idea :)

$ git-grep target-obj-y
Makefile.objs:target-obj-y += trace/
Makefile.target:target-obj-y :=
Makefile.target:dummy := $(call unnest-vars,,target-obj-y)
Makefile.target:target-obj-y-save := $(target-obj-y)
Makefile.target:target-obj-y := $(target-obj-y-save)
Makefile.target:all-obj-y += $(target-obj-y)
trace/Makefile.objs:target-obj-y += generated-helpers.o
trace/Makefile.objs:target-obj-y += control-target.o

>> Perhaps unnest-vars could be more hygienic.
>
> Macro hygiene and Make in the same sentence? (well, not sentence but
> still...).

You're right.  My "perhaps could" should be read like "Perhaps we could
make pigs fly, if we apply enough force, say with a trebuchet".

>> 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?
>
> No, there is no need for that.

Thanks!

> Really the answer is that we are kind of pushing Makefiles to the limit
> here.  We do get good expressiveness, but at the cost of hiding things
> behind black magic.  In the end I think it's a net benefit, but the cost
> does exist.

Concur.

It's actually not bad most of the time.  Once in a great while, I need
to do something more fancy, and then I'm prone to waste a few hours on
(re-)learning how to work with the magic.