Patchwork [v2] kbuild: Set KBUILD_CFLAGS before incl. arch Makefile

login
register
mail settings
Submitter Nick Desaulniers
Date Nov. 7, 2017, 7:46 p.m.
Message ID <20171107194614.89371-1-ndesaulniers@google.com>
Download mbox | patch
Permalink /patch/10047193/
State New
Headers show

Comments

Nick Desaulniers - Nov. 7, 2017, 7:46 p.m.
From: Chris Fries <cfries@google.com>

Set the clang KBUILD_CFLAGS up before including arch/ Makefiles,
so that ld-options (etc.) can work correctly.

This fixes errors with clang such as ld-options trying to CC
against your host architecture, but LD trying to link against
your target architecture.

We didn't notice this problem on Android, because we took the original
LLVMLinux patch into our 4.4 kernels, which did not have this issue. We
ran into this taking the proper upstream patch on newer kernel versions.
The original LLVMLinux patch can be seen at:

http://git.linuxfoundation.org/?p=llvmlinux/kernel.git;a=blobdiff;f=Makefile;h=389006c4ef494cda3a1ee52bf355618673ab4f31;hp=e41a3356abee83f08288362950bfceebd25ec3c2;hb=ef9126da11b18ff34eb1f01561f53c378860336c;hpb=f800c25b7a762d445ba1439a2428c8362157eba6

It seems that when the patch was re-upstreamed, a V2 was requested that
moved the definition of Clang's target triple to be later in the top
level Makefile than the inclusion of the arch specific Makefile,
breaking macros like ld-option when cross compiling. V2 was requested
at:

https://lkml.org/lkml/2017/4/21/116

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Signed-off-by: Chris Fries <cfries@google.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes since v1:
* Rebase on kbuild tree, as per Masahiro.
* Add mka's RB/TB line to commit message.

 Makefile | 64 ++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 32 insertions(+), 32 deletions(-)
Masahiro Yamada - Nov. 9, 2017, 4:31 a.m.
Hi Nick

2017-11-08 4:46 GMT+09:00 Nick Desaulniers <ndesaulniers@google.com>:
> From: Chris Fries <cfries@google.com>
>
> Set the clang KBUILD_CFLAGS up before including arch/ Makefiles,
> so that ld-options (etc.) can work correctly.
>
> This fixes errors with clang such as ld-options trying to CC
> against your host architecture, but LD trying to link against
> your target architecture.

OK.
Your previous patch was applied (and we have not received 0-day report so far),
so this makes sense now.




> We didn't notice this problem on Android, because we took the original
> LLVMLinux patch into our 4.4 kernels, which did not have this issue. We
> ran into this taking the proper upstream patch on newer kernel versions.
> The original LLVMLinux patch can be seen at:
>
> http://git.linuxfoundation.org/?p=llvmlinux/kernel.git;a=blobdiff;f=Makefile;h=389006c4ef494cda3a1ee52bf355618673ab4f31;hp=e41a3356abee83f08288362950bfceebd25ec3c2;hb=ef9126da11b18ff34eb1f01561f53c378860336c;hpb=f800c25b7a762d445ba1439a2428c8362157eba6
>
> It seems that when the patch was re-upstreamed, a V2 was requested that
> moved the definition of Clang's target triple to be later in the top
> level Makefile than the inclusion of the arch specific Makefile,
> breaking macros like ld-option when cross compiling. V2 was requested
> at:
>
> https://lkml.org/lkml/2017/4/21/116

IMO, this description is not helpful in any way in upstream.

Moreover,
785f11aa595bc3d4e74096cbd598ada54ecc0d81
did not break anything.   It sound unfair to me.






> diff --git a/Makefile b/Makefile
> index a7476e6934f1..d349734c7ef7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -472,6 +472,38 @@ ifneq ($(KBUILD_SRC),)
>             $(srctree) $(objtree) $(VERSION) $(PATCHLEVEL)
>  endif
>
> +ifeq ($(cc-name),clang)
> +ifneq ($(CROSS_COMPILE),)
> +CLANG_TARGET   := -target $(notdir $(CROSS_COMPILE:%-=%))
> +GCC_TOOLCHAIN  := $(realpath $(dir $(shell which $(LD)))/..)
> +endif
> +ifneq ($(GCC_TOOLCHAIN),)
> +CLANG_GCC_TC   := -gcc-toolchain $(GCC_TOOLCHAIN)
> +endif
> +KBUILD_CFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
> +KBUILD_AFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
> +KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
> +KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
> +KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
> +KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
> +# Quiet clang warning: comparison of unsigned expression < 0 is always false
> +KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
> +# CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the
> +# source of a reference will be _MergedGlobals and not on of the whitelisted names.
> +# See modpost pattern 2
> +KBUILD_CFLAGS += $(call cc-option, -mno-global-merge,)
> +KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior)
> +KBUILD_CFLAGS += $(call cc-option, -no-integrated-as)
> +KBUILD_AFLAGS += $(call cc-option, -no-integrated-as)
> +else
> +
> +# These warnings generated too much noise in a regular build.
> +# Use make W=1 to enable them (see scripts/Makefile.extrawarn)
> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
> +endif
> +


