[PATCH 000/141] Fix fall-through warnings for Clang
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(-)
In preparation to enable -Wimplicit-fallthrough for Clang, fix multiple warnings by explicitly adding multiple break statements instead of just letting the code fall through, and also add fallthrough pseudo-keywords in places where the code is intended to fall through to the next case.
Link: https://github.com/KSPP/linux/issues/115 Signed-off-by: Gustavo A. R. Silva gustavoars@kernel.org --- 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 + 6 files changed, 6 insertions(+)
diff --git a/sound/soc/codecs/adav80x.c b/sound/soc/codecs/adav80x.c index 4fd99280d7db..75a649108106 100644 --- a/sound/soc/codecs/adav80x.c +++ b/sound/soc/codecs/adav80x.c @@ -373,6 +373,7 @@ static int adav80x_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt) case SND_SOC_DAIFMT_CBM_CFM: capture |= ADAV80X_CAPTURE_MODE_MASTER; playback |= ADAV80X_PLAYBACK_MODE_MASTER; + break; case SND_SOC_DAIFMT_CBS_CFS: break; default: diff --git a/sound/soc/codecs/arizona.c b/sound/soc/codecs/arizona.c index 1228f2de0297..e32871b3f68a 100644 --- a/sound/soc/codecs/arizona.c +++ b/sound/soc/codecs/arizona.c @@ -1034,6 +1034,7 @@ int arizona_out_ev(struct snd_soc_dapm_widget *w, priv->out_down_delay++; break; } + break; default: break; } diff --git a/sound/soc/codecs/cs42l52.c b/sound/soc/codecs/cs42l52.c index f772628f233e..796b894c390f 100644 --- a/sound/soc/codecs/cs42l52.c +++ b/sound/soc/codecs/cs42l52.c @@ -944,6 +944,7 @@ static int cs42l52_beep_event(struct input_dev *dev, unsigned int type, case SND_BELL: if (hz) hz = 261; + break; case SND_TONE: break; default: diff --git a/sound/soc/codecs/cs42l56.c b/sound/soc/codecs/cs42l56.c index 97024a6ac96d..bb9599cc832b 100644 --- a/sound/soc/codecs/cs42l56.c +++ b/sound/soc/codecs/cs42l56.c @@ -1008,6 +1008,7 @@ static int cs42l56_beep_event(struct input_dev *dev, unsigned int type, case SND_BELL: if (hz) hz = 261; + break; case SND_TONE: break; default: diff --git a/sound/soc/codecs/cs47l92.c b/sound/soc/codecs/cs47l92.c index 6e34106c268f..52dc29942ec2 100644 --- a/sound/soc/codecs/cs47l92.c +++ b/sound/soc/codecs/cs47l92.c @@ -201,6 +201,7 @@ static int cs47l92_outclk_ev(struct snd_soc_dapm_widget *w, default: break; } + break; default: break; } diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c index 0bd3bbc2aacf..3af456010b9c 100644 --- a/sound/soc/codecs/wm8962.c +++ b/sound/soc/codecs/wm8962.c @@ -3203,6 +3203,7 @@ static int wm8962_beep_event(struct input_dev *dev, unsigned int type, case SND_BELL: if (hz) hz = 1000; + fallthrough; case SND_TONE: break; default:
On Fri, 2020-11-20 at 12:21 -0600, Gustavo A. R. Silva wrote:
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.
This was a bit hard to parse for a second or three.
Thanks Gustavo.
How was this change done?
On 11/20/20 12:28, Joe Perches wrote:
On Fri, 2020-11-20 at 12:21 -0600, Gustavo A. R. Silva wrote:
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.
This was a bit hard to parse for a second or three.
Thanks Gustavo.
How was this change done?
I audited case by case in order to determine the best fit for each situation. Depending on the surrounding logic, sometimes it makes more sense a goto or a fallthrough rather than merely a break.
Thanks -- Gustavo
In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement 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 gustavoars@kernel.org --- sound/pci/rme9652/hdspm.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index 4a1f576dd9cf..f25a448dd636 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -6321,6 +6321,7 @@ static int snd_hdspm_hwdep_ioctl(struct snd_hwdep *hw, struct file *file, (statusregister & HDSPM_RX_64ch) ? 1 : 0; /* TODO: Mac driver sets it when f_s>48kHz */ status.card_specific.madi.frame_format = 0; + break;
default: break;
On Fri, 20 Nov 2020 19:33:52 +0100, Gustavo A. R. Silva wrote:
In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement 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 gustavoars@kernel.org
Thanks, applied.
Takashi
On Sat, Nov 21, 2020 at 09:30:00AM +0100, Takashi Iwai wrote:
On Fri, 20 Nov 2020 19:33:52 +0100, Gustavo A. R. Silva wrote:
In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement 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 gustavoars@kernel.org
Thanks, applied.
Thanks for all you've taken, Takashi. -- Gustavo
In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement 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 gustavoars@kernel.org --- sound/drivers/pcsp/pcsp_input.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/drivers/pcsp/pcsp_input.c b/sound/drivers/pcsp/pcsp_input.c index 52b475b310c3..e79603fe743d 100644 --- a/sound/drivers/pcsp/pcsp_input.c +++ b/sound/drivers/pcsp/pcsp_input.c @@ -54,6 +54,7 @@ static int pcspkr_input_event(struct input_dev *dev, unsigned int type, case SND_BELL: if (value) value = 1000; + break; case SND_TONE: break; default:
On Fri, 20 Nov 2020 19:34:01 +0100, Gustavo A. R. Silva wrote:
In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement 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 gustavoars@kernel.org
Thanks, applied.
Takashi
In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement 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 gustavoars@kernel.org --- sound/isa/sb/sb8_main.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/isa/sb/sb8_main.c b/sound/isa/sb/sb8_main.c index 86d0d2fdf48a..8d01692c4f2a 100644 --- a/sound/isa/sb/sb8_main.c +++ b/sound/isa/sb/sb8_main.c @@ -506,6 +506,7 @@ static int snd_sb8_open(struct snd_pcm_substream *substream) } else { runtime->hw.rate_max = 15000; } + break; default: break; }
On Fri, 20 Nov 2020 19:34:12 +0100, Gustavo A. R. Silva wrote:
In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement 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 gustavoars@kernel.org
Thanks, applied.
Takashi
In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement 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 gustavoars@kernel.org --- drivers/slimbus/messaging.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/slimbus/messaging.c b/drivers/slimbus/messaging.c index d5879142dbef..f2b5d347d227 100644 --- a/drivers/slimbus/messaging.c +++ b/drivers/slimbus/messaging.c @@ -258,6 +258,7 @@ int slim_xfer_msg(struct slim_device *sbdev, struct slim_val_inf *msg, case SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION: case SLIM_MSG_MC_CLEAR_INFORMATION: txn->rl += msg->num_bytes; + break; default: break; }
On 20/11/2020 18:39, Gustavo A. R. Silva wrote:
In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement 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 gustavoars@kernel.org
Applied thanks,
srini
drivers/slimbus/messaging.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/slimbus/messaging.c b/drivers/slimbus/messaging.c index d5879142dbef..f2b5d347d227 100644 --- a/drivers/slimbus/messaging.c +++ b/drivers/slimbus/messaging.c @@ -258,6 +258,7 @@ int slim_xfer_msg(struct slim_device *sbdev, struct slim_val_inf *msg, case SLIM_MSG_MC_REQUEST_CLEAR_INFORMATION: case SLIM_MSG_MC_CLEAR_INFORMATION: txn->rl += msg->num_bytes;
default: break; }break;
On Tue, Nov 24, 2020 at 10:48:04AM +0000, Srinivas Kandagatla wrote:
On 20/11/2020 18:39, Gustavo A. R. Silva wrote:
In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement 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 gustavoars@kernel.org
Applied thanks,
Thank you, Srini. -- Gustavo
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?!
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 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.
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.
I think clang is just being annoying here, but if I'm the only one who feels this way chances are I'm wrong :)
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.
I'd like to avoid splitting common -W options between default and W=2 just based on the compiler. Getting -Wimplicit-fallthrough enabled found plenty of bugs, so making sure it works correctly for both compilers feels justified to me. (This is just a subset of the same C language short-coming.)
I think clang is just being annoying here, but if I'm the only one who feels this way chances are I'm wrong :)
It's being pretty pedantic, but I don't think it's unreasonable to explicitly state how every case ends. GCC's silence for the case of "fall through to a break" doesn't really seem justified.
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=K...
On Sun, 2020-11-22 at 08:17 -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=K...
Well, it's a problem in an error leg, sure, but it's not a really compelling reason for a 141 patch series, is it? All that fixing this error will do is get the driver to print "oh dear there's a problem" under four more conditions than it previously did.
We've been at this for three years now with nearly a thousand patches, firstly marking all the fall throughs with /* fall through */ and later changing it to fallthrough. At some point we do have to ask if the effort is commensurate with the protection afforded. Please tell me our reward for all this effort isn't a single missing error print.
James
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?
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?
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, Nov 22, 2020 at 11:53:55AM -0800, James Bottomley wrote:
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?
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.
The really important point here is the number of bugs this has prevented and will prevent in the future. See an example of this, below:
https://lore.kernel.org/linux-iio/20190813135802.GB27392@kroah.com/
This work is still relevant, even if the total number of issues/bugs we find in the process is zero (which is not the case).
"The sucky thing about doing hard work to deploy hardening is that the result is totally invisible by definition (things not happening) [..]" - Dmitry Vyukov
Thanks -- Gustavo
On Mon, 2020-11-23 at 07:03 -0600, Gustavo A. R. Silva wrote:
On Sun, Nov 22, 2020 at 11:53:55AM -0800, James Bottomley wrote:
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?
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.
The really important point here is the number of bugs this has prevented and will prevent in the future. See an example of this, below:
https://lore.kernel.org/linux-iio/20190813135802.GB27392@kroah.com/
I think this falls into the same category as the other six bugs: it changes the output/input for parameters but no-one has really noticed, usually because the command is obscure or the bias effect is minor.
This work is still relevant, even if the total number of issues/bugs we find in the process is zero (which is not the case).
Really, no ... something which produces no improvement has no value at all ... we really shouldn't be wasting maintainer time with it because it has a cost to merge. I'm not sure we understand where the balance lies in value vs cost to merge but I am confident in the zero value case.
"The sucky thing about doing hard work to deploy hardening is that the result is totally invisible by definition (things not happening) [..]"
- Dmitry Vyukov
Really, no. Something that can't be measured at all doesn't exist.
And actually hardening is one of those things you can measure (which I do have to admit isn't true for everything in the security space) ... it's number of exploitable bugs found before you did it vs number of exploitable bugs found after you did it. Usually hardening eliminates a class of bug, so the way I've measured hardening before is to go through the CVE list for the last couple of years for product X, find all the bugs that are of the class we're looking to eliminate and say if we had hardened X against this class of bug we'd have eliminated Y% of the exploits. It can be quite impressive if Y is a suitably big number.
James
On Sun, Nov 22, 2020 at 7:22 PM James Bottomley James.Bottomley@hansenpartnership.com wrote:
Well, it's a problem in an error leg, sure, but it's not a really compelling reason for a 141 patch series, is it? All that fixing this error will do is get the driver to print "oh dear there's a problem" under four more conditions than it previously did.
We've been at this for three years now with nearly a thousand patches, firstly marking all the fall throughs with /* fall through */ and later changing it to fallthrough. At some point we do have to ask if the effort is commensurate with the protection afforded. Please tell me our reward for all this effort isn't a single missing error print.
It isn't that much effort, isn't it? Plus we need to take into account the future mistakes that it might prevent, too. So even if there were zero problems found so far, it is still a positive change.
I would agree if these changes were high risk, though; but they are almost trivial.
Cheers, Miguel
On Sun, 2020-11-22 at 21:35 +0100, Miguel Ojeda wrote:
On Sun, Nov 22, 2020 at 7:22 PM James Bottomley James.Bottomley@hansenpartnership.com wrote:
Well, it's a problem in an error leg, sure, but it's not a really compelling reason for a 141 patch series, is it? All that fixing this error will do is get the driver to print "oh dear there's a problem" under four more conditions than it previously did.
We've been at this for three years now with nearly a thousand patches, firstly marking all the fall throughs with /* fall through */ and later changing it to fallthrough. At some point we do have to ask if the effort is commensurate with the protection afforded. Please tell me our reward for all this effort isn't a single missing error print.
It isn't that much effort, isn't it?
Well, it seems to be three years of someone's time plus the maintainer review time and series disruption of nearly a thousand patches. Let's be conservative and assume the producer worked about 30% on the series and it takes about 5-10 minutes per patch to review, merge and for others to rework existing series. So let's say it's cost a person year of a relatively junior engineer producing the patches and say 100h of review and application time. The latter is likely the big ticket item because it's what we have in least supply in the kernel (even though it's 20x vs the producer time).
Plus we need to take into account the future mistakes that it might prevent, too. So even if there were zero problems found so far, it is still a positive change.
Well, the question I was asking is if it's worth the cost which I've tried to outline above.
I would agree if these changes were high risk, though; but they are almost trivial.
It's not about the risk of the changes it's about the cost of implementing them. Even if you discount the producer time (which someone gets to pay for, and if I were the engineering manager, I'd be unhappy about), the review/merge/rework time is pretty significant in exchange for six minor bug fixes. Fine, when a new compiler warning comes along it's certainly reasonable to see if we can benefit from it and the fact that the compiler people think it's worthwhile is enough evidence to assume this initially. But at some point you have to ask whether that assumption is supported by the evidence we've accumulated over the time we've been using it. And if the evidence doesn't support it perhaps it is time to stop the experiment.
James
On Sun, Nov 22, 2020 at 11:36 PM James Bottomley James.Bottomley@hansenpartnership.com wrote:
Well, it seems to be three years of someone's time plus the maintainer review time and series disruption of nearly a thousand patches. Let's be conservative and assume the producer worked about 30% on the series and it takes about 5-10 minutes per patch to review, merge and for others to rework existing series. So let's say it's cost a person year of a relatively junior engineer producing the patches and say 100h of review and application time. The latter is likely the big ticket item because it's what we have in least supply in the kernel (even though it's 20x vs the producer time).
How are you arriving at such numbers? It is a total of ~200 trivial lines.
It's not about the risk of the changes it's about the cost of implementing them. Even if you discount the producer time (which someone gets to pay for, and if I were the engineering manager, I'd be unhappy about), the review/merge/rework time is pretty significant in exchange for six minor bug fixes. Fine, when a new compiler warning comes along it's certainly reasonable to see if we can benefit from it and the fact that the compiler people think it's worthwhile is enough evidence to assume this initially. But at some point you have to ask whether that assumption is supported by the evidence we've accumulated over the time we've been using it. And if the evidence doesn't support it perhaps it is time to stop the experiment.
Maintainers routinely review 1-line trivial patches, not to mention internal API changes, etc.
If some company does not want to pay for that, that's fine, but they don't get to be maintainers and claim `Supported`.
Cheers, Miguel
On Mon, 2020-11-23 at 15:19 +0100, Miguel Ojeda wrote:
On Sun, Nov 22, 2020 at 11:36 PM James Bottomley James.Bottomley@hansenpartnership.com wrote:
Well, it seems to be three years of someone's time plus the maintainer review time and series disruption of nearly a thousand patches. Let's be conservative and assume the producer worked about 30% on the series and it takes about 5-10 minutes per patch to review, merge and for others to rework existing series. So let's say it's cost a person year of a relatively junior engineer producing the patches and say 100h of review and application time. The latter is likely the big ticket item because it's what we have in least supply in the kernel (even though it's 20x vs the producer time).
How are you arriving at such numbers? It is a total of ~200 trivial lines.
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.
It's not about the risk of the changes it's about the cost of implementing them. Even if you discount the producer time (which someone gets to pay for, and if I were the engineering manager, I'd be unhappy about), the review/merge/rework time is pretty significant in exchange for six minor bug fixes. Fine, when a new compiler warning comes along it's certainly reasonable to see if we can benefit from it and the fact that the compiler people think it's worthwhile is enough evidence to assume this initially. But at some point you have to ask whether that assumption is supported by the evidence we've accumulated over the time we've been using it. And if the evidence doesn't support it perhaps it is time to stop the experiment.
Maintainers routinely review 1-line trivial patches, not to mention internal API changes, etc.
We're also complaining about the inability to recruit maintainers:
https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_t...
And burn out:
The whole crux of your argument seems to be maintainers' time isn't important so we should accept all trivial patches ... I'm pushing back on that assumption in two places, firstly the valulessness of the time and secondly that all trivial patches are valuable.
If some company does not want to pay for that, that's fine, but they don't get to be maintainers and claim `Supported`.
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.
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.
James
On Mon, Nov 23, 2020 at 4:58 PM James Bottomley James.Bottomley@hansenpartnership.com wrote:
On Mon, 2020-11-23 at 15:19 +0100, Miguel Ojeda wrote:
On Sun, Nov 22, 2020 at 11:36 PM James Bottomley James.Bottomley@hansenpartnership.com wrote:
[cut]
Maintainers routinely review 1-line trivial patches, not to mention internal API changes, etc.
We're also complaining about the inability to recruit maintainers:
https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_t...
And burn out:
Right.
The whole crux of your argument seems to be maintainers' time isn't important so we should accept all trivial patches ... I'm pushing back on that assumption in two places, firstly the valulessness of the time and secondly that all trivial patches are valuable.
If some company does not want to pay for that, that's fine, but they don't get to be maintainers and claim `Supported`.
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.
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.
Absolutely.
This is just one of the factors involved, but a significant one IMV.
On Mon, 2020-11-23 at 07:58 -0800, James Bottomley wrote:
We're also complaining about the inability to recruit maintainers:
https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_t...
And burn out:
https://www.wired.com/story/open-source-coders-few-tired/
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.
It's unclear how to measure value in consistency.
But one way that costs can be reduced is by automation and _not_ involving maintainers when the patch itself is provably correct.
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.
The linux kernel has something like 1500 different maintainers listed in the MAINTAINERS file. That's not a trivial number.
$ git grep '^M:' MAINTAINERS | sort | uniq -c | wc -l 1543 $ git grep '^M:' MAINTAINERS| cut -f1 -d'<' | sort | uniq -c | wc -l 1446
I think the question you are asking is about trust and how it effects development.
And back to that wired story, the actual number of what you might be considering to be maintainers is likely less than 10% of the listed numbers above.
On Mon, Nov 23, 2020 at 4:58 PM James Bottomley James.Bottomley@hansenpartnership.com 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_t...
And burn out:
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 Mon, 2020-11-23 at 19:56 +0100, Miguel Ojeda wrote:
On Mon, Nov 23, 2020 at 4:58 PM James Bottomley James.Bottomley@hansenpartnership.com 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.
So you think a one line patch should take one minute to produce ... I really don't think that's grounded in reality. I suppose a one line patch only takes a minute to merge with b4 if no-one reviews or tests it, but that's not really desirable.
In fact, this discussion probably took more time than the time it would take to review the 200 lines. :-)
I'm framing the discussion in terms of the whole series of changes we have done for fall through, both what's in the tree currently (889 patches) both in terms of the produce and the consumer. That's what I used for my figures for cost.
We're also complaining about the inability to recruit maintainers:
https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_t...
And burn out:
Accepting trivial and useful 1-line patches
Part of what I'm trying to measure is the "and useful" bit because that's not a given.
is not what makes a voluntary maintainer quit...
so the proverb "straw which broke the camel's back" uniquely doesn't apply to maintainers
Thankless work with demanding deadlines is.
That's another potential reason, but it doesn't may other reasons less valid.
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.
Well, you know, subsystems are very different in terms of the amount of patches a maintainer has to process per release cycle of the kernel. If a maintainer is close to capacity, additional patches, however trivial, become a problem. If a maintainer has spare cycles, trivial patches may look easy.
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.
No, I evaluated the producer side in terms of an FTE. What we're mostly arguing about here is the consumer side: the maintainers and people who have to rework their patch sets. I estimated that at 100h.
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.
You seem to be saying that because you find it easy to merge trivial patches, everyone should. I'm reminded of a friend long ago who thought being a Tees River Pilot was a sinecure because he could navigate the Tees blindfold. What he forgot, of course, is that just because it's easy with a trawler doesn't mean it's easy with an oil tanker. In fact it takes longer to qualify as a Tees River Pilot than it does to get a PhD.
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 Mon, 2020-11-23 at 09:54 +1100, Finn Thain wrote:
But is anyone keeping score of the regressions? If unreported bugs count, what about unreported regressions?
Well, I was curious about the former (obviously no tool will tell me about the latter), so I asked git what patches had a fall-through series named in a fixes tag and these three popped out:
9cf51446e686 bpf, powerpc: Fix misuse of fallthrough in bpf_jit_comp() 6a9dc5fd6170 lib: Revert use of fallthrough pseudo-keyword in lib/ 91dbd73a1739 mips/oprofile: Fix fallthrough placement
I don't think any of these is fixing a significant problem, but they did cause someone time and trouble to investigate.
James
On Sun, Nov 22, 2020 at 11:54 PM Finn Thain fthain@telegraphics.com.au 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, 23 Nov 2020, Miguel Ojeda wrote:
On Mon, 23 Nov 2020, Finn Thain wrote:
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.
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.
What I meant was that you've used pessimism as if it was fact.
For example, "There is no way to guess what the effect would be if the compiler trained programmers to add a knee-jerk 'break' statement to avoid a warning".
Moreover, what I meant was that preventing programmer mistakes is a problem to be solved by development tools. The idea that retro-fitting new language constructs onto mature code is somehow necessary to "prevent future mistakes" is entirely questionable.
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.
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).
Perhaps "handwaving" is a better term?
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.
If someone changes control flow, that is on them. Every kernel developer knows what `break` does.
Sure. And if you put -Wimplicit-fallthrough into the Makefile and if that leads to well-intentioned patches that cause regressions, it is partly on you.
Have you ever considered the overall cost of the countless -Wpresume-incompetence flags?
Perhaps you pay the power bill for a build farm that produces logs that no-one reads? Perhaps you've run git bisect, knowing that the compiler messages are not interesting? Or compiled software in using a language that generates impenetrable messages? If so, here's a tip:
# grep CFLAGS /etc/portage/make.conf CFLAGS="... -Wno-all -Wno-extra ..." CXXFLAGS="${CFLAGS}"
Now allow me some pessimism: the hardware upgrades, gigawatt hours and wait time attributable to obligatory static analyses are a net loss.
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.
No, 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.
Cheers, Miguel
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/alpine.LNX.2.22.394.2004281017310.12@nipp... https://lore.kernel.org/linux-scsi/alpine.LNX.2.22.394.2005211358460.8@nippy...
Hi James.
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=K...
Well, it's a problem in an error leg, sure, but it's not a really compelling reason for a 141 patch series, is it? All that fixing this error will do is get the driver to print "oh dear there's a problem" under four more conditions than it previously did.
You are asking the wrong question here.
Yuo should ask how many hours could have been saved by all the bugs people have been fighting with and then fixed *before* the code hit the kernel at all.
My personal experience is that I, more than once, have had errors related to a missing break in my code. So this warnings is IMO a win.
And if we are only ~100 patches to have it globally enabled then it is a no-brainer in my book.
Sam
On Sun, Nov 22, 2020 at 8:17 AM Kees Cook keescook@chromium.org 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=K...
So looks like the bulk of these are: switch (x) { case 0: ++x; default: break; }
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:;
switch (x) { case 0: ++x; default: return; }
switch (x) { case 0: ++x; default: ; }
Based on your link, and Nathan's comment on my patch, maybe Clang should continue to warn for the above (at least the `default: return;` case) and GCC should change? While the last case looks harmless, there were only 1 or 2 across the tree in my limited configuration testing; I really think we should just add `break`s for those.
On Mon, 23 Nov 2020 17:32:51 -0800 Nick Desaulniers wrote:
On Sun, Nov 22, 2020 at 8:17 AM Kees Cook keescook@chromium.org 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=K...
So looks like the bulk of these are: switch (x) { case 0: ++x; default: break; }
I have a patch that fixes those up for clang: https://reviews.llvm.org/D91895
😍
On Mon, Nov 23, 2020 at 05:32:51PM -0800, Nick Desaulniers wrote:
On Sun, Nov 22, 2020 at 8:17 AM Kees Cook keescook@chromium.org 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=K...
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
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=K...
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
Hi Gustavo,
On Fri, Nov 20, 2020 at 7:21 PM Gustavo A. R. Silva gustavoars@kernel.org wrote:
Hi all,
This series aims to fix almost all remaining fall-through warnings in order to enable -Wimplicit-fallthrough for Clang.
Thanks for this.
Since this warning is reliable in both/all compilers and we are eventually getting rid of all the cases, what about going even further and making it an error right after?
Cheers, Miguel
On Fri, Nov 20, 2020 at 12:21:39PM -0600, Gustavo A. R. Silva wrote:
IB/hfi1: Fix fall-through warnings for Clang IB/mlx4: Fix fall-through warnings for Clang IB/qedr: Fix fall-through warnings for Clang RDMA/mlx5: Fix fall-through warnings for Clang
I picked these four to the rdma tree, thanks
Jason
On Mon, Nov 23, 2020 at 04:03:45PM -0400, Jason Gunthorpe wrote:
On Fri, Nov 20, 2020 at 12:21:39PM -0600, Gustavo A. R. Silva wrote:
IB/hfi1: Fix fall-through warnings for Clang IB/mlx4: Fix fall-through warnings for Clang IB/qedr: Fix fall-through warnings for Clang RDMA/mlx5: Fix fall-through warnings for Clang
I picked these four to the rdma tree, thanks
Awesome. :)
Thank you, Jason. -- Gustavo
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.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[1/1] regulator: as3722: Fix fall-through warnings for Clang commit: b52b417ccac4fae5b1f2ec4f1d46eb91e4493dc5
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
On Mon, Nov 23, 2020 at 08:38:46PM +0000, Mark Brown 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.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[1/1] regulator: as3722: Fix fall-through warnings for Clang commit: b52b417ccac4fae5b1f2ec4f1d46eb91e4493dc5
Thank you, Mark. -- Gustavo
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.
[...]
Applied to 5.11/scsi-queue, thanks!
[054/141] target: Fix fall-through warnings for Clang https://git.kernel.org/mkp/scsi/c/492096ecfa39
participants (17)
-
Dan Carpenter
-
Finn Thain
-
Gustavo A. R. Silva
-
Gustavo A. R. Silva
-
Jakub Kicinski
-
James Bottomley
-
Jason Gunthorpe
-
Joe Perches
-
Kees Cook
-
Mark Brown
-
Martin K. Petersen
-
Miguel Ojeda
-
Nick Desaulniers
-
Rafael J. Wysocki
-
Sam Ravnborg
-
Srinivas Kandagatla
-
Takashi Iwai