[PATCH] Makefile: reuse CC_VERSION_TEXT

[PATCH] Makefile: reuse CC_VERSION_TEXT

[Nick Desaulniers]

I noticed we're invoking $(CC) via $(shell) more than once to check the
version.  Let's reuse the first string captured in $CC_VERSION_TEXT.

Fixes: 315bab4e972d ("kbuild: fix endless syncconfig in case arch Makefile sets CROSS_COMPILE")
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 Makefile | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index a85535eb6a7d..70034d7c1051 100644
--- a/Makefile
+++ b/Makefile
@@ -557,7 +557,13 @@ ifdef building_out_of_srctree
 	{ echo "# this is build directory, ignore it"; echo "*"; } > .gitignore
 endif
 
-ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),)
+# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
+# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
+# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
+# and from include/config/auto.conf.cmd to detect the compiler upgrade.
+CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1)
+
+ifneq ($(findstring clang,$(CC_VERSION_TEXT)),)
 ifneq ($(CROSS_COMPILE),)
 CLANG_FLAGS	+= --target=$(notdir $(CROSS_COMPILE:%-=%))
 GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit))
@@ -576,12 +582,6 @@ KBUILD_AFLAGS	+= $(CLANG_FLAGS)
 export CLANG_FLAGS
 endif
 
-# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
-# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
-# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
-# and from include/config/auto.conf.cmd to detect the compiler upgrade.
-CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1)
-
 ifdef config-build
 # ===========================================================================
 # *config targets only - make sure prerequisites are updated, and descend
-- 
2.30.0.478.g8a0d178c01-goog

Re: [PATCH] Makefile: reuse CC_VERSION_TEXT

[Masahiro Yamada]

On Sat, Feb 6, 2021 at 7:01 AM 'Nick Desaulniers' via Clang Built
Linux <clang-built-linux@googlegroups.com> wrote:
>
> I noticed we're invoking $(CC) via $(shell) more than once to check the
> version.  Let's reuse the first string captured in $CC_VERSION_TEXT.
>
> Fixes: 315bab4e972d ("kbuild: fix endless syncconfig in case arch Makefile sets CROSS_COMPILE")


I did not touch this hunk because I have a plan
for different refactoring, but I have never got
around to do it.

Anyway, you beat me, and I will pick this up.
But, the Fixes tag is questionable because
this is code refactoring.




> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  Makefile | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index a85535eb6a7d..70034d7c1051 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -557,7 +557,13 @@ ifdef building_out_of_srctree
>         { echo "# this is build directory, ignore it"; echo "*"; } > .gitignore
>  endif
>
> -ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),)
> +# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
> +# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
> +# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
> +# and from include/config/auto.conf.cmd to detect the compiler upgrade.
> +CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1)
> +
> +ifneq ($(findstring clang,$(CC_VERSION_TEXT)),)
>  ifneq ($(CROSS_COMPILE),)
>  CLANG_FLAGS    += --target=$(notdir $(CROSS_COMPILE:%-=%))
>  GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit))
> @@ -576,12 +582,6 @@ KBUILD_AFLAGS      += $(CLANG_FLAGS)
>  export CLANG_FLAGS
>  endif
>
> -# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
> -# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
> -# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
> -# and from include/config/auto.conf.cmd to detect the compiler upgrade.
> -CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1)
> -
>  ifdef config-build
>  # ===========================================================================
>  # *config targets only - make sure prerequisites are updated, and descend
> --
> 2.30.0.478.g8a0d178c01-goog
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20210205220125.2931504-1-ndesaulniers%40google.com.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] Makefile: reuse CC_VERSION_TEXT

[Sedat Dilek]

