Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo keyword for switch/case use

[Nathan Chancellor <natechancellor@gmail.com>]

Hi Joe,

On Sun, Aug 04, 2019 at 05:39:28PM -0700, Joe Perches wrote:
> On Sun, 2019-08-04 at 11:09 -0700, Linus Torvalds wrote:
> > On Sun, Aug 4, 2019 at 11:01 AM Joe Perches <joe@perches.com> wrote:
> > > Linus?  Do you have an opinion about this RFC/patch?
> > 
> > So my only real concern is that the comment approach has always been
> > the really traditional one, going back all the way to 'lint' days.
> > 
> > And you obviously cannot use a #define to create a comment, so this
> > whole keyword model will never be able to do that.
> > 
> > At the same time, all the modern tools we care about do seem to be
> > happy with it, either through the gcc attribute, the clang
> > [[clang:fallthrough]] or the (eventual) standard C [[fallthrough]]
> > model.
> 
> (adding Nick Desaulniers and clang-built-linux to cc's)

Thanks for adding us.

> As far as I can tell, clang 10 (and it took hours to compile
> and link the most current version here) does not support

Just a heads up in case you want to mess around with clang in the
future, I wrote a toolchain build script for ClangBuiltLinux to help
with the long compile times by cutting as much cruft as possible (and
it is self contained by default, won't install anything outside of the
repository).

https://github.com/ClangBuiltLinux/tc-build

The slimmest working configuration for testing what you did would probably
be from the following command:

./build-llvm.py --build-stage1-only --projects clang --targets X86

> 	-Wimplicit-fallthrough=3
> and using just -Wimplicit-fallthrough with clang 10 does not emit
> a fallthrough warning even with -Wextra and -Wimplicit-fallthrough
> using switch / case code blocks like:

Unfortunately, -Wimplicit-fallthrough does not work for C right now
(only C++), as pointed out by Nick on LLVM's bug tracker.

https://bugs.llvm.org/show_bug.cgi?id=39382

This patch resolves that while adding support for the attribute.

https://reviews.llvm.org/D64838

Your example properly works when that patch is applied and
-Wimplicit-fallthrough is added to the list of flags.

../lib/test_module.c:24:2: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
        case 2:
        ^
../lib/test_module.c:24:2: note: insert '__attribute__((fallthrough));' to silence this warning
        case 2:
        ^
        __attribute__((fallthrough)); 
../lib/test_module.c:24:2: note: insert 'break;' to avoid fall-through
        case 2:
        ^
        break; 

Hopefully it can get merged soon. I am sure Nathan or Nick can speak
to further progress on that.

> The __has_attribute use is at least clang compatible.
> https://releases.llvm.org/3.7.0/tools/clang/docs/LanguageExtensions.html
> even if it doesn't (seem to?) work.

I was trying to follow along with this thread through the web interface
and kind of got lost, how does it not work? If I apply your compiler
attributes patch with D64838, I see fallthrough get expanded to
__attribute__((__fallthrough__)) by the preprocessor.

> >  - we'd need to make -Wimplicit-fallthrough be dependent on the
> > compiler actually supporting the attribute, not just on supporting the
> > flag.
> 
> I believe that also needs work if ever clang works,
> 
> Makefile:KBUILD_CFLAGS += $(call cc-option,-Wimplicit-fallthrough=3,)
> 
> this will have to be changed for clang as the =<val> isn't (yet?) supported.

GCC's documentation says that -Wimplicit-fallthrough is equivalent to
-Wimplicit-fallthrough=3 so it seems like just making that change would
be all that is needed to support Clang:

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wimplicit-fallthrough

Cheers,
Nathan