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

[Nathan Chancellor <nathan@kernel.org>]

On 7/16/2021 11:57 AM, Gustavo A. R. Silva wrote
> 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.

Correct. It does not seem like GCC warns at all about the use of 
fallthrough attributes at all, for example, against the same clang test 
cases: https://godbolt.org/z/4MvW1TnYa

This could be a conscious decision by the clang developers to deviate 
from GCC, the only way we will know is from the bug report above. I can 
recall this happening once before where it impacted the kernel and the 
clang developers allowed me to add another flag that was default enabled 
but could be disabled separately from the warning to get GCC 
compatibility without sacrificing the additional warning coverage they 
felt was worth deviating from GCC for:

https://github.com/ClangBuiltLinux/linux/issues/887
https://reviews.llvm.org/D72231
https://reviews.llvm.org/D75758

Hence why I suggested -Wimplicit-fallthrough-unreachable.

>>      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