| Message ID | 20190827004155.11366-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kbuild: Do not enable -Wimplicit-fallthrough for clang for now | expand |
On Tue, Aug 27, 2019 at 9:42 AM Nathan Chancellor <natechancellor@gmail.com> 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/61ddbb86d5e68a15e24ccb06d9b399bbf5ce2da7.camel@perches.com/ > [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 <yamada.masahiro@socionext.com> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- 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 >
On Wed, Aug 28, 2019 at 6:21 PM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > Applied to linux-kbuild. Thanks. > > (If other clang folks give tags, I will add them later.) Acked-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> Cheers, Miguel
On Wed, Aug 28, 2019 at 9:45 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Wed, Aug 28, 2019 at 6:21 PM Masahiro Yamada > <yamada.masahiro@socionext.com> wrote: > > > > Applied to linux-kbuild. Thanks. > > > > (If other clang folks give tags, I will add them later.) > > Acked-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> 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 <ndesaulniers@google.com> Thanks for the patch Nathan.
On Wed, Aug 28, 2019 at 10:39 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Wed, Aug 28, 2019 at 9:45 AM Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: > > > > On Wed, Aug 28, 2019 at 6:21 PM Masahiro Yamada > > <yamada.masahiro@socionext.com> wrote: > > > > > > Applied to linux-kbuild. Thanks. > > > > > > (If other clang folks give tags, I will add them later.) > > > > Acked-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> > > 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 <ndesaulniers@google.com> > 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." ;)
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 <yamada.masahiro@socionext.com> >> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> >> --- > > Applied to linux-kbuild. Thanks. > > (If other clang folks give tags, I will add them later.) > Acked-by: Gustavo A. R. Silva <gustavo@embeddedor.com> Thanks -- Gustavo
On Thu, Aug 29, 2019 at 2:44 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Wed, Aug 28, 2019 at 10:39 AM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > > > On Wed, Aug 28, 2019 at 9:45 AM Miguel Ojeda > > <miguel.ojeda.sandonis@gmail.com> wrote: > > > > > > On Wed, Aug 28, 2019 at 6:21 PM Masahiro Yamada > > > <yamada.masahiro@socionext.com> wrote: > > > > > > > > Applied to linux-kbuild. Thanks. > > > > > > > > (If other clang folks give tags, I will add them later.) > > > > > > Acked-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> > > > > 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 <ndesaulniers@google.com> > > 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
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
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/61ddbb86d5e68a15e24ccb06d9b399bbf5ce2da7.camel@perches.com/ [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 <yamada.masahiro@socionext.com> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- Makefile | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)