Re: [PATCH 1/5] kbuild: LLVMLinux: Add Kbuild support for building kernel with Clang

[Behan Webster <behanw@converseincode.com>]

On 03/09/14 14:58, Sam Ravnborg wrote:
> On Tue, Feb 25, 2014 at 05:08:39PM -0800, behanw@converseincode.com wrote:
>> From: Behan Webster <behanw@converseincode.com>
>>
>> Add support to toplevel Makefile for compiling with clang, both for
>> HOSTCC and CC. Use cc-option to prevent gcc option from breaking clang, and
>> from clang options from breaking gcc.
>>
>> Clang 3.4 semantics are the same as gcc semantics for unsupported flags. For
>> unsupported warnings clang 3.4 returns true but shows a warning and gcc shows
>> a warning and returns false.
>>
>> Signed-off-by: Behan Webster <behanw@converseincode.com>
>> Signed-off-by: Jan-Simon Möller <dl9pf@gmx.de>
>> Signed-off-by: Mark Charlebois <charlebm@gmail.com>
>> Cc: PaX Team <pageexec@freemail.hu>
>> ---
>>   Makefile | 32 +++++++++++++++++++++++++++++++-
>>   1 file changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 831b36a..c4ab30d 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -247,6 +247,15 @@ HOSTCXX      = g++
>>   HOSTCFLAGS   = -Wall -Wmissing-prototypes -Wstrict-prototypes -O2 -fomit-frame-pointer
>>   HOSTCXXFLAGS = -O2
>>   
>> +ifeq ($(shell $(HOSTCC) -v 2>&1 | grep -c "clang version"), 1)
>> +HOSTCOMPILER := clang
>> +HOSTCFLAGS  += -Wno-unused-value -Wno-unused-parameter \
>> +		-Wno-missing-field-initializers -fno-delete-null-pointer-checks
>> +else
>> +HOSTCOMPILER := gcc
>> +endif
>> +export HOSTCOMPILER
> I see no use of HOSTCOMPLIER anywhere in this patchset not in the kernel. Can we drop this?
Actually, we're not using it anymore, though we were at one point. We'll 
drop it.

>> +
>>   # Decide whether to build built-in, modular, or both.
>>   # Normally, just do built-in.
>>   
>> @@ -323,6 +332,12 @@ endif
>>   
>>   export quiet Q KBUILD_VERBOSE
>>   
>> +ifeq ($(shell $(CC) -v 2>&1 | grep -c "clang version"), 1)
>> +COMPILER := clang
>> +else
>> +COMPILER := gcc
>> +endif
>> +export COMPILER
> Likewise - COMPILER seems unsued- can it be dropped?
We use COMPILER a lot in upcoming patches. :(

It's here in order to be available early so that the other patches can 
use it. I hope that's okay.

>> # Look for make include files relative to root of kernel src
>>   MAKEFLAGS += --include-dir=$(srctree)
>> @@ -382,7 +397,7 @@ KBUILD_CFLAGS   := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
>>   		   -fno-strict-aliasing -fno-common \
>>   		   -Werror-implicit-function-declaration \
>>   		   -Wno-format-security \
>> -		   -fno-delete-null-pointer-checks
>> +		   $(call cc-option,-fno-delete-null-pointer-checks,)
>>   KBUILD_AFLAGS_KERNEL :=
>>   KBUILD_CFLAGS_KERNEL :=
>>   KBUILD_AFLAGS   := -D__ASSEMBLY__
>> @@ -620,9 +635,24 @@ else
>>   endif
>>   KBUILD_CFLAGS += $(stackp-flag)
>>   
>> +ifeq ($(COMPILER),clang)
> Except that COMPILER is used here. But this does not warrant the export.
Like I said. It is used in at least 4 other patches which are unrelated 
to kbuild... Well, okay, one in the arm portion of kbuild.

I don't actually like using it, and would prefer not to, but we needed 
the equivalent of an "#ifdef __clang__" in the Makefiles for various 
reasons. :(

>> +KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,)
> Is this really needed today?
> https://bugzilla.mozilla.org/show_bug.cgi?id=717713 suggest that this is default.
>
>> +KBUILD_CPPFLAGS += $(call cc-option,-Wno-unknown-warning-option,)
> https://bugzilla.mozilla.org/show_bug.cgi?id=731316 seems to suggest this is default
These haven't always been the defaults. I will check on when they became 
defaults, retest, and reply back.

Though thanks for pointing this out! Very thorough. :)

>> +KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable)
>> +KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier)
>> +KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
> Is it really justified to disable these warnings?
> # of warnign for a defconfig build would be a nice figure to judge from.
To be honest, the number one complaint we get from kernel devs who've 
tried clang is that there are too many warnings. We don't want to turn 
them off, but merely to stem the tide for now so as not to put people 
off. The idea is to turn them on once we've had time to address the 
warnings more fully. I'd quite like to see all (most) of the clang 
warnings on by default eventually. Does that make sense?

However, the unused-variable warning will likely need to stay on IMHO. 
There are quite a few things in the kernel which create variables which 
are intended to be optimized away (last I checked). gcc doesn't complain 
about this, but clang get's very noisy about it.

Though if the consensus is to have more warnings on by default, I'm 
happy to oblige.

>> +# Quiet clang warning: comparison of unsigned expression < 0 is always false
>> +KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare)
> Same with this.
Actually, if is my understanding that comparing signed and unsigned 
values are considered valid in the kernel code. Certainly I've seen many 
patches rejected which were trying to fix "tautological compare errors".

>> +# 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,)
> Should we fix modpost?
We looked at doing this, but the symbol name compares in modpost seem 
like a Good Idea (tm). By using _MergedGlobals you lose information 
which modpost relies on. I think modpost can catch more potential 
problems by not using clang merged globals. But I'll easily be swayed by 
someone who knows better. :)

Thanks for the feedback,

Behan

-- 
Behan Webster
behanw@converseincode.com