Re: [PATCH] kconfig: Add support for -Wimplicit-fallthrough

[Gustavo A. R. Silva]

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@xxxxxxxxxxxx>
> > > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > > Co-developed-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>
> > 
> > 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@xxxxxxxxxx>
> > Tested-by: Nathan Chancellor <nathan@xxxxxxxxxx>
> > 
> > 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
> > > 
> > > 
> >