diff mbox series

[RFC,4/4] kbuild: evaluate cc-option and friends only when building kernel

Message ID 1507089367-10402-4-git-send-email-yamada.masahiro@socionext.com
State New
Headers show
Series [1/4] kbuild: replace $(hdr-arch) with $(SRCARCH) | expand

Commit Message

Masahiro Yamada Oct. 4, 2017, 3:56 a.m. UTC 
$(call cc-option,...) is costly because it invokes the C compiler.

Given that only some targets need compiler flags, it is pointless
to compute them all the time.

The variable no-dot-config-targets lists the targets we can run
without the .config file, such as "make clean", "make help", etc.

My idea is similar here.  We can add no-compiler-targets to list the
targets we can run without the target compiler information.  This
includes no-dot-config-targets + config targets + misc.

When we run only targets listed in the no-compiler-targets, we can
set cc-option and friends to no-op.  The hostcc-option is an exception
since the host compiler is needed for building fixdep, kconfig, etc.

I did not add "dtbs" and "%.dtb" to no-compiler-targets.  This is
intentional.  Theoretically, we can build device tree blobs without
the C compiler.  It it true that in-kernel DT files are pre-processed
by CPP before DTC, but KBUILD_CPPFLAGS is unused.  However, for the
reason of scripts/dtc/ location, Kbuild descends into scripts/mod/
before the DT build.  I do not want to trigger the unrelated re-build
of modpost when building DT.  Perhaps, we can fix this with further
refactoring, but not now.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

---

 Makefile               | 10 ++++++++++
 scripts/Kbuild.include | 14 +++++++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

-- 
2.7.4

Comments

Doug Anderson Oct. 9, 2017, 10:04 p.m. UTC | #1 
Hi,

On Tue, Oct 3, 2017 at 8:56 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include

> index 9ffd3dd..222d0a2 100644

> --- a/scripts/Kbuild.include

> +++ b/scripts/Kbuild.include

> @@ -96,6 +96,13 @@ try-run = $(shell set -e;            \

>         fi;                             \

>         rm -f "$$TMP" "$$TMPO")

>

> +# hostcc-option

> +# Usage: cflags-y += $(call hostcc-option,-march=winchip-c6,-march=i586)

> +hostcc-option = $(call __cc-option, $(HOSTCC),\

> +       $(HOSTCFLAGS) $(HOST_EXTRACFLAGS),$(1),$(2))


I believe you've got a bug here.  You're calling "__cc-option" which
isn't defined if "need-compiler" is not 1, right?

> +

> +ifeq ($(need-compiler),1)


The way this patch works is a bit non-obvious I think.  I wonder if
anyone else will be confused like I am...

Basically if "need-compiler" is not 1 then things like "cc-option"
won't be defined at all.  ...but we'll still _call_ them in other
Makefiles.  This call of an undefined variable will just evaluate to
an empty string.  Thus, for instance:

CFLAGS_KCOV := $(call cc-option,-fsanitize-coverage=trace-pc,)

...will just set CFLAGS_KCOV to the empty string if "need-compiler"
isn't 1, right?


I guess that's fine, but maybe at least document it somewhere?  IMHO
it would be even better if somehow you still defined each of these to
something bogus in an else clause, like:

as-option = --err_noncompile_target
as-instr = --err_noncompile_target
cc-option = --err_noncompile_target
...
...

The idea being that if someone accidentally invoked the C compiler (or
if there was some other conditional code in the Makefile based on the
result of one of these functions) it would be obvious what was going
on.


-Doug
Masahiro Yamada Oct. 10, 2017, 12:23 a.m. UTC | #2 
2017-10-10 7:04 GMT+09:00 Doug Anderson <dianders@chromium.org>:
> Hi,

>

> On Tue, Oct 3, 2017 at 8:56 PM, Masahiro Yamada

> <yamada.masahiro@socionext.com> wrote:

>> diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include

>> index 9ffd3dd..222d0a2 100644

>> --- a/scripts/Kbuild.include

>> +++ b/scripts/Kbuild.include

>> @@ -96,6 +96,13 @@ try-run = $(shell set -e;            \

>>         fi;                             \

>>         rm -f "$$TMP" "$$TMPO")

>>

>> +# hostcc-option

>> +# Usage: cflags-y += $(call hostcc-option,-march=winchip-c6,-march=i586)

>> +hostcc-option = $(call __cc-option, $(HOSTCC),\

>> +       $(HOSTCFLAGS) $(HOST_EXTRACFLAGS),$(1),$(2))

>

> I believe you've got a bug here.  You're calling "__cc-option" which

> isn't defined if "need-compiler" is not 1, right?



Good catch!


>> +

>> +ifeq ($(need-compiler),1)

>

> The way this patch works is a bit non-obvious I think.  I wonder if