Could you move this a bit later, please?



# These warnings generated too much noise in a regular build.
# Use make W=1 to enable them (see scripts/Makefile.extrawarn)
KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
endif

  << INSERT HERE >>

# The arch Makefile can set ARCH_{CPP,A,C}FLAGS to override the default
# values of the respective KBUILD_* variables
ARCH_CPPFLAGS :=
ARCH_AFLAGS :=
ARCH_CFLAGS :=
include arch/$(SRCARCH)/Makefile






arch/$(SRCARCH)/Makefile is included for config targets
for KBUILD_DEFCONFIG, but no reason to compute compiler flags.
I want to cut unnecessary cc-option parsing.
Nick Desaulniers - Nov. 9, 2017, 4:51 p.m.
On Wed, Nov 8, 2017 at 8:31 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 2017-11-08 4:46 GMT+09:00 Nick Desaulniers <ndesaulniers@google.com>:
>> We didn't notice this problem on Android, because we took the original
>> LLVMLinux patch into our 4.4 kernels, which did not have this issue. We
>> ran into this taking the proper upstream patch on newer kernel versions.
>> The original LLVMLinux patch can be seen at:
>>
>> http://git.linuxfoundation.org/?p=llvmlinux/kernel.git;a=blobdiff;f=Makefile;h=389006c4ef494cda3a1ee52bf355618673ab4f31;hp=e41a3356abee83f08288362950bfceebd25ec3c2;hb=ef9126da11b18ff34eb1f01561f53c378860336c;hpb=f800c25b7a762d445ba1439a2428c8362157eba6
>>
>> It seems that when the patch was re-upstreamed, a V2 was requested that
>> moved the definition of Clang's target triple to be later in the top
>> level Makefile than the inclusion of the arch specific Makefile,
>> breaking macros like ld-option when cross compiling. V2 was requested
>> at:
>>
>> https://lkml.org/lkml/2017/4/21/116
>
> IMO, this description is not helpful in any way in upstream.
>
> Moreover,
> 785f11aa595bc3d4e74096cbd598ada54ecc0d81
> did not break anything.   It sound unfair to me.

Ok, I will cut that part description on v3.

>> diff --git a/Makefile b/Makefile
>> index a7476e6934f1..d349734c7ef7 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -472,6 +472,38 @@ ifneq ($(KBUILD_SRC),)
>>             $(srctree) $(objtree) $(VERSION) $(PATCHLEVEL)
>>  endif
>>
>> +ifeq ($(cc-name),clang)
>> +ifneq ($(CROSS_COMPILE),)
>> +CLANG_TARGET   := -target $(notdir $(CROSS_COMPILE:%-=%))
>> +GCC_TOOLCHAIN  := $(realpath $(dir $(shell which $(LD)))/..)
>> +endif
>> +ifneq ($(GCC_TOOLCHAIN),)
>> +CLANG_GCC_TC   := -gcc-toolchain $(GCC_TOOLCHAIN)
>> +endif
>> +KBUILD_CFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
>> +KBUILD_AFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
>> +KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
>> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
>> +KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
>> +KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
>> +KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
>> +# Quiet clang warning: comparison of unsigned expression < 0 is always false
>> +KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
>> +# CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the
>> +# source of a reference will be _MergedGlobals and not on of the whitelisted names.
>> +# See modpost pattern 2
>> +KBUILD_CFLAGS += $(call cc-option, -mno-global-merge,)
>> +KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior)
>> +KBUILD_CFLAGS += $(call cc-option, -no-integrated-as)
>> +KBUILD_AFLAGS += $(call cc-option, -no-integrated-as)
>> +else
>> +
>> +# These warnings generated too much noise in a regular build.
>> +# Use make W=1 to enable them (see scripts/Makefile.extrawarn)
>> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
>> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
>> +endif
>> +
>
>
> Could you move this a bit later, please?
>
>
>
> # These warnings generated too much noise in a regular build.
> # Use make W=1 to enable them (see scripts/Makefile.extrawarn)
> KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
> KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
> endif
>
>   << INSERT HERE >>
>
> # The arch Makefile can set ARCH_{CPP,A,C}FLAGS to override the default
> # values of the respective KBUILD_* variables
> ARCH_CPPFLAGS :=
> ARCH_AFLAGS :=
> ARCH_CFLAGS :=
> include arch/$(SRCARCH)/Makefile
>
>
>
>
>
>
> arch/$(SRCARCH)/Makefile is included for config targets
> for KBUILD_DEFCONFIG, but no reason to compute compiler flags.
> I want to cut unnecessary cc-option parsing.

