* [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 @ 2021-07-14 20:05 Gustavo A. R. Silva 2021-07-15 21:15 ` pr-tracker-bot 2021-07-16 1:04 ` Linus Torvalds 0 siblings, 2 replies; 11+ messages in thread From: Gustavo A. R. Silva @ 2021-07-14 20:05 UTC (permalink / raw) To: Linus Torvalds; +Cc: Kees Cook, linux-kernel, 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(-) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 2021-07-14 20:05 [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 Gustavo A. R. Silva @ 2021-07-15 21:15 ` pr-tracker-bot 2021-07-16 1:04 ` Linus Torvalds 1 sibling, 0 replies; 11+ messages in thread From: pr-tracker-bot @ 2021-07-15 21:15 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Linus Torvalds, Kees Cook, linux-kernel, Gustavo A. R. Silva 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 2021-07-14 20:05 [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 Gustavo A. R. Silva 2021-07-15 21:15 ` pr-tracker-bot @ 2021-07-16 1:04 ` Linus Torvalds 2021-07-16 1:16 ` Gustavo A. R. Silva 2021-07-16 18:47 ` Nathan Chancellor 1 sibling, 2 replies; 11+ messages in thread From: Linus Torvalds @ 2021-07-16 1:04 UTC (permalink / raw) To: Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers Cc: Kees Cook, Linux Kernel Mailing List, clang-built-linux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 2021-07-16 1:04 ` Linus Torvalds @ 2021-07-16 1:16 ` Gustavo A. R. Silva 2021-07-16 1:22 ` Linus Torvalds 2021-07-16 18:47 ` Nathan Chancellor 1 sibling, 1 reply; 11+ messages in thread From: Gustavo A. R. Silva @ 2021-07-16 1:16 UTC (permalink / raw) To: Linus Torvalds, Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers Cc: Kees Cook, Linux Kernel Mailing List, clang-built-linux 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 2021-07-16 1:16 ` Gustavo A. R. Silva @ 2021-07-16 1:22 ` Linus Torvalds 2021-07-16 1:29 ` Gustavo A. R. Silva 0 siblings, 1 reply; 11+ messages in thread From: Linus Torvalds @ 2021-07-16 1:22 UTC (permalink / raw) To: Gustavo A. R. Silva Cc: Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Kees Cook, Linux Kernel Mailing List, clang-built-linux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 2021-07-16 1:22 ` Linus Torvalds @ 2021-07-16 1:29 ` Gustavo A. R. Silva 0 siblings, 0 replies; 11+ messages in thread From: Gustavo A. R. Silva @ 2021-07-16 1:29 UTC (permalink / raw) To: Linus Torvalds Cc: Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Kees Cook, Linux Kernel Mailing List, clang-built-linux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 2021-07-16 1:04 ` Linus Torvalds 2021-07-16 1:16 ` Gustavo A. R. Silva @ 2021-07-16 18:47 ` Nathan Chancellor 2021-07-16 18:57 ` Gustavo A. R. Silva 2021-07-16 19:22 ` Linus Torvalds 1 sibling, 2 replies; 11+ messages in thread From: Nathan Chancellor @ 2021-07-16 18:47 UTC (permalink / raw) To: Linus Torvalds Cc: Gustavo A. R. Silva, Nick Desaulniers, Kees Cook, Linux Kernel Mailing List, clang-built-linux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 2021-07-16 18:47 ` Nathan Chancellor @ 2021-07-16 18:57 ` Gustavo A. R. Silva 2021-07-16 19:18 ` Nathan Chancellor 2021-07-16 19:22 ` Linus Torvalds 1 sibling, 1 reply; 11+ messages in thread From: Gustavo A. R. Silva @ 2021-07-16 18:57 UTC (permalink / raw) To: Nathan Chancellor, Linus Torvalds Cc: Gustavo A. R. Silva, Nick Desaulniers, Kees Cook, Linux Kernel Mailing List, clang-built-linux 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 2021-07-16 18:57 ` Gustavo A. R. Silva @ 2021-07-16 19:18 ` Nathan Chancellor 2021-07-16 19:26 ` Linus Torvalds 0 siblings, 1 reply; 11+ messages in thread From: Nathan Chancellor @ 2021-07-16 19:18 UTC (permalink / raw) To: Gustavo A. R. Silva, Linus Torvalds Cc: Gustavo A. R. Silva, Nick Desaulniers, Kees Cook, Linux Kernel Mailing List, clang-built-linux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 2021-07-16 19:18 ` Nathan Chancellor @ 2021-07-16 19:26 ` Linus Torvalds 0 siblings, 0 replies; 11+ messages in thread From: Linus Torvalds @ 2021-07-16 19:26 UTC (permalink / raw) To: Nathan Chancellor Cc: Gustavo A. R. Silva, Gustavo A. R. Silva, Nick Desaulniers, Kees Cook, Linux Kernel Mailing List, clang-built-linux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 2021-07-16 18:47 ` Nathan Chancellor 2021-07-16 18:57 ` Gustavo A. R. Silva @ 2021-07-16 19:22 ` Linus Torvalds 1 sibling, 0 replies; 11+ messages in thread From: Linus Torvalds @ 2021-07-16 19:22 UTC (permalink / raw) To: Nathan Chancellor Cc: Gustavo A. R. Silva, Nick Desaulniers, Kees Cook, Linux Kernel Mailing List, clang-built-linux 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-07-16 19:26 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-14 20:05 [GIT PULL] fallthrough fixes for Clang for 5.14-rc2 Gustavo A. R. Silva 2021-07-15 21:15 ` pr-tracker-bot 2021-07-16 1:04 ` Linus Torvalds 2021-07-16 1:16 ` Gustavo A. R. Silva 2021-07-16 1:22 ` Linus Torvalds 2021-07-16 1:29 ` Gustavo A. R. Silva 2021-07-16 18:47 ` Nathan Chancellor 2021-07-16 18:57 ` Gustavo A. R. Silva 2021-07-16 19:18 ` Nathan Chancellor 2021-07-16 19:26 ` Linus Torvalds 2021-07-16 19:22 ` Linus Torvalds
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.