Re: [GIT PULL] fallthrough fixes for Clang for 5.14-rc2

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Nathan Chancellor <nathan@kernel.org>
Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Kees Cook <keescook@chromium.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	clang-built-linux <clang-built-linux@googlegroups.com>
Subject: Re: [GIT PULL] fallthrough fixes for Clang for 5.14-rc2
Date: Fri, 16 Jul 2021 12:22:31 -0700	[thread overview]
Message-ID: <CAHk-=wg-qBVjhqoRiV0EdkFSpP1FebmRYwjiv-=GM3EVQYbBqg@mail.gmail.com> (raw)
In-Reply-To: <YPHUJsiaOuqzW0Od@archlinux-ax161>

On Fri, Jul 16, 2021 at 11:47 AM Nathan Chancellor <nathan@kernel.org> wrote:
>
> I am not really sure how to resolve that within checkFallThroughIntoBlock() or
> fillReachableBlocks() but given that this is something specific to the kernel,

It's not at all specific to the kernel. Yes, the particular example
was from the kernel, but the issue is very much generic.

Yes, that particular example was from the kernel and used a CONFIG option.

But I can actually point to user-space code that looks very much like it:

   https://sources.debian.org/src/libreoffice/1:7.0.4-4/stoc/source/simpleregistry/simpleregistry.cxx/?hl=223#L223

look at that code, and tell me it makes sense.

You want to have the fallthrough for the case where abort() isn't
marked as noreturn, but you don't want to get a warning for the case
where a compile environment *does* have that noreturn thing.

See the issue? EXACT SAME THING.

This is in no way kernel-specific. The fact is, code can be
unreachable without it being a bug.

A common example of unreachable code is things like this:

  https://sources.debian.org/src/apparmor/2.13.6-10/parser/libapparmor_re/chfa.cc/?hl=338#L338

Look, it's a "switch (sizeof())", which means that only one of the
cases is ever going to be reachable.

That code doesn't actually use "[[fallthrough]]" right now, and just
uses the implicit fallthrough. But imagine if it was converted to use
that fallthrough annotation. If the "sizeof()" isn't the largest size,
those fallthrough's will be fundamentally unreachable, because the
whole case is unreachable.

Warning about unreachable code is simply WRONG. It happens very
naturally in C, exactly becuse people do conditionals based on
compile-time constants. Those compile-time constants may be about
things like "sizeof", they may be about things like that "abort() may
be no-return or not".

But it can also easily be about patterns where you always check error
returns, and some functions are inline and never (or always) return
errors, so that your code ends up having stuff that is just statically
always true (or always false), and then the implication is that there
is unreachable code that the compiler will just compile away.

And no, this is in no way kernel-specific at all.

That warning needs

 (a) a different flag - because "warn about unreachable" is completely
different from "warn about implicit fallthrough"

 (b) point to where the warning is

but honestly, it would be better to just remove the warning entirely,
because it is just fundamentally wrong for all the reasons outlined
above.

                  Linus