| Message ID | cover.1697183081.git.manos.pitsidianakis@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Strict disable implicit fallthrough | expand |
On 13.10.23 09:47, Emmanouil Pitsidianakis wrote: > In preparation of raising -Wimplicit-fallthrough to 5, replace all > fall-through comments with the fallthrough attribute pseudo-keyword. > > Signed-off-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org> > --- Reviewed-by: David Hildenbrand <david@redhat.com>
On 10/13/23 09:47, Emmanouil Pitsidianakis wrote: > In preparation of raising -Wimplicit-fallthrough to 5, replace all > fall-through comments with the fallthrough attribute pseudo-keyword. > > Signed-off-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org> > --- > target/ppc/cpu_init.c | 8 ++++---- > target/ppc/excp_helper.c | 6 +++--- > target/ppc/mmu-radix64.c | 6 +++--- > target/ppc/mmu_common.c | 12 ++++++------ > target/ppc/translate.c | 6 +++--- > 5 files changed, 19 insertions(+), 19 deletions(-) Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C.
On Fri, Oct 13, 2023 at 10:47:04AM +0300, Emmanouil Pitsidianakis wrote: > > Main questions this RFC poses > ============================= > > - Is this change desirable and net-positive. Yes, IMHO it is worth standardizing on use of the attribute. The allowed use of comments was a nice thing by the compiler for coping with pre-existing code, but using the attribute is best long term for a consistent style. > - Should the `fallthrough;` pseudo-keyword be defined like in the Linux > kernel, or use glib's G_GNUC_FALLTHROUGH, or keep the already existing > QEMU_FALLTHROUGH macro. As a general rule, if glib provides functionality we aim o use that and not reinvent the wheel. IOW, we should just use G_GNUC_FALLTHROUGH. With regards, Daniel
On Fri, 13 Oct 2023, Emmanouil Pitsidianakis wrote: > Hello, > > This RFC is inspired by the kernel's move to -Wimplicit-fallthrough=3 > back in 2019.[0] > We take one step (or two) further by increasing it to 5 which rejects > fall through comments and requires an attribute statement. > > [0]: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a035d552a93b > > The line differences are not many, but they spread all over different > subsystems, architectures and devices. An attempt has been made to split > them in cohesive patches to aid post-RFC review. Part of the RFC is to > determine whether these patch divisions needs improvement. > > Main questions this RFC poses > ============================= > > - Is this change desirable and net-positive. IMO current fall through comments are OK. This new way does not improve it much because it adds one more peculiarity new people have to get used to. Adding a fall through comment when one gets a warning is something one easily can figure out but finding out there's a macro for that would need consulting docs. > - Should the `fallthrough;` pseudo-keyword be defined like in the Linux > kernel, or use glib's G_GNUC_FALLTHROUGH, or keep the already existing > QEMU_FALLTHROUGH macro. If there'w already QEMU_FALTHROUGH why not use that? Looks more obvious than a macro that looks like a strange function or keyword. Then a check could be added to checkpatch.pl to tell people to used QEMU_FALLTHROUGH istead of a fall through comment (matching the same regexp gcc accepts as others will already be checked by gcc) to enforce its usage. > - Should fallthrough comments be removed if they do not include extra > information. If this change is considered useful and not just code churn then replace comments, don't leave both as then the comment do not add any info. > Some external resources > ======================= > > See the RFC discussion in the kernel: > > https://lore.kernel.org/lkml/1d2830aadbe9d8151728a7df5b88528fc72a0095.1564549413.git.joe@perches.com/ > > The `fallthrough;` pseudo-keyword in the kernel source code: > > https://elixir.bootlin.com/linux/latest/C/ident/fallthrough > > In summary, I quote the doc comment and definition: > > /* > * Add the pseudo keyword 'fallthrough' so case statement blocks > * must end with any of these keywords: > * break; > * fallthrough; > * continue; > * goto <label>; > * return [expression]; > * > * gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes > */ > #if __has_attribute(__fallthrough__) > # define fallthrough __attribute__((__fallthrough__)) > #else > # define fallthrough do {} while (0) /* fallthrough */ Is the empty loop needed here? If the compiler does not mind a semicolon after a comment then there could be a semicolon as an empty statement there too. But maybe there are some cases where this would be needed, just curious why it's there in this case? Regards, BALATON Zoltan > #endif > > Background - Motivation > ======================= > > The C switch statement allows you to conditionally goto different labels > depending on a value. A break; statement conveniently goto's the end of > the switch. If a "case" does not end in a break, we say that the control > flow falls through the next case label, if any, implicitly. This can > lead to bugs and QEMU uses the GCC warning -Wimplicit-fallthrough to > prevent this. > > Currently, QEMU is built with -Wimplicit-fallthrough=2. This makes GCC's > static analyzer check for a case-insensitive matches of the .*falls?[ > \t-]*thr(ough|u).* regular expression. This means the following list of > comments taken from QEMU all disable the implicit fallthrough warning: > > - /* FALLTHRU */ > - /* fall through */ > - /* Fall through. */ > - /* Fall through... */ > - /* fall through if hEventTimeout is signaled */ > - /* FALL THROUGH */ > > To keep a constistent code style, this commit adds a macro `fallthrough` > that looks like a C keyword but expands to an attribute statement in > supported compilers (GCC at the moment). > > Note: there was already such a macro, QEMU_FALLTHROUGH, and it was used > only around 7 times in the code base. The first commit replaces it. > > Emmanouil Pitsidianakis (78): > include/qemu/compiler.h: replace QEMU_FALLTHROUGH with fallthrough > block: add fallthrough pseudo-keyword > fpu/softfloat: add fallthrough pseudo-keyword > qapi/opts-visitor: add fallthrough pseudo-keyword > qobject/json: add fallthrough pseudo-keyword > tcg: add fallthrough pseudo-keyword > hw/virtio/virtio-balloon.c: add fallthrough pseudo-keyword > hw/block: add fallthrough pseudo-keyword > hw/acpi/aml-build.c: add fallthrough pseudo-keyword > hw/ide/atapi.c: add fallthrough pseudo-keyword > hw/timer: add fallthrough pseudo-keyword > hw/usb: add fallthrough pseudo-keyword > hw/adc: add fallthrough pseudo-keyword > util/error-report.c: add fallthrough pseudo-keyword > accel/tcg: add fallthrough pseudo-keyword > audio: add fallthrough pseudo-keyword > ui/sdl2.c: add fallthrough pseudo-keyword > ui/win32-kbd-hook.c: add fallthrough pseudo-keyword > target/hppa: add fallthrough pseudo-keyword > target/mips: add fallthrough pseudo-keyword > target/sparc: add fallthrough pseudo-keyword > target/ppc: add fallthrough pseudo-keyword > target/arm: add fallthrough pseudo-keyword > target/alpha: add fallthrough pseudo-keyword > target/i386: add fallthrough pseudo-keyword > target/s390x: add fallthrough pseudo-keyword > target/riscv: add fallthrough pseudo-keyword > target/avr: add fallthrough pseudo-keyword > target/cris: add fallthrough pseudo-keyword > target/nios2: add fallthrough pseudo-keyword > target/xtensa: add fallthrough pseudo-keyword > target/m68k: add fallthrough pseudo-keyword > target/rx: add fallthrough pseudo-keyword > target/tricore: add fallthrough pseudo-keyword > target/sh4: add fallthrough pseudo-keyword > target/openrisc: add fallthrough pseudo-keyword > target/hexagon: add fallthrough pseudo-keyword > system/rtc.c: add fallthrough pseudo-keyword > hw/scsi: add fallthrough pseudo-keyword > hw/sd/sdhci.c: add fallthrough pseudo-keyword > linux-user: add fallthrough pseudo-keyword > hw/i386: add fallthrough pseudo-keyword > hw/misc: add fallthrough pseudo-keyword > hw/m68k/mcf_intc.c: add fallthrough pseudo-keyword > hw/dma: add fallthrough pseudo-keyword > disas: add fallthrough pseudo-keyword > contrib/rdmacm-mux: add fallthrough pseudo-keyword > contrib/vhost-user-scsi: add fallthrough pseudo-keyword > hw/arm: add fallthrough pseudo-keyword > hw/audio: add fallthrough pseudo-keyword > chardev: add fallthrough pseudo-keyword > hw/char: add fallthrough pseudo-keyword > nbd: add fallthrough pseudo-keyword > hw/core: add fallthrough pseudo-keyword > hw/display: add fallthrough pseudo-keyword > hw/input: add fallthrough pseudo-keyword > hw/net: add fallthrough pseudo-keyword > hw/ppc: add fallthrough pseudo-keyword > hw/intc: add fallthrough pseudo-keyword > qga: add fallthrough pseudo-keyword > semihosting: add fallthrough pseudo-keyword > hw/gpio: add fallthrough pseudo-keyword > hw/ipmi: add fallthrough pseudo-keyword > hw/mips: add fallthrough pseudo-keyword > hw/nvme: add fallthrough pseudo-keyword > hw/nvram/eeprom_at24c.c: add fallthrough pseudo-keyword > hw/pci-host/pnv_phb3.c: add fallthrough pseudo-keyword > hw/pci: add fallthrough pseudo-keyword > hw/rdma/rdma_backend.c: add fallthrough pseudo-keyword > hw/rtc: add fallthrough pseudo-keyword > hw/s390x: add fallthrough pseudo-keyword > hw/ssi: add fallthrough pseudo-keyword > hw/watchdog/wdt_diag288.c: add fallthrough pseudo-keyword > hw/cxl/cxl-device-utils.c: add fallthrough pseudo-keyword > migration: add fallthrough pseudo-keyword > qemu-img.c: add fallthrough pseudo-keyword > tests/unit/test-char.c: add fallthrough pseudo-keyword > meson.build: increase -Wimplicit-fallthrough to 5 > > accel/tcg/cputlb.c | 4 +- > accel/tcg/ldst_atomicity.c.inc | 2 +- > accel/tcg/plugin-gen.c | 2 +- > audio/audio.c | 16 ++-- > audio/jackaudio.c | 4 +- > audio/pwaudio.c | 12 +-- > block/block-copy.c | 1 + > block/file-posix.c | 1 + > block/io.c | 1 + > block/iscsi.c | 1 + > block/qcow2-cluster.c | 5 +- > block/vhdx.c | 17 +++- > chardev/char-socket.c | 2 +- > contrib/rdmacm-mux/main.c | 10 +-- > contrib/vhost-user-scsi/vhost-user-scsi.c | 3 +- > disas/hppa.c | 4 +- > disas/m68k.c | 2 +- > disas/sh4.c | 6 +- > disas/sparc.c | 2 +- > docs/devel/style.rst | 23 +++++ > fpu/softfloat-parts.c.inc | 8 +- > fpu/softfloat.c | 7 +- > hw/acpi/aml-build.c | 6 +- > hw/adc/aspeed_adc.c | 12 +-- > hw/adc/zynq-xadc.c | 2 +- > hw/arm/omap1.c | 8 +- > hw/arm/pxa2xx.c | 6 +- > hw/arm/smmuv3.c | 2 +- > hw/arm/stellaris.c | 1 + > hw/audio/asc.c | 2 +- > hw/audio/cs4231a.c | 2 +- > hw/audio/gusemu_hal.c | 2 +- > hw/block/dataplane/xen-block.c | 4 +- > hw/block/m25p80.c | 2 +- > hw/block/onenand.c | 2 +- > hw/block/pflash_cfi01.c | 1 + > hw/block/pflash_cfi02.c | 6 +- > hw/char/nrf51_uart.c | 4 +- > hw/core/loader.c | 2 +- > hw/cxl/cxl-device-utils.c | 4 +- > hw/display/cg3.c | 2 +- > hw/display/cirrus_vga.c | 2 +- > hw/display/tcx.c | 4 +- > hw/dma/omap_dma.c | 32 +++---- > hw/dma/pxa2xx_dma.c | 4 +- > hw/dma/sparc32_dma.c | 2 +- > hw/gpio/omap_gpio.c | 2 +- > hw/i2c/bitbang_i2c.c | 2 +- > hw/i386/intel_iommu.c | 4 +- > hw/i386/kvm/xen_evtchn.c | 2 +- > hw/i386/x86.c | 2 +- > hw/ide/atapi.c | 1 + > hw/input/hid.c | 3 +- > hw/input/tsc2005.c | 4 +- > hw/input/tsc210x.c | 2 +- > hw/intc/apic.c | 2 +- > hw/intc/arm_gicv3_kvm.c | 16 ++-- > hw/intc/armv7m_nvic.c | 12 +-- > hw/intc/xilinx_intc.c | 2 +- > hw/ipmi/ipmi_bmc_extern.c | 2 +- > hw/ipmi/smbus_ipmi.c | 4 +- > hw/m68k/mcf_intc.c | 2 +- > hw/mips/boston.c | 12 +-- > hw/misc/a9scu.c | 2 + > hw/misc/aspeed_scu.c | 2 +- > hw/misc/bcm2835_property.c | 12 +-- > hw/misc/mos6522.c | 4 +- > hw/net/cadence_gem.c | 4 +- > hw/net/can/can_sja1000.c | 4 +- > hw/net/igb_core.c | 2 +- > hw/net/igbvf.c | 2 +- > hw/net/imx_fec.c | 2 +- > hw/net/net_rx_pkt.c | 2 +- > hw/net/pcnet.c | 2 +- > hw/net/rtl8139.c | 6 +- > hw/net/xilinx_ethlite.c | 2 +- > hw/nvme/ctrl.c | 24 +++--- > hw/nvme/dif.c | 4 +- > hw/nvram/eeprom_at24c.c | 2 +- > hw/pci-host/pnv_phb3.c | 2 +- > hw/pci/pcie_aer.c | 3 +- > hw/pci/pcie_doe.c | 2 +- > hw/ppc/pnv_bmc.c | 2 +- > hw/ppc/spapr_events.c | 1 + > hw/rdma/rdma_backend.c | 2 +- > hw/rtc/aspeed_rtc.c | 4 +- > hw/rtc/mc146818rtc.c | 4 +- > hw/s390x/ipl.c | 1 + > hw/s390x/s390-pci-inst.c | 4 +- > hw/s390x/sclp.c | 4 +- > hw/scsi/esp.c | 2 +- > hw/scsi/megasas.c | 2 +- > hw/scsi/scsi-bus.c | 4 +- > hw/scsi/scsi-disk.c | 2 +- > hw/sd/sdhci.c | 8 +- > hw/ssi/npcm7xx_fiu.c | 14 +-- > hw/ssi/omap_spi.c | 48 +++++------ > hw/timer/a9gtimer.c | 8 +- > hw/timer/aspeed_timer.c | 1 + > hw/timer/pxa2xx_timer.c | 94 ++++++++++----------- > hw/timer/renesas_tmr.c | 2 +- > hw/timer/sh_timer.c | 8 +- > hw/usb/dev-mtp.c | 2 +- > hw/usb/dev-wacom.c | 2 +- > hw/usb/hcd-ehci.c | 4 +- > hw/usb/hcd-xhci.c | 4 +- > hw/usb/redirect.c | 4 +- > hw/usb/tusb6010.c | 2 +- > hw/virtio/virtio-balloon.c | 1 + > hw/watchdog/wdt_diag288.c | 2 +- > include/qemu/compiler.h | 30 +++++-- > include/qemu/osdep.h | 4 +- > linux-user/mips/cpu_loop.c | 8 +- > linux-user/mmap.c | 2 +- > linux-user/syscall.c | 2 +- > meson.build | 2 +- > migration/migration.c | 2 +- > nbd/client.c | 4 +- > nbd/common.c | 2 +- > qapi/opts-visitor.c | 1 + > qapi/string-input-visitor.c | 4 +- > qemu-img.c | 2 +- > qemu-nbd.c | 4 +- > qga/main.c | 2 +- > qga/vss-win32/requester.cpp | 1 + > qobject/json-lexer.c | 4 +- > qobject/json-parser.c | 5 +- > semihosting/arm-compat-semi.c | 2 +- > system/rtc.c | 2 +- > target/alpha/helper.c | 6 +- > target/alpha/translate.c | 4 +- > target/arm/helper.c | 34 ++++---- > target/arm/ptw.c | 10 +-- > target/arm/tcg/psci.c | 2 +- > target/arm/tcg/translate-a64.c | 76 ++++++++--------- > target/arm/tcg/translate-m-nocp.c | 2 +- > target/arm/tcg/translate-vfp.c | 2 +- > target/arm/tcg/translate.c | 8 +- > target/avr/translate.c | 4 +- > target/cris/translate.c | 4 +- > target/hexagon/idef-parser/parser-helpers.c | 5 +- > target/hppa/translate.c | 10 +-- > target/i386/cpu.c | 2 +- > target/i386/hvf/x86_decode.c | 1 + > target/i386/kvm/kvm.c | 4 +- > target/i386/tcg/decode-new.c.inc | 6 +- > target/i386/tcg/emit.c.inc | 2 +- > target/i386/tcg/translate.c | 11 +-- > target/loongarch/cpu.c | 4 +- > target/loongarch/translate.c | 2 +- > target/m68k/op_helper.c | 3 +- > target/m68k/translate.c | 10 +-- > target/mips/sysemu/physaddr.c | 2 +- > target/mips/tcg/micromips_translate.c.inc | 4 +- > target/mips/tcg/mips16e_translate.c.inc | 30 +++---- > target/mips/tcg/mxu_translate.c | 8 +- > target/mips/tcg/nanomips_translate.c.inc | 4 +- > target/mips/tcg/op_helper.c | 2 +- > target/mips/tcg/translate.c | 79 ++++++++--------- > target/nios2/helper.c | 6 +- > target/nios2/translate.c | 2 +- > target/openrisc/mmu.c | 2 +- > target/openrisc/translate.c | 2 +- > target/ppc/cpu_init.c | 8 +- > target/ppc/excp_helper.c | 6 +- > target/ppc/mmu-radix64.c | 6 +- > target/ppc/mmu_common.c | 12 +-- > target/ppc/translate.c | 6 +- > target/riscv/insn_trans/trans_rvi.c.inc | 2 +- > target/riscv/insn_trans/trans_rvzce.c.inc | 22 ++--- > target/riscv/translate.c | 4 +- > target/rx/translate.c | 2 +- > target/s390x/cpu.c | 4 +- > target/s390x/kvm/kvm.c | 2 +- > target/s390x/mmu_helper.c | 6 +- > target/s390x/tcg/translate.c | 18 ++-- > target/s390x/tcg/translate_vx.c.inc | 2 +- > target/sh4/helper.c | 2 +- > target/sparc/ldst_helper.c | 4 +- > target/sparc/mmu_helper.c | 6 +- > target/sparc/translate.c | 3 +- > target/sparc/win_helper.c | 1 + > target/tricore/translate.c | 4 +- > target/xtensa/op_helper.c | 8 +- > target/xtensa/translate.c | 2 +- > tcg/aarch64/tcg-target.c.inc | 15 +++- > tcg/arm/tcg-target.c.inc | 5 +- > tcg/i386/tcg-target.c.inc | 20 +++-- > tcg/loongarch64/tcg-target.c.inc | 4 +- > tcg/mips/tcg-target.c.inc | 8 +- > tcg/optimize.c | 8 +- > tcg/ppc/tcg-target.c.inc | 19 +++-- > tcg/riscv/tcg-target.c.inc | 5 +- > tcg/s390x/tcg-target.c.inc | 8 +- > tcg/tcg-op-gvec.c | 24 +++--- > tcg/tcg-op-ldst.c | 2 +- > tcg/tcg.c | 24 +++--- > tcg/tci.c | 2 +- > tcg/tci/tcg-target.c.inc | 2 +- > tests/unit/test-char.c | 2 +- > ui/sdl2.c | 2 +- > ui/win32-kbd-hook.c | 7 -- > util/error-report.c | 2 +- > 203 files changed, 747 insertions(+), 618 deletions(-) > > > base-commit: cea3ea670fe265421131aad90c36fbb87bc4d206 >
Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org> writes: > Hello, > > This RFC is inspired by the kernel's move to -Wimplicit-fallthrough=3 > back in 2019.[0] > We take one step (or two) further by increasing it to 5 which rejects > fall through comments and requires an attribute statement. > > [0]: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a035d552a93b > > The line differences are not many, but they spread all over different > subsystems, architectures and devices. An attempt has been made to split > them in cohesive patches to aid post-RFC review. Part of the RFC is to > determine whether these patch divisions needs improvement. > > Main questions this RFC poses > ============================= > > - Is this change desirable and net-positive. Unwanted fallthrough is an easy mistake to make, and -Wimplicit-fallthrough=N helps avoid it. The question is how far up we need to push N. Right now we're at N=2. Has unwanted fallthrough been a problem? > - Should the `fallthrough;` pseudo-keyword be defined like in the Linux > kernel, or use glib's G_GNUC_FALLTHROUGH, or keep the already existing > QEMU_FALLTHROUGH macro. > - Should fallthrough comments be removed if they do not include extra > information. Valid questions, but they don't need answers until after picking our N. [...]
On Fri, 13 Oct 2023 11:14, "Daniel P. Berrangé" <berrange@redhat.com> wrote: >On Fri, Oct 13, 2023 at 10:47:04AM +0300, Emmanouil Pitsidianakis wrote: >> >> Main questions this RFC poses >> ============================= >> >> - Is this change desirable and net-positive. > >Yes, IMHO it is worth standardizing on use of the attribute. The allowed >use of comments was a nice thing by the compiler for coping with pre-existing >code, but using the attribute is best long term for a consistent style. > >> - Should the `fallthrough;` pseudo-keyword be defined like in the Linux >> kernel, or use glib's G_GNUC_FALLTHROUGH, or keep the already existing >> QEMU_FALLTHROUGH macro. > >As a general rule, if glib provides functionality we aim o use that >and not reinvent the wheel. IOW, we should just use G_GNUC_FALLTHROUGH. I agree. My reasoning was: - The reinvented wheel is only an attribute and not a big bunch of NIH code - The macro def in glib depends on the glib version you use - G_GNUC_FALLTHROUGH looks kind of abrasive to my eye, while `fallthrough` blends in with other switch keywords like break. - C23 standardises fallthrough. We might not ever support C23 but it's good to be consistent with standards and other, larger projects (linux kernel). I think these (except for myself finding G_GNUC_FALLTHROUGH ugly) make a strong case for not using the glib macro, personally. I'd be interested to know if there is a counterpoint to it: because I don't want this change to cause problems in the future. Manos
On Fri, Oct 13, 2023 at 03:51:22PM +0300, Manos Pitsidianakis wrote: > On Fri, 13 Oct 2023 11:14, "Daniel P. Berrangé" <berrange@redhat.com> wrote: > > On Fri, Oct 13, 2023 at 10:47:04AM +0300, Emmanouil Pitsidianakis wrote: > > > > > > Main questions this RFC poses > > > ============================= > > > > > > - Is this change desirable and net-positive. > > > > Yes, IMHO it is worth standardizing on use of the attribute. The allowed > > use of comments was a nice thing by the compiler for coping with pre-existing > > code, but using the attribute is best long term for a consistent style. > > > > > - Should the `fallthrough;` pseudo-keyword be defined like in the Linux > > > kernel, or use glib's G_GNUC_FALLTHROUGH, or keep the already existing > > > QEMU_FALLTHROUGH macro. > > > > As a general rule, if glib provides functionality we aim o use that > > and not reinvent the wheel. IOW, we should just use G_GNUC_FALLTHROUGH. > > I agree. My reasoning was: > > - The reinvented wheel is only an attribute and not a big bunch of NIH code It isn't just about the amount of code, it is the familiarity of the code. QEMU's codebase is glib based, by using glib functionality we leverage developers' general familiarity with / knowledge of glib, as opposed to custom QEMU approaches which need learning. > - The macro def in glib depends on the glib version you use If we need to depend on something that is newer than our min glib version, then we add back compat definitino to 'include/glib-compat.h' for the older version. > - G_GNUC_FALLTHROUGH looks kind of abrasive to my eye, while `fallthrough` > blends in with other switch keywords like break. My impression seeing this series, was exactly the opposite. I did not like 'fallthrough' blending in with the rest of code, and also did not like that it is a macro in lowercase, thus hiding that it is a macro > - C23 standardises fallthrough. We might not ever support C23 but it's good > to be consistent with standards and other, larger projects (linux kernel). We're at least a decade away from being able to use anything from C23. With regards, Daniel
On Fri, 13 Oct 2023 at 13:42, Markus Armbruster <armbru@redhat.com> wrote: > > Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org> writes: > > > Hello, > > > > This RFC is inspired by the kernel's move to -Wimplicit-fallthrough=3 > > back in 2019.[0] > > We take one step (or two) further by increasing it to 5 which rejects > > fall through comments and requires an attribute statement. > > > > [0]: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a035d552a93b > > > > The line differences are not many, but they spread all over different > > subsystems, architectures and devices. An attempt has been made to split > > them in cohesive patches to aid post-RFC review. Part of the RFC is to > > determine whether these patch divisions needs improvement. > > > > Main questions this RFC poses > > ============================= > > > > - Is this change desirable and net-positive. > > Unwanted fallthrough is an easy mistake to make, and > -Wimplicit-fallthrough=N helps avoid it. The question is how far up we > need to push N. Right now we're at N=2. Has unwanted fallthrough been > a problem? Mmm, this is my opinion I think. We have a mechanism for catching "forgot the 'break'" already (our =2 setting) and a way to say "intentional" in a fairly natural way (add the comment). Does pushing N up any further gain us anything except a load of churn? Also, the compiler is not the only thing that processes our code: Coverity also looks for "unexpected fallthrough" issues, so if we wanted to switch away from our current practice we should check whether what we're switching to is an idiom that Coverity recognises. thanks -- PMM
Hello Peter, On Mon, 16 Oct 2023, 17:13 Peter Maydell, <peter.maydell@linaro.org> wrote: > On Fri, 13 Oct 2023 at 13:42, Markus Armbruster <armbru@redhat.com> wrote: > > > > Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org> writes: > > > > > Hello, > > > > > > This RFC is inspired by the kernel's move to -Wimplicit-fallthrough=3 > > > back in 2019.[0] > > > We take one step (or two) further by increasing it to 5 which rejects > > > fall through comments and requires an attribute statement. > > > > > > [0]: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a035d552a93b > > > > > > The line differences are not many, but they spread all over different > > > subsystems, architectures and devices. An attempt has been made to > split > > > them in cohesive patches to aid post-RFC review. Part of the RFC is to > > > determine whether these patch divisions needs improvement. > > > > > > Main questions this RFC poses > > > ============================= > > > > > > - Is this change desirable and net-positive. > > > > Unwanted fallthrough is an easy mistake to make, and > > -Wimplicit-fallthrough=N helps avoid it. The question is how far up we > > need to push N. Right now we're at N=2. Has unwanted fallthrough been > > a problem? > > Mmm, this is my opinion I think. We have a mechanism for > catching "forgot the 'break'" already (our =2 setting) and > a way to say "intentional" in a fairly natural way (add the > comment). Does pushing N up any further gain us anything > except a load of churn? > > Also, the compiler is not the only thing that processes our > code: Coverity also looks for "unexpected fallthrough" issues, > so if we wanted to switch away from our current practice we > should check whether what we're switching to is an idiom > that Coverity recognises. > It is a code style change as the cover letter mentions, it's not related to the static analysis itself. -- Manos >
On Mon, 16 Oct 2023 at 15:58, Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote: > > Hello Peter, > > On Mon, 16 Oct 2023, 17:13 Peter Maydell, <peter.maydell@linaro.org> wrote: >> >> On Fri, 13 Oct 2023 at 13:42, Markus Armbruster <armbru@redhat.com> wrote: >> > >> > Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org> writes: >> > >> > > Hello, >> > > >> > > This RFC is inspired by the kernel's move to -Wimplicit-fallthrough=3 >> > > back in 2019.[0] >> > > We take one step (or two) further by increasing it to 5 which rejects >> > > fall through comments and requires an attribute statement. >> > > >> > > [0]: >> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a035d552a93b >> > > >> > > The line differences are not many, but they spread all over different >> > > subsystems, architectures and devices. An attempt has been made to split >> > > them in cohesive patches to aid post-RFC review. Part of the RFC is to >> > > determine whether these patch divisions needs improvement. >> > > >> > > Main questions this RFC poses >> > > ============================= >> > > >> > > - Is this change desirable and net-positive. >> > >> > Unwanted fallthrough is an easy mistake to make, and >> > -Wimplicit-fallthrough=N helps avoid it. The question is how far up we >> > need to push N. Right now we're at N=2. Has unwanted fallthrough been >> > a problem? >> >> Mmm, this is my opinion I think. We have a mechanism for >> catching "forgot the 'break'" already (our =2 setting) and >> a way to say "intentional" in a fairly natural way (add the >> comment). Does pushing N up any further gain us anything >> except a load of churn? >> >> Also, the compiler is not the only thing that processes our >> code: Coverity also looks for "unexpected fallthrough" issues, >> so if we wanted to switch away from our current practice we >> should check whether what we're switching to is an idiom >> that Coverity recognises. > > > It is a code style change as the cover letter mentions, it's not related to the static analysis itself. Yes, exactly. As a code style change it needs a fairly high level of justification for the code churn, and the cover letter doesn't really provide one... thanks -- PMM
On Mon, 16 Oct 2023, 18:04 Peter Maydell, <peter.maydell@linaro.org> wrote: > On Mon, 16 Oct 2023 at 15:58, Manos Pitsidianakis > <manos.pitsidianakis@linaro.org> wrote: > > > > Hello Peter, > > > > On Mon, 16 Oct 2023, 17:13 Peter Maydell, <peter.maydell@linaro.org> > wrote: > >> > >> On Fri, 13 Oct 2023 at 13:42, Markus Armbruster <armbru@redhat.com> > wrote: > >> > > >> > Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org> writes: > >> > > >> > > Hello, > >> > > > >> > > This RFC is inspired by the kernel's move to > -Wimplicit-fallthrough=3 > >> > > back in 2019.[0] > >> > > We take one step (or two) further by increasing it to 5 which > rejects > >> > > fall through comments and requires an attribute statement. > >> > > > >> > > [0]: > >> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a035d552a93b > >> > > > >> > > The line differences are not many, but they spread all over > different > >> > > subsystems, architectures and devices. An attempt has been made to > split > >> > > them in cohesive patches to aid post-RFC review. Part of the RFC is > to > >> > > determine whether these patch divisions needs improvement. > >> > > > >> > > Main questions this RFC poses > >> > > ============================= > >> > > > >> > > - Is this change desirable and net-positive. > >> > > >> > Unwanted fallthrough is an easy mistake to make, and > >> > -Wimplicit-fallthrough=N helps avoid it. The question is how far up > we > >> > need to push N. Right now we're at N=2. Has unwanted fallthrough > been > >> > a problem? > >> > >> Mmm, this is my opinion I think. We have a mechanism for > >> catching "forgot the 'break'" already (our =2 setting) and > >> a way to say "intentional" in a fairly natural way (add the > >> comment). Does pushing N up any further gain us anything > >> except a load of churn? > >> > >> Also, the compiler is not the only thing that processes our > >> code: Coverity also looks for "unexpected fallthrough" issues, > >> so if we wanted to switch away from our current practice we > >> should check whether what we're switching to is an idiom > >> that Coverity recognises. > > > > > > It is a code style change as the cover letter mentions, it's not related > to the static analysis itself. > > Yes, exactly. As a code style change it needs a fairly high level > of justification for the code churn, and the cover letter > doesn't really provide one... > As I state in the cover letter, I personally find that using one macro instead of a comment regex feels more consistent. But your view is valid as well! Let's consider the RFC retracted then. -- Manos >
Hello, This RFC is inspired by the kernel's move to -Wimplicit-fallthrough=3 back in 2019.[0] We take one step (or two) further by increasing it to 5 which rejects fall through comments and requires an attribute statement. [0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a035d552a93b The line differences are not many, but they spread all over different subsystems, architectures and devices. An attempt has been made to split them in cohesive patches to aid post-RFC review. Part of the RFC is to determine whether these patch divisions needs improvement. Main questions this RFC poses ============================= - Is this change desirable and net-positive. - Should the `fallthrough;` pseudo-keyword be defined like in the Linux kernel, or use glib's G_GNUC_FALLTHROUGH, or keep the already existing QEMU_FALLTHROUGH macro. - Should fallthrough comments be removed if they do not include extra information. Some external resources ======================= See the RFC discussion in the kernel: https://lore.kernel.org/lkml/1d2830aadbe9d8151728a7df5b88528fc72a0095.1564549413.git.joe@perches.com/ The `fallthrough;` pseudo-keyword in the kernel source code: https://elixir.bootlin.com/linux/latest/C/ident/fallthrough In summary, I quote the doc comment and definition: /* * Add the pseudo keyword 'fallthrough' so case statement blocks * must end with any of these keywords: * break; * fallthrough; * continue; * goto <label>; * return [expression]; * * gcc: https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html#Statement-Attributes */ #if __has_attribute(__fallthrough__) # define fallthrough __attribute__((__fallthrough__)) #else # define fallthrough do {} while (0) /* fallthrough */ #endif Background - Motivation ======================= The C switch statement allows you to conditionally goto different labels depending on a value. A break; statement conveniently goto's the end of the switch. If a "case" does not end in a break, we say that the control flow falls through the next case label, if any, implicitly. This can lead to bugs and QEMU uses the GCC warning -Wimplicit-fallthrough to prevent this. Currently, QEMU is built with -Wimplicit-fallthrough=2. This makes GCC's static analyzer check for a case-insensitive matches of the .*falls?[ \t-]*thr(ough|u).* regular expression. This means the following list of comments taken from QEMU all disable the implicit fallthrough warning: - /* FALLTHRU */ - /* fall through */ - /* Fall through. */ - /* Fall through... */ - /* fall through if hEventTimeout is signaled */ - /* FALL THROUGH */ To keep a constistent code style, this commit adds a macro `fallthrough` that looks like a C keyword but expands to an attribute statement in supported compilers (GCC at the moment). Note: there was already such a macro, QEMU_FALLTHROUGH, and it was used only around 7 times in the code base. The first commit replaces it. Emmanouil Pitsidianakis (78): include/qemu/compiler.h: replace QEMU_FALLTHROUGH with fallthrough block: add fallthrough pseudo-keyword fpu/softfloat: add fallthrough pseudo-keyword qapi/opts-visitor: add fallthrough pseudo-keyword qobject/json: add fallthrough pseudo-keyword tcg: add fallthrough pseudo-keyword hw/virtio/virtio-balloon.c: add fallthrough pseudo-keyword hw/block: add fallthrough pseudo-keyword hw/acpi/aml-build.c: add fallthrough pseudo-keyword hw/ide/atapi.c: add fallthrough pseudo-keyword hw/timer: add fallthrough pseudo-keyword hw/usb: add fallthrough pseudo-keyword hw/adc: add fallthrough pseudo-keyword util/error-report.c: add fallthrough pseudo-keyword accel/tcg: add fallthrough pseudo-keyword audio: add fallthrough pseudo-keyword ui/sdl2.c: add fallthrough pseudo-keyword ui/win32-kbd-hook.c: add fallthrough pseudo-keyword target/hppa: add fallthrough pseudo-keyword target/mips: add fallthrough pseudo-keyword target/sparc: add fallthrough pseudo-keyword target/ppc: add fallthrough pseudo-keyword target/arm: add fallthrough pseudo-keyword target/alpha: add fallthrough pseudo-keyword target/i386: add fallthrough pseudo-keyword target/s390x: add fallthrough pseudo-keyword target/riscv: add fallthrough pseudo-keyword target/avr: add fallthrough pseudo-keyword target/cris: add fallthrough pseudo-keyword target/nios2: add fallthrough pseudo-keyword target/xtensa: add fallthrough pseudo-keyword target/m68k: add fallthrough pseudo-keyword target/rx: add fallthrough pseudo-keyword target/tricore: add fallthrough pseudo-keyword target/sh4: add fallthrough pseudo-keyword target/openrisc: add fallthrough pseudo-keyword target/hexagon: add fallthrough pseudo-keyword system/rtc.c: add fallthrough pseudo-keyword hw/scsi: add fallthrough pseudo-keyword hw/sd/sdhci.c: add fallthrough pseudo-keyword linux-user: add fallthrough pseudo-keyword hw/i386: add fallthrough pseudo-keyword hw/misc: add fallthrough pseudo-keyword hw/m68k/mcf_intc.c: add fallthrough pseudo-keyword hw/dma: add fallthrough pseudo-keyword disas: add fallthrough pseudo-keyword contrib/rdmacm-mux: add fallthrough pseudo-keyword contrib/vhost-user-scsi: add fallthrough pseudo-keyword hw/arm: add fallthrough pseudo-keyword hw/audio: add fallthrough pseudo-keyword chardev: add fallthrough pseudo-keyword hw/char: add fallthrough pseudo-keyword nbd: add fallthrough pseudo-keyword hw/core: add fallthrough pseudo-keyword hw/display: add fallthrough pseudo-keyword hw/input: add fallthrough pseudo-keyword hw/net: add fallthrough pseudo-keyword hw/ppc: add fallthrough pseudo-keyword hw/intc: add fallthrough pseudo-keyword qga: add fallthrough pseudo-keyword semihosting: add fallthrough pseudo-keyword hw/gpio: add fallthrough pseudo-keyword hw/ipmi: add fallthrough pseudo-keyword hw/mips: add fallthrough pseudo-keyword hw/nvme: add fallthrough pseudo-keyword hw/nvram/eeprom_at24c.c: add fallthrough pseudo-keyword hw/pci-host/pnv_phb3.c: add fallthrough pseudo-keyword hw/pci: add fallthrough pseudo-keyword hw/rdma/rdma_backend.c: add fallthrough pseudo-keyword hw/rtc: add fallthrough pseudo-keyword hw/s390x: add fallthrough pseudo-keyword hw/ssi: add fallthrough pseudo-keyword hw/watchdog/wdt_diag288.c: add fallthrough pseudo-keyword hw/cxl/cxl-device-utils.c: add fallthrough pseudo-keyword migration: add fallthrough pseudo-keyword qemu-img.c: add fallthrough pseudo-keyword tests/unit/test-char.c: add fallthrough pseudo-keyword meson.build: increase -Wimplicit-fallthrough to 5 accel/tcg/cputlb.c | 4 +- accel/tcg/ldst_atomicity.c.inc | 2 +- accel/tcg/plugin-gen.c | 2 +- audio/audio.c | 16 ++-- audio/jackaudio.c | 4 +- audio/pwaudio.c | 12 +-- block/block-copy.c | 1 + block/file-posix.c | 1 + block/io.c | 1 + block/iscsi.c | 1 + block/qcow2-cluster.c | 5 +- block/vhdx.c | 17 +++- chardev/char-socket.c | 2 +- contrib/rdmacm-mux/main.c | 10 +-- contrib/vhost-user-scsi/vhost-user-scsi.c | 3 +- disas/hppa.c | 4 +- disas/m68k.c | 2 +- disas/sh4.c | 6 +- disas/sparc.c | 2 +- docs/devel/style.rst | 23 +++++ fpu/softfloat-parts.c.inc | 8 +- fpu/softfloat.c | 7 +- hw/acpi/aml-build.c | 6 +- hw/adc/aspeed_adc.c | 12 +-- hw/adc/zynq-xadc.c | 2 +- hw/arm/omap1.c | 8 +- hw/arm/pxa2xx.c | 6 +- hw/arm/smmuv3.c | 2 +- hw/arm/stellaris.c | 1 + hw/audio/asc.c | 2 +- hw/audio/cs4231a.c | 2 +- hw/audio/gusemu_hal.c | 2 +- hw/block/dataplane/xen-block.c | 4 +- hw/block/m25p80.c | 2 +- hw/block/onenand.c | 2 +- hw/block/pflash_cfi01.c | 1 + hw/block/pflash_cfi02.c | 6 +- hw/char/nrf51_uart.c | 4 +- hw/core/loader.c | 2 +- hw/cxl/cxl-device-utils.c | 4 +- hw/display/cg3.c | 2 +- hw/display/cirrus_vga.c | 2 +- hw/display/tcx.c | 4 +- hw/dma/omap_dma.c | 32 +++---- hw/dma/pxa2xx_dma.c | 4 +- hw/dma/sparc32_dma.c | 2 +- hw/gpio/omap_gpio.c | 2 +- hw/i2c/bitbang_i2c.c | 2 +- hw/i386/intel_iommu.c | 4 +- hw/i386/kvm/xen_evtchn.c | 2 +- hw/i386/x86.c | 2 +- hw/ide/atapi.c | 1 + hw/input/hid.c | 3 +- hw/input/tsc2005.c | 4 +- hw/input/tsc210x.c | 2 +- hw/intc/apic.c | 2 +- hw/intc/arm_gicv3_kvm.c | 16 ++-- hw/intc/armv7m_nvic.c | 12 +-- hw/intc/xilinx_intc.c | 2 +- hw/ipmi/ipmi_bmc_extern.c | 2 +- hw/ipmi/smbus_ipmi.c | 4 +- hw/m68k/mcf_intc.c | 2 +- hw/mips/boston.c | 12 +-- hw/misc/a9scu.c | 2 + hw/misc/aspeed_scu.c | 2 +- hw/misc/bcm2835_property.c | 12 +-- hw/misc/mos6522.c | 4 +- hw/net/cadence_gem.c | 4 +- hw/net/can/can_sja1000.c | 4 +- hw/net/igb_core.c | 2 +- hw/net/igbvf.c | 2 +- hw/net/imx_fec.c | 2 +- hw/net/net_rx_pkt.c | 2 +- hw/net/pcnet.c | 2 +- hw/net/rtl8139.c | 6 +- hw/net/xilinx_ethlite.c | 2 +- hw/nvme/ctrl.c | 24 +++--- hw/nvme/dif.c | 4 +- hw/nvram/eeprom_at24c.c | 2 +- hw/pci-host/pnv_phb3.c | 2 +- hw/pci/pcie_aer.c | 3 +- hw/pci/pcie_doe.c | 2 +- hw/ppc/pnv_bmc.c | 2 +- hw/ppc/spapr_events.c | 1 + hw/rdma/rdma_backend.c | 2 +- hw/rtc/aspeed_rtc.c | 4 +- hw/rtc/mc146818rtc.c | 4 +- hw/s390x/ipl.c | 1 + hw/s390x/s390-pci-inst.c | 4 +- hw/s390x/sclp.c | 4 +- hw/scsi/esp.c | 2 +- hw/scsi/megasas.c | 2 +- hw/scsi/scsi-bus.c | 4 +- hw/scsi/scsi-disk.c | 2 +- hw/sd/sdhci.c | 8 +- hw/ssi/npcm7xx_fiu.c | 14 +-- hw/ssi/omap_spi.c | 48 +++++------ hw/timer/a9gtimer.c | 8 +- hw/timer/aspeed_timer.c | 1 + hw/timer/pxa2xx_timer.c | 94 ++++++++++----------- hw/timer/renesas_tmr.c | 2 +- hw/timer/sh_timer.c | 8 +- hw/usb/dev-mtp.c | 2 +- hw/usb/dev-wacom.c | 2 +- hw/usb/hcd-ehci.c | 4 +- hw/usb/hcd-xhci.c | 4 +- hw/usb/redirect.c | 4 +- hw/usb/tusb6010.c | 2 +- hw/virtio/virtio-balloon.c | 1 + hw/watchdog/wdt_diag288.c | 2 +- include/qemu/compiler.h | 30 +++++-- include/qemu/osdep.h | 4 +- linux-user/mips/cpu_loop.c | 8 +- linux-user/mmap.c | 2 +- linux-user/syscall.c | 2 +- meson.build | 2 +- migration/migration.c | 2 +- nbd/client.c | 4 +- nbd/common.c | 2 +- qapi/opts-visitor.c | 1 + qapi/string-input-visitor.c | 4 +- qemu-img.c | 2 +- qemu-nbd.c | 4 +- qga/main.c | 2 +- qga/vss-win32/requester.cpp | 1 + qobject/json-lexer.c | 4 +- qobject/json-parser.c | 5 +- semihosting/arm-compat-semi.c | 2 +- system/rtc.c | 2 +- target/alpha/helper.c | 6 +- target/alpha/translate.c | 4 +- target/arm/helper.c | 34 ++++---- target/arm/ptw.c | 10 +-- target/arm/tcg/psci.c | 2 +- target/arm/tcg/translate-a64.c | 76 ++++++++--------- target/arm/tcg/translate-m-nocp.c | 2 +- target/arm/tcg/translate-vfp.c | 2 +- target/arm/tcg/translate.c | 8 +- target/avr/translate.c | 4 +- target/cris/translate.c | 4 +- target/hexagon/idef-parser/parser-helpers.c | 5 +- target/hppa/translate.c | 10 +-- target/i386/cpu.c | 2 +- target/i386/hvf/x86_decode.c | 1 + target/i386/kvm/kvm.c | 4 +- target/i386/tcg/decode-new.c.inc | 6 +- target/i386/tcg/emit.c.inc | 2 +- target/i386/tcg/translate.c | 11 +-- target/loongarch/cpu.c | 4 +- target/loongarch/translate.c | 2 +- target/m68k/op_helper.c | 3 +- target/m68k/translate.c | 10 +-- target/mips/sysemu/physaddr.c | 2 +- target/mips/tcg/micromips_translate.c.inc | 4 +- target/mips/tcg/mips16e_translate.c.inc | 30 +++---- target/mips/tcg/mxu_translate.c | 8 +- target/mips/tcg/nanomips_translate.c.inc | 4 +- target/mips/tcg/op_helper.c | 2 +- target/mips/tcg/translate.c | 79 ++++++++--------- target/nios2/helper.c | 6 +- target/nios2/translate.c | 2 +- target/openrisc/mmu.c | 2 +- target/openrisc/translate.c | 2 +- target/ppc/cpu_init.c | 8 +- target/ppc/excp_helper.c | 6 +- target/ppc/mmu-radix64.c | 6 +- target/ppc/mmu_common.c | 12 +-- target/ppc/translate.c | 6 +- target/riscv/insn_trans/trans_rvi.c.inc | 2 +- target/riscv/insn_trans/trans_rvzce.c.inc | 22 ++--- target/riscv/translate.c | 4 +- target/rx/translate.c | 2 +- target/s390x/cpu.c | 4 +- target/s390x/kvm/kvm.c | 2 +- target/s390x/mmu_helper.c | 6 +- target/s390x/tcg/translate.c | 18 ++-- target/s390x/tcg/translate_vx.c.inc | 2 +- target/sh4/helper.c | 2 +- target/sparc/ldst_helper.c | 4 +- target/sparc/mmu_helper.c | 6 +- target/sparc/translate.c | 3 +- target/sparc/win_helper.c | 1 + target/tricore/translate.c | 4 +- target/xtensa/op_helper.c | 8 +- target/xtensa/translate.c | 2 +- tcg/aarch64/tcg-target.c.inc | 15 +++- tcg/arm/tcg-target.c.inc | 5 +- tcg/i386/tcg-target.c.inc | 20 +++-- tcg/loongarch64/tcg-target.c.inc | 4 +- tcg/mips/tcg-target.c.inc | 8 +- tcg/optimize.c | 8 +- tcg/ppc/tcg-target.c.inc | 19 +++-- tcg/riscv/tcg-target.c.inc | 5 +- tcg/s390x/tcg-target.c.inc | 8 +- tcg/tcg-op-gvec.c | 24 +++--- tcg/tcg-op-ldst.c | 2 +- tcg/tcg.c | 24 +++--- tcg/tci.c | 2 +- tcg/tci/tcg-target.c.inc | 2 +- tests/unit/test-char.c | 2 +- ui/sdl2.c | 2 +- ui/win32-kbd-hook.c | 7 -- util/error-report.c | 2 +- 203 files changed, 747 insertions(+), 618 deletions(-) base-commit: cea3ea670fe265421131aad90c36fbb87bc4d206