On Sat, Feb 6, 2021 at 2:49 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Sat, Feb 6, 2021 at 7:01 AM 'Nick Desaulniers' via Clang Built
> Linux <clang-built-linux@googlegroups.com> wrote:
> >
> > I noticed we're invoking $(CC) via $(shell) more than once to check the
> > version.  Let's reuse the first string captured in $CC_VERSION_TEXT.
> >
> > Fixes: 315bab4e972d ("kbuild: fix endless syncconfig in case arch Makefile sets CROSS_COMPILE")
>
>
> I did not touch this hunk because I have a plan
> for different refactoring, but I have never got
> around to do it.
>
> Anyway, you beat me, and I will pick this up.
> But, the Fixes tag is questionable because
> this is code refactoring.
>

When I see this... and hear refactoring... As a suggestion/improvement...

Can we have LD_VERSION_TEXT analogue to CC_VERSION_TEXT?
Both are shown when doing a `cat /proc/version` (and IIRC in file
include/generated/compile.h).

Thanks.

- Sedat -

>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> >  Makefile | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index a85535eb6a7d..70034d7c1051 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -557,7 +557,13 @@ ifdef building_out_of_srctree
> >         { echo "# this is build directory, ignore it"; echo "*"; } > .gitignore
> >  endif
> >
> > -ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),)
> > +# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
> > +# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
> > +# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
> > +# and from include/config/auto.conf.cmd to detect the compiler upgrade.
> > +CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1)
> > +
> > +ifneq ($(findstring clang,$(CC_VERSION_TEXT)),)
> >  ifneq ($(CROSS_COMPILE),)
> >  CLANG_FLAGS    += --target=$(notdir $(CROSS_COMPILE:%-=%))
> >  GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit))
> > @@ -576,12 +582,6 @@ KBUILD_AFLAGS      += $(CLANG_FLAGS)
> >  export CLANG_FLAGS
> >  endif
> >
> > -# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
> > -# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
> > -# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
> > -# and from include/config/auto.conf.cmd to detect the compiler upgrade.
> > -CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1)
> > -
> >  ifdef config-build
> >  # ===========================================================================
> >  # *config targets only - make sure prerequisites are updated, and descend
> > --
> > 2.30.0.478.g8a0d178c01-goog
> >
> > --
> > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20210205220125.2931504-1-ndesaulniers%40google.com.
>
>
>
> --
> Best Regards
> Masahiro Yamada
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/CAK7LNARKHvjTcnic%3DZKntH3NY5meehQbJuBr34y9_tn8b-Ym0w%40mail.gmail.com.

Re: [PATCH] Makefile: reuse CC_VERSION_TEXT

[Masahiro Yamada]

On Sat, Feb 6, 2021 at 12:46 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
>
> On Sat, Feb 6, 2021 at 2:49 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Sat, Feb 6, 2021 at 7:01 AM 'Nick Desaulniers' via Clang Built
> > Linux <clang-built-linux@googlegroups.com> wrote:
> > >
> > > I noticed we're invoking $(CC) via $(shell) more than once to check the
> > > version.  Let's reuse the first string captured in $CC_VERSION_TEXT.
> > >
> > > Fixes: 315bab4e972d ("kbuild: fix endless syncconfig in case arch Makefile sets CROSS_COMPILE")
> >
> >
> > I did not touch this hunk because I have a plan
> > for different refactoring, but I have never got
> > around to do it.
> >
> > Anyway, you beat me, and I will pick this up.
> > But, the Fixes tag is questionable because
> > this is code refactoring.
> >
>
> When I see this... and hear refactoring... As a suggestion/improvement...
>
> Can we have LD_VERSION_TEXT analogue to CC_VERSION_TEXT?
> Both are shown when doing a `cat /proc/version` (and IIRC in file
> include/generated/compile.h).

Sorry, I had forgotten to answer this question.

Probably, we should do so.

Feeding it to Kconfig allows us to re-evaluate
config settings when tools are upgraded.

One question is where we should draw a line.
If we record the version text of CC, LD,
we should do that for more tools?

For LD, I will be probably OK because
some features rely on LD_VERSION or $(ld-option ).


-- 
Best Regards
Masahiro Yamada

Re: [PATCH] Makefile: reuse CC_VERSION_TEXT

[Nathan Chancellor]

