[Qemu-devel] [PATCH v3 0/2] make: Cleanup and fix of loading of dependency info

[Qemu-devel] [PATCH v3 0/2] make: Cleanup and fix of loading of dependency info

[Victor Kaplansky]

Changes from v2:

    Address comment by Paolo Bonzini:
        - Store list of generated hex files in a variable and derive from it
          list of dependences to be included.

    Address comment by Alex Bennee and Michael S. Tsirkin:
        - Add a comment about difference between $(@D) and $(*D) in gnu make.
        - Better explanation why touching of some *.dsl sources is necessary.


make build can fail when one switches between commits without running
"make clean".

This is caused by loading old *.d dependency info files and is harmful for
autogenerated sources with their own includes.  This situation may
significantly slow down the process of git bisect.

These two patches clean things up and fix the issue both for further versions,
and between old and new commits.

This also replaces my previous patch "[PATCH] make: explicit dependencies for
ACPI gen sources".

I've tested the fix by validating that lists included by previous "*.d"
approach and new "patsubst" approach are identical.



Victor Kaplansky (2):
  make: fix where dependency *.d are stored.
  make: load only required dependency files.

 hw/i386/Makefile.objs     | 8 +++++++-
 hw/i386/acpi-dsdt.dsl     | 1 -
 hw/i386/q35-acpi-dsdt.dsl | 1 +
 rules.mak                 | 4 ++--
 4 files changed, 10 insertions(+), 4 deletions(-)

-- 
--Victor

[Qemu-devel] [PATCH v3 1/2] make: fix where dependency *.d are stored.

[Victor Kaplansky]

In rules like "bar/%.o: %.c" there is a difference between $(*D) and
$(@D). $(*D) expands to '.', while $(@D) expands to 'bar'.  It is
cleaner to generate *.d in the same directory where appropriate *.o
resides. This allows precise including of dependency info from .d files.

As a hack, we also touch two sources for generated *.hex files.  Without
this hack, anyone doing "git pull; make" will not get *.hex rebuilt
correctly since the dependency file would be missing.

Signed-off-by: Victor Kaplansky <victork@redhat.com>
---
 hw/i386/acpi-dsdt.dsl     | 1 -
 hw/i386/q35-acpi-dsdt.dsl | 1 +
 rules.mak                 | 2 +-
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
index a2d84ec..8dba096 100644
--- a/hw/i386/acpi-dsdt.dsl
+++ b/hw/i386/acpi-dsdt.dsl
@@ -43,7 +43,6 @@ DefinitionBlock (
 
 #include "acpi-dsdt-hpet.dsl"
 
-
 /****************************************************************
  * PIIX4 PM
  ****************************************************************/
diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
index 16eaca3..7be7b37 100644
--- a/hw/i386/q35-acpi-dsdt.dsl
+++ b/hw/i386/q35-acpi-dsdt.dsl
@@ -22,6 +22,7 @@
  * Based on acpi-dsdt.dsl, but heavily modified for q35 chipset.
  */
 
+
 ACPI_EXTRACT_ALL_CODE Q35AcpiDsdtAmlCode
 
 DefinitionBlock (
diff --git a/rules.mak b/rules.mak
index aec27f8..6e35c36 100644
--- a/rules.mak
+++ b/rules.mak
@@ -17,7 +17,7 @@ MAKEFLAGS += -rR
 QEMU_CXXFLAGS = -D__STDC_LIMIT_MACROS $(filter-out -Wstrict-prototypes -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls, $(QEMU_CFLAGS))
 
 # Flags for dependency generation
-QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(*D)/$(*F).d
+QEMU_DGFLAGS += -MMD -MP -MT $@ -MF $(@D)/$(*F).d
 
 # Same as -I$(SRC_PATH) -I., but for the nested source/object directories
 QEMU_INCLUDES += -I$(<D) -I$(@D)
-- 
--Victor

[Qemu-devel] [PATCH v3 2/2] make: load only required dependency files.

[Victor Kaplansky]

The old rules.mak loads dependency .d files using include directive
with file glob pattern "*.d". This breaks the build when build tree has
remanent *.d files from another build.

This patch fixes this by
  - loading precise list of .d files made from *.o and *.mo.
  - specifying explicit list of required dependency info files for
     *.hex autogenerated sources.

Note that Makefile still includes some .d in rood directory by including
"*.d".

Signed-off-by: Victor Kaplansky <victork@redhat.com>
---
 hw/i386/Makefile.objs | 8 +++++++-
 rules.mak             | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index bd4f147..ecdb400 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -7,8 +7,14 @@ obj-$(CONFIG_XEN) += ../xenpv/ xen/
 
 obj-y += kvmvapic.o
 obj-y += acpi-build.o
+
+gen-hex-y += hw/i386/acpi-dsdt.hex
+gen-hex-y += hw/i386/q35-acpi-dsdt.hex
+
 hw/i386/acpi-build.o: hw/i386/acpi-build.c \
-	hw/i386/acpi-dsdt.hex hw/i386/q35-acpi-dsdt.hex
+	$(gen-hex-y)
+
+-include $(gen-hex-y:.hex=.d)
 
 iasl-option=$(shell if test -z "`$(1) $(2) 2>&1 > /dev/null`" \
     ; then echo "$(2)"; else echo "$(3)"; fi ;)
diff --git a/rules.mak b/rules.mak
index 6e35c36..4551b9e 100644
--- a/rules.mak
+++ b/rules.mak
@@ -368,6 +368,6 @@ define unnest-vars
                 $(error $o added in $v but $o-objs is not set)))
         $(shell mkdir -p ./ $(sort $(dir $($v))))
         # Include all the .d files
