[RFC PATCH 0/2] kbuild: Cache exploratory calls to the compiler

[RFC PATCH 0/2] kbuild: Cache exploratory calls to the compiler

[Douglas Anderson]

This two-patch series attempts to speed incremental builds of the
kernel up by a bit.  How much of a speedup you get depends a lot on
your environment, specifically the speed of your workstation and how
fast it takes to invoke the compiler.

In the Chrome OS build environment you get a really big win.  For an
incremental build (via emerge) I measured a speedup from ~1 minute to
~35 seconds.  ...but Chrome OS calls the compiler through a number of
wrapper scripts and also calls the kernel make at least twice for an
emerge (during compile stage and install stage), so it's a bit of a
worst case.

Perhaps a more realistic measure of the speedup others might see is
running "time make help > /dev/null" outside of the Chrome OS build
environment on my system.  When I do this I see that it took more than
1.0 seconds before and less than 0.2 seconds after.  So presumably
this has the ability to shave ~0.8 seconds off an incremental build
for most folks out there.  While 0.8 seconds savings isn't huge, it
does make incremental builds feel a lot snappier.

Please note that I make no illusions of being a Makefile expert nor do
I have any belief that I fully understand the Linux kernel build
system.  Please take this patch series as the start of a discussion
about whether others feel like this type of speedup is worthwhile and
how to best accomplish it.  Specific things to note:

- I'm happy to paint the bikeshed any color that maintainers want.  If
  you'd like the cache named differently, in a slightly different
  format, or you want me to adjust the spacing / names of Makefile
  stuff then please just let me know.

- If this is totally the wrong approach and you have a better idea
  then let me know.  If you want something that's super complicated to
  explain then feel free to post a replacement patch and I'm happy to
  test.

- This patch definitely needs extra testing.  I've tested it on a very
  limited build environment and it seems to be working fine, but I
  could believe that with some weird compiler options or on certain
  architectures you might need some extra escaping here and there.


Douglas Anderson (2):
  kbuild: Add a cache for generated variables
  kbuild: Cache a few more calls to the compiler

 Documentation/kbuild/makefiles.txt |  21 ++++++
 Makefile                           |   9 ++-
 scripts/Kbuild.include             | 133 +++++++++++++++++++++++++++++++------
 3 files changed, 142 insertions(+), 21 deletions(-)

-- 
2.14.1.821.g8fa685d3b7-goog

[RFC PATCH 1/2] kbuild: Add a cache for generated variables

[Douglas Anderson]

While timing a "no-op" build of the kernel (incrementally building the
kernel even though nothing changed) in the Chrome OS build system I
found that it was much slower than I expected.

Digging into things a bit, I found that quite a bit of the time was
spent invoking the C compiler even though we weren't actually building
anything.  Currently in the Chrome OS build system the C compiler is
called through a number of wrappers (one of which is written in
python!) and can take upwards of 100 ms to invoke even if we're not
doing anything difficult, so these invocations of the compiler were
taking a lot of time.  Worse the invocations couldn't seem to take
advantage of the multiple cores on my system.

Certainly it seems like we could make the compiler invocations in the
Chrome OS build system faster, but only to a point.  Inherently
invoking a program as big as a C compiler is a fairly heavy
operation.  Thus even if we can speed the compiler calls it made sense
to track down what was happening.

It turned out that all the compiler invocations were coming from
usages like this in the kernel's Makefile:

KBUILD_CFLAGS += $(call cc-option,-fno-delete-null-pointer-checks,)

Due to the way cc-option and similar statements work the above
contains an implicit call to the C compiler.  ...and due to the fact
that we're storing the result in KBUILD_CFLAGS, a simply expanded
variable, the call will happen every time the Makefile is parsed, even
if there are no users of KBUILD_CFLAGS.

Rather than redoing this computation every time, it makes a lot of
sense to cache the result of all of the Makefile's compiler calls just
like we do when we compile a ".c" file to a ".o" file.  Conceptually
this is quite a simple idea.  ...and since the calls to invoke the
compiler and similar tools are centrally located in the Kbuild.include
file this doesn't even need to be super invasive.

Implementing the cache in a simple-to-use and efficient way is not
quite as simple as it first sounds, though.  To get maximum speed we
really want the cache in a format that make can natively understand
and make doesn't really have an ability to load/parse files. ...but
make _can_ import other Makefiles, so the solution is to store the
cache in Makefile format.  This requires coming up with a valid/unique
Makefile variable name for each value to be cached, but that's
solvable with some cleverness.

After this change, we'll automatically create a "Makefile-cache.o"
file that will contain our cached variables.  We'll load this on each
invocation of make and will avoid recomputing anything that's already
in our cache.  The cache is stored in a format that it shouldn't need
any invalidation since anything that might change should affect the
"key" and any old cached value won't be used.  The cache will be
cleaned by virtue of the ".o" suffix by a "make clean".

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 Documentation/kbuild/makefiles.txt |  21 ++++++
 scripts/Kbuild.include             | 133 +++++++++++++++++++++++++++++++------
 2 files changed, 135 insertions(+), 19 deletions(-)

diff --git a/Documentation/kbuild/makefiles.txt b/Documentation/kbuild/makefiles.txt
index 329e740adea7..679e8483cb4f 100644
--- a/Documentation/kbuild/makefiles.txt
+++ b/Documentation/kbuild/makefiles.txt
@@ -581,6 +581,27 @@ more details, with real examples.
 		LDFLAGS_vmlinux += $(call ld-option, -X)
 
 
+--- 3.14 $(LD) support function cache
+
+One thing to realize about all the calls to the above support functions
+is that each use of them requires a full invocation of an external tool, like
+the C compiler, assembler, or linker.  If nothing else that invocation will
+cause a fork/exec/shared library link.  In some build environments, however, it
+could also involve traversing thorough one or more wrappers.  To put some
+numbers on it, I've measured compiler invocations of as fast as 4ms or
+as slow as 150ms.
+
+Many of the above support functions are used in places where they are
+evaluated on each invocation of Make before anything else can run.  Even on
+a simple command like "make help" we need to evaluate every single one of the
+variables.
+
+To avoid this slow overhead we can cache the result of these invocations.
+If we store this cache in a way that's easy for Make to use (like in Makefile
+format) then it will be very quick and we'll save a lot of time with each
+invocation of make.
+
+
 === 4 Host Program support
 
 Kbuild supports building executables on the host for use during the
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 9ffd3dda3889..de88120f1763 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -8,6 +8,8 @@ squote  := '
 empty   :=
 space   := $(empty) $(empty)
 space_escape := _-_SPACE_-_