On Wed, Feb 24, 2021 at 12:14:04PM +0900, Masahiro Yamada wrote:
> On Sat, Feb 6, 2021 at 12:46 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> >
> > On Sat, Feb 6, 2021 at 2:49 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > On Sat, Feb 6, 2021 at 7:01 AM 'Nick Desaulniers' via Clang Built
> > > Linux <clang-built-linux@googlegroups.com> wrote:
> > > >
> > > > I noticed we're invoking $(CC) via $(shell) more than once to check the
> > > > version.  Let's reuse the first string captured in $CC_VERSION_TEXT.
> > > >
> > > > Fixes: 315bab4e972d ("kbuild: fix endless syncconfig in case arch Makefile sets CROSS_COMPILE")
> > >
> > >
> > > I did not touch this hunk because I have a plan
> > > for different refactoring, but I have never got
> > > around to do it.
> > >
> > > Anyway, you beat me, and I will pick this up.
> > > But, the Fixes tag is questionable because
> > > this is code refactoring.
> > >
> >
> > When I see this... and hear refactoring... As a suggestion/improvement...
> >
> > Can we have LD_VERSION_TEXT analogue to CC_VERSION_TEXT?
> > Both are shown when doing a `cat /proc/version` (and IIRC in file
> > include/generated/compile.h).
> 
> Sorry, I had forgotten to answer this question.
> 
> Probably, we should do so.
> 
> Feeding it to Kconfig allows us to re-evaluate
> config settings when tools are upgraded.
> 
> One question is where we should draw a line.
> If we record the version text of CC, LD,
> we should do that for more tools?
> 
> For LD, I will be probably OK because
> some features rely on LD_VERSION or $(ld-option ).
> 
> 
> -- 
> Best Regards
> Masahiro Yamada

It probably makes sense to do this for tools that we depend on in
Kconfig, like CC and LD. Those will usually paint a good enough picture
for what tools were used on a whole since CC will tell us about the GCC
or LLVM version used (as well as distribution usually) and LD will tell
us about the binutils or LLVM version.

LTO does check $(AR) and $(NM) but it also depends on LLVM=1 LLVM_IAS=1,
which would get covered by the $(CC) and $(LD) print outs.

Cheers,
Nathan

Re: [PATCH] Makefile: reuse CC_VERSION_TEXT

[Nick Desaulniers]

On Fri, Feb 5, 2021 at 5:49 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Sat, Feb 6, 2021 at 7:01 AM 'Nick Desaulniers' via Clang Built
> Linux <clang-built-linux@googlegroups.com> wrote:
> >
> > I noticed we're invoking $(CC) via $(shell) more than once to check the
> > version.  Let's reuse the first string captured in $CC_VERSION_TEXT.
> >
> > Fixes: 315bab4e972d ("kbuild: fix endless syncconfig in case arch Makefile sets CROSS_COMPILE")
>
>
> I did not touch this hunk because I have a plan
> for different refactoring, but I have never got
> around to do it.
>
> Anyway, you beat me, and I will pick this up.
> But, the Fixes tag is questionable because
> this is code refactoring.

Hi Masahiro,
A friendly reminder to please pick this up; I don't see it yet in
linux-next. I'm ok with you dropping the fixes tag when applied.

(Otherwise, I was about to use `grep -m1` instead of `| head -n1 | grep` here.)

>
>
>
>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> >  Makefile | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index a85535eb6a7d..70034d7c1051 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -557,7 +557,13 @@ ifdef building_out_of_srctree
> >         { echo "# this is build directory, ignore it"; echo "*"; } > .gitignore
> >  endif
> >
> > -ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),)
> > +# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
> > +# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
> > +# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
> > +# and from include/config/auto.conf.cmd to detect the compiler upgrade.
> > +CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1)
> > +
> > +ifneq ($(findstring clang,$(CC_VERSION_TEXT)),)
> >  ifneq ($(CROSS_COMPILE),)
> >  CLANG_FLAGS    += --target=$(notdir $(CROSS_COMPILE:%-=%))
> >  GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit))
> > @@ -576,12 +582,6 @@ KBUILD_AFLAGS      += $(CLANG_FLAGS)
> >  export CLANG_FLAGS
> >  endif
> >
> > -# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
> > -# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
> > -# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
> > -# and from include/config/auto.conf.cmd to detect the compiler upgrade.
> > -CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1)
> > -
> >  ifdef config-build
> >  # ===========================================================================
> >  # *config targets only - make sure prerequisites are updated, and descend
> > --


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] Makefile: reuse CC_VERSION_TEXT

