[PATCH v2 0/2] kbuild: Fix corner caches with .cache.mk

[PATCH v2 0/2] kbuild: Fix corner caches with .cache.mk

[Douglas Anderson]

This two-patches fixes two corner cases with .cache.mk that have been
reported recently.  Neither problem was catastrophic, but certainly
several people ran into the problem solved by the first patch (can't
build after gcc upgrade) and wasted time debugging, so it's really a
good idea to fix.

For both patches, please make sure to give them an extra thorough
review.  I _think_ I understand enough about $(MAKECMDGOALS), but I'd
never explored that feature of make before writing these patches so
the patches certainly need someone more experienced to give them a
careful look.

I've only got one more day of work before I'm on Christmas vacation
for 2 weeks, so if there are problems with these patches please give
me a bit of time to fix.  ...or, if someone is in a hurry, I wouldn't
object to someone else hijacking them and posting fixes.

Changes in v2:
- Don't error if MAKECMDGOALS is blank.

Douglas Anderson (2):
  kbuild: Require a 'make clean' if we detect gcc changed underneath us
  kbuild: Don't mess with the .cache.mk when installing

 scripts/Kbuild.include | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

-- 
2.15.1.620.gb9897f4670-goog

[PATCH v2 1/2] kbuild: Require a 'make clean' if we detect gcc changed underneath us

[Douglas Anderson]