-        $(eval -include $(addsuffix *.d, $(sort $(dir $($v)))))
+        $(eval -include $(patsubst %.o,%.d,$(patsubst %.mo,%.d,$($v))))
         $(eval $v := $(filter-out %/,$($v))))
 endef
-- 
--Victor

Re: [Qemu-devel] [PATCH v3 2/2] make: load only required dependency files.

[Michael S. Tsirkin]

On Sun, Aug 09, 2015 at 12:39:59PM +0300, Victor Kaplansky wrote:
> The old rules.mak loads dependency .d files using include directive
> with file glob pattern "*.d". This breaks the build when build tree has
> remanent *.d files from another build.
> 
> This patch fixes this by
>   - loading precise list of .d files made from *.o and *.mo.
>   - specifying explicit list of required dependency info files for
>      *.hex autogenerated sources.
> 
> Note that Makefile still includes some .d in rood directory by including
> "*.d".
> 
> Signed-off-by: Victor Kaplansky <victork@redhat.com>
> ---
>  hw/i386/Makefile.objs | 8 +++++++-
>  rules.mak             | 2 +-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index bd4f147..ecdb400 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -7,8 +7,14 @@ obj-$(CONFIG_XEN) += ../xenpv/ xen/
>  
>  obj-y += kvmvapic.o
>  obj-y += acpi-build.o
> +
> +gen-hex-y += hw/i386/acpi-dsdt.hex
> +gen-hex-y += hw/i386/q35-acpi-dsdt.hex
> +
>  hw/i386/acpi-build.o: hw/i386/acpi-build.c \
> -	hw/i386/acpi-dsdt.hex hw/i386/q35-acpi-dsdt.hex
> +	$(gen-hex-y)
> +
> +-include $(gen-hex-y:.hex=.d)
>  
>  iasl-option=$(shell if test -z "`$(1) $(2) 2>&1 > /dev/null`" \
>      ; then echo "$(2)"; else echo "$(3)"; fi ;)
> diff --git a/rules.mak b/rules.mak
> index 6e35c36..4551b9e 100644
> --- a/rules.mak
> +++ b/rules.mak
> @@ -368,6 +368,6 @@ define unnest-vars
>                  $(error $o added in $v but $o-objs is not set)))
>          $(shell mkdir -p ./ $(sort $(dir $($v))))
>          # Include all the .d files
> -        $(eval -include $(addsuffix *.d, $(sort $(dir $($v)))))
> +        $(eval -include $(patsubst %.o,%.d,$(patsubst %.mo,%.d,$($v))))
>          $(eval $v := $(filter-out %/,$($v))))
>  endef