+right_paren := )
+left_paren := (
 
 ###
 # Name of target with a '.' as filename prefix. foo/bar.o => foo/.bar.o
@@ -69,6 +71,53 @@ endef
 # gcc support functions
 # See documentation in Documentation/kbuild/makefiles.txt
 
+# Tools for caching Makefile variables that are "expensive" to compute.
+#
+# Here we want to help deal with variables that take a long time to compute
+# by making it easy to store these variables in a cache.
+#
+# The canonical example here is testing for compiler flags.  On a simple system
+# each call to the compiler takes 10 ms, but on a system with a compiler that's
+# called through various wrappers it can take upwards of 100 ms.  If we have
+# 100 calls to the compiler this can take 1 second (on a simple system) or 10
+# seconds (on a complicated system).
+#
+# The "cache" will be in Makefile syntax and can be directly included.
+# Any time we try to reference a variable that's not in the cache we'll
+# calculate it and store it in the cache for next time.
+
+# Include values from last time
+-include Makefile-cache.o
+
+# Usage:   $(call cached-val,variable_name,escaped_expensive_operation)
+# Example: $(call cached-val,md5_val,$$(shell md5sum /usr/bin/gcc)
+#
+# If the variable is already defined we'll just use it.  If it's not, it will
+# be calculated and stored in a persistent (disk-based) cache for the next
+# invocation of Make.  The call will evaluate to the value of the variable.
+#
+# This is a bit of voodoo, but basic explanation is that if the variable
+# was undefined then we'll evaluate the expensive operation and store it into
+# the variable.  We'll then store that value in the cache and finally output
+# the value.
+define __set-and-store
+ifeq ($(origin $(1)),undefined)
+  $$(eval $(1) := $$(2))
+  $$(shell echo '$(1) := $$($(1))' >> Makefile-cache.o)
+endif
+endef
+cached-val = $(eval $(call __set-and-store,__cached_$(1)))$(__cached_$(1))
+
+# Usage: $(call __sanitize-opt,Hello=Hola$(comma)Goodbye Adios)
+#
+# Convert all '$', ')', '(', '\', '=', ' ', ',' to '_'
+__sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$(subst \,_,$(subst =,_,$(subst $(space),_,$(subst $(comma),_,$(1))))))))
+
+# Usage = $(call __comma,something_with_comma)
+#
+# Convert ',' to '$(comma)' which can help it getting interpreted by eval.
+__comma = $(subst $(comma),$$(comma),$(1))
+
 # cc-cross-prefix
 # Usage: CROSS_COMPILE := $(call cc-cross-prefix, m68k-linux-gnu- m68k-linux-)
 # Return first prefix where a prefix$(CC) is found in PATH.
