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

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

[Gustavo A. R. Silva]

The following changes since commit e73f0f0ee7541171d89f2e2491130c7771ba58d3:

  Linux 5.14-rc1 (2021-07-11 15:07:40 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git tags/Wimplicit-fallthrough-clang-5.14-rc2

for you to fetch changes up to b7eb335e26a9c7f258c96b3962c283c379d3ede0:

  Makefile: Enable -Wimplicit-fallthrough for Clang (2021-07-14 11:12:21 -0500)

----------------------------------------------------------------
fallthrough fixes for Clang for 5.14-rc2

Hi Linus,

Please, pull the following patches that fix many fall-through
warnings when building with Clang and -Wimplicit-fallthrough.

This pull-request also contains the patch for Makefile that enables
-Wimplicit-fallthrough for Clang, globally.

It's also important to notice that since we have adopted the use of
the pseudo-keyword macro fallthrough; we also want to avoid having
more /* fall through */ comments being introduced. Notice that contrary
to GCC, Clang doesn't recognize any comments as implicit fall-through
markings when the -Wimplicit-fallthrough option is enabled. So, in
order to avoid having more comments being introduced, we have to use
the option -Wimplicit-fallthrough=5 for GCC, which similar to Clang,
will cause a warning in case a code comment is intended to be used
as a fall-through marking. The patch for Makefile also enforces this.

We had almost 4,000 of these issues for Clang in the beginning,
and there might be a couple more out there when building some
architectures with certain configurations. However, with the
recent fixes I think we are in good shape and it is now possible
to enable -Wimplicit-fallthrough for Clang. :)

Thanks!

----------------------------------------------------------------
Gustavo A. R. Silva (27):
      xfs: Fix multiple fall-through warnings for Clang
      mt76: mt7921: Fix fall-through warning for Clang
      nfp: flower-ct: Fix fall-through warning for Clang
      drm/i915: Fix fall-through warning for Clang
      kernel: debug: Fix unreachable code in gdb_serial_stub()
      fcntl: Fix unreachable code in do_fcntl()
      mtd: cfi_util: Fix unreachable code issue
      drm/msm: Fix fall-through warning in msm_gem_new_impl()
      cpufreq: Fix fall-through warning for Clang
      math-emu: Fix fall-through warning
      video: fbdev: Fix fall-through warning for Clang
      scsi: libsas: Fix fall-through warning for Clang
      PCI: Fix fall-through warning for Clang
      mmc: jz4740: Fix fall-through warning for Clang
      iommu/arm-smmu-v3: Fix fall-through warning for Clang
      dmaengine: ipu: Fix fall-through warning for Clang
      s390: Fix fall-through warnings for Clang
      dmaengine: ti: k3-udma: Fix fall-through warning for Clang
      power: supply: Fix fall-through warnings for Clang
      ASoC: Mediatek: MT8183: Fix fall-through warning for Clang
      MIPS: Fix fall-through warnings for Clang
      MIPS: Fix unreachable code issue
      powerpc/powernv: Fix fall-through warning for Clang
      usb: gadget: fsl_qe_udc: Fix fall-through warning for Clang
      dmaengine: mpc512x: Fix fall-through warning for Clang
      powerpc/smp: Fix fall-through warning for Clang
      Makefile: Enable -Wimplicit-fallthrough for Clang

 Makefile                                              |  9 +++------
 arch/mips/include/asm/fpu.h                           |  2 +-
 arch/mips/mm/tlbex.c                                  |  2 ++
 arch/powerpc/platforms/powermac/smp.c                 |  1 +
 arch/s390/kernel/uprobes.c                            |  1 +
 drivers/char/powernv-op-panel.c                       |  1 +
 drivers/cpufreq/longhaul.c                            |  2 --
 drivers/dma/ipu/ipu_idmac.c                           |  2 ++
 drivers/dma/mpc512x_dma.c                             |  1 +
 drivers/dma/ti/k3-udma.c                              |  1 +
 drivers/gpu/drm/i915/gem/i915_gem_shrinker.c          |  1 +
 drivers/gpu/drm/msm/msm_gem.c                         |  2 +-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c           |  1 +
 drivers/mmc/host/jz4740_mmc.c                         |  2 ++
 drivers/mtd/chips/cfi_util.c                          |  4 ++--
 drivers/net/ethernet/netronome/nfp/flower/conntrack.c |  1 +
 drivers/net/wireless/mediatek/mt76/mt7921/main.c      |  1 +
 drivers/pci/proc.c                                    |  2 +-
 drivers/power/supply/ab8500_fg.c                      |  2 ++
 drivers/power/supply/abx500_chargalg.c                |  1 +
 drivers/s390/char/tape_char.c                         |  2 --
 drivers/s390/net/ctcm_fsms.c                          |  1 +
 drivers/s390/net/qeth_l3_main.c                       |  1 +
 drivers/scsi/libsas/sas_discover.c                    |  2 +-
 drivers/usb/gadget/udc/fsl_qe_udc.c                   |  1 +
 drivers/video/fbdev/xilinxfb.c                        |  2 ++
 fs/fcntl.c                                            |  2 +-
 fs/xfs/libxfs/xfs_attr.c                              | 16 ++++++++--------
 include/math-emu/op-common.h                          |  2 +-
 kernel/debug/gdbstub.c                                |  2 +-
 sound/soc/mediatek/mt8183/mt8183-dai-adda.c           |  1 +
 31 files changed, 44 insertions(+), 27 deletions(-)

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

