kees commented 3 years ago


Bugzilla Link	51094
Version	trunk
OS	Linux
CC	@AaronBallman,@dwblaikie,@DougGregor,@MaskRay,@nathanchance,@nickdesaulniers,@zygoloid

Extended Description

There are some places in the kernel where the "fallthrough;" annotation is used after a portion of code that may get elided at build time:

case 1: if (something || !IS_ENALBED(CONFIG_SOMETHING)) return blah; fallthrough; case 2: This looks like:

case 1: fallthrough; case 2: And a warning is generated:

warning: fallthrough annotation in unreachable code [-Wimplicit-fallthrough]

But isn't a useful warning in this case, and should likely be silenced or adjust to not warn where there was actually code there before getting elided. At the least, this warning would be best moved to a separate flag so it can be disabled on kernel builds (i.e. GCC does not warn about these cases).

Some specific examples:

https://github.com/ClangBuiltLinux/continuous-integration2/runs/3058126539?check_suite_focus=true#step:5:120 https://github.com/ClangBuiltLinux/continuous-integration2/runs/3058126329?check_suite_focus=true#step:5:92

llvmbot commented 3 years ago

Will this be included in LLVM 12.0.2 ? When will this be released?

This should ride the trains out to LLVM 14. We should look to re-enable -Wimplifcit-fallthrough for clang-14+.

...in the Linux kernel.

Got it.

Thanks!

nickdesaulniers commented 3 years ago

Will this be included in LLVM 12.0.2 ? When will this be released?

This should ride the trains out to LLVM 14. We should look to re-enable -Wimplifcit-fallthrough for clang-14+.

...in the Linux kernel.

nickdesaulniers commented 3 years ago

Will this be included in LLVM 12.0.2 ? When will this be released?

This should ride the trains out to LLVM 14. We should look to re-enable -Wimplifcit-fallthrough for clang-14+.

llvmbot commented 3 years ago

Committed: https://github.com/llvm/llvm-project/commit/ 9ed4a94d6451046a51ef393cd62f00710820a7e8

For all intents and purposes, this is resolved for the Linux kernel. -Wunreachable-code is off by default, even with -Wall, and as far am I am aware, we will never be able to enable it because of the variety of configuration combinations that may produce dead code. We can leave this open to try and fix this more robustly later down the road (such as not warning if the annotation was preceded by an if statement with a constant condition) or we can just close this out.

Great! :)

Will this be included in LLVM 12.0.2 ? When will this be released?

Thanks, Nathan.

nathanchance commented 3 years ago

Committed: https://github.com/llvm/llvm-project/commit/9ed4a94d6451046a51ef393cd62f00710820a7e8

For all intents and purposes, this is resolved for the Linux kernel. -Wunreachable-code is off by default, even with -Wall, and as far am I am aware, we will never be able to enable it because of the variety of configuration combinations that may produce dead code. We can leave this open to try and fix this more robustly later down the road (such as not warning if the annotation was preceded by an if statement with a constant condition) or we can just close this out.

nathanchance commented 3 years ago

I've pushed https://reviews.llvm.org/D107933 as an initial solution for the kernel based on the discussion so far. I am far from a clang expert but I am happy to try and adapt any other solutions with a little guidance.

dwblaikie commented 3 years ago

I guess I mean: what cases is this warning meant to catch that are buggy enough to be worth a warning?

"fallthrough" after "break" is probably the main one.

Yeah, though we don't warn on break after break (or return, etc, etc) - admittedly fallthrough is one where we can get in before there's loads of code that violates the constraint, so maybe there's merit in it in that regard - if it can be made low-false-positive enough.