@@ -99,19 +148,34 @@ try-run = $(shell set -e;		\
 # as-option
 # Usage: cflags-y += $(call as-option,-Wa$(comma)-isa=foo,)
 
-as-option = $(call try-run,\
-	$(CC) $(KBUILD_CFLAGS) $(1) -c -x assembler /dev/null -o "$$TMP",$(1),$(2))
+as-option = \
+	$(call cached-val,$(call __sanitize-opt,\
+		as_opt_$(CC)_$(KBUILD_CFLAGS)_$(1)_$(2)),\
+	$$(call try-run,\
+	$(CC) $(call __comma,$(KBUILD_CFLAGS)) $(call __comma,$(1)) \
+	-c -x assembler /dev/null \
+	-o "$$$$TMP",$(call __comma,$(1)),$(call __comma,$(2))))
 
 # as-instr
 # Usage: cflags-y += $(call as-instr,instr,option1,option2)
 
-as-instr = $(call try-run,\
-	printf "%b\n" "$(1)" | $(CC) $(KBUILD_AFLAGS) -c -x assembler -o "$$TMP" -,$(2),$(3))
+as-instr = \
+	$(call cached-val,$(call __sanitize-opt,\
+		as_instr_$(CC)_$(KBUILD_AFLAGS)_$(1)_$(2)_$(3)),\
+	$$(call try-run,\
+	printf "%b\n" "$(call __comma,$(1))" | \
+	$(CC) $(call __comma,$(KBUILD_AFLAGS)) -c -x assembler \
+	-o "$$$$TMP" -,$(call __comma,$(2)),$(call __comma,$(3))))
 
 # __cc-option
 # Usage: MY_CFLAGS += $(call __cc-option,$(CC),$(MY_CFLAGS),-march=winchip-c6,-march=i586)
-__cc-option = $(call try-run,\
-	$(1) -Werror $(2) $(3) -c -x c /dev/null -o "$$TMP",$(3),$(4))
+__cc-option = \
+	$(call cached-val,$(call __sanitize-opt,__cc_opt_$(1)_$(2)_$(3)_$(4)),\
+	$$(call try-run,\
+	$(call __comma,$(1)) -Werror \
+	$(call __comma,$(2)) \
+	$(call __comma,$(3)) -c -x c /dev/null \
+	-o "$$$$TMP",$(call __comma,$(strip $(3))),$(call __comma,$(strip $(4)))))
 
 # Do not attempt to build with gcc plugins during cc-option tests.
 # (And this uses delayed resolution so the flags will be up to date.)
@@ -130,24 +194,42 @@ hostcc-option = $(call __cc-option, $(HOSTCC),\
 
 # cc-option-yn
 # Usage: flag := $(call cc-option-yn,-march=winchip-c6)
-cc-option-yn = $(call try-run,\
-	$(CC) -Werror $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",y,n)
+cc-option-yn = \
+	$(call cached-val,$(call __sanitize-opt,\
+		cc_opt_yn_$(CC)_$(KBUILD_CPPFLAGS)_$(CC_OPTION_CFLAGS)$(1)),\
+	$$(call try-run,\
+	$(CC) -Werror $(call __comma,$(KBUILD_CPPFLAGS)) \
+	$(call __comma,$(CC_OPTION_CFLAGS)) \
+	$(call __comma,$(1)) \
+	-c -x c /dev/null -o "$$$$TMP",y,n))
 
 # cc-disable-warning
 # Usage: cflags-y += $(call cc-disable-warning,unused-but-set-variable)
-cc-disable-warning = $(call try-run,\
-	$(CC) -Werror $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS) -W$(strip $(1)) -c -x c /dev/null -o "$$TMP",-Wno-$(strip $(1)))
+cc-disable-warning = \
+	$(call cached-val,$(call __sanitize-opt,\
+		cc_dis_wn_$(CC)_$(KBUILD_CPPFLAGS)_$(CC_OPTION_CFLAGS)$(1)_$(2)),\
+	$$(call try-run,\
+	$(CC) -Werror $(call __comma,$(KBUILD_CPPFLAGS)) \
+	$(call __comma,$(CC_OPTION_CFLAGS)) \
+	-W$(strip $(call __comma,$(1))) -c -x c /dev/null \
+	-o "$$$$TMP",-Wno-$(strip $(call __comma,$(1)))))
 
 # cc-name
 # Expands to either gcc or clang
-cc-name = $(shell $(CC) -v 2>&1 | grep -q "clang version" && echo clang || echo gcc)
+cc-name = \
+	$(call cached-val,$(call __sanitize-opt,cc_name_$(CC)),\
+	$$(shell $(CC) -v 2>&1 | grep -q "clang version" && echo clang || echo gcc))
 
 # cc-version
-cc-version = $(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-version.sh $(CC))
+cc-version = \
+	$(call cached-val,$(call __sanitize-opt,cc_version_$(CC)),\
+	$$(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-version.sh $(CC)))
 
 # cc-fullversion
-cc-fullversion = $(shell $(CONFIG_SHELL) \
-	$(srctree)/scripts/gcc-version.sh -p $(CC))
+cc-fullversion = \
+	$(call cached-val,$(call __sanitize-opt,cc_full_version_$(CC)),\
+	$$(shell $(CONFIG_SHELL) \
+	$(srctree)/scripts/gcc-version.sh -p $(CC)))
 
 # cc-ifversion
 # Usage:  EXTRA_CFLAGS += $(call cc-ifversion, -lt, 0402, -O1)
@@ -159,18 +241,31 @@ cc-if-fullversion = $(shell [ $(cc-fullversion) $(1) $(2) ] && echo $(3) || echo
 
 # cc-ldoption
 # Usage: ldflags += $(call cc-ldoption, -Wl$(comma)--hash-style=both)
-cc-ldoption = $(call try-run,\
-	$(CC) $(1) -nostdlib -x c /dev/null -o "$$TMP",$(1),$(2))
+cc-ldoption = \
+	$(call cached-val,$(call __sanitize-opt,\
+		cc_ld_opt_$(CC)_$(1)_$(2)),\
+	$$(call try-run,\
+	$(CC) $(call __comma,$(1)) -nostdlib -x c /dev/null \
+	-o "$$$$TMP",$(call __comma,$(1)),$(call __comma,$(2))))
 
 # ld-option
 # Usage: LDFLAGS += $(call ld-option, -X)
-ld-option = $(call try-run,\
-	$(CC) -x c /dev/null -c -o "$$TMPO" ; $(LD) $(1) "$$TMPO" -o "$$TMP",$(1),$(2))
+ld-option = \
+	$(call cached-val,$(call __sanitize-opt,\
+		ld_opt_$(CC)_$(LD)_$(1)_$(2)),\
+	$$(call try-run,\
+	$(CC) -x c /dev/null -c -o "$$$$TMPO" ; \
+	$(LD) $(call __comma,$(1)) "$$$$TMPO" \
+	-o "$$$$TMP",$(call __comma,$(1)),$(call __comma,$(2))))
 
 # ar-option
 # Usage: KBUILD_ARFLAGS := $(call ar-option,D)
 # Important: no spaces around options
-ar-option = $(call try-run, $(AR) rc$(1) "$$TMP",$(1),$(2))
+ar-option = \
+	$(call cached-val,$(call __sanitize-opt,\
+		ar_opt_$(AR)_$(1)_$(2)),\
+	$$(call try-run, $(AR) rc$(call __comma,$(1)) \
+	"$$$$TMP",$(call __comma,$(1)),$(call __comma,$(2))))
 
 # ld-version
 # Note this is mainly for HJ Lu's 3 number binutil versions
-- 
2.14.1.821.g8fa685d3b7-goog

[RFC PATCH 2/2] kbuild: Cache a few more calls to the compiler

[Douglas Anderson]

These are a few stragglers that I left out of the original patch to
cache calls to the C compiler ("kbuild: Add a cache for generated
variables") because they bleed out into the main Makefile and thus
uglify things a little bit.  The idea is the same here, though.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 Makefile | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index d1119941261c..7b7342d79771 100644
--- a/Makefile
+++ b/Makefile
@@ -652,7 +652,10 @@ KBUILD_CFLAGS += $(call cc-ifversion, -lt, 0409, \
 KBUILD_CFLAGS	+= $(call cc-option,--param=allow-store-data-races=0)
 
 # check for 'asm goto'
-ifeq ($(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)), y)
+ASM_GOTO = $(call cached-val,$(call __sanitize-opt,asm_goto_$(CC)),\
+	   $$(shell $(CONFIG_SHELL) \
+	   $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)))
+ifeq ($(ASM_GOTO), y)
 	KBUILD_CFLAGS += -DCC_HAVE_ASM_GOTO
 	KBUILD_AFLAGS += -DCC_HAVE_ASM_GOTO
 endif
@@ -788,7 +791,9 @@ KBUILD_CFLAGS	+= $(call cc-option,-fdata-sections,)
 endif
 
 # arch Makefile may override CC so keep this after arch Makefile is included
-NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
+NOSTDINC_FLAGS += -nostdinc -isystem \
+		  $(call cached-val,$(call __sanitize-opt,cc_include_$(CC)),\
+		  $$(shell $(CC) -print-file-name=include))
 CHECKFLAGS     += $(NOSTDINC_FLAGS)
 
 # warn about C99 declaration after statement
-- 
2.14.1.821.g8fa685d3b7-goog

Re: [RFC PATCH 1/2] kbuild: Add a cache for generated variables

[Masahiro Yamada]

Hi Douglas,


2017-09-27 2:55 GMT+09:00 Douglas Anderson <dianders@chromium.org>:
> While timing a "no-op" build of the kernel (incrementally building the
> kernel even though nothing changed) in the Chrome OS build system I
> found that it was much slower than I expected.
>
> Digging into things a bit, I found that quite a bit of the time was
> spent invoking the C compiler even though we weren't actually building
> anything.  Currently in the Chrome OS build system the C compiler is
> called through a number of wrappers (one of which is written in
> python!) and can take upwards of 100 ms to invoke even if we're not
> doing anything difficult, so these invocations of the compiler were
> taking a lot of time.  Worse the invocations couldn't seem to take
> advantage of the multiple cores on my system.
>
> Certainly it seems like we could make the compiler invocations in the
> Chrome OS build system faster, but only to a point.  Inherently
> invoking a program as big as a C compiler is a fairly heavy
> operation.  Thus even if we can speed the compiler calls it made sense
> to track down what was happening.
>
> It turned out that all the compiler invocations were coming from
> usages like this in the kernel's Makefile:
>
> KBUILD_CFLAGS += $(call cc-option,-fno-delete-null-pointer-checks,)
>
> Due to the way cc-option and similar statements work the above
> contains an implicit call to the C compiler.  ...and due to the fact
> that we're storing the result in KBUILD_CFLAGS, a simply expanded
> variable, the call will happen every time the Makefile is parsed, even
> if there are no users of KBUILD_CFLAGS.
>
> Rather than redoing this computation every time, it makes a lot of
> sense to cache the result of all of the Makefile's compiler calls just
> like we do when we compile a ".c" file to a ".o" file.  Conceptually
> this is quite a simple idea.  ...and since the calls to invoke the
> compiler and similar tools are centrally located in the Kbuild.include
> file this doesn't even need to be super invasive.
>
> Implementing the cache in a simple-to-use and efficient way is not
> quite as simple as it first sounds, though.  To get maximum speed we
> really want the cache in a format that make can natively understand
> and make doesn't really have an ability to load/parse files. ...but
> make _can_ import other Makefiles, so the solution is to store the
> cache in Makefile format.  This requires coming up with a valid/unique
> Makefile variable name for each value to be cached, but that's
> solvable with some cleverness.
>
> After this change, we'll automatically create a "Makefile-cache.o"
> file that will contain our cached variables.  We'll load this on each
> invocation of make and will avoid recomputing anything that's already
> in our cache.  The cache is stored in a format that it shouldn't need
> any invalidation since anything that might change should affect the
> "key" and any old cached value won't be used.  The cache will be
> cleaned by virtue of the ".o" suffix by a "make clean".
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>



Thanks for the patches.  The idea is interesting.

I am not a Chrome developer, but cc-option could be improved somehow.


I examined two approaches to mitigate the pain.

[1] Skip cc-option completely when we run non-build targets
    such as "make help", "make clean", etc.

[2] Cache the result of cc-option like this patch


I wrote some patches for [1]
https://patchwork.kernel.org/patch/9983825/
https://patchwork.kernel.org/patch/9983829/
https://patchwork.kernel.org/patch/9983833/
https://patchwork.kernel.org/patch/9983827/

Comments are welcome.  :)


[1] does not solve the slowness in the incremental build.
Instead, it makes non-build targets faster
from a clean source tree because cc-option is zero cost.


[2] solves the issues in the incremental build.
One funny thing is, it computes cc-option and store the cache file
even for "make clean", where we know the cache file will be
immediately deleted.


We can apply [1] or [2], or maybe both of them
because [1] and [2] are trying to solve the different issues.



The cache approach seemed clever, but I do not like the implementation.
The code is even more unreadable with lots of escape sequence.


Here is my suggestion for simpler implementation (including bike-shed)


Implement a new function "shell-cache".  It works like $(shell ...)

The difference is
$(call shell-cached,...) returns the cached result, or run the command
if not cached.



Also, add try-run-cached.  This is a cached variant of try-run.


I just played with it, and seems working.
(I did not have spend much time for testing, more wider test is needed.)


The code is like something like this:



make-cache := $(if $(KBUILD_EXTMOD),$(KBUILD_EXTMOD)/,$(if
$(obj),$(obj)/)).cache.mk

-include $(make-cache)

__sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst
$(left_paren),_,$(subst \,_,$(subst =,_,$(subst $(space),_,$(subst
$(comma),_,$(1))))))))

