* Re: [PATCH] kconfig: Add support for -Wimplicit-fallthrough
2021-11-14 21:18 ` Nathan Chancellor
@ 2021-11-14 21:25 ` Linus Torvalds
2021-11-14 21:31 ` Gustavo A. R. Silva
2021-11-15 0:17 ` Nathan Chancellor
2 siblings, 0 replies; 9+ messages in thread
From: Linus Torvalds @ 2021-11-14 21:25 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Gustavo A. R. Silva, Kees Cook, Linux Kbuild mailing list,
Linux Kernel Mailing List, linux-hardening, Nick Desaulniers,
llvm
On Sun, Nov 14, 2021 at 1:18 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> It feels a little odd to have this in Kconfig but if it works and gets
> the warning enabled, then so be it.
We've actually been actively moving more and more of the compiler
flags configuration to Kconfig time.
The Kconfig language makes it fairly easy and natural to do these
days, and in some cases the compiler flags end up having a number of
other dependencies (not in this case, but look at things like
CC_HAS_KASAN_GENERIC etc, where the existence of some compiler flag
ends up then also affecting whether other options can be enabled).
So the whole "move it to config time" ends up being the direction
we've been going, and the only really unusual case here is that it
ends up being done as a string config rather than as a boolean.
And that's just because the different compilers also have different
flags in this case ;(
Linus
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] kconfig: Add support for -Wimplicit-fallthrough
2021-11-14 21:18 ` Nathan Chancellor
2021-11-14 21:25 ` Linus Torvalds
@ 2021-11-14 21:31 ` Gustavo A. R. Silva
2021-11-15 0:17 ` Nathan Chancellor
2 siblings, 0 replies; 9+ messages in thread
From: Gustavo A. R. Silva @ 2021-11-14 21:31 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Linus Torvalds, Kees Cook, linux-kbuild, linux-kernel,
linux-hardening, Nick Desaulniers, llvm
On Sun, Nov 14, 2021 at 02:18:41PM -0700, Nathan Chancellor wrote:
> On Sat, Nov 13, 2021 at 06:57:25PM -0600, Gustavo A. R. Silva wrote:
> > Add Kconfig support for -Wimplicit-fallthrough for both GCC and Clang.
> >
> > The compiler option is under configuration CC_IMPLICIT_FALLTHROUGH,
> > which is enabled by default.
> >
> > Special thanks to Nathan Chancellor who fixed the Clang bug[1][2]. This
> > bugfix only appears in Clang 14.0.0, so older versions still contain
> > the bug and -Wimplicit-fallthrough won't be enabled for them, for now.
> >
> > This concludes a long journey and now we are finally getting rid
> > of the unintentional fallthrough bug-class in the kernel, entirely. :)
> >
> > [1] https://github.com/llvm/llvm-project/commit/9ed4a94d6451046a51ef393cd62f00710820a7e8
> > [2] https://bugs.llvm.org/show_bug.cgi?id=51094
> >
> > Link: https://github.com/KSPP/linux/issues/115
> > Link: https://github.com/ClangBuiltLinux/linux/issues/236
> > Co-developed-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Co-developed-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>
> This appears to do the right thing with both clang-13 and clang-14.
>
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> Tested-by: Nathan Chancellor <nathan@kernel.org>
Thanks, Nathan.
--
Gustavo
>
> It feels a little odd to have this in Kconfig but if it works and gets
> the warning enabled, then so be it.
>
> > ---
> > Makefile | 6 +-----
> > init/Kconfig | 5 +++++
> > 2 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 30c7c81d0437..f18a50daad00 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -786,7 +786,7 @@ stackp-flags-$(CONFIG_STACKPROTECTOR_STRONG) := -fstack-protector-strong
> > KBUILD_CFLAGS += $(stackp-flags-y)
> >
> > KBUILD_CFLAGS-$(CONFIG_WERROR) += -Werror
> > -KBUILD_CFLAGS += $(KBUILD_CFLAGS-y)
> > +KBUILD_CFLAGS += $(KBUILD_CFLAGS-y) $(CONFIG_CC_IMPLICIT_FALLTHROUGH)
> >
> > ifdef CONFIG_CC_IS_CLANG
> > KBUILD_CPPFLAGS += -Qunused-arguments
> > @@ -798,10 +798,6 @@ KBUILD_CFLAGS += -Wno-gnu
> > KBUILD_CFLAGS += -mno-global-merge
> > else
> >
> > -# 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=5,)
> > # gcc inanely warns about local variables called 'main'
> > KBUILD_CFLAGS += -Wno-main
> > endif
> > diff --git a/init/Kconfig b/init/Kconfig
> > index 11f8a845f259..b0582cd3e096 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -885,6 +885,11 @@ config ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
> > config CC_HAS_INT128
> > def_bool !$(cc-option,$(m64-flag) -D__SIZEOF_INT128__=0) && 64BIT
> >
> > +config CC_IMPLICIT_FALLTHROUGH
> > + string
> > + default "-Wimplicit-fallthrough=5" if CC_IS_GCC
> > + default "-Wimplicit-fallthrough" if CC_IS_CLANG && $(cc-option,-Wunreachable-code-fallthrough)
> > +
> > #
> > # For architectures that know their GCC __int128 support is sound
> > #
> > --
> > 2.27.0
> >
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kconfig: Add support for -Wimplicit-fallthrough
2021-11-14 21:18 ` Nathan Chancellor
2021-11-14 21:25 ` Linus Torvalds
2021-11-14 21:31 ` Gustavo A. R. Silva
@ 2021-11-15 0:17 ` Nathan Chancellor
2021-11-15 0:35 ` Gustavo A. R. Silva
2 siblings, 1 reply; 9+ messages in thread
From: Nathan Chancellor @ 2021-11-15 0:17 UTC (permalink / raw)
To: Gustavo A. R. Silva, Linus Torvalds
Cc: Kees Cook, linux-kbuild, linux-kernel, linux-hardening,
Nick Desaulniers, llvm
On Sun, Nov 14, 2021 at 02:18:41PM -0700, Nathan Chancellor wrote:
> On Sat, Nov 13, 2021 at 06:57:25PM -0600, Gustavo A. R. Silva wrote:
> > Add Kconfig support for -Wimplicit-fallthrough for both GCC and Clang.
> >
> > The compiler option is under configuration CC_IMPLICIT_FALLTHROUGH,
> > which is enabled by default.
> >
> > Special thanks to Nathan Chancellor who fixed the Clang bug[1][2]. This
> > bugfix only appears in Clang 14.0.0, so older versions still contain
> > the bug and -Wimplicit-fallthrough won't be enabled for them, for now.
> >
> > This concludes a long journey and now we are finally getting rid
> > of the unintentional fallthrough bug-class in the kernel, entirely. :)
> >
> > [1] https://github.com/llvm/llvm-project/commit/9ed4a94d6451046a51ef393cd62f00710820a7e8
> > [2] https://bugs.llvm.org/show_bug.cgi?id=51094
> >
> > Link: https://github.com/KSPP/linux/issues/115
> > Link: https://github.com/ClangBuiltLinux/linux/issues/236
> > Co-developed-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Co-developed-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>
> This appears to do the right thing with both clang-13 and clang-14.
Now that I gave this a look for the GCC side, I think it is wrong.
-Wimplicit-fallthrough=5 was under cc-option because it was only
available in GCC 7.x and newer so the build is now broken for GCC 5.x
and 6.x:
gcc: error: unrecognized command line option '-Wimplicit-fallthrough=5';
did you mean '-Wno-fallthrough'?
I think this needs to be added (I can send a formal patch tomorrow
unless someone wants to beat me to it):
diff --git a/init/Kconfig b/init/Kconfig
index 036b750e8d8a..85882c317235 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -887,7 +887,7 @@ config CC_HAS_INT128
config CC_IMPLICIT_FALLTHROUGH
string
- default "-Wimplicit-fallthrough=5" if CC_IS_GCC
+ default "-Wimplicit-fallthrough=5" if $(cc-option,-Wimplicit-fallthrough=5)
default "-Wimplicit-fallthrough" if CC_IS_CLANG && $(cc-option,-Wunreachable-code-fallthrough)
#
Cheers,
Nathan
> Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> Tested-by: Nathan Chancellor <nathan@kernel.org>
>
> It feels a little odd to have this in Kconfig but if it works and gets
> the warning enabled, then so be it.
>
> > ---
> > Makefile | 6 +-----
> > init/Kconfig | 5 +++++
> > 2 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 30c7c81d0437..f18a50daad00 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -786,7 +786,7 @@ stackp-flags-$(CONFIG_STACKPROTECTOR_STRONG) := -fstack-protector-strong
> > KBUILD_CFLAGS += $(stackp-flags-y)
> >
> > KBUILD_CFLAGS-$(CONFIG_WERROR) += -Werror
> > -KBUILD_CFLAGS += $(KBUILD_CFLAGS-y)
> > +KBUILD_CFLAGS += $(KBUILD_CFLAGS-y) $(CONFIG_CC_IMPLICIT_FALLTHROUGH)
> >
> > ifdef CONFIG_CC_IS_CLANG
> > KBUILD_CPPFLAGS += -Qunused-arguments
> > @@ -798,10 +798,6 @@ KBUILD_CFLAGS += -Wno-gnu
> > KBUILD_CFLAGS += -mno-global-merge
> > else
> >
> > -# 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=5,)
> > # gcc inanely warns about local variables called 'main'
> > KBUILD_CFLAGS += -Wno-main
> > endif
> > diff --git a/init/Kconfig b/init/Kconfig
> > index 11f8a845f259..b0582cd3e096 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -885,6 +885,11 @@ config ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
> > config CC_HAS_INT128
> > def_bool !$(cc-option,$(m64-flag) -D__SIZEOF_INT128__=0) && 64BIT
> >
> > +config CC_IMPLICIT_FALLTHROUGH
> > + string
> > + default "-Wimplicit-fallthrough=5" if CC_IS_GCC
> > + default "-Wimplicit-fallthrough" if CC_IS_CLANG && $(cc-option,-Wunreachable-code-fallthrough)
> > +
> > #
> > # For architectures that know their GCC __int128 support is sound
> > #
> > --
> > 2.27.0
> >
> >
>
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] kconfig: Add support for -Wimplicit-fallthrough
2021-11-15 0:17 ` Nathan Chancellor
@ 2021-11-15 0:35 ` Gustavo A. R. Silva
2021-11-15 0:44 ` Masahiro Yamada
0 siblings, 1 reply; 9+ messages in thread
From: Gustavo A. R. Silva @ 2021-11-15 0:35 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Linus Torvalds, Kees Cook, linux-kbuild, linux-kernel,
linux-hardening, Nick Desaulniers, llvm
On Sun, Nov 14, 2021 at 05:17:51PM -0700, Nathan Chancellor wrote:
> On Sun, Nov 14, 2021 at 02:18:41PM -0700, Nathan Chancellor wrote:
> > On Sat, Nov 13, 2021 at 06:57:25PM -0600, Gustavo A. R. Silva wrote:
> > > Add Kconfig support for -Wimplicit-fallthrough for both GCC and Clang.
> > >
> > > The compiler option is under configuration CC_IMPLICIT_FALLTHROUGH,
> > > which is enabled by default.
> > >
> > > Special thanks to Nathan Chancellor who fixed the Clang bug[1][2]. This
> > > bugfix only appears in Clang 14.0.0, so older versions still contain
> > > the bug and -Wimplicit-fallthrough won't be enabled for them, for now.
> > >
> > > This concludes a long journey and now we are finally getting rid
> > > of the unintentional fallthrough bug-class in the kernel, entirely. :)
> > >
> > > [1] https://github.com/llvm/llvm-project/commit/9ed4a94d6451046a51ef393cd62f00710820a7e8
> > > [2] https://bugs.llvm.org/show_bug.cgi?id=51094
> > >
> > > Link: https://github.com/KSPP/linux/issues/115
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/236
> > > Co-developed-by: Kees Cook <keescook@chromium.org>
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > Co-developed-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> >
> > This appears to do the right thing with both clang-13 and clang-14.
>
> Now that I gave this a look for the GCC side, I think it is wrong.
>
> -Wimplicit-fallthrough=5 was under cc-option because it was only
> available in GCC 7.x and newer so the build is now broken for GCC 5.x
> and 6.x:
>
> gcc: error: unrecognized command line option '-Wimplicit-fallthrough=5';
> did you mean '-Wno-fallthrough'?
I'll send a patch for this right away. Thanks for the report, Nathan! :)
--
Gustavo
>
> I think this needs to be added (I can send a formal patch tomorrow
> unless someone wants to beat me to it):
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 036b750e8d8a..85882c317235 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -887,7 +887,7 @@ config CC_HAS_INT128
>
> config CC_IMPLICIT_FALLTHROUGH
> string
> - default "-Wimplicit-fallthrough=5" if CC_IS_GCC
> + default "-Wimplicit-fallthrough=5" if $(cc-option,-Wimplicit-fallthrough=5)
> default "-Wimplicit-fallthrough" if CC_IS_CLANG && $(cc-option,-Wunreachable-code-fallthrough)
>
> #
>
> Cheers,
> Nathan
>
> > Reviewed-by: Nathan Chancellor <nathan@kernel.org>
> > Tested-by: Nathan Chancellor <nathan@kernel.org>
> >
> > It feels a little odd to have this in Kconfig but if it works and gets
> > the warning enabled, then so be it.
> >
> > > ---
> > > Makefile | 6 +-----
> > > init/Kconfig | 5 +++++
> > > 2 files changed, 6 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index 30c7c81d0437..f18a50daad00 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -786,7 +786,7 @@ stackp-flags-$(CONFIG_STACKPROTECTOR_STRONG) := -fstack-protector-strong
> > > KBUILD_CFLAGS += $(stackp-flags-y)
> > >
> > > KBUILD_CFLAGS-$(CONFIG_WERROR) += -Werror
> > > -KBUILD_CFLAGS += $(KBUILD_CFLAGS-y)
> > > +KBUILD_CFLAGS += $(KBUILD_CFLAGS-y) $(CONFIG_CC_IMPLICIT_FALLTHROUGH)
> > >
> > > ifdef CONFIG_CC_IS_CLANG
> > > KBUILD_CPPFLAGS += -Qunused-arguments
> > > @@ -798,10 +798,6 @@ KBUILD_CFLAGS += -Wno-gnu
> > > KBUILD_CFLAGS += -mno-global-merge
> > > else
> > >
> > > -# 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=5,)
> > > # gcc inanely warns about local variables called 'main'
> > > KBUILD_CFLAGS += -Wno-main
> > > endif
> > > diff --git a/init/Kconfig b/init/Kconfig
> > > index 11f8a845f259..b0582cd3e096 100644
> > > --- a/init/Kconfig
> > > +++ b/init/Kconfig
> > > @@ -885,6 +885,11 @@ config ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
> > > config CC_HAS_INT128
> > > def_bool !$(cc-option,$(m64-flag) -D__SIZEOF_INT128__=0) && 64BIT
> > >
> > > +config CC_IMPLICIT_FALLTHROUGH
> > > + string
> > > + default "-Wimplicit-fallthrough=5" if CC_IS_GCC
> > > + default "-Wimplicit-fallthrough" if CC_IS_CLANG && $(cc-option,-Wunreachable-code-fallthrough)
> > > +
> > > #
> > > # For architectures that know their GCC __int128 support is sound
> > > #
> > > --
> > > 2.27.0
> > >
> > >
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kconfig: Add support for -Wimplicit-fallthrough
2021-11-15 0:35 ` Gustavo A. R. Silva
@ 2021-11-15 0:44 ` Masahiro Yamada
2021-11-15 1:00 ` Gustavo A. R. Silva
0 siblings, 1 reply; 9+ messages in thread
From: Masahiro Yamada @ 2021-11-15 0:44 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Nathan Chancellor, Linus Torvalds, Kees Cook,
Linux Kbuild mailing list, Linux Kernel Mailing List,
linux-hardening, Nick Desaulniers, llvm
On Mon, Nov 15, 2021 at 9:30 AM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> On Sun, Nov 14, 2021 at 05:17:51PM -0700, Nathan Chancellor wrote:
> > On Sun, Nov 14, 2021 at 02:18:41PM -0700, Nathan Chancellor wrote:
> > > On Sat, Nov 13, 2021 at 06:57:25PM -0600, Gustavo A. R. Silva wrote:
> > > > Add Kconfig support for -Wimplicit-fallthrough for both GCC and Clang.
> > > >
> > > > The compiler option is under configuration CC_IMPLICIT_FALLTHROUGH,
> > > > which is enabled by default.
> > > >
> > > > Special thanks to Nathan Chancellor who fixed the Clang bug[1][2]. This
> > > > bugfix only appears in Clang 14.0.0, so older versions still contain
> > > > the bug and -Wimplicit-fallthrough won't be enabled for them, for now.
> > > >
> > > > This concludes a long journey and now we are finally getting rid
> > > > of the unintentional fallthrough bug-class in the kernel, entirely. :)
> > > >
> > > > [1] https://github.com/llvm/llvm-project/commit/9ed4a94d6451046a51ef393cd62f00710820a7e8
> > > > [2] https://bugs.llvm.org/show_bug.cgi?id=51094
> > > >
> > > > Link: https://github.com/KSPP/linux/issues/115
> > > > Link: https://github.com/ClangBuiltLinux/linux/issues/236
> > > > Co-developed-by: Kees Cook <keescook@chromium.org>
> > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > > Co-developed-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> > >
> > > This appears to do the right thing with both clang-13 and clang-14.
> >
> > Now that I gave this a look for the GCC side, I think it is wrong.
> >
> > -Wimplicit-fallthrough=5 was under cc-option because it was only
> > available in GCC 7.x and newer so the build is now broken for GCC 5.x
> > and 6.x:
> >
> > gcc: error: unrecognized command line option '-Wimplicit-fallthrough=5';
> > did you mean '-Wno-fallthrough'?
>
> I'll send a patch for this right away. Thanks for the report, Nathan! :)
Please use a subject prefix different than "kconfig:"
I want to see "kconfig:' only for changes in scripts/kconfig/.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kconfig: Add support for -Wimplicit-fallthrough
2021-11-15 0:44 ` Masahiro Yamada
@ 2021-11-15 1:00 ` Gustavo A. R. Silva
2021-11-15 2:16 ` Masahiro Yamada
0 siblings, 1 reply; 9+ messages in thread
From: Gustavo A. R. Silva @ 2021-11-15 1:00 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Nathan Chancellor, Linus Torvalds, Kees Cook,
Linux Kbuild mailing list, Linux Kernel Mailing List,
linux-hardening, Nick Desaulniers, llvm
On Mon, Nov 15, 2021 at 09:44:40AM +0900, Masahiro Yamada wrote:
> On Mon, Nov 15, 2021 at 9:30 AM Gustavo A. R. Silva
> <gustavoars@kernel.org> wrote:
> >
>
> Please use a subject prefix different than "kconfig:"
>
> I want to see "kconfig:' only for changes in scripts/kconfig/.
How about kbuild for this case, instead?
Thanks
--
Gustavo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kconfig: Add support for -Wimplicit-fallthrough
2021-11-15 1:00 ` Gustavo A. R. Silva
@ 2021-11-15 2:16 ` Masahiro Yamada
0 siblings, 0 replies; 9+ messages in thread
From: Masahiro Yamada @ 2021-11-15 2:16 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Nathan Chancellor, Linus Torvalds, Kees Cook,
Linux Kbuild mailing list, Linux Kernel Mailing List,
linux-hardening, Nick Desaulniers, llvm
On Mon, Nov 15, 2021 at 9:54 AM Gustavo A. R. Silva
<gustavoars@kernel.org> wrote:
>
> On Mon, Nov 15, 2021 at 09:44:40AM +0900, Masahiro Yamada wrote:
> > On Mon, Nov 15, 2021 at 9:30 AM Gustavo A. R. Silva
> > <gustavoars@kernel.org> wrote:
> > >
> >
> > Please use a subject prefix different than "kconfig:"
> >
> > I want to see "kconfig:' only for changes in scripts/kconfig/.
>
> How about kbuild for this case, instead?
Sounds good to me.
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 9+ messages in thread