I would like to enable -Wimplicit-fallthrough=2 by default when available. It was introduced in GCC7. It caught a couple of bugs, but it does need a bit of extra comments to explain when a switch case statement fall-through is deliberate. Luckily with -Wimplicit-fallthrough=2 various existing comments already do that. I have fixed the bugs, but adding explicit break statements where necessary and added comments where the fall-through was correct. There is just one place that produces a warning for a fallthrough that I am not sure is deliberate or not. priv/guest_mips_toIR.c: In function ‘disInstr_MIPS_WRK’: priv/guest_mips_toIR.c:30205:13: warning: this statement may fall through [-Wimplicit-fallthrough=] if (VEX_MIPS_COMP_ID(archinfo->hwcaps) == VEX_PRID_COMP_CAVIUM) { ^ priv/guest_mips_toIR.c:30211:7: note: here case 0xC: /* INSV */ ^~~~
+1 for this; I am all in favour of more compile-time analysis. As far as the guest_mips_toIR.c fallthrough goes, I'd guess it is intended. I say this because it looks as if all 3 of case 0xA: /* LX */ case 0xC: /* INSV */ case 0x38: { /* EXTR.W */ are handled by the call to disDSPInstr_MIPS_WRK at line 30212. At least, from a quick check, it looks like disDSPInstr_MIPS_WRK handles them all. But it is so huge it's hard to tell for sure.
Created attachment 118772 [details] Enable GCC7 -Wimplicit-fallthrough=2 by default when available The changes to Makefile.all.am and configure.ac are simply to test if -Wimplicit-fallthrough=2 is available and enable it by default. https://developers.redhat.com/blog/2017/03/10/wimplicit-fallthrough-in-gcc-7/ VEX/priv/guest_amd64_toIR.c: VROUNDPS should not fallthrough to VROUNDPD and VROUNDPD should not fallthrough to VROUNDSS/VROUNDSD. VEX/priv/guest_arm64_toIR.c: Move fallthrough comments so GCC recognizes them. VEX/priv/guest_arm_toIR.c: Likewise VEX/priv/guest_mips_toIR.c: Add fillthrough comments for cases that just add signaling. Add breaks after emitting an ILLEGAL_INSTRUCTION. I am unsure about the following block: /* --- MIPS32(r2) DSP ASE(r2) / Cavium Specfic (LX) instructions --- */ case 0xA: /* LX */ if (VEX_MIPS_COMP_ID(archinfo->hwcaps) == VEX_PRID_COMP_CAVIUM) { if (dis_instr_CVM(cins)) break; goto decode_failure; } /* ??? */ case 0xC: /* INSV */ case 0x38: { /* EXTR.W */ if (VEX_MIPS_PROC_DSP(archinfo->hwcaps)) { UInt retVal = disDSPInstr_MIPS_WRK ( cins ); if (0 != retVal ) { goto decode_failure_dsp; } break; } else { goto decode_failure_dsp; } break; } Should the LX (0xA) case fall through to INSV EXTR.W or is that a missing break? VEX/priv/guest_ppc_toIR.c: Add a fallthrough comment when the immedizate offset is partially given by simm16. dadd, dsub, dmul and ddiv should not fallthrough to dcmpo, dcmpu if they couldn't be decoded. Add comment for xsabsqp, xsxexpqp,xsnabsqp, xsnegqp, xsxsigqp fallthrough to VSX Scalar Quad-Precision instructions. VEX/priv/guest_x86_toIR.c: Add fallthrough comments for REP[N][E] cases. VEX/priv/host_arm64_isel.c: Replace comment with explicit goto irreducible. I don't trust the comment is correct and if the intent is to signal irreducible lets just do that. VEX/priv/host_arm_isel.c: Likewise. VEX/priv/host_mips_defs.c: Add explicit fallthrough comments in mkFormBIT. VEX/priv/host_mips_isel.c: Add fallthrough comments after vassert(mode64). coregrind/m_gdbserver/m_gdbserver.c: Add comment explaining that if give_control_back_to_vgdb returns then the fallthrough to the vg_assert is correct. coregrind/m_syswrap/syswrap-linux.c: Move vfork comment so GCC recognizes it. VKI_BPF_BTF_LOAD should not fallthrough to VKI_BPF_TASK_FD_QUERY. memcheck/mc_malloc_wrappers.c: Change fallback comments to fallthrough.
(In reply to Mark Wielaard from comment #0) > I would like to enable -Wimplicit-fallthrough=2 by default when available. > It was introduced in GCC7. It caught a couple of bugs, but it does need a > bit of extra comments to explain when a switch case statement fall-through > is deliberate. Luckily with -Wimplicit-fallthrough=2 various existing > comments already do that. I have fixed the bugs, but adding explicit break > statements where necessary and added comments where the fall-through was > correct. > > There is just one place that produces a warning for a fallthrough that I am > not sure is deliberate or not. > > priv/guest_mips_toIR.c: In function ‘disInstr_MIPS_WRK’: > priv/guest_mips_toIR.c:30205:13: warning: this statement may fall through > [-Wimplicit-fallthrough=] > if (VEX_MIPS_COMP_ID(archinfo->hwcaps) == VEX_PRID_COMP_CAVIUM) { > ^ > priv/guest_mips_toIR.c:30211:7: note: here > case 0xC: /* INSV */ > ^~~~ This fallthrough is deliberate. I have made a comment in the git log to state this. Otherwise, changes in MIPS files look good to me. Thanks.
commit f04ae9f359411a2fff42f6c4b72fb8b0e56f5f58 Author: Mark Wielaard <mark@wildebeest.org> Date: Tue Mar 12 23:17:32 2019 +0100 Use gcc -Wimplicit-fallthrough=2 by default if available GCC 7 instroduced -Wimplicit-fallthrough https://developers.redhat.com/blog/2017/03/10/wimplicit-fallthrough-in-gcc-7 It caught a couple of bugs, but it does need a bit of extra comments to explain when a switch case statement fall-through is deliberate. Luckily with -Wimplicit-fallthrough=2 various existing comments already do that. I have fixed the bugs, but adding explicit break statements where necessary and added comments where the fall-through was correct. https://bugs.kde.org/show_bug.cgi?id=405430