define __run-and-store
ifeq ($(origin $(1)),undefined)
  $$(eval $(1) := $$(shell $$2))
  $$(shell echo '$(1) := $$($(1))' >> $(make-cache))
endif
endef

# $(call,shell-cached,my_command)
# This works like $(shell my_command), but the result is cached
__shell-cached = $(eval $(call __run-and-store,$1))$($1)
shell-cached = $(call __shell-cached,__cached_$(call __sanitize-opt,$(1)),$1)

...


__try-run = set -e; \
     TMP="$(TMPOUT).$$$$.tmp"; \
     TMPO="$(TMPOUT).$$$$.o"; \
     if ($(1)) >/dev/null 2>&1; \
     then echo "$(2)"; \
     else echo "$(3)"; \
     fi; \
     rm -f "$$TMP" "$$TMPO"

# try-run
# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
# Exit code chooses option. "$$TMP" serves as a temporary file and is
# automatically cleaned up.
try-run = $(shell $(__try-run))

# try-run-cached
# This works like try-run, but the result is cached.
try-run-cached = $(call shell-cached,$(__try-run))




Then, you can replace

  $(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)

with

  $(call shell-cached,$(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh
$(CC) $(KBUILD_CFLAGS)




replace

__cc-option = $(call try-run,\
         $(1) -Werror $(2) $(3) -c -x c /dev/null -o "$$TMP",$(3),$(4))

with

__cc-option = $(call try-run-cached,\
         $(1) -Werror $(2) $(3) -c -x c /dev/null -o "$$TMP",$(3),$(4))




What do you think?

A little more comments below.




> +# Tools for caching Makefile variables that are "expensive" to compute.
> +#
> +# Here we want to help deal with variables that take a long time to compute
> +# by making it easy to store these variables in a cache.
> +#
> +# The canonical example here is testing for compiler flags.  On a simple system
> +# each call to the compiler takes 10 ms, but on a system with a compiler that's
> +# called through various wrappers it can take upwards of 100 ms.  If we have
> +# 100 calls to the compiler this can take 1 second (on a simple system) or 10
> +# seconds (on a complicated system).
> +#
> +# The "cache" will be in Makefile syntax and can be directly included.
> +# Any time we try to reference a variable that's not in the cache we'll
> +# calculate it and store it in the cache for next time.
> +
> +# Include values from last time
> +-include Makefile-cache.o


Kbuild.include is included, so is Makefile-cache.o
every time the build system descend into sub-directories.

It is not efficient to include cached data unneeded in sub-directories.
I prefixed $(obj)/

Another problem is when building external modules.
Makefile-cache.o is always created/updated in the kernel build tree.

When we build external modules, the kernel source may be located under
/usr/src/ , which is generally read-only for normal users.
So, I prefixed $(KBUILD_EXTMOD) to create the cache file
in the module tree if KBUILD_EXTMOD is defined.

I do not like the suffix .o
I prefer file name to be something else
that starts with . to hide it.








> +# Usage:   $(call cached-val,variable_name,escaped_expensive_operation)
> +# Example: $(call cached-val,md5_val,$$(shell md5sum /usr/bin/gcc)
> +#
> +# If the variable is already defined we'll just use it.  If it's not, it will
> +# be calculated and stored in a persistent (disk-based) cache for the next
> +# invocation of Make.  The call will evaluate to the value of the variable.
> +#
> +# This is a bit of voodoo, but basic explanation is that if the variable
> +# was undefined then we'll evaluate the expensive operation and store it into
> +# the variable.  We'll then store that value in the cache and finally output
> +# the value.
> +define __set-and-store
> +ifeq ($(origin $(1)),undefined)
> +  $$(eval $(1) := $$(2))
> +  $$(shell echo '$(1) := $$($(1))' >> Makefile-cache.o)
> +endif
> +endef
> +cached-val = $(eval $(call __set-and-store,__cached_$(1)))$(__cached_$(1))


This seems working, but I do not understand this trick.


__set-and-store takes two arguments,
but here, it is called with one argument __cached_$(1)

Probably, $$(try-run, ...) will be passed as $2,
but I do not know why this works.




> +# Usage: $(call __sanitize-opt,Hello=Hola$(comma)Goodbye Adios)
> +#
> +# Convert all '$', ')', '(', '\', '=', ' ', ',' to '_'
> +__sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$(subst \,_,$(subst =,_,$(subst $(space),_,$(subst $(comma),_,$(1))))))))
> +
> +# Usage = $(call __comma,something_with_comma)
> +#
> +# Convert ',' to '$(comma)' which can help it getting interpreted by eval.
> +__comma = $(subst $(comma),$$(comma),$(1))
> +
>  # cc-cross-prefix
>  # Usage: CROSS_COMPILE := $(call cc-cross-prefix, m68k-linux-gnu- m68k-linux-)
>  # Return first prefix where a prefix$(CC) is found in PATH.
> @@ -99,19 +148,34 @@ try-run = $(shell set -e;          \
>  # as-option
>  # Usage: cflags-y += $(call as-option,-Wa$(comma)-isa=foo,)
>
> -as-option = $(call try-run,\
> -       $(CC) $(KBUILD_CFLAGS) $(1) -c -x assembler /dev/null -o "$$TMP",$(1),$(2))
> +as-option = \
> +       $(call cached-val,$(call __sanitize-opt,\
> +               as_opt_$(CC)_$(KBUILD_CFLAGS)_$(1)_$(2)),\
> +       $$(call try-run,\
> +       $(CC) $(call __comma,$(KBUILD_CFLAGS)) $(call __comma,$(1)) \
> +       -c -x assembler /dev/null \
> +       -o "$$$$TMP",$(call __comma,$(1)),$(call __comma,$(2))))
>
>  # as-instr
>  # Usage: cflags-y += $(call as-instr,instr,option1,option2)
>
> -as-instr = $(call try-run,\
> -       printf "%b\n" "$(1)" | $(CC) $(KBUILD_AFLAGS) -c -x assembler -o "$$TMP" -,$(2),$(3))
> +as-instr = \
> +       $(call cached-val,$(call __sanitize-opt,\
> +               as_instr_$(CC)_$(KBUILD_AFLAGS)_$(1)_$(2)_$(3)),\
> +       $$(call try-run,\
> +       printf "%b\n" "$(call __comma,$(1))" | \
> +       $(CC) $(call __comma,$(KBUILD_AFLAGS)) -c -x assembler \
> +       -o "$$$$TMP" -,$(call __comma,$(2)),$(call __comma,$(3))))
>
>  # __cc-option
>  # Usage: MY_CFLAGS += $(call __cc-option,$(CC),$(MY_CFLAGS),-march=winchip-c6,-march=i586)
> -__cc-option = $(call try-run,\
> -       $(1) -Werror $(2) $(3) -c -x c /dev/null -o "$$TMP",$(3),$(4))
> +__cc-option = \
> +       $(call cached-val,$(call __sanitize-opt,__cc_opt_$(1)_$(2)_$(3)_$(4)),\
> +       $$(call try-run,\
> +       $(call __comma,$(1)) -Werror \
> +       $(call __comma,$(2)) \
> +       $(call __comma,$(3)) -c -x c /dev/null \
> +       -o "$$$$TMP",$(call __comma,$(strip $(3))),$(call __comma,$(strip $(4)))))


I did not follow how this was working here...




-- 
Best Regards
Masahiro Yamada

Re: [RFC PATCH 1/2] kbuild: Add a cache for generated variables

[Doug Anderson]

Hi,

On Tue, Oct 3, 2017 at 9:05 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Thanks for the patches.  The idea is interesting.
>
> I am not a Chrome developer, but cc-option could be improved somehow.
>
>
> I examined two approaches to mitigate the pain.
>
> [1] Skip cc-option completely when we run non-build targets
>     such as "make help", "make clean", etc.
>
> [2] Cache the result of cc-option like this patch
>
>
> I wrote some patches for [1]
> https://patchwork.kernel.org/patch/9983825/
> https://patchwork.kernel.org/patch/9983829/
> https://patchwork.kernel.org/patch/9983833/
> https://patchwork.kernel.org/patch/9983827/
>
> Comments are welcome.  :)

