Date:   Sun, 31 Dec 2017 16:46:47 +0900
From:   Masahiro Yamada <yamada.masahiro@...ionext.com>
To:     Douglas Anderson <dianders@...omium.org>
Cc:     malat@...ian.org, dave.hansen@...el.com,
        Yang Shi <yang.s@...baba-inc.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Matthias Kaehlcke <mka@...omium.org>,
        Cao jin <caoj.fnst@...fujitsu.com>,
        Arnd Bergmann <arnd@...db.de>,
        Marcin Nowakowski <marcin.nowakowski@...s.com>,
        Mark Charlebois <charlebm@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [PATCH v2 1/2] kbuild: Require a 'make clean' if we detect gcc
 changed underneath us

Hi Douglas,

2017-12-23 7:14 GMT+09:00 Douglas Anderson <dianders@...omium.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@...baba-inc.com>
> Reported-by: Dave Hansen <dave.hansen@...el.com>
> Reported-by: Mathieu Malaterre <malat@...ian.org>
> Signed-off-by: Douglas Anderson <dianders@...omium.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

Powered by blists - more mailing lists