(Warning on unreachable fallthrough is also recommended by the C++ standard: https://eel.is/c++draft/dcl.attr.fallthrough#2.sentence-2)

But maybe we should only diagnose these under -Wunreachable-code?

Yeah, maybe. I would expect that we don't warn under that because we don't model the fallthrough annotation as "code".

Yeah, figure it'd need some explicit support there - but it does feel like it should fall under that bucket. (I mean, arguably as a separate/sub-flag - since maybe we can make unreachable-fallthrough more reliable than the general unreachable-code? Though I'm not actually sure that's true...)

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 3 years ago

I guess I mean: what cases is this warning meant to catch that are buggy enough to be worth a warning?

"fallthrough" after "break" is probably the main one.

(Warning on unreachable fallthrough is also recommended by the C++ standard: https://eel.is/c++draft/dcl.attr.fallthrough#2.sentence-2)

But maybe we should only diagnose these under -Wunreachable-code?

Yeah, maybe. I would expect that we don't warn under that because we don't model the fallthrough annotation as "code".

dwblaikie commented 3 years ago

Are there cases we can keep, while avoiding the kernel example, realistically?

Yes, I think so: if we turned off all the checking for constant if conditions in the CFG builder, we would remove the false positives from the kernel code while retaining the rest of our unreachable fallthrough diagnostics. It might be better to apply that treatment only to constant conditions involving a macro expansion, but that would be hard to check for in some cases and I'm not sure it's worth it.

Constant conditions with a macro anywhere in them? Though could also have cases where the constant is defined differently based on a macro, but where the condition isn't itself a macro:

const bool Thing =

if ...

true

else

false

endif

;

etc... If we handle all if conditions as not being able to create unreachable code, what sort of cases are left? I guess fallthrough after a noreturn call?

I guess I mean: what cases is this warning meant to catch that are buggy enough to be worth a warning?

Looking at the current test coverage for the warning, cases like fallthrough after return - sure, looks like something worth diagnosing. But maybe we should only diagnose these under -Wunreachable-code? (if we don't already warn under that)

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 3 years ago

Are there cases we can keep, while avoiding the kernel example, realistically?

Yes, I think so: if we turned off all the checking for constant if conditions in the CFG builder, we would remove the false positives from the kernel code while retaining the rest of our unreachable fallthrough diagnostics. It might be better to apply that treatment only to constant conditions involving a macro expansion, but that would be hard to check for in some cases and I'm not sure it's worth it.

dwblaikie commented 3 years ago

There have already been work-arounds added for this warning. Here's the one for templates:

https://reviews.llvm.org/D31069

Can a similar test be added there for this bug's case?

Seems like this'd be a generalization of that fix - well, perhaps so general that there's no cases where we really want to warn about unreachable fallthrough anymore?

Are there cases we can keep, while avoiding the kernel example, realistically?

kees commented 3 years ago

There have already been work-arounds added for this warning. Here's the one for templates:

https://reviews.llvm.org/D31069

Can a similar test be added there for this bug's case?

llvmbot commented 3 years ago

Another example of an undesirable "unreachable code" warning in Linux:

That's the same pattern. Those if(IS_ENABLED()) macros are expanding to if (0) or if (1) by the preprocessor.

Yeah, I just wanted to mention another recently reported instance of this issue.

I wonder if an else could help here? ie.

if (IS_ENABLED(CONFIG_MIPS_EJTAG_FDC_TTY))

break;
fallthrough;

would become:

if (IS_ENABLED(CONFIG_MIPS_EJTAG_FDC_TTY))

break; else fallthrough;

mmh.. I don't think that's the right fix or a workaround that Linus would accept upstream.

nickdesaulniers commented 3 years ago

Another example of an undesirable "unreachable code" warning in Linux:

That's the same pattern. Those if(IS_ENABLED()) macros are expanding to if (0) or if (1) by the preprocessor.

I wonder if an else could help here? ie.

if (IS_ENABLED(CONFIG_MIPS_EJTAG_FDC_TTY))
break;
fallthrough;

would become:

if (IS_ENABLED(CONFIG_MIPS_EJTAG_FDC_TTY))
break; else fallthrough;

llvmbot commented 3 years ago

Another example of an undesirable "unreachable code" warning in Linux:

arch/mips/kernel/idle.c:

197 case CPU_P5600:
198 /
199 Incoming Fast Debug Channel (FDC) data during a wait
200 instruction causes the wait never to resume, even if an
201 interrupt is received. Avoid using wait at all if FDC data is
202 likely to be received.
203 /
204 if (IS_ENABLED(CONFIG_MIPS_EJTAG_FDC_TTY))
205 break;
206 fallthrough;
207 case CPU_M14KC:

arch/mips/kernel/idle.c:206:3: warning: fallthrough annotation in unreachable code [-Wimplicit-fallthrough]

AaronBallman commented 3 years ago

Reduced test case:

// clang -Wimplicit-fallthrough void bar(void);

void foo(int x) { switch (x) { case 0: if (1) { break; } attribute((fallthrough)); case 1: bar(); } }

$ gcc -Wimplicit-fallthrough -c foo.c $ clang-13 -Wimplicit-fallthrough -c foo.c foo.c:9:13: warning: fallthrough annotation in unreachable code [-Wimplicit-fallthrough] attribute((fallthrough)); ^

I would expect the diagnostic in this case. That fallthrough is in unreachable code. The original case was due to macros that sometimes elide the code, which is a bit different and would require additional handling.

dwblaikie commented 3 years ago

Clang's CFG does have some way to be optimistic/pessimistic (I think the improvements were made when an attempt was made to improve -Wunreachable-code to help with bugs like "goto fail"). It was mostly meant for things like "sizeof" (cases where code should not be unreachable just because "sizeof(int) > 4" - but in theory it should/could do something for macros too, but not sure how to generalize it to all the ways macros could be used to make conditional code look constant...

But yeah, separating the diagnostic into its own group and turning it off by default's probably good?

And/or check why the unreachable fallthrough diagnostic was added - in case there are cases that'd be helpful while skipping the unhelpful ones.

nickdesaulniers commented 3 years ago

Reduced test case:

// clang -Wimplicit-fallthrough void bar(void);

void foo(int x) { switch (x) { case 0: if (1) { break; } attribute((fallthrough)); case 1: bar(); } }

$ gcc -Wimplicit-fallthrough -c foo.c $ clang-13 -Wimplicit-fallthrough -c foo.c foo.c:9:13: warning: fallthrough annotation in unreachable code [-Wimplicit-fallthrough] attribute((fallthrough)); ^

llvmbot commented 3 years ago

Also, "the warning doesn't even say where the problem happens. No file name, no line numbers."[1]

Is there a reproduced example where clang doesn't show line/column information?

As I noted above, this is fixed in main with commit 1b4800c26259 ("[clang][parser] Set source ranges for GNU-style attributes") so it will be in the clang 13.0.0 release.

Awesome. :)

Thanks, Nathan.

llvmbot commented 3 years ago

% clang -fsyntax-only -Wimplicit-fallthrough a.c a.c:14:9: warning: result of comparison of constant 256 with expression of type 'unsigned char' is always true [-Wtautological-constant-out-of-range-compare] if (c < A(256)) ~ ^ ~~ a.c:21:5: warning: fallthrough annotation in unreachable code [-Wimplicit-fallthrough] attribute((fallthrough)); ^ 2 warnings generated.

If we want that -Wimplicit-fallthrough does not warn on if (B) where B is a macro evaluating to non-zero, there needs some changes.

In general terms, we want -Wimplicit-fallthrough not to warn about any unreachable code.

Thanks

Gustavo

nathanchance commented 3 years ago

Also, "the warning doesn't even say where the problem happens. No file name, no line numbers."[1]

Is there a reproduced example where clang doesn't show line/column information?

As I noted above, this is fixed in main with commit 1b4800c26259 ("[clang][parser] Set source ranges for GNU-style attributes") so it will be in the clang 13.0.0 release.

llvmbot commented 3 years ago

Also, "the warning doesn't even say where the problem happens. No file name, no line numbers."[1]

Is there a reproduced example where clang doesn't show line/column information?

Linus and I are building linux kernels with "make -j128" and the output looks like this:

CC drivers/media/platform/qcom/camss/camss-csid-4-7.o CC drivers/usb/gadget/function/f_uac1.o warning: fallthrough annotation in unreachable code [-Wimplicit-fallthrough] CC drivers/mfd/rave-sp.o CC drivers/mfd/rohm-bd70528.o

That "unreachable code" warning comes from kernel/sched/core.c:3137

3137 case cpuset: 3138 if (IS_ENABLED(CONFIG_CPUSETS)) { 3139 cpuset_cpus_allowed_fallback(p); 3140 state = possible; 3141 break; 3142 } 3143 fallthrough;

but the file and line number doesn't show up with the warning.

% cd clang/test % clang -fsyntax-only -Wimplicit-fallthrough SemaCXX/switch-implicit-fallthrough.cpp ... SemaCXX/switch-implicit-fallthrough.cpp:183:7: warning: fallthrough annotation in unreachable code [-Wimplicit-fallthrough]
  [[clang::fallthrough]];  // expected-warning{{fallthrough annotation
in unreachable code}}

