Messages in this thread | | | Subject | Re: [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 | From | "Gustavo A. R. Silva" <> | Date | Fri, 16 Jul 2021 13:57:21 -0500 |
| |
On 7/16/21 13:47, Nathan Chancellor wrote: > On Thu, Jul 15, 2021 at 06:04:15PM -0700, Linus Torvalds wrote: >> On Wed, Jul 14, 2021 at 1:03 PM Gustavo A. R. Silva >> <gustavoars@kernel.org> wrote: >>> >>> git://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git tags/Wimplicit-fallthrough-clang-5.14-rc2 >> >> Grr. >> >> I merged this, but when I actually tested it on my clang build, it >> turns out that the clang "-Wimplicit-fallthrough" flag is unbelievable >> garbage. >> >> I get >> >> warning: fallthrough annotation in unreachable code [-Wimplicit-fallthrough] >> >> and the stupid warning doesn't even say WHERE THE PROBLEM HAPPENS. >> >> No file name, no line numbers. Just this pointless garbage warning. >> >> Honestly, how does a compiler even do something that broken? Am I >> supposed to use my sixth sense to guide me in finding the warning? >> >> I like the concept of the fallthrough warning, but it looks like the >> clang implementation of it is so unbelievably broken that it's getting >> disabled again. >> >> Yeah, I can >> >> (a) build the kernel without any parallelism >> >> (b) use ">&" to get both output and errors into the same file >> >> (c) see that it says >> >> CC kernel/sched/core.o >> warning: fallthrough annotation in unreachable code [-Wimplicit-fallthrough] >> 1 warning generated. >> >> and now I see at least which _file_ it is that causes that warning. >> >> I can then use my incredible powers of deduction (it's almost like a >> sixth sense, but helped by the fact that there's only one single >> "fallthrough" statement in that file) to figure out that it's >> triggered by this code: >> >> case cpuset: >> if (IS_ENABLED(CONFIG_CPUSETS)) { >> cpuset_cpus_allowed_fallback(p); >> state = possible; >> break; >> } >> fallthrough; >> case possible: >> >> and it all makes it clear that the clang warning is just incredibly >> broken garbage not only in that lack of filename and line number, but >> just in general. > > I commented this on the LLVM bug tracker but I will copy and paste it > here for posterity: > > "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
I think that if the "1" in this case, depends on the initial configuration, as it is the case with CONFIG_CPUSETS, then Clang should not cause a warning either. That's how GCC seems to be treating these scenarios.
-- Gustavo
> 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." > > Hopefully that would be an adequate solution, otherwise someone with more clang > internal will have to take a look. > > Cheers, > Nathan >
| |