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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 03/09/14 14:58, Sam Ravnborg wrote:
On Tue, Feb 25, 2014 at 05:08:39PM -0800, behanw@xxxxxxxxxxxxxxxxxx wrote:
From: Behan Webster <behanw@xxxxxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Jan-Simon Möller <dl9pf@xxxxxx>
Signed-off-by: Mark Charlebois <charlebm@xxxxxxxxx>
Cc: PaX Team <pageexec@xxxxxxxxxxx>
---
  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@xxxxxxxxxxxxxxxxxx

--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Newbies FAQ]     [LKML]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Trinity Fuzzer Tool]

  Powered by Linux