| Message ID | 1393376923-21892-2-git-send-email-behanw@converseincode.com |
|---|---|
| State | Superseded |
| Headers | show |
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? > + > # 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? > > # 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. > +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 > +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. > +# Quiet clang warning: comparison of unsigned expression < 0 is always false > +KBUILD_CFLAGS += $(call cc-disable-warning, tautological-compare) Same with this. > +# 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? Sam -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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
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 + # 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 # 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) +KBUILD_CPPFLAGS += $(call cc-option,-Qunused-arguments,) +KBUILD_CPPFLAGS += $(call cc-option,-Wno-unknown-warning-option,) +KBUILD_CFLAGS += $(call cc-disable-warning, unused-variable) +KBUILD_CFLAGS += $(call cc-disable-warning, format-invalid-specifier) +KBUILD_CFLAGS += $(call cc-disable-warning, gnu) +# 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,) +else + # This warning generated too much noise in a regular build. # Use make W=1 to enable this warning (see scripts/Makefile.build) KBUILD_CFLAGS += $(call cc-disable-warning, unused-but-set-variable) +endif ifdef CONFIG_FRAME_POINTER KBUILD_CFLAGS += -fno-omit-frame-pointer -fno-optimize-sibling-calls