include

ifdef C

define A(x) x-1

define B 0

else

define A(x) x+0

define B 1

endif

extern unsigned char c;

int main(int argc, char *argv[]) { if (c < A(256)) puts("meow");

switch (argc) { case 1: if (B) return 1; attribute((fallthrough)); case 2: puts("2"); break; } }

% clang -fsyntax-only -Wimplicit-fallthrough a.c a.c:14:9: warning: result of comparison of constant 256 with expression of type 'unsigned char' is always true [-Wtautological-constant-out-of-range-compare] if (c < A(256)) ~ ^ ~~ a.c:21:5: warning: fallthrough annotation in unreachable code [-Wimplicit-fallthrough] attribute((fallthrough)); ^ 2 warnings generated.

If we want that -Wimplicit-fallthrough does not warn on if (B) where B is a macro evaluating to non-zero, there needs some changes.

Yeah; that's what we want[1].

Thanks!

Gustavo

[1] https://lore.kernel.org/lkml/CAHk-=wg-qBVjhqoRiV0EdkFSpP1FebmRYwjiv-=GM3EVQYbBqg@mail.gmail.com/

MaskRay commented 3 years ago

Also, "the warning doesn't even say where the problem happens. No file name, no line numbers."[1]

Is there a reproduced example where clang doesn't show line/column information?