Please add space after each comma.

> -- 
> --Victor

Re: [Qemu-devel] [PATCH v3 2/2] make: load only required dependency files.

[Peter Maydell]

On 9 August 2015 at 12:39, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, Aug 09, 2015 at 12:39:59PM +0300, Victor Kaplansky wrote:
>> -        $(eval -include $(addsuffix *.d, $(sort $(dir $($v)))))
>> +        $(eval -include $(patsubst %.o,%.d,$(patsubst %.mo,%.d,$($v))))
>>          $(eval $v := $(filter-out %/,$($v))))
>>  endef
>
> Please add space after each comma.

(I thought we discussed this in v1 review?)

We don't seem to be completely consistent in our makefile
style, but I would say that the majority of it is written
in the same style used in the GNU Make docs, with no spaces
after commas.

In particular, $(patsubst %.o,%.d,$(THING)) and
$(patsubst %.o, %.d, $(THING)) do *not* expand to the same thing;
Make syntax for functions says the delimiter is 'comma', not
'comma followed by whitespace':

FOO=bar.o baz.o boz.o

all:
    @echo '$(patsubst %.o,%.d,$(FOO))'
    @echo '$(patsubst %.o, %.d, $(FOO))'
manooth$ make -f test.mak
bar.d baz.d boz.d
 bar.d  baz.d  boz.d

Admittedly most of the time the extra whitespace that ends
up in the output is unlikely to be problematic, but there
might be rare cases when it is, so the better style
recommendation is to not have spaces after the commas,
because that is always safe and always the behaviour
the author actually wanted.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 2/2] make: load only required dependency files.

[Michael S. Tsirkin]

On Sun, Aug 09, 2015 at 12:54:37PM +0100, Peter Maydell wrote:
> On 9 August 2015 at 12:39, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Aug 09, 2015 at 12:39:59PM +0300, Victor Kaplansky wrote:
> >> -        $(eval -include $(addsuffix *.d, $(sort $(dir $($v)))))
> >> +        $(eval -include $(patsubst %.o,%.d,$(patsubst %.mo,%.d,$($v))))
> >>          $(eval $v := $(filter-out %/,$($v))))
> >>  endef
> >
> > Please add space after each comma.
> 
> (I thought we discussed this in v1 review?)
> 
> We don't seem to be completely consistent in our makefile
> style, but I would say that the majority of it is written
> in the same style used in the GNU Make docs, with no spaces
> after commas.
> 
> In particular, $(patsubst %.o,%.d,$(THING)) and
> $(patsubst %.o, %.d, $(THING)) do *not* expand to the same thing;
> Make syntax for functions says the delimiter is 'comma', not
> 'comma followed by whitespace':
> 
> FOO=bar.o baz.o boz.o
> 
> all:
>     @echo '$(patsubst %.o,%.d,$(FOO))'
>     @echo '$(patsubst %.o, %.d, $(FOO))'
> manooth$ make -f test.mak
> bar.d baz.d boz.d
>  bar.d  baz.d  boz.d
> 
> Admittedly most of the time the extra whitespace that ends
> up in the output is unlikely to be problematic, but there
> might be rare cases when it is, so the better style
> recommendation is to not have spaces after the commas,
> because that is always safe and always the behaviour
> the author actually wanted.
> 
> thanks
> -- PMM

OK, I'll merge this as is, we can tweak this with a patch on top if
desired.

Still - just corious about the motivation.
Extra whitespace in input -> extra whitespace in output, should
be harmless in both cases, should it not? E.g. do we prefer

    A\
B

or

	A \
	B
?

First option never adds whitespace, but second one is more readable.
I'd argue if you write code where extra whitespace breaks things you are
doing something wrong.


-- 
MST

Re: [Qemu-devel] [PATCH v3 2/2] make: load only required dependency files.

[Peter Maydell]

On 9 August 2015 at 13:20, Michael S. Tsirkin <mst@redhat.com> wrote:
> Still - just corious about the motivation.
> Extra whitespace in input -> extra whitespace in output, should
> be harmless in both cases, should it not?