With the new try-run-cached macros, if the arch/$(SRCARCH)/Makefile
gets included twice, with the compiler flags not set correctly for
clang to cross compile, and the results are cached, wont they be wrong
the second time the arch specific Makefile is included?
Masahiro Yamada - Nov. 10, 2017, 2:52 a.m.
Hi Nick,


2017-11-10 1:51 GMT+09:00 Nick Desaulniers <ndesaulniers@google.com>:
> On Wed, Nov 8, 2017 at 8:31 PM, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> 2017-11-08 4:46 GMT+09:00 Nick Desaulniers <ndesaulniers@google.com>:
>>> We didn't notice this problem on Android, because we took the original
>>> LLVMLinux patch into our 4.4 kernels, which did not have this issue. We
>>> ran into this taking the proper upstream patch on newer kernel versions.
>>> The original LLVMLinux patch can be seen at:
>>>
>>> http://git.linuxfoundation.org/?p=llvmlinux/kernel.git;a=blobdiff;f=Makefile;h=389006c4ef494cda3a1ee52bf355618673ab4f31;hp=e41a3356abee83f08288362950bfceebd25ec3c2;hb=ef9126da11b18ff34eb1f01561f53c378860336c;hpb=f800c25b7a762d445ba1439a2428c8362157eba6
>>>
>>> It seems that when the patch was re-upstreamed, a V2 was requested that
>>> moved the definition of Clang's target triple to be later in the top
>>> level Makefile than the inclusion of the arch specific Makefile,
>>> breaking macros like ld-option when cross compiling. V2 was requested
>>> at:
>>>
>>> https://lkml.org/lkml/2017/4/21/116
>>
>> IMO, this description is not helpful in any way in upstream.
>>
>> Moreover,
>> 785f11aa595bc3d4e74096cbd598ada54ecc0d81
>> did not break anything.   It sound unfair to me.
>
> Ok, I will cut that part description on v3.
>
>>> diff --git a/Makefile b/Makefile
>>> index a7476e6934f1..d349734c7ef7 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -472,6 +472,38 @@ ifneq ($(KBUILD_SRC),)
>>>             $(srctree) $(objtree) $(VERSION) $(PATCHLEVEL)
>>>  endif
>>>
>>> +ifeq ($(cc-name),clang)
>>> +ifneq ($(CROSS_COMPILE),)
>>> +CLANG_TARGET   := -target $(notdir $(CROSS_COMPILE:%-=%))
>>> +GCC_TOOLCHAIN  := $(realpath $(dir $(shell which $(LD)))/..)
>>> +endif
>>> +ifneq ($(GCC_TOOLCHAIN),)
>>> +CLANG_GCC_TC   := -gcc-toolchain $(GCC_TOOLCHAIN)
>>> +endif
>>> +KBUILD_CFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
>>> +KBUILD_AFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
>>> +KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
>>> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
>>> +KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
>>> +KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
>>> +KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
>>> +# Quiet clang warning: comparison of unsigned expression < 0 is always false
>>> +KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
>>> +# CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the
>>> +# source of a reference will be _MergedGlobals and not on of the whitelisted names.
>>> +# See modpost pattern 2
>>> +KBUILD_CFLAGS += $(call cc-option, -mno-global-merge,)
>>> +KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior)
>>> +KBUILD_CFLAGS += $(call cc-option, -no-integrated-as)
>>> +KBUILD_AFLAGS += $(call cc-option, -no-integrated-as)
>>> +else
>>> +
>>> +# These warnings generated too much noise in a regular build.
>>> +# Use make W=1 to enable them (see scripts/Makefile.extrawarn)
>>> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
>>> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
>>> +endif
>>> +
>>
>>
>> Could you move this a bit later, please?
>>
>>
>>
>> # These warnings generated too much noise in a regular build.
>> # Use make W=1 to enable them (see scripts/Makefile.extrawarn)
>> KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
>> KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
>> endif
>>
>>   << INSERT HERE >>
>>
>> # The arch Makefile can set ARCH_{CPP,A,C}FLAGS to override the default
>> # values of the respective KBUILD_* variables
>> ARCH_CPPFLAGS :=
>> ARCH_AFLAGS :=
>> ARCH_CFLAGS :=
>> include arch/$(SRCARCH)/Makefile
>>
>>
>>
>>
>>
>>
>> arch/$(SRCARCH)/Makefile is included for config targets
>> for KBUILD_DEFCONFIG, but no reason to compute compiler flags.
>> I want to cut unnecessary cc-option parsing.
>
> With the new try-run-cached macros, if the arch/$(SRCARCH)/Makefile
> gets included twice, with the compiler flags not set correctly for
> clang to cross compile, and the results are cached, wont they be wrong
> the second time the arch specific Makefile is included?
> --

