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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190805011815.GA110280@archlinux-threadripper>
Date:   Sun, 4 Aug 2019 18:18:15 -0700
From:   Nathan Chancellor <natechancellor@...il.com>
To:     Joe Perches <joe@...ches.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
        Kees Cook <keescook@...omium.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Borislav Petkov <bp@...en8.de>,
        "H . Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Pavel Machek <pavel@....cz>,
        "Gustavo A . R . Silva" <gustavo@...eddedor.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Jiri Olsa <jolsa@...hat.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Shawn Landden <shawn@....icu>,
        the arch/x86 maintainers <x86@...nel.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Neil Horman <nhorman@...driver.com>,
        David Miller <davem@...emloft.net>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        clang-built-linux@...glegroups.com
Subject: Re: [RFC PATCH] compiler_attributes.h: Add 'fallthrough' pseudo
 keyword for switch/case use

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@...ches.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