OK, I'll take a look at them.  I'm not massively familiar with the
kernel Makefiles, but hopefully I can figure some of it out.  :-P


> [1] does not solve the slowness in the incremental build.
> Instead, it makes non-build targets faster
> from a clean source tree because cc-option is zero cost.
>
>
> [2] solves the issues in the incremental build.
> One funny thing is, it computes cc-option and store the cache file
> even for "make clean", where we know the cache file will be
> immediately deleted.
>
>
> We can apply [1] or [2], or maybe both of them
> because [1] and [2] are trying to solve the different issues.

Yeah, I'm much more interested in [2] than [1].  I run incremental
builds almost constantly and hate the slowness.  :(  I can certainly
appreciate that the inefficient compiler setup in Chrome OS isn't your
problem and that an extra .5 seconds for an incremental build for most
people isn't that huge, though.  ...but I'l probably move forward with
[2] since it still helps me a lot and still should help others.
Solving [1] seems worthwhile too, though...


> The cache approach seemed clever, but I do not like the implementation.
> The code is even more unreadable with lots of escape sequence.
>
>
> Here is my suggestion for simpler implementation (including bike-shed)
>
>
> Implement a new function "shell-cache".  It works like $(shell ...)
>
> The difference is
> $(call shell-cached,...) returns the cached result, or run the command
> if not cached.
>
>
>
> Also, add try-run-cached.  This is a cached variant of try-run.
>
>
> I just played with it, and seems working.
> (I did not have spend much time for testing, more wider test is needed.)
>
>
> The code is like something like this:
>
>
>
> make-cache := $(if $(KBUILD_EXTMOD),$(KBUILD_EXTMOD)/,$(if
> $(obj),$(obj)/)).cache.mk
>
> -include $(make-cache)

Thanks, this is much better!  :)


> define __run-and-store
> ifeq ($(origin $(1)),undefined)
>   $$(eval $(1) := $$(shell $$2))
>   $$(shell echo '$(1) := $$($(1))' >> $(make-cache))
> endif
> endef

I like your idea.  Essentially you're saying that we can just defer
the shell command, which was the really essential part that needed to
be deferred.  Nice!  It seems much nicer / cleaner.  Still a tiny bit
of voodoo, but with all of your improvements the voodoo is at least
contained to a very small part of the file.

OK, things seem pretty nice with this and I'll submit out a new patch.
I'm a bit conflicted about whether I should put myself as authorship
or you, since mostly the patch is your code now.  ;-)  For now I'll
leave my authorship but feel free to claim it and change me to just
"Suggested-by".  :-)

NOTE: one thing I noticed about your example code is that you weren't
always consistent about $(1) vs. $1.  I've changed things to be
consistent at the expense of extra parenthesis.  If you hate it, let
me know.