Several people reported that the commit 3298b690b21c ("kbuild: Add a
cache for generated variables") caused them problems when they updated
gcc versions.  Specifically the reports all looked something similar
to this:

> In file included from ./include/uapi/linux/uuid.h:21:0,
>                  from ./include/linux/uuid.h:19,
>                  from ./include/linux/mod_devicetable.h:12,
>                  from scripts/mod/devicetable-offsets.c:2:
> ./include/linux/string.h:8:20: fatal error: stdarg.h: No such file or
>                  directory
>  #include <stdarg.h>

Masahiro Yamada determined that the problem was with:

  NOSTDINC_FLAGS += -nostdinc -isystem $(call shell-cached,$(CC)
  -print-file-name=include)

Specifically that the stale result of -print-file-name is stored in
the cache file.  It was determined that a "make clean" fixed the
problems in all cases.

In this particular case we could certainly try to clean just the cache
when we detect a gcc update, but it seems like overall it's a bad idea
to do an incremental build when gcc changes.  We should warn the user
and tell them that they need a 'make clean'.

Fixes: 3298b690b21c ("kbuild: Add a cache for generated variables")
Reported-by: Yang Shi <yang.s@alibaba-inc.com>
Reported-by: Dave Hansen <dave.hansen@intel.com>
Reported-by: Mathieu Malaterre <malat@debian.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Don't error if MAKECMDGOALS is blank.

 scripts/Kbuild.include | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 065324a8046f..f7efb59d85d1 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -222,6 +222,10 @@ cc-version = $(call shell-cached,$(CONFIG_SHELL) $(srctree)/scripts/gcc-version.
 cc-fullversion = $(call shell-cached,$(CONFIG_SHELL) \
 	$(srctree)/scripts/gcc-version.sh -p $(CC))
 
+# cc-fullversion-uncached
+cc-fullversion-uncached := $(shell $(CONFIG_SHELL) \
+	$(srctree)/scripts/gcc-version.sh -p $(CC))
+
 # cc-ifversion
 # Usage:  EXTRA_CFLAGS += $(call cc-ifversion, -lt, 0402, -O1)
 cc-ifversion = $(shell [ $(cc-version) $(1) $(2) ] && echo $(3) || echo $(4))
@@ -475,3 +479,16 @@ endif
 endef
 #
 ###############################################################################
+
+# Require a 'make clean' if the compiler changed; not only does the .cache.mk
+# need to be thrown out but we should also start with fresh object files.
+#
+# NOTE: it's important that we don't error out when the goal is actually to
+# try to make clean, distclean or mrproper.
+ifeq ($(filter %clean,$(MAKECMDGOALS))$(filter mrproper,$(MAKECMDGOALS)),)
+  ifneq ($(MAKECMDGOALS),)
+    ifneq ($(cc-fullversion-uncached),$(cc-fullversion))
+      $(error Detected new CC version ($(cc-fullversion-uncached) vs $(cc-fullversion)).  Please 'make clean')
+    endif
+  endif
+endif
-- 
2.15.1.620.gb9897f4670-goog

Re: [PATCH v2 1/2] kbuild: Require a 'make clean' if we detect gcc changed underneath us

[Masahiro Yamada]

Hi Douglas,

2017-12-23 7:14 GMT+09:00 Douglas Anderson <dianders@chromium.org>:
> Several people reported that the commit 3298b690b21c ("kbuild: Add a
> cache for generated variables") caused them problems when they updated
> gcc versions.  Specifically the reports all looked something similar
> to this:
>
>> In file included from ./include/uapi/linux/uuid.h:21:0,
>>                  from ./include/linux/uuid.h:19,
>>                  from ./include/linux/mod_devicetable.h:12,
>>                  from scripts/mod/devicetable-offsets.c:2:
>> ./include/linux/string.h:8:20: fatal error: stdarg.h: No such file or
>>                  directory
>>  #include <stdarg.h>
>
> Masahiro Yamada determined that the problem was with:
>
>   NOSTDINC_FLAGS += -nostdinc -isystem $(call shell-cached,$(CC)
>   -print-file-name=include)
>
> Specifically that the stale result of -print-file-name is stored in
> the cache file.  It was determined that a "make clean" fixed the
> problems in all cases.
>
> In this particular case we could certainly try to clean just the cache
> when we detect a gcc update, but it seems like overall it's a bad idea
> to do an incremental build when gcc changes.  We should warn the user
> and tell them that they need a 'make clean'.
>
> Fixes: 3298b690b21c ("kbuild: Add a cache for generated variables")
> Reported-by: Yang Shi <yang.s@alibaba-inc.com>
> Reported-by: Dave Hansen <dave.hansen@intel.com>
> Reported-by: Mathieu Malaterre <malat@debian.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2:
> - Don't error if MAKECMDGOALS is blank.
>
>  scripts/Kbuild.include | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> index 065324a8046f..f7efb59d85d1 100644
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -222,6 +222,10 @@ cc-version = $(call shell-cached,$(CONFIG_SHELL) $(srctree)/scripts/gcc-version.
>  cc-fullversion = $(call shell-cached,$(CONFIG_SHELL) \
>         $(srctree)/scripts/gcc-version.sh -p $(CC))
>
> +# cc-fullversion-uncached
> +cc-fullversion-uncached := $(shell $(CONFIG_SHELL) \
> +       $(srctree)/scripts/gcc-version.sh -p $(CC))
> +

No.
You used ':=' flavor assignment, so the gcc-version.sh
script is evaluated here.

The top-level Makefile includes scripts/Kbuild.include at line 278,
then defines CC at line 284.

Since $(CC) is not defined here,
the resulted cc-fullversion-uncached is
"Error: No compiler specified. Usage:  ./scripts/gcc-version.sh <gcc-command>"


cc-fullversion-uncached works as expected
only after Kbuild descends into sub-directories.






>  # cc-ifversion
>  # Usage:  EXTRA_CFLAGS += $(call cc-ifversion, -lt, 0402, -O1)
>  cc-ifversion = $(shell [ $(cc-version) $(1) $(2) ] && echo $(3) || echo $(4))
> @@ -475,3 +479,16 @@ endif
>  endef
>  #
>  ###############################################################################
> +
> +# Require a 'make clean' if the compiler changed; not only does the .cache.mk
> +# need to be thrown out but we should also start with fresh object files.
> +#
> +# NOTE: it's important that we don't error out when the goal is actually to
> +# try to make clean, distclean or mrproper.
> +ifeq ($(filter %clean,$(MAKECMDGOALS))$(filter mrproper,$(MAKECMDGOALS)),)
> +  ifneq ($(MAKECMDGOALS),)
> +    ifneq ($(cc-fullversion-uncached),$(cc-fullversion))


As I noted above, CC is not defined yet when parsing the top-level Makefile.

Evaluating "cc-fullversion" here
adds a strange cache line, like follows:

__cached__./scripts/gcc-version.sh_-p_ := Error: No compiler
specified. Usage:  ./scripts/gcc-version.sh <gcc-command>


This check only works only in the second inclusion or later.

At the first inclusion,
both 'cc-fullversion-uncached' and 'cc-fullversion'
contain "Error: No compiler specified. ..."


scripts/Kbuild.include is included every time Kbuild
descends into a sub-directory.

It is pointless to check gcc version multiple times.



> +      $(error Detected new CC version ($(cc-fullversion-uncached) vs $(cc-fullversion)).  Please 'make clean')
> +    endif
> +  endif
> +endif
> --
> 2.15.1.620.gb9897f4670-goog
>

After all, I recommend to move this code into the top-level Makefile,
around line 585.


--- a/Makefile
+++ b/Makefile
@@ -585,6 +585,14 @@ virt-y             := virt/
 endif # KBUILD_EXTMOD

 ifeq ($(dot-config),1)
+# Require a 'make clean' if the compiler changed; not only does the .cache.mk
+# need to be thrown out but we should also start with fresh object files.
+cc-fullversion-uncached := $(shell $(CONFIG_SHELL)
$(srctree)/scripts/gcc-version.sh -p $(CC))
+
+ifneq ($(cc-fullversion-uncached),$(cc-fullversion))
+   $(error Detected new CC version ($(cc-fullversion-uncached) vs
$(cc-fullversion)).  Please 'make clean')
+endif
+
 # Read in config
 -include include/config/auto.conf




-- 
Best Regards
Masahiro Yamada

[PATCH v2 2/2] kbuild: Don't mess with the .cache.mk when installing

[Douglas Anderson]

As pointed out by Masahiro Yamada people often run "sudo make install"
or "sudo make modules_install".  In theory, that could cause a cache
file (or the directory containing it) to be created by root.  After
doing this then subsequent invocations to make would yell with a whole
bunch of warnings like:

  /bin/sh: ./.cache.mk: Permission denied

These yells would be mostly harmless (we'd just go on running without
the cache), but they're ugly.

The above situation would be fairly unlikely because it should only
happen if someone does "sudo make install" before doing a normal
"make", which is an invalid thing to do.  If you did a normal "make"
before the "sudo make install" then all the cache files and
directories should have already been created by a normal user and,
even if the superuser needed to add to the existing files, we wouldn't
end up with a permission problem.

The above situation would also not have been catastrophic because
presumably if the user was able to run "sudo make install" then they
should also be able to run "sudo make clean" to clean things up.

However, even though the problem described is relatively minor, it
probably makes sense to add a heuristic to avoid creating cache files
when one of the make goals looks like an install.  This heuristic
doesn't add a ton of overhead and it might save someone time tracking
down strange "Permission denied" messages.  We'll consider this
heuristic good enough because the problem really shouldn't be that
serious.

Fixes: 3298b690b21c ("kbuild: Add a cache for generated variables")
Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2: None

 scripts/Kbuild.include | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index f7efb59d85d1..314585b3efe4 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -124,6 +124,12 @@ __sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$
 # previous invocation of make!) we'll return the value.  If not, we'll
 # compute it and store the result for future runs.
 #
+# NOTE: we won't ever add to the cache if one of the make goals looks like
+# it is installing.  Technically there's no reason we can't cache things
+# related to install but it's likely that 'make install' is called as root.
+# If we create the cache file as root we might have a hard time adding to it
+# (or deleting it in make clean).
+#
 # This is a bit of voodoo, but basic explanation is that if the variable
 # was undefined then we'll evaluate the shell command and store the result
 # into the variable.  We'll then store that value in the cache and finally
@@ -137,12 +143,14 @@ __sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$
 define __run-and-store
 ifeq ($(origin $(1)),undefined)
   $$(eval $(1) := $$(shell $$(2)))
+ifeq ($(filter %install,$(MAKECMDGOALS)),)
 ifeq ($(create-cache-dir),1)
   $$(shell mkdir -p $(dir $(make-cache)))
   $$(eval create-cache-dir :=)
 endif
   $$(shell echo '$(1) := $$($(1))' >> $(make-cache))
 endif
+endif
 endef
 __shell-cached = $(eval $(call __run-and-store,$(1)))$($(1))
 shell-cached = $(call __shell-cached,__cached_$(call __sanitize-opt,$(1)),$(1))
-- 
2.15.1.620.gb9897f4670-goog

Re: [PATCH v2 2/2] kbuild: Don't mess with the .cache.mk when installing

[Masahiro Yamada]

Hi Douglas,

2017-12-23 7:14 GMT+09:00 Douglas Anderson <dianders@chromium.org>:
> As pointed out by Masahiro Yamada people often run "sudo make install"
> or "sudo make modules_install".  In theory, that could cause a cache
> file (or the directory containing it) to be created by root.  After
> doing this then subsequent invocations to make would yell with a whole
> bunch of warnings like:
>
>   /bin/sh: ./.cache.mk: Permission denied
>
> These yells would be mostly harmless (we'd just go on running without
> the cache), but they're ugly.
>
> The above situation would be fairly unlikely because it should only
> happen if someone does "sudo make install" before doing a normal
> "make", which is an invalid thing to do.  If you did a normal "make"
> before the "sudo make install" then all the cache files and
> directories should have already been created by a normal user and,
> even if the superuser needed to add to the existing files, we wouldn't
> end up with a permission problem.
>
> The above situation would also not have been catastrophic because
> presumably if the user was able to run "sudo make install" then they
> should also be able to run "sudo make clean" to clean things up.
>
> However, even though the problem described is relatively minor, it
> probably makes sense to add a heuristic to avoid creating cache files
> when one of the make goals looks like an install.  This heuristic
> doesn't add a ton of overhead and it might save someone time tracking
> down strange "Permission denied" messages.  We'll consider this
> heuristic good enough because the problem really shouldn't be that
> serious.
>
> Fixes: 3298b690b21c ("kbuild: Add a cache for generated variables")
> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2: None
>
>  scripts/Kbuild.include | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> index f7efb59d85d1..314585b3efe4 100644
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -124,6 +124,12 @@ __sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$
>  # previous invocation of make!) we'll return the value.  If not, we'll
>  # compute it and store the result for future runs.
>  #
> +# NOTE: we won't ever add to the cache if one of the make goals looks like
> +# it is installing.  Technically there's no reason we can't cache things
> +# related to install but it's likely that 'make install' is called as root.
> +# If we create the cache file as root we might have a hard time adding to it
> +# (or deleting it in make clean).
> +#
>  # This is a bit of voodoo, but basic explanation is that if the variable
>  # was undefined then we'll evaluate the shell command and store the result
>  # into the variable.  We'll then store that value in the cache and finally
> @@ -137,12 +143,14 @@ __sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$
>  define __run-and-store
>  ifeq ($(origin $(1)),undefined)
>    $$(eval $(1) := $$(shell $$(2)))
> +ifeq ($(filter %install,$(MAKECMDGOALS)),)

Hmm, this may not always disable caching when installing something.

The check for %install is only effective at the top-level Makefile.
It may call sub-makefile such as scripts/Makefile.modinst,
scripts/Makefile.dtbinst,
etc. with empty MAKECMDGOALS.


Maybe, introduce a new flag, for example, KBUILD_NOCACHE?

I found KBUILD_NOCMDDEP flag in the same file,
so adding this flag should not be so odd.

(Please feel free to rename it if you have an idea for better naming)



In top Makefile, before including Kbuild.include,
then

ifeq ($(shell id -u),0)
export KBUILD_NOCACHE := 1
endif


I considered this a bit, then I thought
checking user id would have more sense...





>  ifeq ($(create-cache-dir),1)
>    $$(shell mkdir -p $(dir $(make-cache)))
>    $$(eval create-cache-dir :=)
>  endif
>    $$(shell echo '$(1) := $$($(1))' >> $(make-cache))
>  endif
> +endif
>  endef
>  __shell-cached = $(eval $(call __run-and-store,$(1)))$($(1))
>  shell-cached = $(call __shell-cached,__cached_$(call __sanitize-opt,$(1)),$(1))
> --
> 2.15.1.620.gb9897f4670-goog
>



-- 
Best Regards
Masahiro Yamada