diff mbox series

[XEN,v3,16/23] xen/build: introduce if_changed and if_changed_rule

Message ID 20200226113355.2532224-17-anthony.perard@citrix.com (mailing list archive)
State Superseded
Headers show
Series xen: Build system improvements | expand

Commit Message

Anthony PERARD Feb. 26, 2020, 11:33 a.m. UTC
The if_changed macro from Linux can record the command used to build a
target then compare it on rebuild. Thus if a command has changed, for
example due to introducing new flags in CFLAGS or due to using a
different compiler, the target will be rebuilt.

if_changed_rule checks dependencies like if_changed, but execute
rule_$(1) instead of cmd_$(1) when the command is different. A rule_
macro can call more than one cmd_ macro. One of the cmd_ macro in a
rule need to be call using a macro that record the command line, so
cmd_and_record is introduced. It is similar to cmd_and_fixup from
Linux but without a call to fixdep which we don't have yet. (We will
later replace cmd_and_record by cmd_and_fixup.)

Example of a rule_ macro:
define rule_cc_o_c
    $(call cmd_and_record,cc_o_o)
    $(call cmd,objcopy)
endef

This needs one of the call to use cmd_and_record, otherwise no .*.cmd
file will be created, and the target will keep been rebuilt.

In order for if_changed to works correctly, we need to load the .%.cmd
files that the macro generates, this is done by adding targets in to
the $(targets) variable. We use intermediate_targets to add %.init.o
dependency %.o to target since there aren't in obj-y.

We also add $(MAKECMDGOALS) to targets so that when running for
example `make common/memory.i`, make will load the associated .%.cmd
dependency file.

Beside the if_changed*, we import the machinery used for a "beautify
output". The important one is when running make with V=2 which help to
debug the makefiles by printing why a target is been rebuilt, via the
$(echo-why) macro.

if_changed and if_changed_rule aren't used yet.

Most of this code is copied from Linux v5.4.

Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
 .gitignore                 |   1 +
 xen/Makefile               |  53 +++++++++++++++++-
 xen/Rules.mk               |  33 +++++++++++-
 xen/scripts/Kbuild.include | 107 +++++++++++++++++++++++++++++++++++++
 4 files changed, 192 insertions(+), 2 deletions(-)

Comments

Jan Beulich March 4, 2020, 3:45 p.m. UTC | #1
On 26.02.2020 12:33, Anthony PERARD wrote:
> The if_changed macro from Linux can record the command used to build a
> target then compare it on rebuild. Thus if a command has changed, for
> example due to introducing new flags in CFLAGS or due to using a
> different compiler, the target will be rebuilt.

As to using a different compiler - I suppose this means "a compiler
with a different executable name" here? What about me having, say
gcc-5 in use, and then updating my system such that a 5.2 based
compiler of this name would be upgraded to a 5.4 based one of this
same name. If this newer compiler has better capabilities (that we
would want to use if available), would this or anything else trigger
a rebuild then too?

> --- a/.gitignore
> +++ b/.gitignore
> @@ -6,6 +6,7 @@
>  *.o
>  *.d
>  *.d2
> +.*.cmd
>  *.opic
>  *.a
>  *.so