> # $(call,shell-cached,my_command)
> # This works like $(shell my_command), but the result is cached
> __shell-cached = $(eval $(call __run-and-store,$1))$($1)
> shell-cached = $(call __shell-cached,__cached_$(call __sanitize-opt,$(1)),$1)
>
> ...
>
>
> __try-run = set -e; \
>      TMP="$(TMPOUT).$$$$.tmp"; \
>      TMPO="$(TMPOUT).$$$$.o"; \
>      if ($(1)) >/dev/null 2>&1; \
>      then echo "$(2)"; \
>      else echo "$(3)"; \
>      fi; \
>      rm -f "$$TMP" "$$TMPO"
>
> # try-run
> # Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
> # Exit code chooses option. "$$TMP" serves as a temporary file and is
> # automatically cleaned up.
> try-run = $(shell $(__try-run))
>
> # try-run-cached
> # This works like try-run, but the result is cached.
> try-run-cached = $(call shell-cached,$(__try-run))
>
>
>
>
> Then, you can replace
>
>   $(shell $(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh $(CC) $(KBUILD_CFLAGS)
>
> with
>
>   $(call shell-cached,$(CONFIG_SHELL) $(srctree)/scripts/gcc-goto.sh
> $(CC) $(KBUILD_CFLAGS)
>
>
>
>
> replace
>
> __cc-option = $(call try-run,\
>          $(1) -Werror $(2) $(3) -c -x c /dev/null -o "$$TMP",$(3),$(4))
>
> with
>
> __cc-option = $(call try-run-cached,\
>          $(1) -Werror $(2) $(3) -c -x c /dev/null -o "$$TMP",$(3),$(4))
>
>
>
>
> What do you think?

Yes, much better!  Due to where you've placed this I now seem to get
by without my $(call __comma,...) calls and also the "$$$$"
conversion.  Yay!

...interestingly I need to add ":" to the "__sanitize-opt" now,
though.  That's for the arm64 line:

  brokengasinst := $(call as-instr,1:\n.inst 0\n.rept . -
1b\n\nnop\n.endr\n,,-DCONFIG_BROKEN_GAS_INST=1)



> A little more comments below.
>
>
>
>
>> +# Tools for caching Makefile variables that are "expensive" to compute.
>> +#
>> +# Here we want to help deal with variables that take a long time to compute
>> +# by making it easy to store these variables in a cache.
>> +#
>> +# The canonical example here is testing for compiler flags.  On a simple system
>> +# each call to the compiler takes 10 ms, but on a system with a compiler that's
>> +# called through various wrappers it can take upwards of 100 ms.  If we have
>> +# 100 calls to the compiler this can take 1 second (on a simple system) or 10
>> +# seconds (on a complicated system).
>> +#
>> +# The "cache" will be in Makefile syntax and can be directly included.
>> +# Any time we try to reference a variable that's not in the cache we'll
>> +# calculate it and store it in the cache for next time.
>> +
>> +# Include values from last time
>> +-include Makefile-cache.o
>
>
> Kbuild.include is included, so is Makefile-cache.o
> every time the build system descend into sub-directories.
>
> It is not efficient to include cached data unneeded in sub-directories.
> I prefixed $(obj)/
>
> Another problem is when building external modules.
> Makefile-cache.o is always created/updated in the kernel build tree.
>
> When we build external modules, the kernel source may be located under
> /usr/src/ , which is generally read-only for normal users.
> So, I prefixed $(KBUILD_EXTMOD) to create the cache file
> in the module tree if KBUILD_EXTMOD is defined.
>
> I do not like the suffix .o
> I prefer file name to be something else
> that starts with . to hide it.

Thanks for all the improvements!


>> +# Usage:   $(call cached-val,variable_name,escaped_expensive_operation)
>> +# Example: $(call cached-val,md5_val,$$(shell md5sum /usr/bin/gcc)
>> +#
>> +# If the variable is already defined we'll just use it.  If it's not, it will
>> +# be calculated and stored in a persistent (disk-based) cache for the next
>> +# invocation of Make.  The call will evaluate to the value of the variable.
>> +#
>> +# This is a bit of voodoo, but basic explanation is that if the variable
>> +# was undefined then we'll evaluate the expensive operation and store it into
>> +# the variable.  We'll then store that value in the cache and finally output
>> +# the value.
>> +define __set-and-store
>> +ifeq ($(origin $(1)),undefined)
>> +  $$(eval $(1) := $$(2))
>> +  $$(shell echo '$(1) := $$($(1))' >> Makefile-cache.o)
>> +endif
>> +endef
>> +cached-val = $(eval $(call __set-and-store,__cached_$(1)))$(__cached_$(1))
>
>
> This seems working, but I do not understand this trick.
>
>
> __set-and-store takes two arguments,
> but here, it is called with one argument __cached_$(1)
>
> Probably, $$(try-run, ...) will be passed as $2,
> but I do not know why this works.

Whew!  It's not just me that's confused by all this...  ;-)  It looks
like you continued to use this in your suggestion, so I guess you
thought it was useful, at least...  Yeah, it's a bit of magic.  The
goal was to have one less set of evaluating and passing going around.

So really '__set-and-store' takes _1_ argument.  It outputs a string
where it uses this argument.  Also part of the output string is a
reference to $(2).  This will refer to the caller's second variable.

===

Maybe a "simpler" example?

define example
$$(eval $(1) := $$(2))
endef

ex_usage = $(eval $(call example,$(1)))$($(1))

.PHONY: ex
ex:
  @echo $(call ex_usage,myvar,myval)
  @echo $(myvar)

If you do "make ex" you'll see "myval" printed twice.

--

Walking through that, let's first remove the "call" which we can do in
this case pretty easily because there are no "ifdefs", unlike the real
code.  Since the first argument to "example" is $(1), the above is the
same as:

ex_usage = $(eval $$(eval $(1) := $$(2)))$($(1))

...now we need to resolve that recursively where $(1) is 'myvar' and
$(2) is 'myval'.  So we get:

1. $(eval $$(eval $(1) := $$(2)))$($(1))
2. $(eval $$(eval myvar := $$(2)))$(myvar)
3. $(eval myvar := $(2))$(myvar)
4. $(eval myvar := myval)$(myvar)

--

An alternate version of my "simpler" example that doesn't use this magic:

define example
$$(eval $(1) := $(2))
endef

ex_usage = $(eval $(call example,$(1),$(2)))$($(1))

.PHONY: ex
ex:
  @echo $(call ex_usage,myvar,myval)
  @echo $(myvar)

...that works OK for the simple example case but you get into weird
esoteric issues because of the extra eval.  You'll find that the two
definitions behave differently for:

  @echo $(call ex_usage,myvar,my$$$$(cat /proc/cmdline)val)

...in that example the "intention" is that bash should be passed the
string: echo my$(cat /proc/cmdline)val, so it escapes the "$" once to
"defer" it and twice because it wants the "$" to go through to bash.

--

...and yes, I already punched myself in the face for the complexity of
all of the above.  ;-)

It's certainly possible that there's some way to get rid of an eval in
the above.  If you have a suggestion that would reduce the number
needed that'd be interesting.


>> +# Usage: $(call __sanitize-opt,Hello=Hola$(comma)Goodbye Adios)
>> +#
>> +# Convert all '$', ')', '(', '\', '=', ' ', ',' to '_'
>> +__sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$(subst \,_,$(subst =,_,$(subst $(space),_,$(subst $(comma),_,$(1))))))))
>> +
>> +# Usage = $(call __comma,something_with_comma)
>> +#
>> +# Convert ',' to '$(comma)' which can help it getting interpreted by eval.
>> +__comma = $(subst $(comma),$$(comma),$(1))
>> +
>>  # cc-cross-prefix
>>  # Usage: CROSS_COMPILE := $(call cc-cross-prefix, m68k-linux-gnu- m68k-linux-)
>>  # Return first prefix where a prefix$(CC) is found in PATH.
>> @@ -99,19 +148,34 @@ try-run = $(shell set -e;          \
>>  # as-option
>>  # Usage: cflags-y += $(call as-option,-Wa$(comma)-isa=foo,)
>>
>> -as-option = $(call try-run,\
>> -       $(CC) $(KBUILD_CFLAGS) $(1) -c -x assembler /dev/null -o "$$TMP",$(1),$(2))
>> +as-option = \
>> +       $(call cached-val,$(call __sanitize-opt,\
>> +               as_opt_$(CC)_$(KBUILD_CFLAGS)_$(1)_$(2)),\
>> +       $$(call try-run,\
>> +       $(CC) $(call __comma,$(KBUILD_CFLAGS)) $(call __comma,$(1)) \
>> +       -c -x assembler /dev/null \
>> +       -o "$$$$TMP",$(call __comma,$(1)),$(call __comma,$(2))))
>>
>>  # as-instr
>>  # Usage: cflags-y += $(call as-instr,instr,option1,option2)
>>
>> -as-instr = $(call try-run,\
>> -       printf "%b\n" "$(1)" | $(CC) $(KBUILD_AFLAGS) -c -x assembler -o "$$TMP" -,$(2),$(3))
>> +as-instr = \
>> +       $(call cached-val,$(call __sanitize-opt,\
>> +               as_instr_$(CC)_$(KBUILD_AFLAGS)_$(1)_$(2)_$(3)),\
>> +       $$(call try-run,\
>> +       printf "%b\n" "$(call __comma,$(1))" | \
>> +       $(CC) $(call __comma,$(KBUILD_AFLAGS)) -c -x assembler \
>> +       -o "$$$$TMP" -,$(call __comma,$(2)),$(call __comma,$(3))))
>>
>>  # __cc-option
>>  # Usage: MY_CFLAGS += $(call __cc-option,$(CC),$(MY_CFLAGS),-march=winchip-c6,-march=i586)
>> -__cc-option = $(call try-run,\
>> -       $(1) -Werror $(2) $(3) -c -x c /dev/null -o "$$TMP",$(3),$(4))
>> +__cc-option = \
>> +       $(call cached-val,$(call __sanitize-opt,__cc_opt_$(1)_$(2)_$(3)_$(4)),\
>> +       $$(call try-run,\
>> +       $(call __comma,$(1)) -Werror \
>> +       $(call __comma,$(2)) \
>> +       $(call __comma,$(3)) -c -x c /dev/null \
>> +       -o "$$$$TMP",$(call __comma,$(strip $(3))),$(call __comma,$(strip $(4)))))
>
>
> I did not follow how this was working here...

Hoo boy.  Let's see...  Which part were you curious about?  I can try
to explain the bits and you can tell me if I got them all.  Note that
many of these aren't super important anymore since they're not needed
with your improvements:


* $(call __sanitize-opt,__cc_opt_$(1)_$(2)_$(3)_$(4)):

Tries to make a variable name that will be different if you have any
different parameters.


* $$(call try-run

The "$$" around this call is because it's deferred and a general
concept for "cached-val".  If (and only if) we decide that we need to
recompute this value then we'll run this through an "eval".  By having
the $$ then this action won't be taken unless the string is eval-ed.
Note that plenty of stuff _will_ still happen right away, though.  All
of the $(call __comma,$(1)) are _not_ deferred.  That's fine since
that doesn't lead to a fork-exec and thus isn't "expensive".


*  $(call __comma,$(1))

I'll admit that I didn't dig all the way into this.  I found at least
one case where "make" was incorrectly mixing up arguments due to the
"eval" and the "__comma" fixed it.  I decided it was important to do
this for all arguments.  Possibly other things might need to be
escaped too, but I didn't find any cases that needed it...  ...but no
longer needed with your improvements...


* $$$$TMP

An extra level of escaping is needed since it goes through an extra
"eval".  This means $$ => $$$$


* $(strip $(3))

This was important because otherwise the cache otherwise wasn't
completely "stable".  If you ran "make" the first time you'd get a
cache, then running "make" the 2nd time would give you extra entries
in the cache.  After the 3rd time you were OK, but it seemed nice to
make it stable faster.

To understand this, first understand that some people call __cc_option
with a space after the ','.  Make treats this as important, thus these
two are different:

val := $(call cc-option, -opt1, -opt2)
val := $(call cc-option,-opt1,-opt2)

In one case val would be " -opt1" or " -opt2".  In the other case
"-opt1" or "-opt2".

However, when we cache things the space suddenly becomes irrelevant.
That's because we've end up with something like:

  __cached_result :=  -opt1

...and even though there are two spaces after the ":=" make throws that away.

The result of all of that is that we'd get slightly different numbers
of spaces after the cache was populated.  That could probably be
ignored except that some flags are additive and previous flags are
passed as arguments to later calls to __cc-option.  That means that
previous flags are passed to __sanitize-opt.  ...and in __sanitize-opt
spaces are significant.  ...so you'd come up with a slightly different
variable name on the first run and the subsequent runs...

Probably waaaay more than you expected for a simple $(strip) call, huh?

In any case this doesn't seem to matter anymore with your improvements
(I didn't dig into why), though I have noticed that the cache does
still return slightly different spacing for some of the results...

---

OK, v2 coming right up!  Thanks again for all your help on this!


-Doug

Re: [RFC PATCH 1/2] kbuild: Add a cache for generated variables

[Masahiro Yamada]

Hi Douglas,


2017-10-05 7:38 GMT+09:00 Doug Anderson <dianders@chromium.org>:
> Hi,
>
> On Tue, Oct 3, 2017 at 9:05 PM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> Thanks for the patches.  The idea is interesting.
>>
>> I am not a Chrome developer, but cc-option could be improved somehow.
>>
>>
>> I examined two approaches to mitigate the pain.
>>
>> [1] Skip cc-option completely when we run non-build targets
>>     such as "make help", "make clean", etc.
>>
>> [2] Cache the result of cc-option like this patch
>>
>>
>> I wrote some patches for [1]
>> https://patchwork.kernel.org/patch/9983825/
>> https://patchwork.kernel.org/patch/9983829/
>> https://patchwork.kernel.org/patch/9983833/
>> https://patchwork.kernel.org/patch/9983827/
>>
>> Comments are welcome.  :)
>
> OK, I'll take a look at them.  I'm not massively familiar with the
> kernel Makefiles, but hopefully I can figure some of it out.  :-P
>
>
>> [1] does not solve the slowness in the incremental build.
>> Instead, it makes non-build targets faster
>> from a clean source tree because cc-option is zero cost.
>>
>>
>> [2] solves the issues in the incremental build.
>> One funny thing is, it computes cc-option and store the cache file
>> even for "make clean", where we know the cache file will be
>> immediately deleted.
>>
>>
>> We can apply [1] or [2], or maybe both of them
>> because [1] and [2] are trying to solve the different issues.
>
> Yeah, I'm much more interested in [2] than [1].  I run incremental
> builds almost constantly and hate the slowness.  :(  I can certainly
> appreciate that the inefficient compiler setup in Chrome OS isn't your
> problem and that an extra .5 seconds for an incremental build for most
> people isn't that huge, though.  ...but I'l probably move forward with
> [2] since it still helps me a lot and still should help others.
> Solving [1] seems worthwhile too, though...


I agree.

[2] is more helpful because developers spend most of time
for the incremental build.

[1] can improve it even more, but it just addresses some minor issues.



>
>> The cache approach seemed clever, but I do not like the implementation.
>> The code is even more unreadable with lots of escape sequence.
>>
>>
>> Here is my suggestion for simpler implementation (including bike-shed)
>>
>>
>> Implement a new function "shell-cache".  It works like $(shell ...)
>>
>> The difference is
>> $(call shell-cached,...) returns the cached result, or run the command
>> if not cached.
>>
>>
>>
>> Also, add try-run-cached.  This is a cached variant of try-run.
>>
>>
>> I just played with it, and seems working.
>> (I did not have spend much time for testing, more wider test is needed.)
>>
>>
>> The code is like something like this:
>>
>>
>>
>> make-cache := $(if $(KBUILD_EXTMOD),$(KBUILD_EXTMOD)/,$(if
>> $(obj),$(obj)/)).cache.mk
>>
>> -include $(make-cache)
>
> Thanks, this is much better!  :)
>
>
>> define __run-and-store
>> ifeq ($(origin $(1)),undefined)
>>   $$(eval $(1) := $$(shell $$2))
>>   $$(shell echo '$(1) := $$($(1))' >> $(make-cache))
>> endif
>> endef
>
> I like your idea.  Essentially you're saying that we can just defer
> the shell command, which was the really essential part that needed to
> be deferred.  Nice!  It seems much nicer / cleaner.  Still a tiny bit
> of voodoo, but with all of your improvements the voodoo is at least
> contained to a very small part of the file.
>
> OK, things seem pretty nice with this and I'll submit out a new patch.
> I'm a bit conflicted about whether I should put myself as authorship
> or you, since mostly the patch is your code now.  ;-)  For now I'll
> leave my authorship but feel free to claim it and change me to just
> "Suggested-by".  :-)


