Hi all,
This series aims to fix almost all remaining fall-through warnings in
order to enable -Wimplicit-fallthrough for Clang.
In preparation to enable -Wimplicit-fallthrough for Clang, explicitly
add multiple break/goto/return/fallthrough statements instead of just
letting the code fall through to the next case.
Notice that in order to enable -Wimplicit-fallthrough for Clang, this
change[1] is meant to be reverted at some point. So, this patch helps
to move in that direction.
Something important to mention is that there is currently a discrepancy
between GCC and Clang when dealing with switch fall-through to empty case
statements or to cases that only contain a break/continue/return
statement[2][3][4].
Now that the -Wimplicit-fallthrough option has been globally enabled[5],
any compiler should really warn on missing either a fallthrough annotation
or any of the other case-terminating statements (break/continue/return/
goto) when falling through to the next case statement. Making exceptions
to this introduces variation in case handling which may continue to lead
to bugs, misunderstandings, and a general lack of robustness. The point
of enabling options like -Wimplicit-fallthrough is to prevent human error
and aid developers in spotting bugs before their code is even built/
submitted/committed, therefore eliminating classes of bugs. So, in order
to really accomplish this, we should, and can, move in the direction of
addressing any error-prone scenarios and get rid of the unintentional
fallthrough bug-class in the kernel, entirely, even if there is some minor
redundancy. Better to have explicit case-ending statements than continue to
have exceptions where one must guess as to the right result. The compiler
will eliminate any actual redundancy.
Note that there is already a patch in mainline that addresses almost
40,000 of these issues[6].
I'm happy to carry this whole series in my own tree if people are OK
with it. :)
[1] commit e2079e93f562c ("kbuild: Do not enable -Wimplicit-fallthrough for clang for now")
[2] ClangBuiltLinux#636
[3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91432
[4] https://godbolt.org/z/xgkvIh
[5] commit a035d552a93b ("Makefile: Globally enable fall-through warning")
[6] commit 4169e889e588 ("include: jhash/signal: Fix fall-through warnings for Clang")
Thanks!
Gustavo A. R. Silva (141):
afs: Fix fall-through warnings for Clang
ASoC: codecs: Fix fall-through warnings for Clang
cifs: Fix fall-through warnings for Clang
drm/amdgpu: Fix fall-through warnings for Clang
drm/radeon: Fix fall-through warnings for Clang
gfs2: Fix fall-through warnings for Clang
gpio: Fix fall-through warnings for Clang
IB/hfi1: Fix fall-through warnings for Clang
igb: Fix fall-through warnings for Clang
ima: Fix fall-through warnings for Clang
ipv4: Fix fall-through warnings for Clang
ixgbe: Fix fall-through warnings for Clang
media: dvb-frontends: Fix fall-through warnings for Clang
media: usb: dvb-usb-v2: Fix fall-through warnings for Clang
netfilter: Fix fall-through warnings for Clang
nfsd: Fix fall-through warnings for Clang
nfs: Fix fall-through warnings for Clang
qed: Fix fall-through warnings for Clang
qlcnic: Fix fall-through warnings for Clang
scsi: aic7xxx: Fix fall-through warnings for Clang
scsi: aic94xx: Fix fall-through warnings for Clang
scsi: bfa: Fix fall-through warnings for Clang
staging: rtl8723bs: core: Fix fall-through warnings for Clang
staging: vt6655: Fix fall-through warnings for Clang
bnxt_en: Fix fall-through warnings for Clang
ceph: Fix fall-through warnings for Clang
drbd: Fix fall-through warnings for Clang
drm/amd/display: Fix fall-through warnings for Clang
e1000: Fix fall-through warnings for Clang
ext2: Fix fall-through warnings for Clang
ext4: Fix fall-through warnings for Clang
floppy: Fix fall-through warnings for Clang
fm10k: Fix fall-through warnings for Clang
IB/mlx4: Fix fall-through warnings for Clang
IB/qedr: Fix fall-through warnings for Clang
ice: Fix fall-through warnings for Clang
Input: pcspkr - Fix fall-through warnings for Clang
isofs: Fix fall-through warnings for Clang
ixgbevf: Fix fall-through warnings for Clang
kprobes/x86: Fix fall-through warnings for Clang
mm: Fix fall-through warnings for Clang
net: 3c509: Fix fall-through warnings for Clang
net: cassini: Fix fall-through warnings for Clang
net/mlx4: Fix fall-through warnings for Clang
net: mscc: ocelot: Fix fall-through warnings for Clang
netxen_nic: Fix fall-through warnings for Clang
nfp: Fix fall-through warnings for Clang
perf/x86: Fix fall-through warnings for Clang
pinctrl: Fix fall-through warnings for Clang
RDMA/mlx5: Fix fall-through warnings for Clang
reiserfs: Fix fall-through warnings for Clang
security: keys: Fix fall-through warnings for Clang
selinux: Fix fall-through warnings for Clang
target: Fix fall-through warnings for Clang
uprobes/x86: Fix fall-through warnings for Clang
vxge: Fix fall-through warnings for Clang
watchdog: Fix fall-through warnings for Clang
xen-blkfront: Fix fall-through warnings for Clang
regulator: as3722: Fix fall-through warnings for Clang
habanalabs: Fix fall-through warnings for Clang
tee: Fix fall-through warnings for Clang
HID: usbhid: Fix fall-through warnings for Clang
HID: input: Fix fall-through warnings for Clang
ACPI: Fix fall-through warnings for Clang
airo: Fix fall-through warnings for Clang
ALSA: hdspm: Fix fall-through warnings for Clang
ALSA: pcsp: Fix fall-through warnings for Clang
ALSA: sb: Fix fall-through warnings for Clang
ath5k: Fix fall-through warnings for Clang
atm: fore200e: Fix fall-through warnings for Clang
braille_console: Fix fall-through warnings for Clang
can: peak_usb: Fix fall-through warnings for Clang
carl9170: Fix fall-through warnings for Clang
cfg80211: Fix fall-through warnings for Clang
crypto: ccree - Fix fall-through warnings for Clang
decnet: Fix fall-through warnings for Clang
dm raid: Fix fall-through warnings for Clang
drm/amd/pm: Fix fall-through warnings for Clang
drm: Fix fall-through warnings for Clang
drm/i915/gem: Fix fall-through warnings for Clang
drm/nouveau/clk: Fix fall-through warnings for Clang
drm/nouveau: Fix fall-through warnings for Clang
drm/nouveau/therm: Fix fall-through warnings for Clang
drm/via: Fix fall-through warnings for Clang
firewire: core: Fix fall-through warnings for Clang
hwmon: (corsair-cpro) Fix fall-through warnings for Clang
hwmon: (max6621) Fix fall-through warnings for Clang
i3c: master: cdns: Fix fall-through warnings for Clang
ide: Fix fall-through warnings for Clang
iio: adc: cpcap: Fix fall-through warnings for Clang
iwlwifi: iwl-drv: Fix fall-through warnings for Clang
libata: Fix fall-through warnings for Clang
mac80211: Fix fall-through warnings for Clang
media: atomisp: Fix fall-through warnings for Clang
media: dvb_frontend: Fix fall-through warnings for Clang
media: rcar_jpu: Fix fall-through warnings for Clang
media: saa7134: Fix fall-through warnings for Clang
mmc: sdhci-of-arasan: Fix fall-through warnings for Clang
mt76: mt7615: Fix fall-through warnings for Clang
mtd: cfi: Fix fall-through warnings for Clang
mtd: mtdchar: Fix fall-through warnings for Clang
mtd: onenand: Fix fall-through warnings for Clang
mtd: rawnand: fsmc: Fix fall-through warnings for Clang
mtd: rawnand: stm32_fmc2: Fix fall-through warnings for Clang
net: ax25: Fix fall-through warnings for Clang
net: bridge: Fix fall-through warnings for Clang
net: core: Fix fall-through warnings for Clang
netfilter: ipt_REJECT: Fix fall-through warnings for Clang
net: netrom: Fix fall-through warnings for Clang
net/packet: Fix fall-through warnings for Clang
net: plip: Fix fall-through warnings for Clang
net: rose: Fix fall-through warnings for Clang
nl80211: Fix fall-through warnings for Clang
phy: qcom-usb-hs: Fix fall-through warnings for Clang
rds: Fix fall-through warnings for Clang
rt2x00: Fix fall-through warnings for Clang
rtl8xxxu: Fix fall-through warnings for Clang
rtw88: Fix fall-through warnings for Clang
rxrpc: Fix fall-through warnings for Clang
scsi: aacraid: Fix fall-through warnings for Clang
scsi: aha1740: Fix fall-through warnings for Clang
scsi: csiostor: Fix fall-through warnings for Clang
scsi: lpfc: Fix fall-through warnings for Clang
scsi: stex: Fix fall-through warnings for Clang
sctp: Fix fall-through warnings for Clang
slimbus: messaging: Fix fall-through warnings for Clang
staging: qlge: Fix fall-through warnings for Clang
staging: vt6656: Fix fall-through warnings for Clang
SUNRPC: Fix fall-through warnings for Clang
tipc: Fix fall-through warnings for Clang
tpm: Fix fall-through warnings for Clang
ubi: Fix fall-through warnings for Clang
usb: Fix fall-through warnings for Clang
video: fbdev: lxfb_ops: Fix fall-through warnings for Clang
video: fbdev: pm2fb: Fix fall-through warnings for Clang
virtio_net: Fix fall-through warnings for Clang
wcn36xx: Fix fall-through warnings for Clang
xen/manage: Fix fall-through warnings for Clang
xfrm: Fix fall-through warnings for Clang
zd1201: Fix fall-through warnings for Clang
Input: libps2 - Fix fall-through warnings for Clang
arch/x86/events/core.c | 2 +-
arch/x86/kernel/kprobes/core.c | 1 +
arch/x86/kernel/uprobes.c | 2 ++
drivers/accessibility/braille/braille_console.c | 1 +
drivers/acpi/sbshc.c | 1 +
drivers/ata/libata-eh.c | 1 +
drivers/atm/fore200e.c | 1 +
drivers/block/drbd/drbd_receiver.c | 1 +
drivers/block/drbd/drbd_req.c | 1 +
drivers/block/floppy.c | 1 +
drivers/block/xen-blkfront.c | 1 +
drivers/char/tpm/eventlog/tpm1.c | 1 +
drivers/crypto/ccree/cc_cipher.c | 3 +++
drivers/firewire/core-topology.c | 1 +
drivers/gpio/gpio-ath79.c | 1 +
drivers/gpio/gpiolib-acpi.c | 1 +
drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 1 +
drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 1 +
drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 1 +
drivers/gpu/drm/amd/amdgpu/vi.c | 1 +
drivers/gpu/drm/amd/display/dc/bios/bios_parser.c | 1 +
drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c | 2 ++
drivers/gpu/drm/amd/display/dc/core/dc_link.c | 1 +
drivers/gpu/drm/amd/pm/powerplay/si_dpm.c | 2 +-
.../gpu/drm/amd/pm/powerplay/smumgr/polaris10_smumgr.c | 1 +
drivers/gpu/drm/drm_bufs.c | 1 +
drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 1 +
drivers/gpu/drm/nouveau/nouveau_bo.c | 1 +
drivers/gpu/drm/nouveau/nouveau_connector.c | 1 +
drivers/gpu/drm/nouveau/nvkm/subdev/clk/nv50.c | 1 +
drivers/gpu/drm/nouveau/nvkm/subdev/therm/gf119.c | 1 +
drivers/gpu/drm/radeon/ci_dpm.c | 2 +-
drivers/gpu/drm/radeon/r300.c | 1 +
drivers/gpu/drm/radeon/si_dpm.c | 2 +-
drivers/gpu/drm/via/via_irq.c | 1 +
drivers/hid/hid-input.c | 1 +
drivers/hid/usbhid/hid-core.c | 2 ++
drivers/hwmon/corsair-cpro.c | 1 +
drivers/hwmon/max6621.c | 2 +-
drivers/i3c/master/i3c-master-cdns.c | 2 ++
drivers/ide/siimage.c | 1 +
drivers/iio/adc/cpcap-adc.c | 1 +
drivers/infiniband/hw/hfi1/qp.c | 1 +
drivers/infiniband/hw/hfi1/tid_rdma.c | 5 +++++
drivers/infiniband/hw/mlx4/mad.c | 1 +
drivers/infiniband/hw/mlx5/qp.c | 1 +
drivers/infiniband/hw/qedr/main.c | 1 +
drivers/input/misc/pcspkr.c | 1 +
drivers/input/serio/libps2.c | 2 +-
drivers/md/dm-raid.c | 1 +
drivers/media/dvb-core/dvb_frontend.c | 1 +
drivers/media/dvb-frontends/cx24120.c | 1 +
drivers/media/dvb-frontends/dib0090.c | 2 ++
drivers/media/dvb-frontends/drxk_hard.c | 1 +
drivers/media/dvb-frontends/m88rs2000.c | 1 +
drivers/media/pci/saa7134/saa7134-tvaudio.c | 1 +
drivers/media/platform/rcar_jpu.c | 1 +
drivers/media/usb/dvb-usb-v2/af9015.c | 1 +
drivers/media/usb/dvb-usb-v2/lmedm04.c | 1 +
drivers/misc/habanalabs/gaudi/gaudi.c | 1 +
drivers/mmc/host/sdhci-of-arasan.c | 4 ++++
drivers/mtd/chips/cfi_cmdset_0001.c | 1 +
drivers/mtd/chips/cfi_cmdset_0002.c | 2 ++
drivers/mtd/chips/cfi_cmdset_0020.c | 2 ++
drivers/mtd/mtdchar.c | 1 +
drivers/mtd/nand/onenand/onenand_samsung.c | 1 +
drivers/mtd/nand/raw/fsmc_nand.c | 1 +
drivers/mtd/nand/raw/stm32_fmc2_nand.c | 2 ++
drivers/mtd/ubi/build.c | 1 +
drivers/net/can/usb/peak_usb/pcan_usb_core.c | 2 ++
drivers/net/ethernet/3com/3c509.c | 1 +
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 1 +
drivers/net/ethernet/intel/e1000/e1000_hw.c | 1 +
drivers/net/ethernet/intel/fm10k/fm10k_mbx.c | 2 ++
drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 1 +
drivers/net/ethernet/intel/igb/e1000_phy.c | 1 +
drivers/net/ethernet/intel/igb/igb_ethtool.c | 1 +
drivers/net/ethernet/intel/igb/igb_ptp.c | 1 +
drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c | 2 ++
drivers/net/ethernet/intel/ixgbe/ixgbe_common.c | 1 +
drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c | 1 +
drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 1 +
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 1 +
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c | 1 +
drivers/net/ethernet/mscc/ocelot_vcap.c | 1 +
drivers/net/ethernet/neterion/vxge/vxge-config.c | 1 +
drivers/net/ethernet/netronome/nfp/nfp_net_repr.c | 1 +
drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c | 1 +
drivers/net/ethernet/qlogic/qed/qed_l2.c | 1 +
drivers/net/ethernet/qlogic/qed/qed_sriov.c | 1 +
drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c | 1 +
drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 1 +
drivers/net/ethernet/sun/cassini.c | 1 +
drivers/net/plip/plip.c | 2 ++
drivers/net/virtio_net.c | 1 +
drivers/net/wireless/ath/ath5k/mac80211-ops.c | 1 +
drivers/net/wireless/ath/carl9170/tx.c | 1 +
drivers/net/wireless/ath/wcn36xx/smd.c | 2 +-
drivers/net/wireless/cisco/airo.c | 1 +
drivers/net/wireless/intel/iwlwifi/iwl-drv.c | 2 +-
drivers/net/wireless/mediatek/mt76/mt7615/eeprom.c | 2 +-
drivers/net/wireless/ralink/rt2x00/rt2x00queue.c | 1 +
drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 8 ++++----
drivers/net/wireless/realtek/rtw88/fw.c | 2 +-
drivers/net/wireless/zydas/zd1201.c | 2 +-
drivers/phy/qualcomm/phy-qcom-usb-hs.c | 1 +
drivers/pinctrl/renesas/pinctrl-rza1.c | 1 +
drivers/regulator/as3722-regulator.c | 3 ++-
drivers/scsi/aacraid/commsup.c | 1 +
drivers/scsi/aha1740.c | 1 +
drivers/scsi/aic7xxx/aic79xx_core.c | 4 +++-
drivers/scsi/aic7xxx/aic7xxx_core.c | 4 ++--
drivers/scsi/aic94xx/aic94xx_scb.c | 2 ++
drivers/scsi/aic94xx/aic94xx_task.c | 2 ++
drivers/scsi/bfa/bfa_fcs_lport.c | 2 +-
drivers/scsi/bfa/bfa_ioc.c | 6 ++++--
drivers/scsi/csiostor/csio_wr.c | 1 +
drivers/scsi/lpfc/lpfc_bsg.c | 1 +
drivers/scsi/stex.c | 1 +
drivers/slimbus/messaging.c | 1 +
drivers/staging/media/atomisp/pci/runtime/isys/src/rx.c | 1 +
drivers/staging/qlge/qlge_main.c | 1 +
drivers/staging/rtl8723bs/core/rtw_cmd.c | 1 +
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 1 +
drivers/staging/rtl8723bs/core/rtw_wlan_util.c | 1 +
drivers/staging/vt6655/device_main.c | 1 +
drivers/staging/vt6655/rxtx.c | 2 ++
drivers/staging/vt6656/main_usb.c | 1 +
drivers/target/target_core_iblock.c | 1 +
drivers/target/target_core_pr.c | 1 +
drivers/tee/tee_core.c | 1 +
drivers/usb/gadget/function/f_fs.c | 2 ++
drivers/usb/gadget/function/f_loopback.c | 2 +-
drivers/usb/gadget/function/f_sourcesink.c | 1 +
drivers/usb/gadget/udc/dummy_hcd.c | 2 ++
drivers/usb/host/fotg210-hcd.c | 2 +-
drivers/usb/host/isp116x-hcd.c | 1 +
drivers/usb/host/max3421-hcd.c | 1 +
drivers/usb/host/oxu210hp-hcd.c | 1 +
drivers/usb/misc/yurex.c | 1 +
drivers/usb/musb/tusb6010.c | 1 +
drivers/usb/storage/ene_ub6250.c | 1 +
drivers/usb/storage/uas.c | 1 +
drivers/video/fbdev/geode/lxfb_ops.c | 1 +
drivers/video/fbdev/pm2fb.c | 1 +
drivers/watchdog/machzwd.c | 1 +
drivers/xen/manage.c | 1 +
fs/afs/cmservice.c | 5 +++++
fs/afs/fsclient.c | 4 ++++
fs/afs/vlclient.c | 1 +
fs/ceph/dir.c | 2 ++
fs/cifs/inode.c | 1 +
fs/cifs/sess.c | 1 +
fs/cifs/smbdirect.c | 1 +
fs/ext2/inode.c | 1 +
fs/ext4/super.c | 1 +
fs/gfs2/inode.c | 2 ++
fs/gfs2/recovery.c | 1 +
fs/isofs/rock.c | 1 +
fs/nfs/nfs3acl.c | 1 +
fs/nfs/nfs4client.c | 1 +
fs/nfs/nfs4proc.c | 2 ++
fs/nfs/nfs4state.c | 1 +
fs/nfs/pnfs.c | 2 ++
fs/nfsd/nfs4state.c | 1 +
fs/nfsd/nfsctl.c | 1 +
fs/reiserfs/namei.c | 1 +
mm/mm_init.c | 1 +
mm/vmscan.c | 1 +
net/ax25/af_ax25.c | 1 +
net/bridge/br_input.c | 1 +
net/core/dev.c | 1 +
net/decnet/dn_route.c | 2 +-
net/ipv4/ah4.c | 1 +
net/ipv4/esp4.c | 1 +
net/ipv4/fib_semantics.c | 1 +
net/ipv4/ip_vti.c | 1 +
net/ipv4/ipcomp.c | 1 +
net/ipv4/netfilter/ipt_REJECT.c | 1 +
net/mac80211/cfg.c | 2 ++
net/netfilter/nf_conntrack_proto_dccp.c | 1 +
net/netfilter/nf_tables_api.c | 1 +
net/netfilter/nft_ct.c | 1 +
net/netrom/nr_route.c | 4 ++++
net/packet/af_packet.c | 1 +
net/rds/tcp_connect.c | 1 +
net/rds/threads.c | 2 ++
net/rose/rose_route.c | 2 ++
net/rxrpc/af_rxrpc.c | 1 +
net/sctp/input.c | 3 ++-
net/sunrpc/rpc_pipe.c | 1 +
net/sunrpc/xprtsock.c | 1 +
net/tipc/link.c | 1 +
net/wireless/nl80211.c | 1 +
net/wireless/util.c | 1 +
net/xfrm/xfrm_interface.c | 1 +
security/integrity/ima/ima_main.c | 1 +
security/integrity/ima/ima_policy.c | 2 ++
security/keys/process_keys.c | 1 +
security/selinux/hooks.c | 1 +
sound/drivers/pcsp/pcsp_input.c | 1 +
sound/isa/sb/sb8_main.c | 1 +
sound/pci/rme9652/hdspm.c | 1 +
sound/soc/codecs/adav80x.c | 1 +
sound/soc/codecs/arizona.c | 1 +
sound/soc/codecs/cs42l52.c | 1 +
sound/soc/codecs/cs42l56.c | 1 +
sound/soc/codecs/cs47l92.c | 1 +
sound/soc/codecs/wm8962.c | 1 +
209 files changed, 264 insertions(+), 26 deletions(-)
--
2.27.0
In preparation to enable -Wimplicit-fallthrough for Clang, fix multiple
warnings by explicitly adding multiple break statements instead of
letting the code fall through to the next case.
Link: https://github.com/KSPP/linux/issues/115
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
drivers/crypto/ccree/cc_cipher.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/crypto/ccree/cc_cipher.c b/drivers/crypto/ccree/cc_cipher.c
index dafa6577a845..cdfee501fbd9 100644
--- a/drivers/crypto/ccree/cc_cipher.c
+++ b/drivers/crypto/ccree/cc_cipher.c
@@ -97,6 +97,7 @@ static int validate_keys_sizes(struct cc_cipher_ctx *ctx_p, u32 size)
case S_DIN_to_SM4:
if (size == SM4_KEY_SIZE)
return 0;
+ break;
default:
break;
}
@@ -139,9 +140,11 @@ static int validate_data_size(struct cc_cipher_ctx *ctx_p,
case DRV_CIPHER_CBC:
if (IS_ALIGNED(size, SM4_BLOCK_SIZE))
return 0;
+ break;
default:
break;
}
+ break;
default:
break;
}
--
2.27.0
Hi,
On 11/20/20 12:53, Jakub Kicinski wrote:
> On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote:
>> This series aims to fix almost all remaining fall-through warnings in
>> order to enable -Wimplicit-fallthrough for Clang.
>>
>> In preparation to enable -Wimplicit-fallthrough for Clang, explicitly
>> add multiple break/goto/return/fallthrough statements instead of just
>> letting the code fall through to the next case.
>>
>> Notice that in order to enable -Wimplicit-fallthrough for Clang, this
>> change[1] is meant to be reverted at some point. So, this patch helps
>> to move in that direction.
>>
>> Something important to mention is that there is currently a discrepancy
>> between GCC and Clang when dealing with switch fall-through to empty case
>> statements or to cases that only contain a break/continue/return
>> statement[2][3][4].
>
> Are we sure we want to make this change? Was it discussed before?
>
> Are there any bugs Clangs puritanical definition of fallthrough helped
> find?
>
> IMVHO compiler warnings are supposed to warn about issues that could
> be bugs. Falling through to default: break; can hardly be a bug?!
The justification for this is explained in this same changelog text:
Now that the -Wimplicit-fallthrough option has been globally enabled[5],
any compiler should really warn on missing either a fallthrough annotation
or any of the other case-terminating statements (break/continue/return/
goto) when falling through to the next case statement. Making exceptions
to this introduces variation in case handling which may continue to lead
to bugs, misunderstandings, and a general lack of robustness. The point
of enabling options like -Wimplicit-fallthrough is to prevent human error
and aid developers in spotting bugs before their code is even built/
submitted/committed, therefore eliminating classes of bugs. So, in order
to really accomplish this, we should, and can, move in the direction of
addressing any error-prone scenarios and get rid of the unintentional
fallthrough bug-class in the kernel, entirely, even if there is some minor
redundancy. Better to have explicit case-ending statements than continue to
have exceptions where one must guess as to the right result. The compiler
will eliminate any actual redundancy.
Note that there is already a patch in mainline that addresses almost
40,000 of these issues[6].
[1] commit e2079e93f562c ("kbuild: Do not enable -Wimplicit-fallthrough for clang for now")
[2] ClangBuiltLinux#636
[3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91432
[4] https://godbolt.org/z/xgkvIh
[5] commit a035d552a93b ("Makefile: Globally enable fall-through warning")
[6] commit 4169e889e588 ("include: jhash/signal: Fix fall-through warnings for Clang")
Thanks
--
Gustavo
On Fri, Nov 20, 2020 at 8:34 PM Gustavo A. R. Silva
<[email protected]> wrote:
>
> In preparation to enable -Wimplicit-fallthrough for Clang, fix multiple
> warnings by explicitly adding multiple break statements instead of
> letting the code fall through to the next case.
>
> Link: https://github.com/KSPP/linux/issues/115
> Signed-off-by: Gustavo A. R. Silva <[email protected]>
> ---
> drivers/crypto/ccree/cc_cipher.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/crypto/ccree/cc_cipher.c b/drivers/crypto/ccree/cc_cipher.c
> index dafa6577a845..cdfee501fbd9 100644
> --- a/drivers/crypto/ccree/cc_cipher.c
> +++ b/drivers/crypto/ccree/cc_cipher.c
> @@ -97,6 +97,7 @@ static int validate_keys_sizes(struct cc_cipher_ctx *ctx_p, u32 size)
> case S_DIN_to_SM4:
> if (size == SM4_KEY_SIZE)
> return 0;
> + break;
> default:
> break;
> }
> @@ -139,9 +140,11 @@ static int validate_data_size(struct cc_cipher_ctx *ctx_p,
> case DRV_CIPHER_CBC:
> if (IS_ALIGNED(size, SM4_BLOCK_SIZE))
> return 0;
> + break;
> default:
> break;
> }
> + break;
> default:
> break;
> }
> --
> 2.27.0
>
Acked-by: Gilad Ben-Yossef <[email protected]>
Thanks,
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
values of β will give rise to dom!
On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote:
> Please tell me
> our reward for all this effort isn't a single missing error print.
There were quite literally dozens of logical defects found
by the fallthrough additions. Very few were logging only.
On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote:
> On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote:
> > Please tell me our reward for all this effort isn't a single
> > missing error print.
>
> There were quite literally dozens of logical defects found
> by the fallthrough additions. Very few were logging only.
So can you give us the best examples (or indeed all of them if someone
is keeping score)? hopefully this isn't a US election situation ...
James
On Sun, 2020-11-22 at 11:12 -0800, James Bottomley wrote:
> On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote:
> > On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote:
> > > Please tell me our reward for all this effort isn't a single
> > > missing error print.
> >
> > There were quite literally dozens of logical defects found
> > by the fallthrough additions. Very few were logging only.
>
> So can you give us the best examples (or indeed all of them if someone
> is keeping score)? hopefully this isn't a US election situation ...
Gustavo? Are you running for congress now?
https://lwn.net/Articles/794944/
On Sun, 2020-11-22 at 11:22 -0800, Joe Perches wrote:
> On Sun, 2020-11-22 at 11:12 -0800, James Bottomley wrote:
> > On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote:
> > > On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote:
> > > > Please tell me our reward for all this effort isn't a single
> > > > missing error print.
> > >
> > > There were quite literally dozens of logical defects found
> > > by the fallthrough additions. Very few were logging only.
> >
> > So can you give us the best examples (or indeed all of them if
> > someone is keeping score)? hopefully this isn't a US election
> > situation ...
>
> Gustavo? Are you running for congress now?
>
> https://lwn.net/Articles/794944/
That's 21 reported fixes of which about 50% seem to produce no change
in code behaviour at all, a quarter seem to have no user visible effect
with the remaining quarter producing unexpected errors on obscure
configuration parameters, which is why no-one really noticed them
before.
James
On Sun, 22 Nov 2020, Miguel Ojeda wrote:
>
> It isn't that much effort, isn't it? Plus we need to take into account
> the future mistakes that it might prevent, too.
We should also take into account optimisim about future improvements in
tooling.
> So even if there were zero problems found so far, it is still a positive
> change.
>
It is if you want to spin it that way.
> I would agree if these changes were high risk, though; but they are
> almost trivial.
>
This is trivial:
case 1:
this();
+ fallthrough;
case 2:
that();
But what we inevitably get is changes like this:
case 3:
this();
+ break;
case 4:
hmmm();
Why? Mainly to silence the compiler. Also because the patch author argued
successfully that they had found a theoretical bug, often in mature code.
But is anyone keeping score of the regressions? If unreported bugs count,
what about unreported regressions?
> Cheers,
> Miguel
>
On Sun, Nov 22, 2020 at 11:54 PM Finn Thain <[email protected]> wrote:
>
> We should also take into account optimisim about future improvements in
> tooling.
Not sure what you mean here. There is no reliable way to guess what
the intention was with a missing fallthrough, even if you parsed
whitespace and indentation.
> It is if you want to spin it that way.
How is that a "spin"? It is a fact that we won't get *implicit*
fallthrough mistakes anymore (in particular if we make it a hard
error).
> But what we inevitably get is changes like this:
>
> case 3:
> this();
> + break;
> case 4:
> hmmm();
>
> Why? Mainly to silence the compiler. Also because the patch author argued
> successfully that they had found a theoretical bug, often in mature code.
If someone changes control flow, that is on them. Every kernel
developer knows what `break` does.
> But is anyone keeping score of the regressions? If unreported bugs count,
> what about unreported regressions?
Introducing `fallthrough` does not change semantics. If you are really
keen, you can always compare the objects because the generated code
shouldn't change.
Cheers,
Miguel
On Mon, Nov 23, 2020 at 4:58 PM James Bottomley
<[email protected]> wrote:
>
> Well, I used git. It says that as of today in Linus' tree we have 889
> patches related to fall throughs and the first series went in in
> october 2017 ... ignoring a couple of outliers back to February.
I can see ~10k insertions over ~1k commits and 15 years that mention a
fallthrough in the entire repo. That is including some commits (like
the biggest one, 960 insertions) that have nothing to do with C
fallthrough. A single kernel release has an order of magnitude more
changes than this...
But if we do the math, for an author, at even 1 minute per line change
and assuming nothing can be automated at all, it would take 1 month of
work. For maintainers, a couple of trivial lines is noise compared to
many other patches.
In fact, this discussion probably took more time than the time it
would take to review the 200 lines. :-)
> We're also complaining about the inability to recruit maintainers:
>
> https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/
>
> And burn out:
>
> http://antirez.com/news/129
Accepting trivial and useful 1-line patches is not what makes a
voluntary maintainer quit... Thankless work with demanding deadlines is.
> The whole crux of your argument seems to be maintainers' time isn't
> important so we should accept all trivial patches
I have not said that, at all. In fact, I am a voluntary one and I
welcome patches like this. It takes very little effort on my side to
review and it helps the kernel overall. Paid maintainers are the ones
that can take care of big features/reviews.
> What I'm actually trying to articulate is a way of measuring value of
> the patch vs cost ... it has nothing really to do with who foots the
> actual bill.
I understand your point, but you were the one putting it in terms of a
junior FTE. In my view, 1 month-work (worst case) is very much worth
removing a class of errors from a critical codebase.
> One thesis I'm actually starting to formulate is that this continual
> devaluing of maintainers is why we have so much difficulty keeping and
> recruiting them.
That may very well be true, but I don't feel anybody has devalued
maintainers in this discussion.
Cheers,
Miguel
On Sun, Nov 22, 2020 at 09:54:29AM +0200, Gilad Ben-Yossef wrote:
> On Fri, Nov 20, 2020 at 8:34 PM Gustavo A. R. Silva
> <[email protected]> wrote:
> >
> > In preparation to enable -Wimplicit-fallthrough for Clang, fix multiple
> > warnings by explicitly adding multiple break statements instead of
> > letting the code fall through to the next case.
> >
> > Link: https://github.com/KSPP/linux/issues/115
> > Signed-off-by: Gustavo A. R. Silva <[email protected]>
> > ---
[..]
>
> Acked-by: Gilad Ben-Yossef <[email protected]>
Thanks, Gilad.
--
Gustavo
On Tue, 2020-11-24 at 11:58 +1100, Finn Thain wrote:
> it's not for me to prove that such patches don't affect code
> generation. That's for the patch author and (unfortunately) for reviewers.
Ideally, that proof would be provided by the compilation system itself
and not patch authors nor reviewers nor maintainers.
Unfortunately gcc does not guarantee repeatability or deterministic output.
To my knowledge, neither does clang.
On Mon, 23 Nov 2020, Joe Perches wrote:
> On Tue, 2020-11-24 at 11:58 +1100, Finn Thain wrote:
> > it's not for me to prove that such patches don't affect code
> > generation. That's for the patch author and (unfortunately) for
> > reviewers.
>
> Ideally, that proof would be provided by the compilation system itself
> and not patch authors nor reviewers nor maintainers.
>
> Unfortunately gcc does not guarantee repeatability or deterministic
> output. To my knowledge, neither does clang.
>
Yes, I've said the same thing myself. But having attempted it, I now think
this is a hard problem. YMMV.
https://lore.kernel.org/linux-scsi/[email protected]/
https://lore.kernel.org/linux-scsi/[email protected]/
On 25/11/2020 00:32, Miguel Ojeda wrote:
> I have said *authoring* lines of *this* kind takes a minute per line.
> Specifically: lines fixing the fallthrough warning mechanically and
> repeatedly where the compiler tells you to, and doing so full-time for
> a month.
<snip>
> It is useful since it makes intent clear.
To make the intent clear, you have to first be certain that you
understand the intent; otherwise by adding either a break or a
fallthrough to suppress the warning you are just destroying the
information that "the intent of this code is unknown".
Figuring out the intent of a piece of unfamiliar code takes more
than 1 minute; just because
case foo:
thing;
case bar:
break;
produces identical code to
case foo:
thing;
break;
case bar:
break;
doesn't mean that *either* is correct — maybe the author meant
to write
case foo:
return thing;
case bar:
break;
and by inserting that break you've destroyed the marker that
would direct someone who knew what the code was about to look
at that point in the code and spot the problem.
Thus, you *always* have to look at more than just the immediate
mechanical context of the code, to make a proper judgement that
yes, this was the intent. If you think that that sort of thing
can be done in an *average* time of one minute, then I hope you
stay away from code I'm responsible for!
One minute would be an optimistic target for code that, as the
maintainer, one is already somewhat familiar with. For code
that you're seeing for the first time, as is usually the case
with the people doing these mechanical fix-a-warning patches,
it's completely unrealistic.
A warning is only useful because it makes you *think* about the
code. If you suppress the warning without doing that thinking,
then you made the warning useless; and if the warning made you
think about code that didn't *need* it, then the warning was
useless from the start.
So make your mind up: does Clang's stricter -Wimplicit-fallthrough
flag up code that needs thought (in which case the fixes take
effort both to author and to review) or does it flag up code
that can be mindlessly "fixed" (in which case the warning is
worthless)? Proponents in this thread seem to be trying to
have it both ways.
-ed
Hi Miguel,
On Thu, Nov 26, 2020 at 3:54 PM Miguel Ojeda
<[email protected]> wrote:
> On Wed, Nov 25, 2020 at 11:44 PM Edward Cree <[email protected]> wrote:
> > To make the intent clear, you have to first be certain that you
> > understand the intent; otherwise by adding either a break or a
> > fallthrough to suppress the warning you are just destroying the
> > information that "the intent of this code is unknown".
>
> If you don't know what the intent of your own code is, then you
> *already* have a problem in your hands.
The maintainer is not necessarily the owner/author of the code, and
thus may not know the intent of the code.
> > or does it flag up code
> > that can be mindlessly "fixed" (in which case the warning is
> > worthless)? Proponents in this thread seem to be trying to
> > have it both ways.
>
> A warning is not worthless just because you can mindlessly fix it.
> There are many counterexamples, e.g. many
> checkpatch/lint/lang-format/indentation warnings, functional ones like
> the `if (a = b)` warning...
BTW, you cannot mindlessly fix the latter, as you cannot know if
"(a == b)" or "((a = b))" was intended, without understanding the code
(and the (possibly unavailable) data sheet, and the hardware, ...).
P.S. So far I've stayed out of this thread, as I like it if the compiler
flags possible mistakes. After all I was the one fixing new
"may be used uninitialized" warnings thrown up by gcc-4.1, until
(a bit later than) support for that compiler was removed...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Nov 26, 2020 at 4:28 PM Geert Uytterhoeven <[email protected]> wrote:
>
> The maintainer is not necessarily the owner/author of the code, and
> thus may not know the intent of the code.
Agreed, I was not blaming maintainers -- just trying to point out that
the problem is there :-)
In those cases, it is still very useful: we add the `fallthrough` and
a comment saying `FIXME: fallthrough intended? Figure this out...`.
Thus a previous unknown unknown is now a known unknown. And no new
unknown unknowns will be introduced since we enabled the warning
globally.
> BTW, you cannot mindlessly fix the latter, as you cannot know if
> "(a == b)" or "((a = b))" was intended, without understanding the code
> (and the (possibly unavailable) data sheet, and the hardware, ...).
That's right, I was referring to the cases where the compiler saves
someone time from a typo they just made.
Cheers,
Miguel
On Fri, Nov 20, 2020 at 12:34:56PM -0600, Gustavo A. R. Silva wrote:
> In preparation to enable -Wimplicit-fallthrough for Clang, fix multiple
> warnings by explicitly adding multiple break statements instead of
> letting the code fall through to the next case.
>
> Link: https://github.com/KSPP/linux/issues/115
> Signed-off-by: Gustavo A. R. Silva <[email protected]>
> ---
> drivers/crypto/ccree/cc_cipher.c | 3 +++
> 1 file changed, 3 insertions(+)
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Sun, Nov 22, 2020 at 08:17:03AM -0800, Kees Cook wrote:
> On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote:
> > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote:
> > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote:
> > > > > This series aims to fix almost all remaining fall-through warnings in
> > > > > order to enable -Wimplicit-fallthrough for Clang.
> > > > >
> > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly
> > > > > add multiple break/goto/return/fallthrough statements instead of just
> > > > > letting the code fall through to the next case.
> > > > >
> > > > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this
> > > > > change[1] is meant to be reverted at some point. So, this patch helps
> > > > > to move in that direction.
> > > > >
> > > > > Something important to mention is that there is currently a discrepancy
> > > > > between GCC and Clang when dealing with switch fall-through to empty case
> > > > > statements or to cases that only contain a break/continue/return
> > > > > statement[2][3][4].
> > > >
> > > > Are we sure we want to make this change? Was it discussed before?
> > > >
> > > > Are there any bugs Clangs puritanical definition of fallthrough helped
> > > > find?
> > > >
> > > > IMVHO compiler warnings are supposed to warn about issues that could
> > > > be bugs. Falling through to default: break; can hardly be a bug?!
> > >
> > > It's certainly a place where the intent is not always clear. I think
> > > this makes all the cases unambiguous, and doesn't impact the machine
> > > code, since the compiler will happily optimize away any behavioral
> > > redundancy.
> >
> > If none of the 140 patches here fix a real bug, and there is no change
> > to machine code then it sounds to me like a W=2 kind of a warning.
>
> FWIW, this series has found at least one bug so far:
> https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=Kr_CQ@mail.gmail.com/
This is a fallthrough to a return and not to a break. That should
trigger a warning. The fallthrough to a break should not generate a
warning.
The bug we're trying to fix is "missing break statement" but if the
result of the bug is "we hit a break statement" then now we're just
talking about style. GCC should limit itself to warning about
potentially buggy code.
regards,
dan carpenter
On Mon, Nov 23, 2020 at 05:32:51PM -0800, Nick Desaulniers wrote:
> On Sun, Nov 22, 2020 at 8:17 AM Kees Cook <[email protected]> wrote:
> >
> > On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote:
> > > If none of the 140 patches here fix a real bug, and there is no change
> > > to machine code then it sounds to me like a W=2 kind of a warning.
> >
> > FWIW, this series has found at least one bug so far:
> > https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=Kr_CQ@mail.gmail.com/
>
> So looks like the bulk of these are:
> switch (x) {
> case 0:
> ++x;
> default:
> break;
> }
This should not generate a warning.
>
> I have a patch that fixes those up for clang:
> https://reviews.llvm.org/D91895
>
> There's 3 other cases that don't quite match between GCC and Clang I
> observe in the kernel:
> switch (x) {
> case 0:
> ++x;
> default:
> goto y;
> }
> y:;
This should generate a warning.
>
> switch (x) {
> case 0:
> ++x;
> default:
> return;
> }
Warn for this.
>
> switch (x) {
> case 0:
> ++x;
> default:
> ;
> }
Don't warn for this.
If adding a break statement changes the flow of the code then warn about
potentially missing break statements, but if it doesn't change anything
then don't warn about it.
regards,
dan carpenter