I admit these entries aren't sorted very well, but anyway - how
did you end up with this insertion point? There are entries
starting with . at the very top of the file. (As an aside, I
wonder why it's *.d and *.d2 rather than .*.d and .*.d2 .)

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -52,7 +52,57 @@ dist: install
>  
>  ifeq ($(root-make-done),)
>  # section to run before calling Rules.mk, but only once.
> +
> +# Beautify output
> +# ---------------------------------------------------------------------------
> +#
> +# Normally, we echo the whole command before executing it. By making
> +# that echo $($(quiet)$(cmd)), we now have the possibility to set
> +# $(quiet) to choose other forms of output instead, e.g.
> +#
> +#         quiet_cmd_cc_o_c = Compiling $(RELDIR)/$@
> +#         cmd_cc_o_c       = $(CC) $(c_flags) -c -o $@ $<
> +#
> +# If $(quiet) is empty, the whole command will be printed.
> +# If it is set to "quiet_", only the short version will be printed.
> +# If it is set to "silent_", nothing will be printed at all, since
> +# the variable $(silent_cmd_cc_o_c) doesn't exist.
> +#
> +# A simple variant is to prefix commands with $(Q) - that's useful
> +# for commands that shall be hidden in non-verbose mode.
>  #
> +#	$(Q)ln $@ :<
> +#
> +# If KBUILD_VERBOSE equals 0 then the above command will be hidden.
> +# If KBUILD_VERBOSE equals 1 then the above command is displayed.
> +#
> +# To put more focus on warnings, be less verbose as default
> +# Use 'make V=1' to see the full commands
> +
> +ifeq ("$(origin V)", "command line")
> +  KBUILD_VERBOSE = $(V)
> +endif
> +ifndef KBUILD_VERBOSE
> +  KBUILD_VERBOSE = 0
> +endif
> +
> +ifeq ($(KBUILD_VERBOSE),1)
> +  quiet =
> +  Q =
> +else
> +  quiet=quiet_
> +  Q = @
> +endif
> +
> +# If the user is running make -s (silent mode), suppress echoing of
> +# commands
> +
> +ifneq ($(findstring s,$(filter-out --%,$(MAKEFLAGS))),)
> +  quiet=silent_
> +endif

Throughout the above, can the uses of = please become consistent?
Preferable all with a blank on the left and - unless there's no
value getting assigned - one on the right, plus := preferred over
= where not prohibited by other constraints (none here afaics).

> --- a/xen/scripts/Kbuild.include
> +++ b/xen/scripts/Kbuild.include
> @@ -2,11 +2,30 @@
>  ####
>  # kbuild: Generic definitions
>  
> +# Convenient variables
> +squote  := '
> +empty   :=
> +space   := $(empty) $(empty)
> +space_escape := _-_SPACE_-_
> +pound := \#

Nit: To fit with the three ones above space_escape you want to
add two blanks here.

Jan
Anthony PERARD March 18, 2020, 10:44 a.m. UTC | #2
On Wed, Mar 04, 2020 at 04:45:36PM +0100, Jan Beulich wrote:
> On 26.02.2020 12:33, Anthony PERARD wrote:
> > The if_changed macro from Linux can record the command used to build a
> > target then compare it on rebuild. Thus if a command has changed, for
> > example due to introducing new flags in CFLAGS or due to using a
> > different compiler, the target will be rebuilt.
> 
> As to using a different compiler - I suppose this means "a compiler
> with a different executable name" here? What about me having, say
> gcc-5 in use, and then updating my system such that a 5.2 based
> compiler of this name would be upgraded to a 5.4 based one of this
> same name. If this newer compiler has better capabilities (that we
> would want to use if available), would this or anything else trigger
> a rebuild then too?

I think I should have written "command line" instead of just "command".
When writing about "different compiler" I was mostly thinking about GCC
vs clang, not really about versions. I think Linux has something that
detects when the compiler version changes, but that maybe to only
trigger kconfig, to regenerate the .config file.

But as you say, if the newer compiler has better capabilities, and the
*FLAGS are changed, then that would trigger a rebuild if other
dependency hasn't changed.

I'll try to reword the commit message, and copy some documentation from
Linux, since it has some for this.

> 
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -6,6 +6,7 @@
> >  *.o
> >  *.d
> >  *.d2
> > +.*.cmd
> >  *.opic
> >  *.a
> >  *.so
> 
> I admit these entries aren't sorted very well, but anyway - how
> did you end up with this insertion point? There are entries

I basically put it with the other dependency files, *.d and *.d2.

> starting with . at the very top of the file. (As an aside, I
> wonder why it's *.d and *.d2 rather than .*.d and .*.d2 .)

I'll move .*.cmd to the top and ignore that .gitignore ignore more
than necessary.

> > --- a/xen/Makefile
> > +++ b/xen/Makefile
> > @@ -52,7 +52,57 @@ dist: install
> > +ifneq ($(findstring s,$(filter-out --%,$(MAKEFLAGS))),)
> > +  quiet=silent_
> > +endif
> 
> Throughout the above, can the uses of = please become consistent?
> Preferable all with a blank on the left and - unless there's no
> value getting assigned - one on the right, plus := preferred over
> = where not prohibited by other constraints (none here afaics).

I'll try.

> > --- a/xen/scripts/Kbuild.include
> > +++ b/xen/scripts/Kbuild.include
> > @@ -2,11 +2,30 @@
> >  ####
> >  # kbuild: Generic definitions
> >  
> > +# Convenient variables
> > +squote  := '
> > +empty   :=
> > +space   := $(empty) $(empty)
> > +space_escape := _-_SPACE_-_
> > +pound := \#
> 
> Nit: To fit with the three ones above space_escape you want to
> add two blanks here.

Will do.

Thanks,
Jan Beulich March 18, 2020, 11:14 a.m. UTC | #3
On 18.03.2020 11:44, Anthony PERARD wrote:
> On Wed, Mar 04, 2020 at 04:45:36PM +0100, Jan Beulich wrote:
>> On 26.02.2020 12:33, Anthony PERARD wrote:
>>> The if_changed macro from Linux can record the command used to build a
>>> target then compare it on rebuild. Thus if a command has changed, for
>>> example due to introducing new flags in CFLAGS or due to using a
>>> different compiler, the target will be rebuilt.
>>
>> As to using a different compiler - I suppose this means "a compiler
>> with a different executable name" here? What about me having, say
>> gcc-5 in use, and then updating my system such that a 5.2 based
>> compiler of this name would be upgraded to a 5.4 based one of this
>> same name. If this newer compiler has better capabilities (that we
>> would want to use if available), would this or anything else trigger
>> a rebuild then too?
> 
> I think I should have written "command line" instead of just "command".
> When writing about "different compiler" I was mostly thinking about GCC
> vs clang, not really about versions. I think Linux has something that
> detects when the compiler version changes, but that maybe to only
> trigger kconfig, to regenerate the .config file.
> 
> But as you say, if the newer compiler has better capabilities, and the
> *FLAGS are changed, then that would trigger a rebuild if other
> dependency hasn't changed.

"Would" as in "would", or merely "it would be nice if it did"? I'm
simply not seeing where such a detection would be happening.

Jan
diff mbox series

Patch

diff --git a/.gitignore b/.gitignore
index 4ca679ddbc9a..c73f9f480780 100644
--- a/.gitignore
+++ b/.gitignore
@@ -6,6 +6,7 @@ 
 *.o
 *.d
 *.d2
+.*.cmd
 *.opic
 *.a
 *.so
diff --git a/xen/Makefile b/xen/Makefile
index da017dc29d36..fbd087e6f360 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -52,7 +52,57 @@  dist: install
 
 ifeq ($(root-make-done),)
 # section to run before calling Rules.mk, but only once.
+
+# Beautify output
+# ---------------------------------------------------------------------------
+#
+# Normally, we echo the whole command before executing it. By making
+# that echo $($(quiet)$(cmd)), we now have the possibility to set
+# $(quiet) to choose other forms of output instead, e.g.
+#
+#         quiet_cmd_cc_o_c = Compiling $(RELDIR)/$@
+#         cmd_cc_o_c       = $(CC) $(c_flags) -c -o $@ $<
+#
+# If $(quiet) is empty, the whole command will be printed.
+# If it is set to "quiet_", only the short version will be printed.
+# If it is set to "silent_", nothing will be printed at all, since
+# the variable $(silent_cmd_cc_o_c) doesn't exist.
+#
+# A simple variant is to prefix commands with $(Q) - that's useful
+# for commands that shall be hidden in non-verbose mode.
 #
+#	$(Q)ln $@ :<
+#
+# If KBUILD_VERBOSE equals 0 then the above command will be hidden.
+# If KBUILD_VERBOSE equals 1 then the above command is displayed.
+#
+# To put more focus on warnings, be less verbose as default
+# Use 'make V=1' to see the full commands
+
+ifeq ("$(origin V)", "command line")
+  KBUILD_VERBOSE = $(V)
+endif
+ifndef KBUILD_VERBOSE
+  KBUILD_VERBOSE = 0
+endif
+
+ifeq ($(KBUILD_VERBOSE),1)
+  quiet =
+  Q =
+else
+  quiet=quiet_
+  Q = @
+endif
+
+# If the user is running make -s (silent mode), suppress echoing of
+# commands
+
+ifneq ($(findstring s,$(filter-out --%,$(MAKEFLAGS))),)
+  quiet=silent_
+endif
+
+export quiet Q KBUILD_VERBOSE
+
 # To make sure we do not include .config for any of the *config targets
 # catch them early, and hand them over to tools/kconfig/Makefile
 
@@ -258,7 +308,8 @@  _clean: delete-unfresh-files
 	$(MAKE) $(clean) arch/x86
 	$(MAKE) $(clean) test
 	$(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) clean
-	find . \( -name "*.o" -o -name ".*.d" -o -name ".*.d2" -o -name "*.gcno" \) -exec rm -f {} \;
+	find . \( -name "*.o" -o -name ".*.d" -o -name ".*.d2" \
+		-o -name "*.gcno" -o -name ".*.cmd" \) -exec rm -f {} \;
 	rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core
 	rm -f include/asm-*/asm-offsets.h
 	rm -f .banner
diff --git a/xen/Rules.mk b/xen/Rules.mk
index f1311c45a372..8807a2e21c94 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -38,6 +38,7 @@  ALL_OBJS-y               += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
 ALL_OBJS-$(CONFIG_CRYPTO)   += $(BASEDIR)/crypto/built_in.o
 
 # Initialise some variables
+targets :=
 CFLAGS-y :=
 AFLAGS-y :=
 
@@ -65,6 +66,10 @@  $(foreach o,$(filter-out %/,$(obj-y) $(obj-bin-y) $(extra-y)),$(eval $(call gend
 subdir-y := $(subdir-y) $(filter %/, $(obj-y))
 obj-y    := $(patsubst %/, %/built_in.o, $(obj-y))
 
+# $(subdir-obj-y) is the list of objects in $(obj-y) which uses dir/ to
+# tell kbuild to descend
+subdir-obj-y := $(filter %/built_in.o, $(obj-y))
+
 $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS-y += -DINIT_SECTIONS_ONLY
 
 ifeq ($(CONFIG_COVERAGE),y)
@@ -120,6 +125,10 @@  else
 endif
 endif
 
+targets += built_in.o
+targets += $(filter-out $(subdir-obj-y), $(obj-y)) $(extra-y)
+targets += $(MAKECMDGOALS)
+
 built_in_bin.o: $(obj-bin-y) $(extra-y)
 ifeq ($(obj-bin-y),)
 	$(CC) $(a_flags) -c -x assembler /dev/null -o $@
@@ -128,7 +137,7 @@  else
 endif
 
 # Force execution of pattern rules (for which PHONY cannot be directly used).
-.PHONY: FORCE
+PHONY += FORCE
 FORCE:
 
 %/built_in.o: FORCE
@@ -176,4 +185,26 @@  $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): %.init.o: %.o Makefile
 %.s: %.S Makefile
 	$(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@
 
+# Add intermediate targets:
+# When building objects with specific suffix patterns, add intermediate
+# targets that the final targets are derived from.
+intermediate_targets = $(foreach sfx, $(2), \
+				$(patsubst %$(strip $(1)),%$(sfx), \
+					$(filter %$(strip $(1)), $(targets))))
+# %.init.o <- %.o
+targets += $(call intermediate_targets, .init.o, .o)
+
 -include $(DEPS_INCLUDE)
+
+# Read all saved command lines and dependencies for the $(targets) we
+# may be building above, using $(if_changed{,_dep}). As an
+# optimization, we don't need to read them if the target does not
+# exist, we will rebuild anyway in that case.
+
+existing-targets := $(wildcard $(sort $(targets)))
+
+-include $(foreach f,$(existing-targets),$(dir $(f)).$(notdir $(f)).cmd)
+
+# Declare the contents of the PHONY variable as phony.  We keep that
+# information in a variable so we can use it in if_changed and friends.
+.PHONY: $(PHONY)
diff --git a/xen/scripts/Kbuild.include b/xen/scripts/Kbuild.include
index 14bd4e110b45..f24d664db5ff 100644
--- a/xen/scripts/Kbuild.include
+++ b/xen/scripts/Kbuild.include
@@ -2,11 +2,30 @@ 
 ####
 # kbuild: Generic definitions
 
+# Convenient variables
+squote  := '
+empty   :=
+space   := $(empty) $(empty)
+space_escape := _-_SPACE_-_
+pound := \#
+
+###
+# Name of target with a '.' as filename prefix. foo/bar.o => foo/.bar.o
+dot-target = $(@D)/.$(@F)
+
 ###
 # dependencies
 DEPS = .*.d
 DEPS_INCLUDE = $(addsuffix .d2, $(basename $(wildcard $(DEPS))))
 
+###
+# real prerequisites without phony targets
+real-prereqs = $(filter-out $(PHONY), $^)
+
+###
+# Escape single quote for use in echo statements
+escsq = $(subst $(squote),'\$(squote)',$1)
+
 # as-insn: Check whether assembler supports an instruction.
 # Usage: cflags-y += $(call as-insn,CC FLAGS,"insn",option-yes,option-no)
 as-insn = $(if $(shell echo 'void _(void) { asm volatile ( $(2) ); }' \
@@ -32,3 +51,91 @@  cc-ifversion = $(shell [ $(CONFIG_GCC_VERSION)0 $(1) $(2)000 ] && echo $(3) || e
 # Usage:
 # $(MAKE) $(clean) dir
 clean := -f $(BASEDIR)/scripts/Makefile.clean clean -C
+
+# echo command.
+# Short version is used, if $(quiet) equals `quiet_', otherwise full one.
+echo-cmd = $(if $($(quiet)cmd_$(1)),\
+        echo '  $(call escsq,$($(quiet)cmd_$(1)))$(echo-why)';)
+
+# printing commands
+cmd = @set -e; $(echo-cmd) $(cmd_$(1))
+
+###
+# if_changed      - execute command if any prerequisite is newer than
+#                   target, or command line has changed
+# if_changed_rule - as if_changed but execute rule instead
+
+ifneq ($(KBUILD_NOCMDDEP),1)
+# Check if both commands are the same including their order. Result is empty
+# string if equal. User may override this check using make KBUILD_NOCMDDEP=1
+cmd-check = $(filter-out $(subst $(space),$(space_escape),$(strip $(cmd_$@))), \
+                         $(subst $(space),$(space_escape),$(strip $(cmd_$1))))
+else
+cmd-check = $(if $(strip $(cmd_$@)),,1)
+endif
+
+# Replace >$< with >$$< to preserve $ when reloading the .cmd file
+# (needed for make)
+# Replace >#< with >$(pound)< to avoid starting a comment in the .cmd file
+# (needed for make)
+# Replace >'< with >'\''< to be able to enclose the whole string in '...'
+# (needed for the shell)
+make-cmd = $(call escsq,$(subst $(pound),$$(pound),$(subst $$,$$$$,$(cmd_$(1)))))
+
+# Find any prerequisites that is newer than target or that does not exist.
+# PHONY targets skipped in both cases.
+any-prereq = $(filter-out $(PHONY),$?)$(filter-out $(PHONY) $(wildcard $^),$^)
+
+# Execute command if command has changed or prerequisite(s) are updated.
+if_changed = $(if $(any-prereq)$(cmd-check),                                 \
+        $(cmd);                                                              \
+        printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd, @:)
+
+# Usage: $(call if_changed_rule,foo)
+# Will check if $(cmd_foo) or any of the prerequisites changed,
+# and if so will execute $(rule_foo).
+if_changed_rule = $(if $(any-prereq)$(cmd-check),$(rule_$(1)),@:)
+
+cmd_and_record =                                                             \
+        $(cmd);                                                              \
+        printf '%s\n' 'cmd_$@ := $(make-cmd)' > $(dot-target).cmd
+
+###
+# why - tell why a target got built
+#       enabled by make V=2
+#       Output (listed in the order they are checked):
+#          (1) - due to target is PHONY
+#          (2) - due to target missing
+#          (3) - due to: file1.h file2.h
+#          (4) - due to command line change
+#          (5) - due to missing .cmd file
+#          (6) - due to target not in $(targets)
+# (1) PHONY targets are always build
+# (2) No target, so we better build it
+# (3) Prerequisite is newer than target
+# (4) The command line stored in the file named dir/.target.cmd
+#     differed from actual command line. This happens when compiler
+#     options changes
+# (5) No dir/.target.cmd file (used to store command line)
+# (6) No dir/.target.cmd file and target not listed in $(targets)
+#     This is a good hint that there is a bug in the kbuild file
+ifeq ($(KBUILD_VERBOSE),2)
+why =                                                                        \
+    $(if $(filter $@, $(PHONY)),- due to target is PHONY,                    \
+        $(if $(wildcard $@),                                                 \
+            $(if $(any-prereq),- due to: $(any-prereq),                      \
+                $(if $(cmd-check),                                           \
+                    $(if $(cmd_$@),- due to command line change,             \
+                        $(if $(filter $@, $(targets)),                       \
+                            - due to missing .cmd file,                      \
+                            - due to $(notdir $@) not in $$(targets)         \
+                         )                                                   \
+                     )                                                       \
+                 )                                                           \
+             ),                                                              \
+             - due to target missing                                         \
+         )                                                                   \
+     )
+
+echo-why = $(call escsq, $(strip $(why)))
+endif