No worry.
Without your patches, I would have never come up with this.
The code improvement happened in general review process.



> NOTE: one thing I noticed about your example code is that you weren't
> always consistent about $(1) vs. $1.  I've changed things to be
> consistent at the expense of extra parenthesis.  If you hate it, let
> me know.

I accept your choice.
Keeping the consistent coding style is good.


If we decide to use $1 instead of $(1),
we should convert all instances tree-wide
for consistency.



>>
>> __set-and-store takes two arguments,
>> but here, it is called with one argument __cached_$(1)
>>
>> Probably, $$(try-run, ...) will be passed as $2,
>> but I do not know why this works.
>
> Whew!  It's not just me that's confused by all this...  ;-)  It looks
> like you continued to use this in your suggestion, so I guess you
> thought it was useful, at least...  Yeah, it's a bit of magic.  The
> goal was to have one less set of evaluating and passing going around.
>
> So really '__set-and-store' takes _1_ argument.  It outputs a string
> where it uses this argument.  Also part of the output string is a
> reference to $(2).  This will refer to the caller's second variable.
>
> ===
>
> Maybe a "simpler" example?
>
> define example
> $$(eval $(1) := $$(2))
> endef
>
> ex_usage = $(eval $(call example,$(1)))$($(1))
>
> .PHONY: ex
> ex:
>   @echo $(call ex_usage,myvar,myval)
>   @echo $(myvar)
>
> If you do "make ex" you'll see "myval" printed twice.
>
> --
>
> Walking through that, let's first remove the "call" which we can do in
> this case pretty easily because there are no "ifdefs", unlike the real
> code.  Since the first argument to "example" is $(1), the above is the
> same as:
>
> ex_usage = $(eval $$(eval $(1) := $$(2)))$($(1))
>
> ...now we need to resolve that recursively where $(1) is 'myvar' and
> $(2) is 'myval'.  So we get:
>
> 1. $(eval $$(eval $(1) := $$(2)))$($(1))
> 2. $(eval $$(eval myvar := $$(2)))$(myvar)
> 3. $(eval myvar := $(2))$(myvar)
> 4. $(eval myvar := myval)$(myvar)


This example is very helpful!

Now I understood this.
Thanks for clear explanation!



> --
>
> An alternate version of my "simpler" example that doesn't use this magic:
>
> define example
> $$(eval $(1) := $(2))
> endef
>
> ex_usage = $(eval $(call example,$(1),$(2)))$($(1))
>
> .PHONY: ex
> ex:
>   @echo $(call ex_usage,myvar,myval)
>   @echo $(myvar)
>
> ...that works OK for the simple example case but you get into weird
> esoteric issues because of the extra eval.  You'll find that the two
> definitions behave differently for:
>
>   @echo $(call ex_usage,myvar,my$$$$(cat /proc/cmdline)val)
>
> ...in that example the "intention" is that bash should be passed the
> string: echo my$(cat /proc/cmdline)val, so it escapes the "$" once to
> "defer" it and twice because it wants the "$" to go through to bash.