> anyone else will be confused like I am...

>

> Basically if "need-compiler" is not 1 then things like "cc-option"

> won't be defined at all.  ...but we'll still _call_ them in other

> Makefiles.  This call of an undefined variable will just evaluate to

> an empty string.  Thus, for instance:

>

> CFLAGS_KCOV := $(call cc-option,-fsanitize-coverage=trace-pc,)

>

> ...will just set CFLAGS_KCOV to the empty string if "need-compiler"

> isn't 1, right?

>

>

> I guess that's fine, but maybe at least document it somewhere?  IMHO

> it would be even better if somehow you still defined each of these to

> something bogus in an else clause, like:

>

> as-option = --err_noncompile_target

> as-instr = --err_noncompile_target

> cc-option = --err_noncompile_target

> ...

> ...

>

> The idea being that if someone accidentally invoked the C compiler (or

> if there was some other conditional code in the Makefile based on the

> result of one of these functions) it would be obvious what was going

> on.



Make sense.  Thanks for your idea!



I am still wondering if I should apply this one
because your cache approach provides much more benefits.

(that is why I prefixed this 4/4 with RFC.)


Maybe I will send v2, or maybe not.




-- 
Best Regards
Masahiro Yamada
Doug Anderson Oct. 10, 2017, 12:27 a.m. UTC | #3 
Hi,

On Mon, Oct 9, 2017 at 5:23 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> I am still wondering if I should apply this one

> because your cache approach provides much more benefits.

>

> (that is why I prefixed this 4/4 with RFC.)

>

>

> Maybe I will send v2, or maybe not.


I'm not sure I have a strong opinion either way.  In general your idea
makes a lot of sense--no need to do extra work if you don't need to.
It's all a question of how clean / maintainable it will be I guess.

-Doug
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index a4fd682..80e1a38 100644
--- a/Makefile
+++ b/Makefile
@@ -224,9 +224,13 @@  no-dot-config-targets := clean mrproper distclean \
 			 $(version_h) headers_% archheaders archscripts \
 			 kernelversion %src-pkg
 
+no-compiler-targets := $(no-dot-config-targets) config %config \
+		       kernelrelease image_name
+
 config-targets := 0
 mixed-targets  := 0
 dot-config     := 1
+need-compiler  := 1
 
 ifneq ($(filter $(no-dot-config-targets), $(MAKECMDGOALS)),)
 	ifeq ($(filter-out $(no-dot-config-targets), $(MAKECMDGOALS)),)
@@ -234,6 +238,12 @@  ifneq ($(filter $(no-dot-config-targets), $(MAKECMDGOALS)),)
 	endif
 endif
 
+ifneq ($(filter $(no-compiler-targets), $(MAKECMDGOALS)),)
+	ifeq ($(filter-out $(no-compiler-targets), $(MAKECMDGOALS)),)
+		need-compiler := 0
+	endif
+endif
+
 ifeq ($(KBUILD_EXTMOD),)
         ifneq ($(filter config %config,$(MAKECMDGOALS)),)
                 config-targets := 1
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
index 9ffd3dd..222d0a2 100644
--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -96,6 +96,13 @@  try-run = $(shell set -e;		\
 	fi;				\
 	rm -f "$$TMP" "$$TMPO")
 
+# hostcc-option
+# Usage: cflags-y += $(call hostcc-option,-march=winchip-c6,-march=i586)
+hostcc-option = $(call __cc-option, $(HOSTCC),\
+	$(HOSTCFLAGS) $(HOST_EXTRACFLAGS),$(1),$(2))
+
+ifeq ($(need-compiler),1)
+
 # as-option
 # Usage: cflags-y += $(call as-option,-Wa$(comma)-isa=foo,)
 
@@ -123,11 +130,6 @@  CC_OPTION_CFLAGS = $(filter-out $(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS))
 cc-option = $(call __cc-option, $(CC),\
 	$(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS),$(1),$(2))
 
-# hostcc-option
-# Usage: cflags-y += $(call hostcc-option,-march=winchip-c6,-march=i586)
-hostcc-option = $(call __cc-option, $(HOSTCC),\
-	$(HOSTCFLAGS) $(HOST_EXTRACFLAGS),$(1),$(2))
-
 # cc-option-yn
 # Usage: flag := $(call cc-option-yn,-march=winchip-c6)
 cc-option-yn = $(call try-run,\
@@ -180,6 +182,8 @@  ld-version = $(shell $(LD) --version | $(srctree)/scripts/ld-version.sh)
 # Usage:  $(call ld-ifversion, -ge, 22252, y)
 ld-ifversion = $(shell [ $(ld-version) $(1) $(2) ] && echo $(3) || echo $(4))
 
+endif
+
 ######
 
 ###