Good point.

The cached data from arch/$(SRCARCH)/Makefile for configuration
is not used for the second run.
That means some garbage data in the cache file, but less than 10, I think.

You do not need to give CC or CROSS_COMPILE for kernel configuration
in the first place.

So,
   make ARCH=arm64 defconfig
   make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu-

should be fine.

Even if we place lots of clang's cc-option earlier,
users may not give the same CC for configuration and building.


I think more optimized way is to skip computing cc-option for
"make *config", "make clean" etc.

I tried to do that.
https://patchwork.kernel.org/patch/9983827/


I decided to take time for cleaner implementation,
but that is what I'd like to achieve in the future.

Patch

diff --git a/Makefile b/Makefile
index a7476e6934f1..d349734c7ef7 100644
--- a/Makefile
+++ b/Makefile
@@ -472,6 +472,38 @@  ifneq ($(KBUILD_SRC),)
 	    $(srctree) $(objtree) $(VERSION) $(PATCHLEVEL)
 endif
 
+ifeq ($(cc-name),clang)
+ifneq ($(CROSS_COMPILE),)
+CLANG_TARGET	:= -target $(notdir $(CROSS_COMPILE:%-=%))
+GCC_TOOLCHAIN	:= $(realpath $(dir $(shell which $(LD)))/..)
+endif
+ifneq ($(GCC_TOOLCHAIN),)
+CLANG_GCC_TC	:= -gcc-toolchain $(GCC_TOOLCHAIN)
+endif
+KBUILD_CFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
+KBUILD_AFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
+KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
+KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
+KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
+KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
+KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
+# Quiet clang warning: comparison of unsigned expression < 0 is always false
+KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
+# CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the
+# source of a reference will be _MergedGlobals and not on of the whitelisted names.
+# See modpost pattern 2
+KBUILD_CFLAGS += $(call cc-option, -mno-global-merge,)
+KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior)
+KBUILD_CFLAGS += $(call cc-option, -no-integrated-as)
+KBUILD_AFLAGS += $(call cc-option, -no-integrated-as)
+else
+
+# These warnings generated too much noise in a regular build.
+# Use make W=1 to enable them (see scripts/Makefile.extrawarn)
+KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
+KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
+endif
+
 ifeq ($(config-targets),1)
 # ===========================================================================
 # *config targets only - make sure prerequisites are updated, and descend
@@ -682,38 +714,6 @@  ifdef CONFIG_CC_STACKPROTECTOR
 endif
 KBUILD_CFLAGS += $(stackp-flag)
 
-ifeq ($(cc-name),clang)
-ifneq ($(CROSS_COMPILE),)
-CLANG_TARGET	:= -target $(notdir $(CROSS_COMPILE:%-=%))
-GCC_TOOLCHAIN	:= $(realpath $(dir $(shell which $(LD)))/..)
-endif
-ifneq ($(GCC_TOOLCHAIN),)
-CLANG_GCC_TC	:= -gcc-toolchain $(GCC_TOOLCHAIN)
-endif
-KBUILD_CFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
-KBUILD_AFLAGS += $(CLANG_TARGET) $(CLANG_GCC_TC)
-KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
-KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
-KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
-KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
-KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
-# Quiet clang warning: comparison of unsigned expression < 0 is always false
-KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
-# CLANG uses a _MergedGlobals as optimization, but this breaks modpost, as the
-# source of a reference will be _MergedGlobals and not on of the whitelisted names.
-# See modpost pattern 2
-KBUILD_CFLAGS += $(call cc-option, -mno-global-merge,)
-KBUILD_CFLAGS += $(call cc-option, -fcatch-undefined-behavior)
-KBUILD_CFLAGS += $(call cc-option, -no-integrated-as)
-KBUILD_AFLAGS += $(call cc-option, -no-integrated-as)
-else
-
-# These warnings generated too much noise in a regular build.
-# Use make W=1 to enable them (see scripts/Makefile.extrawarn)
-KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable)
-KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
-endif
-
 ifdef CONFIG_FRAME_POINTER
 KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
 else