% cd clang/test % clang -fsyntax-only -Wimplicit-fallthrough SemaCXX/switch-implicit-fallthrough.cpp ... SemaCXX/switch-implicit-fallthrough.cpp:183:7: warning: fallthrough annotation in unreachable code [-Wimplicit-fallthrough]
[[clang::fallthrough]]; // expected-warning{{fallthrough annotation in unreachable code}}

include

ifdef C

define A(x) x-1

define B 0

else

define A(x) x+0

define B 1

endif

extern unsigned char c;

int main(int argc, char *argv[]) { if (c < A(256)) puts("meow");

switch (argc) { case 1: if (B) return 1; attribute((fallthrough)); case 2: puts("2"); break; } }

% clang -fsyntax-only -Wimplicit-fallthrough a.c a.c:14:9: warning: result of comparison of constant 256 with expression of type 'unsigned char' is always true [-Wtautological-constant-out-of-range-compare] if (c < A(256)) ~ ^ ~~ a.c:21:5: warning: fallthrough annotation in unreachable code [-Wimplicit-fallthrough] attribute((fallthrough)); ^ 2 warnings generated.

If we want that -Wimplicit-fallthrough does not warn on if (B) where B is a macro evaluating to non-zero, there needs some changes.

nathanchance commented 3 years ago

It is actually the fact that

case 1: if (something || !IS_ENABLED(CONFIG_SOMETHING)) return blah; fallthrough; case 2:

looks like

case 1: return blah; fallthrough; case 2:

For example: https://godbolt.org/z/GdPeMbdo8

int foo(int a) { switch (a) { case 0: if (0) return 0; attribute((fallthrough)); // no warning case 1: if (1) return 1; attribute((fallthrough)); // warning case 2: return 3; default: return 4; } }

I am not really sure how to resolve that within checkFallThroughIntoBlock() or fillReachableBlocks() but given that this is something specific to the kernel, we could introduce -Wimplicit-fallthrough-unreachable then disable it within the kernel.

The file location not showing up was fixed by commit 1b4800c26259 ("[clang][parser] Set source ranges for GNU-style attributes"). The differential revision mentions this issue specifically.

llvmbot commented 3 years ago

Also, "the warning doesn't even say where the problem happens. No file name, no line numbers."[1]

I think that should be fixed too: display the file and line number where the warning happened.[2]

Thanks

[1] https://lore.kernel.org/lkml/CAHk-=wjQeeUiv+P_4cZfCy-hY13yGqCGS-scKGhuJ-SAzz2doA@mail.gmail.com/ [2] https://lore.kernel.org/lkml/CAHk-=wj8GqP8ughGBbwcyrBNNdtcXVo_G=XjQ1MAUVUuJfUtGg@mail.gmail.com/

llvm / llvm-project

-Wimplicit-fallthrough needlessly warning about unreachable code #50438

Extended Description

if ...

else

endif

Thanks

include

ifdef C

define A(x) x-1

define B 0

else

define A(x) x+0

define B 1

endif

Thanks!

include

ifdef C

define A(x) x-1

define B 0

else

define A(x) x+0

define B 1

endif

Thanks