| Message ID | 20230831132546.3525721-1-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Markus Armbruster <armbru@redhat.com> writes: > Local variables shadowing other local variables or parameters make the > code needlessly hard to understand. Bugs love to hide in such code. > Evidence: PATCH 1. > > Enabling -Wshadow would prevent bugs like this one. But we'd have to > clean up all the offenders first. We got a lot of them. > > Enabling -Wshadow=local should be less work for almost as much gain. > I took a stab at it. There's a small, exciting part, and a large, > boring part. > > The exciting part is dark preprocessor sorcery to let us nest macro > calls without shadowing: PATCH 7. > > The boring part is cleaning up all the other warnings. I did some > [PATCH 2-6], but ran out of steam long before finishing the job. Some > 160 unique warnings remain. > > To see them, enable -Wshadow=local like so: > > diff --git a/meson.build b/meson.build > index 98e68ef0b1..9fc4c7ac9d 100644 > --- a/meson.build > +++ b/meson.build > @@ -466,6 +466,9 @@ warn_flags = [ > '-Wno-tautological-type-limit-compare', > '-Wno-psabi', > '-Wno-gnu-variable-sized-type-not-at-end', > + '-Wshadow=local', > + '-Wno-error=shadow=local', > + '-Wno-error=shadow=compatible-local', > ] > > if targetos != 'darwin' > > You may want to drop the -Wno-error lines. > > Subsystems with -Wshadow=local warnings: > > virtio-gpu > virtio > Device Tree > Overall TCG CPUs Philippe's "[PATCH 00/11] (few more) Steps towards enabling -Wshadow" takes care of this one. > Overall Audio backends > Open Sound System (OSS) Audio backend > vhost > vhost-user-gpu > Cryptography > M68K TCG CPUs > Dump > ACPI/SMBIOS > Allwinner-a10 Likewise. > ARM TCG CPUs > MPS2 > ASPEED BMCs > ARM SMMU > Virt > Machine core > PC Chipset > X86 TCG CPUs > PC > VT-d Emulation > IDE Likewise. > ARM cores > OpenPIC interrupt controller > q800 Likewise. > petalogix_ml605 > MicroBlaze TCG CPUs > Versatile PB > Network devices > NiosII TCG CPUs > nvme > PowerNV (Non-Virtualized) > sPAPR (pseries) > OpenTitan > RISC-V TCG CPUs > SCSI > USB > Linux user > Network packet abstractions Likewise. > Network device backends Likewise. > Network Block Device (NBD) > Semihosting > Memory API > Seccomp > Main loop > Hexagon TCG CPUs > X86 KVM CPUs > MIPS TCG CPUs Likewise. > PowerPC TCG CPUs > TriCore TCG CPUs > Common TCG code Likewise. > qtest > Throttling infrastructure > Vhost-user block device backend server > > Files with -Wshadow=local warnings: > > accel/tcg/tb-maint.c Likewise. > audio/audio.c > audio/ossaudio.c > contrib/vhost-user-gpu/vhost-user-gpu.c > contrib/vhost-user-gpu/vugpu.h > crypto/cipher-gnutls.c.inc > crypto/tls-cipher-suites.c > disas/m68k.c > dump/dump.c > hw/acpi/cpu_hotplug.c > hw/arm/allwinner-r40.c Likewise. > hw/arm/armsse.c > hw/arm/armv7m.c > hw/arm/aspeed_ast2600.c Likewise. > hw/arm/smmuv3-internal.h > hw/arm/smmuv3.c > hw/arm/virt.c Likewise. > hw/core/machine.c > hw/i2c/aspeed_i2c.c > hw/i2c/pm_smbus.c > hw/i386/acpi-build.c > hw/i386/acpi-microvm.c > hw/i386/intel_iommu.c > hw/i386/pc.c > hw/i386/x86.c > hw/ide/ahci.c Likewise. > hw/intc/arm_gicv3_its.c > hw/intc/openpic.c > hw/loongarch/virt.c > hw/m68k/bootinfo.h Likewise. > hw/microblaze/petalogix_ml605_mmu.c > hw/misc/arm_sysctl.c > hw/misc/aspeed_i3c.c > hw/net/vhost_net.c > hw/nios2/10m50_devboard.c > hw/nvme/ns.c > hw/ppc/pnv_psi.c > hw/ppc/spapr.c > hw/ppc/spapr_drc.c > hw/ppc/spapr_pci.c > hw/riscv/opentitan.c > hw/scsi/mptsas.c > hw/smbios/smbios.c > hw/usb/desc.c > hw/usb/dev-hub.c > hw/usb/dev-storage.c > hw/usb/hcd-xhci.c > hw/usb/host-libusb.c > hw/virtio/vhost.c > hw/virtio/virtio-pci.c > include/hw/cxl/cxl_device.h > include/hw/ppc/fdt.h > include/hw/virtio/virtio-gpu.h > include/sysemu/device_tree.h Likewise. > linux-user/flatload.c > linux-user/mmap.c > linux-user/strace.c > linux-user/syscall.c > net/eth.c Likewise. > qemu-nbd.c > semihosting/arm-compat-semi.c > softmmu/device_tree.c > softmmu/memory.c > softmmu/physmem.c > softmmu/qemu-seccomp.c > softmmu/vl.c target/arm/hvf/hvf.c Likewise. > target/arm/tcg/mve_helper.c Likewise. > target/arm/tcg/translate-m-nocp.c Likewise. > target/hexagon/helper_funcs_generated.c.inc This is actually target/hexagon/gen_helper_funcs.py > target/hexagon/mmvec/macros.h > target/hexagon/op_helper.c > target/hexagon/translate.c > target/i386/cpu.c > target/i386/kvm/kvm.c > target/i386/tcg/seg_helper.c > target/i386/tcg/sysemu/svm_helper.c > target/i386/tcg/translate.c > target/m68k/translate.c Likewise. > target/mips/tcg/msa_helper.c Likewise. > target/mips/tcg/nanomips_translate.c.inc Likewise. > target/mips/tcg/translate.c Likewise. > target/ppc/int_helper.c > target/riscv/cpu.c > target/riscv/vector_helper.c > target/tricore/translate.c > tcg/tcg.c Likewise. > tests/qtest/m48t59-test.c > tests/qtest/pflash-cfi02-test.c > tests/unit/test-throttle.c > util/vhost-user-server.c
diff --git a/meson.build b/meson.build index 98e68ef0b1..9fc4c7ac9d 100644 --- a/meson.build +++ b/meson.build @@ -466,6 +466,9 @@ warn_flags = [ '-Wno-tautological-type-limit-compare', '-Wno-psabi', '-Wno-gnu-variable-sized-type-not-at-end', + '-Wshadow=local', + '-Wno-error=shadow=local', + '-Wno-error=shadow=compatible-local', ] if targetos != 'darwin'
Local variables shadowing other local variables or parameters make the code needlessly hard to understand. Bugs love to hide in such code. Evidence: PATCH 1. Enabling -Wshadow would prevent bugs like this one. But we'd have to clean up all the offenders first. We got a lot of them. Enabling -Wshadow=local should be less work for almost as much gain. I took a stab at it. There's a small, exciting part, and a large, boring part. The exciting part is dark preprocessor sorcery to let us nest macro calls without shadowing: PATCH 7. The boring part is cleaning up all the other warnings. I did some [PATCH 2-6], but ran out of steam long before finishing the job. Some 160 unique warnings remain. To see them, enable -Wshadow=local like so: You may want to drop the -Wno-error lines. Subsystems with -Wshadow=local warnings: virtio-gpu virtio Device Tree Overall TCG CPUs Overall Audio backends Open Sound System (OSS) Audio backend vhost vhost-user-gpu Cryptography M68K TCG CPUs Dump ACPI/SMBIOS Allwinner-a10 ARM TCG CPUs MPS2 ASPEED BMCs ARM SMMU Virt Machine core PC Chipset X86 TCG CPUs PC VT-d Emulation IDE ARM cores OpenPIC interrupt controller q800 petalogix_ml605 MicroBlaze TCG CPUs Versatile PB Network devices NiosII TCG CPUs nvme PowerNV (Non-Virtualized) sPAPR (pseries) OpenTitan RISC-V TCG CPUs SCSI USB Linux user Network packet abstractions Network device backends Network Block Device (NBD) Semihosting Memory API Seccomp Main loop Hexagon TCG CPUs X86 KVM CPUs MIPS TCG CPUs PowerPC TCG CPUs TriCore TCG CPUs Common TCG code qtest Throttling infrastructure Vhost-user block device backend server Files with -Wshadow=local warnings: accel/tcg/tb-maint.c audio/audio.c audio/ossaudio.c contrib/vhost-user-gpu/vhost-user-gpu.c contrib/vhost-user-gpu/vugpu.h crypto/cipher-gnutls.c.inc crypto/tls-cipher-suites.c disas/m68k.c dump/dump.c hw/acpi/cpu_hotplug.c hw/arm/allwinner-r40.c hw/arm/armsse.c hw/arm/armv7m.c hw/arm/aspeed_ast2600.c hw/arm/smmuv3-internal.h hw/arm/smmuv3.c hw/arm/virt.c hw/core/machine.c hw/i2c/aspeed_i2c.c hw/i2c/pm_smbus.c hw/i386/acpi-build.c hw/i386/acpi-microvm.c hw/i386/intel_iommu.c hw/i386/pc.c hw/i386/x86.c hw/ide/ahci.c hw/intc/arm_gicv3_its.c hw/intc/openpic.c hw/loongarch/virt.c hw/m68k/bootinfo.h hw/microblaze/petalogix_ml605_mmu.c hw/misc/arm_sysctl.c hw/misc/aspeed_i3c.c hw/net/vhost_net.c hw/nios2/10m50_devboard.c hw/nvme/ns.c hw/ppc/pnv_psi.c hw/ppc/spapr.c hw/ppc/spapr_drc.c hw/ppc/spapr_pci.c hw/riscv/opentitan.c hw/scsi/mptsas.c hw/smbios/smbios.c hw/usb/desc.c hw/usb/dev-hub.c hw/usb/dev-storage.c hw/usb/hcd-xhci.c hw/usb/host-libusb.c hw/virtio/vhost.c hw/virtio/virtio-pci.c include/hw/cxl/cxl_device.h include/hw/ppc/fdt.h include/hw/virtio/virtio-gpu.h include/sysemu/device_tree.h linux-user/flatload.c linux-user/mmap.c linux-user/strace.c linux-user/syscall.c net/eth.c qemu-nbd.c semihosting/arm-compat-semi.c softmmu/device_tree.c softmmu/memory.c softmmu/physmem.c softmmu/qemu-seccomp.c softmmu/vl.c target/arm/tcg/mve_helper.c target/arm/tcg/translate-m-nocp.c target/hexagon/helper_funcs_generated.c.inc target/hexagon/mmvec/macros.h target/hexagon/op_helper.c target/hexagon/translate.c target/i386/cpu.c target/i386/kvm/kvm.c target/i386/tcg/seg_helper.c target/i386/tcg/sysemu/svm_helper.c target/i386/tcg/translate.c target/m68k/translate.c target/mips/tcg/msa_helper.c target/mips/tcg/nanomips_translate.c.inc target/mips/tcg/translate.c target/ppc/int_helper.c target/riscv/cpu.c target/riscv/vector_helper.c target/tricore/translate.c tcg/tcg.c tests/qtest/m48t59-test.c tests/qtest/pflash-cfi02-test.c tests/unit/test-throttle.c util/vhost-user-server.c Markus Armbruster (7): migration/rdma: Fix save_page method to fail on polling error migration: Clean up local variable shadowing ui: Clean up local variable shadowing block/dirty-bitmap: Clean up local variable shadowing block/vdi: Clean up local variable shadowing block: Clean up local variable shadowing qobject atomics osdep: Make a few macros more hygienic include/qapi/qmp/qobject.h | 8 +++++--- include/qemu/atomic.h | 11 ++++++----- include/qemu/osdep.h | 34 ++++++++++++++++++--------------- block.c | 7 ++++--- block/monitor/bitmap-qmp-cmds.c | 2 +- block/qcow2-bitmap.c | 3 +-- block/rbd.c | 2 +- block/stream.c | 1 - block/vdi.c | 7 +++---- block/vvfat.c | 34 ++++++++++++++++----------------- hw/block/xen-block.c | 6 +++--- migration/block.c | 4 ++-- migration/ram.c | 8 +++----- migration/rdma.c | 14 +++++++++----- migration/vmstate.c | 2 +- ui/gtk.c | 14 +++++++------- ui/spice-display.c | 9 +++++---- ui/vnc-palette.c | 2 -- ui/vnc.c | 12 ++++++------ ui/vnc-enc-zrle.c.inc | 9 ++++----- 20 files changed, 97 insertions(+), 92 deletions(-)