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

From: Masahiro Yamada <yamada.masahiro@socionext.com>
To: Nick Desaulniers <ndesaulniers@google.com>
Cc: "Behan Webster" <behanw@converseincode.com>,
	"Jan-Simon Möller" <dl9pf@gmx.de>,
	"Mark Charlebois" <charlebm@gmail.com>,
	"Greg Hackmann" <ghackmann@google.com>,
	"Matthias Kaehlcke" <mka@chromium.org>,
	"Chris Fries" <cfries@google.com>,
	"Michal Marek" <mmarek@suse.com>,
	"Linux Kbuild mailing list" <linux-kbuild@vger.kernel.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] kbuild: Set KBUILD_CFLAGS before incl. arch Makefile
Date: Fri, 10 Nov 2017 11:52:01 +0900	[thread overview]
Message-ID: <CAK7LNAS1NaqPRhK6FOXN=YTMhLagpSrR2=tXn-uWZbpzr=NeoQ@mail.gmail.com> (raw)
In-Reply-To: <CAKwvOdnsmx43ocFqCLc5ymWkMP8mfju7w0TNbnKmzcbTn3=MAg@mail.gmail.com>

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.

-- 
Best Regards
Masahiro Yamada