Clang is updating to support -Wimplicit-fallthrough on C
https://reviews.llvm.org/D64838. Since clang does not
support the comment version of fallthrough annotations
this update causes an additional 50k warnings. Most
of these warnings (>49k) are duplicates from header files.
This patch is intended to be reverted after the warnings
have been cleaned up.
Signed-off-by: Nathan Huckleberry <[email protected]>
---
Makefile | 4 ++++
scripts/Makefile.extrawarn | 3 +++
2 files changed, 7 insertions(+)
diff --git a/Makefile b/Makefile
index 1b23f95db176..93b9744e66a2 100644
--- a/Makefile
+++ b/Makefile
@@ -846,7 +846,11 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
KBUILD_CFLAGS += -Wdeclaration-after-statement
# Warn about unmarked fall-throughs in switch statement.
+# If the compiler is clang, this warning is only enabled if W=1 in
+# Makefile.extrawarn
+ifndef CONFIG_CC_IS_CLANG
KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
+endif
# Variable Length Arrays (VLAs) should not be used anywhere in the kernel
KBUILD_CFLAGS += -Wvla
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index a74ce2e3c33e..e12359d69bb7 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -30,6 +30,9 @@ warning-1 += $(call cc-option, -Wunused-but-set-variable)
warning-1 += $(call cc-option, -Wunused-const-variable)
warning-1 += $(call cc-option, -Wpacked-not-aligned)
warning-1 += $(call cc-option, -Wstringop-truncation)
+ifdef CONFIG_CC_IS_CLANG
+KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
+endif
# The following turn off the warnings enabled by -Wextra
warning-1 += -Wno-missing-field-initializers
warning-1 += -Wno-sign-compare
--
2.23.0.rc1.153.gdeed80330f-goog
On Thu, Aug 15, 2019 at 11:20:29AM -0700, 'Nathan Huckleberry' via Clang Built Linux wrote:
> Clang is updating to support -Wimplicit-fallthrough on C
> https://reviews.llvm.org/D64838. Since clang does not
> support the comment version of fallthrough annotations
> this update causes an additional 50k warnings. Most
> of these warnings (>49k) are duplicates from header files.
>
> This patch is intended to be reverted after the warnings
> have been cleaned up.
>
> Signed-off-by: Nathan Huckleberry <[email protected]>
> ---
> Makefile | 4 ++++
> scripts/Makefile.extrawarn | 3 +++
> 2 files changed, 7 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index 1b23f95db176..93b9744e66a2 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -846,7 +846,11 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
> KBUILD_CFLAGS += -Wdeclaration-after-statement
>
> # Warn about unmarked fall-throughs in switch statement.
> +# If the compiler is clang, this warning is only enabled if W=1 in
> +# Makefile.extrawarn
> +ifndef CONFIG_CC_IS_CLANG
> KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> +endif
>
> # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
> KBUILD_CFLAGS += -Wvla
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index a74ce2e3c33e..e12359d69bb7 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -30,6 +30,9 @@ warning-1 += $(call cc-option, -Wunused-but-set-variable)
> warning-1 += $(call cc-option, -Wunused-const-variable)
> warning-1 += $(call cc-option, -Wpacked-not-aligned)
> warning-1 += $(call cc-option, -Wstringop-truncation)
> +ifdef CONFIG_CC_IS_CLANG
> +KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
Shouldn't this be warning-1?
> +endif
> # The following turn off the warnings enabled by -Wextra
> warning-1 += -Wno-missing-field-initializers
> warning-1 += -Wno-sign-compare
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>
I am still not a huge fan of the CONFIG_CC_IS_CLANG ifdefs but I don't
really see a much cleaner way to get around this. Some that come to
mind:
* Leave Makefile alone and add
KBUILD_CFLAGS += -Wno-implicit-fallthrough
in the CONFIG_CC_IS_CLANG section of scripts/Makefile.extrawarn
* Revert commit bfd77145f35c ("Makefile: Convert -Wimplicit-fallthrough=3
to just -Wimplicit-fallthrough for clang") for the time being and just
rely on adding -Wimplicit-fallthrough to KCFLAGS for testing.
Regardless:
Reviewed-by: Nathan Chancellor <[email protected]>
On Thu, Aug 15, 2019 at 1:45 PM Nathan Chancellor
<[email protected]> wrote:
>
> On Thu, Aug 15, 2019 at 11:20:29AM -0700, 'Nathan Huckleberry' via Clang Built Linux wrote:
> > Clang is updating to support -Wimplicit-fallthrough on C
> > https://reviews.llvm.org/D64838. Since clang does not
> > support the comment version of fallthrough annotations
> > this update causes an additional 50k warnings. Most
> > of these warnings (>49k) are duplicates from header files.
> >
> > This patch is intended to be reverted after the warnings
> > have been cleaned up.
> >
> > Signed-off-by: Nathan Huckleberry <[email protected]>
> > ---
> > Makefile | 4 ++++
> > scripts/Makefile.extrawarn | 3 +++
> > 2 files changed, 7 insertions(+)
> >
> > diff --git a/Makefile b/Makefile
> > index 1b23f95db176..93b9744e66a2 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -846,7 +846,11 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
> > KBUILD_CFLAGS += -Wdeclaration-after-statement
> >
> > # Warn about unmarked fall-throughs in switch statement.
> > +# If the compiler is clang, this warning is only enabled if W=1 in
> > +# Makefile.extrawarn
> > +ifndef CONFIG_CC_IS_CLANG
> > KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> > +endif
> >
> > # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
> > KBUILD_CFLAGS += -Wvla
> > diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> > index a74ce2e3c33e..e12359d69bb7 100644
> > --- a/scripts/Makefile.extrawarn
> > +++ b/scripts/Makefile.extrawarn
> > @@ -30,6 +30,9 @@ warning-1 += $(call cc-option, -Wunused-but-set-variable)
> > warning-1 += $(call cc-option, -Wunused-const-variable)
> > warning-1 += $(call cc-option, -Wpacked-not-aligned)
> > warning-1 += $(call cc-option, -Wstringop-truncation)
> > +ifdef CONFIG_CC_IS_CLANG
> > +KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
>
> Shouldn't this be warning-1?
+1
>
> > +endif
> > # The following turn off the warnings enabled by -Wextra
> > warning-1 += -Wno-missing-field-initializers
> > warning-1 += -Wno-sign-compare
> > --
> > 2.23.0.rc1.153.gdeed80330f-goog
> >
>
> I am still not a huge fan of the CONFIG_CC_IS_CLANG ifdefs but I don't
> really see a much cleaner way to get around this. Some that come to
> mind:
>
> * Leave Makefile alone and add
>
> KBUILD_CFLAGS += -Wno-implicit-fallthrough
>
> in the CONFIG_CC_IS_CLANG section of scripts/Makefile.extrawarn
Yeah, I think this might be cleaner. -Wimplicit-fallthrough
-Wno-implicit-fallthrough will be passed to Clang, but "last one
wins." A smaller patch makes it more likely to be revertable without
potentially having to resolve any conflicts. Would you mind sending a
V2 with that change? You can include Nathan's Suggested-by tag.
--
Thanks,
~Nick Desaulniers
Clang is updating to support -Wimplicit-fallthrough on C
https://reviews.llvm.org/D64838. Since clang does not
support the comment version of fallthrough annotations
this update causes an additional 50k warnings. Most
of these warnings (>49k) are duplicates from header files.
This patch is intended to be reverted after the warnings
have been cleaned up.
Signed-off-by: Nathan Huckleberry <[email protected]>
Suggested-by: Nathan Chancellor <[email protected]>
Reviewed-by: Nathan Chancellor <[email protected]>
---
Changes v1->v2
* Move code to preexisting ifdef
scripts/Makefile.extrawarn | 1 +
1 file changed, 1 insertion(+)
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index a74ce2e3c33e..95973a1ee999 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -70,5 +70,6 @@ KBUILD_CFLAGS += -Wno-initializer-overrides
KBUILD_CFLAGS += -Wno-format
KBUILD_CFLAGS += -Wno-sign-compare
KBUILD_CFLAGS += -Wno-format-zero-length
+KBUILD_CFLAGS += $(call cc-option,-Wno-implicit-fallthrough)
endif
endif
--
2.23.0.rc1.153.gdeed80330f-goog
On Thu, Aug 15, 2019 at 10:45 PM Nathan Chancellor
<[email protected]> wrote:
>
> I am still not a huge fan of the CONFIG_CC_IS_CLANG ifdefs but I don't
> really see a much cleaner way to get around this. Some that come to
> mind:
Yeah...
> * Revert commit bfd77145f35c ("Makefile: Convert -Wimplicit-fallthrough=3
> to just -Wimplicit-fallthrough for clang") for the time being and just
> rely on adding -Wimplicit-fallthrough to KCFLAGS for testing.
I would avoid applying commits that will have to be reverted just for
Clang, particularly since it is not fully supported yet.
Cheers,
Miguel
On Thu, Aug 15, 2019 at 3:59 PM 'Nathan Huckleberry' via Clang Built
Linux <[email protected]> wrote:
>
> Clang is updating to support -Wimplicit-fallthrough on C
> https://reviews.llvm.org/D64838. Since clang does not
> support the comment version of fallthrough annotations
> this update causes an additional 50k warnings. Most
> of these warnings (>49k) are duplicates from header files.
>
> This patch is intended to be reverted after the warnings
> have been cleaned up.
>
> Signed-off-by: Nathan Huckleberry <[email protected]>
> Suggested-by: Nathan Chancellor <[email protected]>
> Reviewed-by: Nathan Chancellor <[email protected]>
> ---
> Changes v1->v2
> * Move code to preexisting ifdef
> scripts/Makefile.extrawarn | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index a74ce2e3c33e..95973a1ee999 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -70,5 +70,6 @@ KBUILD_CFLAGS += -Wno-initializer-overrides
> KBUILD_CFLAGS += -Wno-format
> KBUILD_CFLAGS += -Wno-sign-compare
> KBUILD_CFLAGS += -Wno-format-zero-length
> +KBUILD_CFLAGS += $(call cc-option,-Wno-implicit-fallthrough)
Clang seems to support -Wno-implicit-fallthrough since 3.2. You can
remove the cc-option, since you'll need a newer clang than that to
build the kernel.
--
Thanks,
~Nick Desaulniers
On Thu, Aug 15, 2019 at 3:59 PM Miguel Ojeda
<[email protected]> wrote:
>
> On Thu, Aug 15, 2019 at 10:45 PM Nathan Chancellor
> <[email protected]> wrote:
> > * Revert commit bfd77145f35c ("Makefile: Convert -Wimplicit-fallthrough=3
> > to just -Wimplicit-fallthrough for clang") for the time being and just
> > rely on adding -Wimplicit-fallthrough to KCFLAGS for testing.
>
> I would avoid applying commits that will have to be reverted just for
> Clang, particularly since it is not fully supported yet.
"not fully supported yet" you say? *drops monocle*
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS?h=v5.3-rc4#n4001
--
Thanks,
~Nick Desaulniers
On Fri, Aug 16, 2019 at 1:05 AM Nick Desaulniers
<[email protected]> wrote:
>
> On Thu, Aug 15, 2019 at 3:59 PM Miguel Ojeda
> <[email protected]> wrote:
> >
> > On Thu, Aug 15, 2019 at 10:45 PM Nathan Chancellor
> > <[email protected]> wrote:
> > > * Revert commit bfd77145f35c ("Makefile: Convert -Wimplicit-fallthrough=3
> > > to just -Wimplicit-fallthrough for clang") for the time being and just
> > > rely on adding -Wimplicit-fallthrough to KCFLAGS for testing.
> >
> > I would avoid applying commits that will have to be reverted just for
> > Clang, particularly since it is not fully supported yet.
>
> "not fully supported yet" you say? *drops monocle*
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS?h=v5.3-rc4#n4001
By fully supported I mean it already works and people can rely on it
out of the box using a released version of Clang/LLVM. Is that the
case already? If so, many places need updating!
* include/linux/compiler-clang.h should check for the minimum
supported version
* Documentation/process/programming-language.rst should be updated
* https://github.com/ClangBuiltLinux/linux/wiki does not mention anything
etc.
Cheers,
Miguel
Hi.
On Fri, Aug 16, 2019 at 7:59 AM Nathan Huckleberry <[email protected]> wrote:
>
> Clang is updating to support -Wimplicit-fallthrough on C
> https://reviews.llvm.org/D64838. Since clang does not
> support the comment version of fallthrough annotations
> this update causes an additional 50k warnings. Most
> of these warnings (>49k) are duplicates from header files.
>
> This patch is intended to be reverted after the warnings
> have been cleaned up.
>
> Signed-off-by: Nathan Huckleberry <[email protected]>
> Suggested-by: Nathan Chancellor <[email protected]>
> Reviewed-by: Nathan Chancellor <[email protected]>
> ---
> Changes v1->v2
> * Move code to preexisting ifdef
> scripts/Makefile.extrawarn | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index a74ce2e3c33e..95973a1ee999 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -70,5 +70,6 @@ KBUILD_CFLAGS += -Wno-initializer-overrides
> KBUILD_CFLAGS += -Wno-format
> KBUILD_CFLAGS += -Wno-sign-compare
> KBUILD_CFLAGS += -Wno-format-zero-length
> +KBUILD_CFLAGS += $(call cc-option,-Wno-implicit-fallthrough)
> endif
> endif
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>
Perhaps, is the following even cleaner?
diff --git a/Makefile b/Makefile
index 1b23f95db176..cebc6bf5372e 100644
--- a/Makefile
+++ b/Makefile
@@ -751,6 +751,9 @@ else
# These warnings generated too much noise in a regular build.
# Use make W=1 to enable them (see scripts/Makefile.extrawarn)
KBUILD_CFLAGS += -Wno-unused-but-set-variable
+
+# Warn about unmarked fall-throughs in switch statement.
+KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
endif
KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
@@ -845,9 +848,6 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC)
-print-file-name=include)
# warn about C99 declaration after statement
KBUILD_CFLAGS += -Wdeclaration-after-statement
-# Warn about unmarked fall-throughs in switch statement.
-KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
-
# Variable Length Arrays (VLAs) should not be used anywhere in the kernel
KBUILD_CFLAGS += -Wvla
--
Best Regards
Masahiro Yamada
On Mon, Aug 19, 2019 at 01:43:08AM +0900, Masahiro Yamada wrote:
> Hi.
>
> On Fri, Aug 16, 2019 at 7:59 AM Nathan Huckleberry <[email protected]> wrote:
> >
> > Clang is updating to support -Wimplicit-fallthrough on C
> > https://reviews.llvm.org/D64838. Since clang does not
> > support the comment version of fallthrough annotations
> > this update causes an additional 50k warnings. Most
> > of these warnings (>49k) are duplicates from header files.
> >
> > This patch is intended to be reverted after the warnings
> > have been cleaned up.
> >
> > Signed-off-by: Nathan Huckleberry <[email protected]>
> > Suggested-by: Nathan Chancellor <[email protected]>
> > Reviewed-by: Nathan Chancellor <[email protected]>
> > ---
> > Changes v1->v2
> > * Move code to preexisting ifdef
> > scripts/Makefile.extrawarn | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> > index a74ce2e3c33e..95973a1ee999 100644
> > --- a/scripts/Makefile.extrawarn
> > +++ b/scripts/Makefile.extrawarn
> > @@ -70,5 +70,6 @@ KBUILD_CFLAGS += -Wno-initializer-overrides
> > KBUILD_CFLAGS += -Wno-format
> > KBUILD_CFLAGS += -Wno-sign-compare
> > KBUILD_CFLAGS += -Wno-format-zero-length
> > +KBUILD_CFLAGS += $(call cc-option,-Wno-implicit-fallthrough)
> > endif
> > endif
> > --
> > 2.23.0.rc1.153.gdeed80330f-goog
> >
>
>
> Perhaps, is the following even cleaner?
>
>
>
> diff --git a/Makefile b/Makefile
> index 1b23f95db176..cebc6bf5372e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -751,6 +751,9 @@ else
> # These warnings generated too much noise in a regular build.
> # Use make W=1 to enable them (see scripts/Makefile.extrawarn)
> KBUILD_CFLAGS += -Wno-unused-but-set-variable
> +
> +# Warn about unmarked fall-throughs in switch statement.
> +KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> endif
>
> KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
> @@ -845,9 +848,6 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC)
> -print-file-name=include)
> # warn about C99 declaration after statement
> KBUILD_CFLAGS += -Wdeclaration-after-statement
>
> -# Warn about unmarked fall-throughs in switch statement.
> -KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> -
> # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
> KBUILD_CFLAGS += -Wvla
>
>
>
> --
> Best Regards
> Masahiro Yamada
I like this more than anything suggested so far. I think a comment
should be added regarding why this is only enabled for GCC right now but
that is pretty easy to revert once we have figured out the right course
of action.
Cheers,
Nathan
On Mon, Aug 19, 2019 at 3:43 AM Nathan Chancellor
<[email protected]> wrote:
>
> On Mon, Aug 19, 2019 at 01:43:08AM +0900, Masahiro Yamada wrote:
> > Hi.
> >
> > On Fri, Aug 16, 2019 at 7:59 AM Nathan Huckleberry <[email protected]> wrote:
> > >
> > > Clang is updating to support -Wimplicit-fallthrough on C
> > > https://reviews.llvm.org/D64838. Since clang does not
> > > support the comment version of fallthrough annotations
> > > this update causes an additional 50k warnings. Most
> > > of these warnings (>49k) are duplicates from header files.
> > >
> > > This patch is intended to be reverted after the warnings
> > > have been cleaned up.
> > >
> > > Signed-off-by: Nathan Huckleberry <[email protected]>
> > > Suggested-by: Nathan Chancellor <[email protected]>
> > > Reviewed-by: Nathan Chancellor <[email protected]>
> > > ---
> > > Changes v1->v2
> > > * Move code to preexisting ifdef
> > > scripts/Makefile.extrawarn | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> > > index a74ce2e3c33e..95973a1ee999 100644
> > > --- a/scripts/Makefile.extrawarn
> > > +++ b/scripts/Makefile.extrawarn
> > > @@ -70,5 +70,6 @@ KBUILD_CFLAGS += -Wno-initializer-overrides
> > > KBUILD_CFLAGS += -Wno-format
> > > KBUILD_CFLAGS += -Wno-sign-compare
> > > KBUILD_CFLAGS += -Wno-format-zero-length
> > > +KBUILD_CFLAGS += $(call cc-option,-Wno-implicit-fallthrough)
> > > endif
> > > endif
> > > --
> > > 2.23.0.rc1.153.gdeed80330f-goog
> > >
> >
> >
> > Perhaps, is the following even cleaner?
> >
> >
> >
> > diff --git a/Makefile b/Makefile
> > index 1b23f95db176..cebc6bf5372e 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -751,6 +751,9 @@ else
> > # These warnings generated too much noise in a regular build.
> > # Use make W=1 to enable them (see scripts/Makefile.extrawarn)
> > KBUILD_CFLAGS += -Wno-unused-but-set-variable
> > +
> > +# Warn about unmarked fall-throughs in switch statement.
> > +KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> > endif
> >
> > KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
> > @@ -845,9 +848,6 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC)
> > -print-file-name=include)
> > # warn about C99 declaration after statement
> > KBUILD_CFLAGS += -Wdeclaration-after-statement
> >
> > -# Warn about unmarked fall-throughs in switch statement.
> > -KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> > -
> > # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
> > KBUILD_CFLAGS += -Wvla
> >
> >
> >
> > --
> > Best Regards
> > Masahiro Yamada
>
> I like this more than anything suggested so far. I think a comment
> should be added regarding why this is only enabled for GCC right now but
> that is pretty easy to revert once we have figured out the right course
> of action.
Agree. This is well-explained in the commit log,
but adding a short comment will be nice.
BTW, I personally like the traditional
comment version of fallthrough annotations.
Is there a plan for Clang to support it
as well as the attribute?
Thanks.
--
Best Regards
Masahiro Yamada
This functionally reverts commit bfd77145f35c ("Makefile: Convert
-Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang").
clang enabled support for -Wimplicit-fallthrough in C in r369414 [1],
which causes a lot of warnings when building the kernel for two reasons:
1. Clang does not support the /* fall through */ comments. There seems
to be a general consensus in the LLVM community that this is not
something they want to support. Joe Perches wrote a script to convert
all of the comments to a "fallthrough" keyword that will be added to
compiler_attributes.h [2] [3], which catches the vast majority of the
comments. There doesn't appear to be any consensus in the kernel
community when to do this conversion.
2. Clang and GCC disagree about falling through to final case statements
with no content or cases that simply break:
https://godbolt.org/z/c8csDu
This difference contributes at least 50 warnings in an allyesconfig
build for x86, not considering other architectures. This difference
will need to be discussed to see which compiler is right [4] [5].
[1]: https://github.com/llvm/llvm-project/commit/1e0affb6e564b7361b0aadb38805f26deff4ecfc
[2]: https://lore.kernel.org/lkml/[email protected]/
[3]: https://lore.kernel.org/lkml/1d2830aadbe9d8151728a7df5b88528fc72a0095.1564549413.git.joe@perches.com/
[4]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91432
[5]: https://github.com/ClangBuiltLinux/linux/issues/636
Given these two problems need discussion and coordination, do not enable
-Wimplicit-fallthrough with clang right now. Add a comment to explain
what is going on as well. This commit should be reverted once these two
issues are fully flushed out and resolved.
Suggested-by: Masahiro Yamada <[email protected]>
Signed-off-by: Nathan Chancellor <[email protected]>
---
Makefile | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index f125625efd60..6007a56bdbee 100644
--- a/Makefile
+++ b/Makefile
@@ -751,6 +751,11 @@ else
# These warnings generated too much noise in a regular build.
# Use make W=1 to enable them (see scripts/Makefile.extrawarn)
KBUILD_CFLAGS += -Wno-unused-but-set-variable
+
+# Warn about unmarked fall-throughs in switch statement.
+# Disabled for clang while comment to attribute conversion happens and
+# https://github.com/ClangBuiltLinux/linux/issues/636 is discussed.
+KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
endif
KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
@@ -845,9 +850,6 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
# warn about C99 declaration after statement
KBUILD_CFLAGS += -Wdeclaration-after-statement
-# Warn about unmarked fall-throughs in switch statement.
-KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
-
# Variable Length Arrays (VLAs) should not be used anywhere in the kernel
KBUILD_CFLAGS += -Wvla
--
2.23.0
On Tue, Aug 27, 2019 at 9:42 AM Nathan Chancellor
<[email protected]> wrote:
>
> This functionally reverts commit bfd77145f35c ("Makefile: Convert
> -Wimplicit-fallthrough=3 to just -Wimplicit-fallthrough for clang").
>
> clang enabled support for -Wimplicit-fallthrough in C in r369414 [1],
> which causes a lot of warnings when building the kernel for two reasons:
>
> 1. Clang does not support the /* fall through */ comments. There seems
> to be a general consensus in the LLVM community that this is not
> something they want to support. Joe Perches wrote a script to convert
> all of the comments to a "fallthrough" keyword that will be added to
> compiler_attributes.h [2] [3], which catches the vast majority of the
> comments. There doesn't appear to be any consensus in the kernel
> community when to do this conversion.
>
> 2. Clang and GCC disagree about falling through to final case statements
> with no content or cases that simply break:
>
> https://godbolt.org/z/c8csDu
>
> This difference contributes at least 50 warnings in an allyesconfig
> build for x86, not considering other architectures. This difference
> will need to be discussed to see which compiler is right [4] [5].
>
> [1]: https://github.com/llvm/llvm-project/commit/1e0affb6e564b7361b0aadb38805f26deff4ecfc
> [2]: https://lore.kernel.org/lkml/[email protected]/
> [3]: https://lore.kernel.org/lkml/1d2830aadbe9d8151728a7df5b88528fc72a0095.1564549413.git.joe@perches.com/
> [4]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91432
> [5]: https://github.com/ClangBuiltLinux/linux/issues/636
>
> Given these two problems need discussion and coordination, do not enable
> -Wimplicit-fallthrough with clang right now. Add a comment to explain
> what is going on as well. This commit should be reverted once these two
> issues are fully flushed out and resolved.
>
> Suggested-by: Masahiro Yamada <[email protected]>
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
Applied to linux-kbuild. Thanks.
(If other clang folks give tags, I will add them later.)
> Makefile | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index f125625efd60..6007a56bdbee 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -751,6 +751,11 @@ else
> # These warnings generated too much noise in a regular build.
> # Use make W=1 to enable them (see scripts/Makefile.extrawarn)
> KBUILD_CFLAGS += -Wno-unused-but-set-variable
> +
> +# Warn about unmarked fall-throughs in switch statement.
> +# Disabled for clang while comment to attribute conversion happens and
> +# https://github.com/ClangBuiltLinux/linux/issues/636 is discussed.
> +KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> endif
>
> KBUILD_CFLAGS += $(call cc-disable-warning, unused-const-variable)
> @@ -845,9 +850,6 @@ NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
> # warn about C99 declaration after statement
> KBUILD_CFLAGS += -Wdeclaration-after-statement
>
> -# Warn about unmarked fall-throughs in switch statement.
> -KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough,)
> -
> # Variable Length Arrays (VLAs) should not be used anywhere in the kernel
> KBUILD_CFLAGS += -Wvla
>
> --
> 2.23.0
>
--
Best Regards
Masahiro Yamada
On Wed, Aug 28, 2019 at 6:21 PM Masahiro Yamada
<[email protected]> wrote:
>
> Applied to linux-kbuild. Thanks.
>
> (If other clang folks give tags, I will add them later.)
Acked-by: Miguel Ojeda <[email protected]>
Cheers,
Miguel
On Wed, Aug 28, 2019 at 9:45 AM Miguel Ojeda
<[email protected]> wrote:
>
> On Wed, Aug 28, 2019 at 6:21 PM Masahiro Yamada
> <[email protected]> wrote:
> >
> > Applied to linux-kbuild. Thanks.
> >
> > (If other clang folks give tags, I will add them later.)
>
> Acked-by: Miguel Ojeda <[email protected]>
I verified that GCC didn't get support for -Wimplicit-fallthrough
until GCC ~7.1 release, so the cc-option guard is still required.
Acked-by: Nick Desaulniers <[email protected]>
Thanks for the patch Nathan.
--
Thanks,
~Nick Desaulniers
On Wed, Aug 28, 2019 at 10:39 AM Nick Desaulniers
<[email protected]> wrote:
>
> On Wed, Aug 28, 2019 at 9:45 AM Miguel Ojeda
> <[email protected]> wrote:
> >
> > On Wed, Aug 28, 2019 at 6:21 PM Masahiro Yamada
> > <[email protected]> wrote:
> > >
> > > Applied to linux-kbuild. Thanks.
> > >
> > > (If other clang folks give tags, I will add them later.)
> >
> > Acked-by: Miguel Ojeda <[email protected]>
>
> I verified that GCC didn't get support for -Wimplicit-fallthrough
> until GCC ~7.1 release, so the cc-option guard is still required.
> Acked-by: Nick Desaulniers <[email protected]>
> Thanks for the patch Nathan.
Also, there's an inconsistency between Makefile vs
scripts/Makefile.extrawarn that's been bugging me: it seems we enable
GCC only flags in Makefile, then disable certain Clang flags in
scripts/Makefile.extrawarn. Not necessary to fix here and now, but I
hope one day to have one file that has all of the compiler specific
flags in one place (maybe its own file), so I only have to look in one
place. I know, I know, "patches welcome." ;)
--
Thanks,
~Nick Desaulniers
On 8/28/19 11:20 AM, Masahiro Yamada wrote:
>>
>> Given these two problems need discussion and coordination, do not enable
>> -Wimplicit-fallthrough with clang right now. Add a comment to explain
>> what is going on as well. This commit should be reverted once these two
>> issues are fully flushed out and resolved.
>>
>> Suggested-by: Masahiro Yamada <[email protected]>
>> Signed-off-by: Nathan Chancellor <[email protected]>
>> ---
>
> Applied to linux-kbuild. Thanks.
>
> (If other clang folks give tags, I will add them later.)
>
Acked-by: Gustavo A. R. Silva <[email protected]>
Thanks
--
Gustavo
On Thu, Aug 29, 2019 at 2:44 AM Nick Desaulniers
<[email protected]> wrote:
>
> On Wed, Aug 28, 2019 at 10:39 AM Nick Desaulniers
> <[email protected]> wrote:
> >
> > On Wed, Aug 28, 2019 at 9:45 AM Miguel Ojeda
> > <[email protected]> wrote:
> > >
> > > On Wed, Aug 28, 2019 at 6:21 PM Masahiro Yamada
> > > <[email protected]> wrote:
> > > >
> > > > Applied to linux-kbuild. Thanks.
> > > >
> > > > (If other clang folks give tags, I will add them later.)
> > >
> > > Acked-by: Miguel Ojeda <[email protected]>
> >
> > I verified that GCC didn't get support for -Wimplicit-fallthrough
> > until GCC ~7.1 release, so the cc-option guard is still required.
> > Acked-by: Nick Desaulniers <[email protected]>
> > Thanks for the patch Nathan.
>
> Also, there's an inconsistency between Makefile vs
> scripts/Makefile.extrawarn that's been bugging me: it seems we enable
> GCC only flags in Makefile, then disable certain Clang flags in
> scripts/Makefile.extrawarn.
All the flags listed in scripts/Makefile.extrawarn depend on W= option.
The options in Makefile are passed irrespective of W=.
There is no inconsistency.
> Not necessary to fix here and now, but I
> hope one day to have one file that has all of the compiler specific
> flags in one place (maybe its own file), so I only have to look in one
> place. I know, I know, "patches welcome." ;)
>
> --
> Thanks,
> ~Nick Desaulniers
--
Best Regards
Masahiro Yamada