Yeah, I agree that it's generally harmless. My rationale for
preferring no-space-after-comma is that it's less misleading:
if you're used to other languages where space-after-comma has
no meaning at all, then it's easy to think that the two are
identical when they're not.

End-of-line-continuations are I think slightly different
because the fact that the space remains in the result is
a bit more obvious (the continuation just folds things).

-- PMM

Re: [Qemu-devel] [PATCH v3 2/2] make: load only required dependency files.

[Michael S. Tsirkin]

On Mon, Aug 10, 2015 at 11:14:50AM +0100, Peter Maydell wrote:
> On 9 August 2015 at 13:20, Michael S. Tsirkin <mst@redhat.com> wrote:
> > Still - just corious about the motivation.
> > Extra whitespace in input -> extra whitespace in output, should
> > be harmless in both cases, should it not?
> 
> Yeah, I agree that it's generally harmless. My rationale for
> preferring no-space-after-comma is that it's less misleading:
> if you're used to other languages where space-after-comma has
> no meaning at all, then it's easy to think that the two are
> identical when they're not.
> 
> End-of-line-continuations are I think slightly different
> because the fact that the space remains in the result is
> a bit more obvious (the continuation just folds things).
> 
> -- PMM

Fair enough, thanks for the clarification.

Re: [Qemu-devel] [PATCH v3 2/2] make: load only required dependency files.

[Victor Kaplansky]

On Sun, Aug 09, 2015 at 12:54:37PM +0100, Peter Maydell wrote:
> On 9 August 2015 at 12:39, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, Aug 09, 2015 at 12:39:59PM +0300, Victor Kaplansky wrote:
> >> -        $(eval -include $(addsuffix *.d, $(sort $(dir $($v)))))
> >> +        $(eval -include $(patsubst %.o,%.d,$(patsubst %.mo,%.d,$($v))))
> >>          $(eval $v := $(filter-out %/,$($v))))
> >>  endef
> >
> > Please add space after each comma.
> 
> (I thought we discussed this in v1 review?)
> 
> We don't seem to be completely consistent in our makefile
> style, but I would say that the majority of it is written
> in the same style used in the GNU Make docs, with no spaces
> after commas.

Looking into qemu sources I see that most patsubst patterns are written
without spaces after first comma.  About the same situation is in linux
kernel sources.  So, as for now, the style recommendation probably would
be not to use spaces, but spaces after comma are acceptable.

If we want to change this style recommendation, we can create another
cosmetic patch addressing the change in style.

-- Victor

Re: [Qemu-devel] [PATCH v3 0/2] make: Cleanup and fix of loading of dependency info

[Michael S. Tsirkin]

On Sun, Aug 09, 2015 at 12:39:45PM +0300, Victor Kaplansky wrote:
> Changes from v2:
> 
>     Address comment by Paolo Bonzini:
>         - Store list of generated hex files in a variable and derive from it
>           list of dependences to be included.
> 
>     Address comment by Alex Bennee and Michael S. Tsirkin:
>         - Add a comment about difference between $(@D) and $(*D) in gnu make.
>         - Better explanation why touching of some *.dsl sources is necessary.
> 
> 
> make build can fail when one switches between commits without running
> "make clean".
> 
> This is caused by loading old *.d dependency info files and is harmful for
> autogenerated sources with their own includes.  This situation may
> significantly slow down the process of git bisect.
> 
> These two patches clean things up and fix the issue both for further versions,
> and between old and new commits.
> 
> This also replaces my previous patch "[PATCH] make: explicit dependencies for
> ACPI gen sources".
> 
> I've tested the fix by validating that lists included by previous "*.d"
> approach and new "patsubst" approach are identical.
> 

Looks good to me, though there's a minor coding style nit in patch 2/2.

Anyone has objections to merge this through my tree?

> 
> Victor Kaplansky (2):
>   make: fix where dependency *.d are stored.
>   make: load only required dependency files.
> 
>  hw/i386/Makefile.objs     | 8 +++++++-
>  hw/i386/acpi-dsdt.dsl     | 1 -
>  hw/i386/q35-acpi-dsdt.dsl | 1 +
>  rules.mak                 | 4 ++--
>  4 files changed, 10 insertions(+), 4 deletions(-)
> 
> -- 
> --Victor