You are right.

If we want to get the same result by using the alternate version,
we need one more level of escaping, so end up with crazy escaping:

      @echo $(call ex_usage,myvar,my$$$$$$$$(cat /proc/cmdline)val)

So, the magic is unclear, but very helpful.



>>>
>>>  # __cc-option
>>>  # Usage: MY_CFLAGS += $(call __cc-option,$(CC),$(MY_CFLAGS),-march=winchip-c6,-march=i586)
>>> -__cc-option = $(call try-run,\
>>> -       $(1) -Werror $(2) $(3) -c -x c /dev/null -o "$$TMP",$(3),$(4))
>>> +__cc-option = \
>>> +       $(call cached-val,$(call __sanitize-opt,__cc_opt_$(1)_$(2)_$(3)_$(4)),\
>>> +       $$(call try-run,\
>>> +       $(call __comma,$(1)) -Werror \
>>> +       $(call __comma,$(2)) \
>>> +       $(call __comma,$(3)) -c -x c /dev/null \
>>> +       -o "$$$$TMP",$(call __comma,$(strip $(3))),$(call __comma,$(strip $(4)))))
>>
>>
>> I did not follow how this was working here...
>
> Hoo boy.  Let's see...  Which part were you curious about?  I can try
> to explain the bits and you can tell me if I got them all.  Note that
> many of these aren't super important anymore since they're not needed
> with your improvements:

Right, all of these are not necessary any more.
I did not mean that you should explain all of these.

I did not want read them,
so I started to consider simpler idea that I can understand.

Sorry if you used much time for me,
but this discussion helped me a lot to understand it deeply.

>
>
> * $(call __sanitize-opt,__cc_opt_$(1)_$(2)_$(3)_$(4)):
>
> Tries to make a variable name that will be different if you have any
> different parameters.

Yes, this is an easy part.

>
> * $$(call try-run
>
> The "$$" around this call is because it's deferred and a general
> concept for "cached-val".  If (and only if) we decide that we need to
> recompute this value then we'll run this through an "eval".  By having
> the $$ then this action won't be taken unless the string is eval-ed.
> Note that plenty of stuff _will_ still happen right away, though.  All
> of the $(call __comma,$(1)) are _not_ deferred.  That's fine since
> that doesn't lead to a fork-exec and thus isn't "expensive".

Actually, I could not understand how this worked in v1,
but with the help of your simplified example above,
the intention is clear.  Thanks.



> *  $(call __comma,$(1))
>
> I'll admit that I didn't dig all the way into this.  I found at least
> one case where "make" was incorrectly mixing up arguments due to the
> "eval" and the "__comma" fixed it.  I decided it was important to do
> this for all arguments.  Possibly other things might need to be
> escaped too, but I didn't find any cases that needed it...  ...but no
> longer needed with your improvements...

OK, this is probably for corner cases, not that important part.

>
> * $$$$TMP
>
> An extra level of escaping is needed since it goes through an extra
> "eval".  This means $$ => $$$$

This is now clear to me
with the simplified example and its alternate version.


>
> * $(strip $(3))
>
> This was important because otherwise the cache otherwise wasn't
> completely "stable".  If you ran "make" the first time you'd get a
> cache, then running "make" the 2nd time would give you extra entries
> in the cache.  After the 3rd time you were OK, but it seemed nice to
> make it stable faster.
>
> To understand this, first understand that some people call __cc_option
> with a space after the ','.  Make treats this as important, thus these
> two are different:
>
> val := $(call cc-option, -opt1, -opt2)
> val := $(call cc-option,-opt1,-opt2)
>
> In one case val would be " -opt1" or " -opt2".  In the other case
> "-opt1" or "-opt2".
>
> However, when we cache things the space suddenly becomes irrelevant.
> That's because we've end up with something like:
>
>   __cached_result :=  -opt1
>
> ...and even though there are two spaces after the ":=" make throws that away.
>
> The result of all of that is that we'd get slightly different numbers
> of spaces after the cache was populated.  That could probably be
> ignored except that some flags are additive and previous flags are
> passed as arguments to later calls to __cc-option.  That means that
> previous flags are passed to __sanitize-opt.  ...and in __sanitize-opt
> spaces are significant.  ...so you'd come up with a slightly different
> variable name on the first run and the subsequent runs...
>
> Probably waaaay more than you expected for a simple $(strip) call, huh?

Understood.
The whitespace problem is nasty.


> In any case this doesn't seem to matter anymore with your improvements
> (I didn't dig into why), though I have noticed that the cache does
> still return slightly different spacing for some of the results...
>

As far as I tested, I always see only one space after ":=" in v2.

I did not consider this deeply,
but something is working nicely behind the scene.





-- 
Best Regards
Masahiro Yamada

Re: [RFC PATCH 1/2] kbuild: Add a cache for generated variables

[Doug Anderson]

Hi,

On Thu, Oct 5, 2017 at 12:26 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> As far as I tested, I always see only one space after ":=" in v2.
>
> I did not consider this deeply,
> but something is working nicely behind the scene.

Try adding this to the end of the main Makefile:

+$(info LDFLAGS_BUILD_ID = $(LDFLAGS_BUILD_ID))
+$(info KBUILD_ARFLAGS = $(KBUILD_ARFLAGS))
+$(info KBUILD_CFLAGS = $(KBUILD_CFLAGS))
+$(info KBUILD_AFLAGS = $(KBUILD_AFLAGS))
+$(info KBUILD_CPPFLAGS = $(KBUILD_CPPFLAGS))
+$(info REALMODE_CFLAGS = $(REALMODE_CFLAGS))
+
 endif  # skip-makefile

Record what you see.  Then apply my patches and run your build again
(actually, run it twice and look at the 2nd time, just to be sure).  I
think you'll see slightly different spacing in the flags for the two
runs.  I don't think this is terribly important, though.

-Doug

Re: [RFC PATCH 1/2] kbuild: Add a cache for generated variables

[Masahiro Yamada]

2017-10-06 5:58 GMT+09:00 Doug Anderson <dianders@chromium.org>:
> Hi,
>
> On Thu, Oct 5, 2017 at 12:26 AM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> As far as I tested, I always see only one space after ":=" in v2.
>>
>> I did not consider this deeply,
>> but something is working nicely behind the scene.
>
> Try adding this to the end of the main Makefile:
>
> +$(info LDFLAGS_BUILD_ID = $(LDFLAGS_BUILD_ID))
> +$(info KBUILD_ARFLAGS = $(KBUILD_ARFLAGS))
> +$(info KBUILD_CFLAGS = $(KBUILD_CFLAGS))
> +$(info KBUILD_AFLAGS = $(KBUILD_AFLAGS))
> +$(info KBUILD_CPPFLAGS = $(KBUILD_CPPFLAGS))
> +$(info REALMODE_CFLAGS = $(REALMODE_CFLAGS))
> +
>  endif  # skip-makefile
>
> Record what you see.  Then apply my patches and run your build again
> (actually, run it twice and look at the 2nd time, just to be sure).  I
> think you'll see slightly different spacing in the flags for the two
> runs.  I don't think this is terribly important, though.
>

Yes.  I see slight difference
in the resulted KBUILD_CFLAGS etc.

What I meant is:

In v2, I always see only one space after the assignment operator ":="
in .cache.mk
even without $(strip )


$(call cc-option,-fno-PIE)
$(call cc-option, -fno-PIE)
$(call cc-option,                   -fno-PIE)


All of the three gave me the same result.




-- 
Best Regards
Masahiro Yamada