[Masahiro Yamada]

On Wed, Feb 24, 2021 at 5:10 AM 'Nick Desaulniers' via Clang Built
Linux <clang-built-linux@googlegroups.com> wrote:
>
> On Fri, Feb 5, 2021 at 5:49 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > On Sat, Feb 6, 2021 at 7:01 AM 'Nick Desaulniers' via Clang Built
> > Linux <clang-built-linux@googlegroups.com> wrote:
> > >
> > > I noticed we're invoking $(CC) via $(shell) more than once to check the
> > > version.  Let's reuse the first string captured in $CC_VERSION_TEXT.
> > >
> > > Fixes: 315bab4e972d ("kbuild: fix endless syncconfig in case arch Makefile sets CROSS_COMPILE")
> >
> >
> > I did not touch this hunk because I have a plan
> > for different refactoring, but I have never got
> > around to do it.
> >
> > Anyway, you beat me, and I will pick this up.
> > But, the Fixes tag is questionable because
> > this is code refactoring.
>
> Hi Masahiro,
> A friendly reminder to please pick this up; I don't see it yet in
> linux-next. I'm ok with you dropping the fixes tag when applied.


Sorry, I had completely forgotten to do this.
Thanks for the reminder.

I inserted this before Nathan's "remove # characters" patch.




> (Otherwise, I was about to use `grep -m1` instead of `| head -n1 | grep` here.)
>
> >
> >
> >
> >
> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > > ---
> > >  Makefile | 14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index a85535eb6a7d..70034d7c1051 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -557,7 +557,13 @@ ifdef building_out_of_srctree
> > >         { echo "# this is build directory, ignore it"; echo "*"; } > .gitignore
> > >  endif
> > >
> > > -ifneq ($(shell $(CC) --version 2>&1 | head -n 1 | grep clang),)
> > > +# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
> > > +# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
> > > +# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
> > > +# and from include/config/auto.conf.cmd to detect the compiler upgrade.
> > > +CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1)
> > > +
> > > +ifneq ($(findstring clang,$(CC_VERSION_TEXT)),)
> > >  ifneq ($(CROSS_COMPILE),)
> > >  CLANG_FLAGS    += --target=$(notdir $(CROSS_COMPILE:%-=%))
> > >  GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)elfedit))
> > > @@ -576,12 +582,6 @@ KBUILD_AFLAGS      += $(CLANG_FLAGS)
> > >  export CLANG_FLAGS
> > >  endif
> > >
> > > -# The expansion should be delayed until arch/$(SRCARCH)/Makefile is included.
> > > -# Some architectures define CROSS_COMPILE in arch/$(SRCARCH)/Makefile.
> > > -# CC_VERSION_TEXT is referenced from Kconfig (so it needs export),
> > > -# and from include/config/auto.conf.cmd to detect the compiler upgrade.
> > > -CC_VERSION_TEXT = $(shell $(CC) --version 2>/dev/null | head -n 1)
> > > -
> > >  ifdef config-build
> > >  # ===========================================================================
> > >  # *config targets only - make sure prerequisites are updated, and descend
> > > --
>
>
> --
> Thanks,
> ~Nick Desaulniers
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/CAKwvOd%3Dqh0%3DtU9fr60DxcJepUfg6%2BBkqScwifW4s8A2A%3DofKpw%40mail.gmail.com.



-- 
Best Regards
Masahiro Yamada