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

[PATCH v3 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.  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.

Sorry for the big delay between v2 and v3.  I never quite caught up
with email after the holidays, but hopefully better late than never...

Changes in v3:
- Fix as per Masahiro Yamada (move change to main Makefile)
- Use "uid 0" as the heuristic instead of install
- Do the checking in the main Makefile instead of Kbuild.include

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 root

 Makefile               | 15 +++++++++++++++
 scripts/Kbuild.include |  2 ++
 2 files changed, 17 insertions(+)

-- 
2.16.2.660.g709887971b-goog

[PATCH v3 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 v3:
- Fix as per Masahiro Yamada (move change to main Makefile)

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

 Makefile | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Makefile b/Makefile
index c4322dea3ca2..f1e61470640b 100644
--- a/Makefile
+++ b/Makefile
@@ -573,6 +573,15 @@ 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
 
-- 
2.16.2.660.g709887971b-goog

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

[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 we're running as root.  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 v3:
- Use "uid 0" as the heuristic instead of install
- Do the checking in the main Makefile instead of Kbuild.include

Changes in v2: None

 Makefile               | 6 ++++++
 scripts/Kbuild.include | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/Makefile b/Makefile
index f1e61470640b..c4d2131831ba 100644
--- a/Makefile
+++ b/Makefile
@@ -268,6 +268,12 @@ __build_one_by_one:
 
 else
 
+# Don't create Makefile caches if running as root since they can't be deleted
+# easily; in the real world we might be root when doing "sudo make install"
+ifeq ($(shell id -u),0)
+export KBUILD_NOCACHE := 1
+endif
+
 # We need some generic definitions (do not try to remake the file).
 scripts/Kbuild.include: ;
 include scripts/Kbuild.include
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 065324a8046f..581f93f20119 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -137,12 +137,14 @@ __sanitize-opt = $(subst $$,_,$(subst $(right_paren),_,$(subst $(left_paren),_,$
 define __run-and-store
 ifeq ($(origin $(1)),undefined)
   $$(eval $(1) := $$(shell $$(2)))
+ifneq ($(KBUILD_NOCACHE),1)
 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.16.2.660.g709887971b-goog

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

[Ingo Molnar]

* Douglas Anderson <dianders@chromium.org> wrote:

> +# Don't create Makefile caches if running as root since they can't be deleted
> +# easily; in the real world we might be root when doing "sudo make install"
> +ifeq ($(shell id -u),0)
> +export KBUILD_NOCACHE := 1
> +endif

Please don't do this - many prominent kernel developers build their kernels as 
root - this makes the build slower for them, and also bifurcates testing.

Thanks,

	Ingo

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

[Doug Anderson]

Hi,

On Mon, Mar 12, 2018 at 11:16 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Douglas Anderson <dianders@chromium.org> wrote:
>
>> +# Don't create Makefile caches if running as root since they can't be deleted
>> +# easily; in the real world we might be root when doing "sudo make install"
>> +ifeq ($(shell id -u),0)
>> +export KBUILD_NOCACHE := 1
>> +endif
>
> Please don't do this - many prominent kernel developers build their kernels as
> root - this makes the build slower for them, and also bifurcates testing.

Ah, interesting.  I hadn't realized that!

I'm OK with dropping this patch myself.  It was mostly addressing a
potential problem pointed out by Masahiro Yamada when we were talking
about .cache.mk, but I don't think anyone has actually experienced the
problems listed in the CL description.  I suppose the other option
would be to go back to keying off the "install" target (like v2 of
this patch did), but it kinda feels like if someone did "sudo make
install" and then followed up by a non-sudo "make" they probably would
be able to figure out how to fix it without much trouble (just do a
"sudo make clean").

In case it's not obvious, though, we should still try to land patch #1
of this series.  That's a real problems that people reported when the
.cache.mk stuff first landed and then folks tried to upgrade their
gcc.

-Doug

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

[Nick Desaulniers]

On Mon, Mar 12, 2018 at 11:23 PM Doug Anderson <dianders@chromium.org>
wrote:

> Hi,

> On Mon, Mar 12, 2018 at 11:16 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Douglas Anderson <dianders@chromium.org> wrote:
> >
> >> +# Don't create Makefile caches if running as root since they can't be
deleted
> >> +# easily; in the real world we might be root when doing "sudo make
install"
> >> +ifeq ($(shell id -u),0)
> >> +export KBUILD_NOCACHE := 1
> >> +endif
> >
> > Please don't do this - many prominent kernel developers build their
kernels as
> > root - this makes the build slower for them, and also bifurcates
testing.

> Ah, interesting.  I hadn't realized that!

> I'm OK with dropping this patch myself.  It was mostly addressing a
> potential problem pointed out by Masahiro Yamada when we were talking
> about .cache.mk, but I don't think anyone has actually experienced the
> problems listed in the CL description.

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

I feel like I've definitely seen that permission error before.

Is there any issue if I:

$ make clean
$ make
$ sudo make install
<hack around>
$ make

Or it's only a problem if:

$ make clean
$ sudo make install
<hack around>
$ make
--
Thanks,
~Nick Desaulniers

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

[Doug Anderson]

Hi,

On Tue, Mar 13, 2018 at 9:33 AM, Nick Desaulniers
<ndesaulniers@google.com> wrote:
> On Mon, Mar 12, 2018 at 11:23 PM Doug Anderson <dianders@chromium.org>
> wrote:
>
>> Hi,
>
>> On Mon, Mar 12, 2018 at 11:16 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Douglas Anderson <dianders@chromium.org> wrote:
>> >
>> >> +# Don't create Makefile caches if running as root since they can't be
> deleted
>> >> +# easily; in the real world we might be root when doing "sudo make
> install"
>> >> +ifeq ($(shell id -u),0)
>> >> +export KBUILD_NOCACHE := 1
>> >> +endif
>> >
>> > Please don't do this - many prominent kernel developers build their
> kernels as
>> > root - this makes the build slower for them, and also bifurcates
> testing.
>
>> Ah, interesting.  I hadn't realized that!
>
>> I'm OK with dropping this patch myself.  It was mostly addressing a
>> potential problem pointed out by Masahiro Yamada when we were talking
>> about .cache.mk, but I don't think anyone has actually experienced the
>> problems listed in the CL description.
>
>>    /bin/sh: ./.cache.mk: Permission denied
>
> I feel like I've definitely seen that permission error before.

Hopefully the error message was obvious enough that it didn't take you
too long to think to type "sudo make clean"?  I know it's better for
people not to have to figure this out, but my hope is at least it's
not too arcane of an errror message.


> Is there any issue if I:
>
> $ make clean
> $ make
> $ sudo make install
> <hack around>
> $ make

I don't personally know of any problems with the above flow.


> Or it's only a problem if:
>
> $ make clean
> $ sudo make install
> <hack around>
> $ make

Yeah, just that one.

NOTE: as Masahiro noted in response to the cover letter [1], he's come
up with a better solution to the problem that was solved by the
.cache.mk and he's planning to remove it shortly.


[1] https://lkml.org/lkml/2018/3/13/100

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

[Linus Torvalds]

On Tue, Mar 13, 2018 at 9:44 AM, Doug Anderson <dianders@chromium.org> wrote:
>> Is there any issue if I:
>>
>> $ make clean
>> $ make
>> $ sudo make install
>> <hack around>
>> $ make
>
> I don't personally know of any problems with the above flow.

This is my workflow, and what I suggest people do - only do "make
install" as root, everything else as a normal user.

And in fact, it occasionally _has_ broken, when "make install" has
done more than just install things, and created root-owned files and
directories (particularly the generated headers).

And then I complain to people, because then things like "make clean"
as a normal user ends up breaking too when there's some root-owned
file.

So it can break, but it's pretty rare. Usually it's something like
"people didn't use the proper sequence to update a file only if it
changed, and just blindly over-wrote a new version.

Generally, I prefer "make install" not even checking dependencies at
all, and just blindly copy things. And I'd definitely be ok with an
error rather than root generating files, although then I'd not
special-case mkcache, but all the *other* random file changes we do.

If Ingo wants to build as root, maybe we could even make him set some
environment flag to avoid errors.

(But I don't actually like the patch in question, because I really
think KBUILD_NOCACHE is much too specific a special case, and it
should be way more generic).

                Linus

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

[Doug Anderson]

Hi,

On Tue, Mar 13, 2018 at 10:39 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Mar 13, 2018 at 9:44 AM, Doug Anderson <dianders@chromium.org> wrote:
>>> Is there any issue if I:
>>>
>>> $ make clean
>>> $ make
>>> $ sudo make install
>>> <hack around>
>>> $ make
>>
>> I don't personally know of any problems with the above flow.
>
> This is my workflow, and what I suggest people do - only do "make
> install" as root, everything else as a normal user.
>
> And in fact, it occasionally _has_ broken, when "make install" has
> done more than just install things, and created root-owned files and
> directories (particularly the generated headers).
>
> And then I complain to people, because then things like "make clean"
> as a normal user ends up breaking too when there's some root-owned
> file.
>
> So it can break, but it's pretty rare. Usually it's something like
> "people didn't use the proper sequence to update a file only if it
> changed, and just blindly over-wrote a new version.
>
> Generally, I prefer "make install" not even checking dependencies at
> all, and just blindly copy things. And I'd definitely be ok with an
> error rather than root generating files, although then I'd not
> special-case mkcache, but all the *other* random file changes we do.
>
> If Ingo wants to build as root, maybe we could even make him set some
> environment flag to avoid errors.
>
> (But I don't actually like the patch in question, because I really
> think KBUILD_NOCACHE is much too specific a special case, and it
> should be way more generic).

Please consider the patch abandoned.  As per other threads pointed to
in this conversation, Masahiro Yamada is planning to fully gut the
".cache.mk" from the build system anyway.  :-)

-Doug

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

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> If Ingo wants to build as root, maybe we could even make him set some
> environment flag to avoid errors.

I only build as root infrequently, but I think PeterZ does it more frequently?

Distro package builds are also often done as root.

I don't mind warnings, etc. - I only objected to the workaround of silently 
turning off a build optimization when root.

Thanks,

	Ingo

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

[Peter Zijlstra]

On Wed, Mar 14, 2018 at 08:23:16AM +0100, Ingo Molnar wrote:
> 
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > If Ingo wants to build as root, maybe we could even make him set some
> > environment flag to avoid errors.
> 
> I only build as root infrequently, but I think PeterZ does it more frequently?

Yeah, a bunch of my testboxes don't even have a regular user account.

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

[Masahiro Yamada]

Hi Douglas,


2018-03-13 15:11 GMT+09:00 Douglas Anderson <dianders@chromium.org>:
> This two-patches fixes two corner cases with .cache.mk that have been
> reported.  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.
>
> Sorry for the big delay between v2 and v3.  I never quite caught up
> with email after the holidays, but hopefully better late than never...


If v3 had been sent in time for v4.15
I would have immediately queued it up.

But, the situation has changed.

The direction of the development is
to move the compiler flags evaluation to the Kconfig phase,
and remove the Kbuild cache.


If you are interested in the background,
here are some of resources:

Linus' comment that finally moved this work forward:
https://lkml.org/lkml/2018/2/7/527

Tons of discussion about the initial version:
https://patchwork.kernel.org/patch/10207385/

The core patch in the latest version:
https://patchwork.kernel.org/patch/10225355/


And, I am removing the Kbuild cache entirely:
https://patchwork.kernel.org/patch/10225437/


I do not know how to handle your patches
since I am removing the addressed code for 4.17-rc1,
which is comming in a month or so.

If this is a fatal problem, we can queue it up
only for 4.16, but I do not hear problem reports these days...



> Changes in v3:
> - Fix as per Masahiro Yamada (move change to main Makefile)
> - Use "uid 0" as the heuristic instead of install
> - Do the checking in the main Makefile instead of Kbuild.include
>
> 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 root
>
>  Makefile               | 15 +++++++++++++++
>  scripts/Kbuild.include |  2 ++
>  2 files changed, 17 insertions(+)
>
> --
> 2.16.2.660.g709887971b-goog
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Masahiro Yamada

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

[Doug Anderson]

Hi,

On Tue, Mar 13, 2018 at 12:49 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Hi Douglas,
>
>
> 2018-03-13 15:11 GMT+09:00 Douglas Anderson <dianders@chromium.org>:
>> This two-patches fixes two corner cases with .cache.mk that have been
>> reported.  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.
>>
>> Sorry for the big delay between v2 and v3.  I never quite caught up
>> with email after the holidays, but hopefully better late than never...
>
>
> If v3 had been sent in time for v4.15
> I would have immediately queued it up.

Sigh.  My apologies again for sitting on this for so long.  :(


> But, the situation has changed.
>
> The direction of the development is
> to move the compiler flags evaluation to the Kconfig phase,
> and remove the Kbuild cache.
>
>
> If you are interested in the background,
> here are some of resources:
>
> Linus' comment that finally moved this work forward:
> https://lkml.org/lkml/2018/2/7/527
>
> Tons of discussion about the initial version:
> https://patchwork.kernel.org/patch/10207385/
>
> The core patch in the latest version:
> https://patchwork.kernel.org/patch/10225355/
>
>
> And, I am removing the Kbuild cache entirely:
> https://patchwork.kernel.org/patch/10225437/

Oh, that's great!


> I do not know how to handle your patches
> since I am removing the addressed code for 4.17-rc1,
> which is comming in a month or so.
>
> If this is a fatal problem, we can queue it up
> only for 4.16, but I do not hear problem reports these days...

Just drop them.  Ingo hated patch 2/2, and (like you) I haven't heard
any complaints about the problems described by patch 1/2 in the last
few months.  I got back to this because I told myself I would try to
re-focus on upstream stuff while I was at ELC, not because I suddenly
got another stream of complaints.  ;)


Anyway, glad to here you've come up with a better solution for the
future and we can get rid of the cache.  Thank you for all your hard
work!


-Doug