[pr-tracker-bot]

The pull request you sent on Wed, 14 Jul 2021 15:05:23 -0500:

> git://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git tags/Wimplicit-fallthrough-clang-5.14-rc2

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/e9338abf0e186336022293d2e454c106761f262b

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

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

[Linus Torvalds]

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.

Yeah, I'm a bit pissed off at this. This clang warning really is
WRONG. It's so wrong in so many ways that I don't know what to say.

Except "yeah, that broken option is getting reverted again, because
the clang people messed up completely".

It's sad to see that people wasted time and effort on trying to make
clang happy, when it turns out that clang just gets this so _totally_
wrong.

                     Linus

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

[Gustavo A. R. Silva]

On 7/15/21 20:04, 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]

Kees just opened a bug report for this:

https://bugs.llvm.org/show_bug.cgi?id=51094

--
Gustavo

> 
> 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.
> 
> Yeah, I'm a bit pissed off at this. This clang warning really is
> WRONG. It's so wrong in so many ways that I don't know what to say.
> 
> Except "yeah, that broken option is getting reverted again, because
> the clang people messed up completely".
> 
> It's sad to see that people wasted time and effort on trying to make
> clang happy, when it turns out that clang just gets this so _totally_
> wrong.
> 
>                      Linus
> 

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

[Linus Torvalds]

On Thu, Jul 15, 2021 at 6:14 PM Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:
>
> Kees just opened a bug report for this:
>
> https://bugs.llvm.org/show_bug.cgi?id=51094

I don't have an account on that bugzilla, but it might be worth adding
the note that no warning or error should EVER not say where it
happens.

That's the thing that made me pissed off in the first place. I build
my kernels with "make -j128", and if the warning doesn't specify the
filename and the line number, the warning is just unacceptably bad.

How can a compiler _ever_ give a warning without specifying where it is?

The fact that the warning is also entirely wrong-headed in the first
place is just the extra cherry on top.

But at least it should hopefully make it easy to fix in clang - just
remove the incredibly broken thing entirely.

             Linus

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

[Gustavo A. R. Silva]

On 7/15/21 20:22, Linus Torvalds wrote:
> On Thu, Jul 15, 2021 at 6:14 PM Gustavo A. R. Silva
> <gustavo@embeddedor.com> wrote:
>>
>> Kees just opened a bug report for this:
>>
>> https://bugs.llvm.org/show_bug.cgi?id=51094
> 
> I don't have an account on that bugzilla, but it might be worth adding
> the note that no warning or error should EVER not say where it
> happens.

Yeah; I'll add that to the report.

Here is the current description of the bug:

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

> That's the thing that made me pissed off in the first place. I build
> my kernels with "make -j128", and if the warning doesn't specify the
> filename and the line number, the warning is just unacceptably bad.
> 
> How can a compiler _ever_ give a warning without specifying where it is?
> 
> The fact that the warning is also entirely wrong-headed in the first
> place is just the extra cherry on top.
> 
> But at least it should hopefully make it easy to fix in clang - just
> remove the incredibly broken thing entirely.
> 
>              Linus
> 

--
Gustavo

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

[Nathan Chancellor]

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

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

[Gustavo A. R. Silva]

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
> 

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

[Nathan Chancellor]

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

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

[Linus Torvalds]

On Fri, Jul 16, 2021 at 12:18 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> Hence why I suggested -Wimplicit-fallthrough-unreachable.

As long as it's a warning that the kernel would never set, that's fine.

I think it's an entirely bogus warning, but at some point as long as
we don't need to care about it, we can happily ignore it.

Or just continue to say "clang is spewing bogus warnings, don't use it".

But the sane naming for that warning should certainly not have
anything at all to do with "implicit". Quite the reverse. The warning
is about an  _explicit_ fallthrough being unreachable, and as such
thje warning name should reflect that.

So make it just "-Wfallthrough-unreachable" (maybe even
"-Wexplicit-..") to allow people who want that pointless warning to
enable it.

               Linus

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

[Linus Torvalds]

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