Re: [v2] Help wanted for enabling -Wshadow=local
Warner Losh writes: > On Tue, Sep 26, 2023 at 8:43 AM Markus Armbruster wrote: > >> Brian, Gerd, Jason, Marc-André, Michael, we need your help to enable >> -Wshadow=local. >> >> Paolo, you already took care of several subsystems (thanks!), except you >> left a few warnings in target/i386/tcg/seg_helper.c. >> >> >> Local variables shadowing other local variables or parameters make the >> code needlessly hard to understand. Bugs love to hide in such code. >> Evidence: "[PATCH v3 1/7] migration/rdma: Fix save_page method to fail >> on polling error". >> >> Enabling -Wshadow would prevent bugs like this one. But we have to >> clean up all the offenders first. >> >> People responded quickly to my first call for help. Thank you so much! >> >> I'm collecting patches in my git repo at >> https://repo.or.cz/qemu/armbru.git in branch shadow-next, output of >> git-shortlog appended. I'm happy to do pull requests. I don't mind >> maintainers merging patches for their subsystems; interference should be >> minimal. >> >> My test build is down to 19 files with warnings. Sorted by subsystems, >> files covered by multiple subsystems marked "(*NUMBER*)": >> > > Please make sure it's disabled for the bsd-user build. I have 3 patches in > my queue > to fix that, but I'm recovering from an illness and trying to land a large > number of patches > from my gsoc student Karim and git publish is being a pain. If this can > wait a week, I'll > likely be better enough by then and can submit the patches. They are all > trivial, but one > depends on the tcg header cleanups. Waiting a week or two for bsd-user is no problem. We don't need to commit all -Wshadow cleanups in one go. Get well! [...]
Re: [v2] Help wanted for enabling -Wshadow=local
On Tue, Sep 26, 2023 at 8:43 AM Markus Armbruster wrote: > Brian, Gerd, Jason, Marc-André, Michael, we need your help to enable > -Wshadow=local. > > Paolo, you already took care of several subsystems (thanks!), except you > left a few warnings in target/i386/tcg/seg_helper.c. > > > Local variables shadowing other local variables or parameters make the > code needlessly hard to understand. Bugs love to hide in such code. > Evidence: "[PATCH v3 1/7] migration/rdma: Fix save_page method to fail > on polling error". > > Enabling -Wshadow would prevent bugs like this one. But we have to > clean up all the offenders first. > > People responded quickly to my first call for help. Thank you so much! > > I'm collecting patches in my git repo at > https://repo.or.cz/qemu/armbru.git in branch shadow-next, output of > git-shortlog appended. I'm happy to do pull requests. I don't mind > maintainers merging patches for their subsystems; interference should be > minimal. > > My test build is down to 19 files with warnings. Sorted by subsystems, > files covered by multiple subsystems marked "(*NUMBER*)": > Please make sure it's disabled for the bsd-user build. I have 3 patches in my queue to fix that, but I'm recovering from an illness and trying to land a large number of patches from my gsoc student Karim and git publish is being a pain. If this can wait a week, I'll likely be better enough by then and can submit the patches. They are all trivial, but one depends on the tcg header cleanups. Thanks Warner > Guest CPU cores (TCG) > - > Hexagon TCG CPUs > M: Brian Cain > target/hexagon/gen_helper_funcs.py > target/hexagon/mmvec/macros.h > target/hexagon/op_helper.c > target/hexagon/translate.c > > X86 TCG CPUs > M: Paolo Bonzini > M: Richard Henderson > M: Eduardo Habkost > target/i386/tcg/seg_helper.c > > Devices > --- > Network devices > M: Jason Wang > hw/net/vhost_net.c(*2*) > > USB > M: Gerd Hoffmann > hw/usb/desc.c > hw/usb/dev-hub.c > hw/usb/dev-storage.c > hw/usb/hcd-xhci.c > hw/usb/host-libusb.c > > vhost > M: Michael S. Tsirkin > contrib/vhost-user-gpu/vhost-user-gpu.c(*2*) > contrib/vhost-user-gpu/vugpu.h(*2*) > hw/net/vhost_net.c(*2*) > hw/virtio/vhost.c > > virtio > M: Michael S. Tsirkin > hw/virtio/virtio-pci.c > include/hw/virtio/virtio-gpu.h(*2*) > > virtio-gpu > M: Gerd Hoffmann > include/hw/virtio/virtio-gpu.h(*2*) > > vhost-user-gpu > M: Marc-André Lureau > R: Gerd Hoffmann > contrib/vhost-user-gpu/vhost-user-gpu.c(*2*) > contrib/vhost-user-gpu/vugpu.h(*2*) > > Subsystems > -- > Overall Audio backends > M: Gerd Hoffmann > M: Marc-André Lureau > audio/audio.c > > Open Sound System (OSS) Audio backend > M: Gerd Hoffmann > audio/ossaudio.c > > Dump > M: Marc-André Lureau > dump/dump.c > > > Patches collected so far: > > Alberto Garcia (1): > test-throttle: don't shadow 'index' variable in do_test_accounting() > > Alistair Francis (4): > hw/riscv: opentitan: Fixup local variables shadowing > target/riscv: cpu: Fixup local variables shadowing > target/riscv: vector_helper: Fixup local variables shadowing > softmmu/device_tree: Fixup local variables shadowing > > Ani Sinha (1): > hw/acpi: changes towards enabling -Wshadow=local > > Cédric Le Goater (13): > hw/ppc: Clean up local variable shadowing in _FDT helper routine > pnv/psi: Clean up local variable shadowing > spapr: Clean up local variable shadowing in spapr_dt_cpus() > spapr: Clean up local variable shadowing in spapr_init_cpus() > spapr: Clean up local variable shadowing in spapr_get_fw_dev_path() > spapr/drc: Clean up local variable shadowing in > rtas_ibm_configure_connector() > spapr/pci: Clean up local variable shadowing in spapr_phb_realize() > spapr/drc: Clean up local variable shadowing in prop_get_fdt() > aspeed/i2c: Clean up local variable shadowing > aspeed: Clean up local variable shadowing > aspeed/i3c: Rename variable shadowing a local > aspeed/timer: Clean up local variable shadowing > target/ppc: Rename variables to avoid local variable shadowing in > VUPKPX > > Daniel P. Berrangé (2): > crypto: remove shadowed 'ret' variable > seccomp: avoid shadowing of 'action' variable > > Eric Blake (1): > qemu-nbd: changes towards enabling -Wshadow=local > > Klaus Jensen (1): > hw/nvme: Clean up local variable shadowing in nvme_ns_init() > > Laurent Vivier (1): > disas/m68k: clean up local variable shadowing > > Markus Armbruster (8): > meson: Enable -Wshadow as a warning > 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
Re: [v2] Help wanted for enabling -Wshadow=local
On 9/26/23 16:42, Markus Armbruster wrote: Overall Audio backends M: Gerd Hoffmann M: Marc-André Lureau audio/audio.c I can handle this too, but I first need to send out my next pull request. Paolo
Re: [v2] Help wanted for enabling -Wshadow=local
Markus Armbruster writes: > Brian, Gerd, Jason, Marc-André, Michael, we need your help to enable > -Wshadow=local. > > Paolo, you already took care of several subsystems (thanks!), except you > left a few warnings in target/i386/tcg/seg_helper.c. Nope, I missed a patch; seg_helper.c is clean, too. [...]
[v2] Help wanted for enabling -Wshadow=local
Brian, Gerd, Jason, Marc-André, Michael, we need your help to enable -Wshadow=local. Paolo, you already took care of several subsystems (thanks!), except you left a few warnings in target/i386/tcg/seg_helper.c. Local variables shadowing other local variables or parameters make the code needlessly hard to understand. Bugs love to hide in such code. Evidence: "[PATCH v3 1/7] migration/rdma: Fix save_page method to fail on polling error". Enabling -Wshadow would prevent bugs like this one. But we have to clean up all the offenders first. People responded quickly to my first call for help. Thank you so much! I'm collecting patches in my git repo at https://repo.or.cz/qemu/armbru.git in branch shadow-next, output of git-shortlog appended. I'm happy to do pull requests. I don't mind maintainers merging patches for their subsystems; interference should be minimal. My test build is down to 19 files with warnings. Sorted by subsystems, files covered by multiple subsystems marked "(*NUMBER*)": Guest CPU cores (TCG) - Hexagon TCG CPUs M: Brian Cain target/hexagon/gen_helper_funcs.py target/hexagon/mmvec/macros.h target/hexagon/op_helper.c target/hexagon/translate.c X86 TCG CPUs M: Paolo Bonzini M: Richard Henderson M: Eduardo Habkost target/i386/tcg/seg_helper.c Devices --- Network devices M: Jason Wang hw/net/vhost_net.c(*2*) USB M: Gerd Hoffmann hw/usb/desc.c hw/usb/dev-hub.c hw/usb/dev-storage.c hw/usb/hcd-xhci.c hw/usb/host-libusb.c vhost M: Michael S. Tsirkin contrib/vhost-user-gpu/vhost-user-gpu.c(*2*) contrib/vhost-user-gpu/vugpu.h(*2*) hw/net/vhost_net.c(*2*) hw/virtio/vhost.c virtio M: Michael S. Tsirkin hw/virtio/virtio-pci.c include/hw/virtio/virtio-gpu.h(*2*) virtio-gpu M: Gerd Hoffmann include/hw/virtio/virtio-gpu.h(*2*) vhost-user-gpu M: Marc-André Lureau R: Gerd Hoffmann contrib/vhost-user-gpu/vhost-user-gpu.c(*2*) contrib/vhost-user-gpu/vugpu.h(*2*) Subsystems -- Overall Audio backends M: Gerd Hoffmann M: Marc-André Lureau audio/audio.c Open Sound System (OSS) Audio backend M: Gerd Hoffmann audio/ossaudio.c Dump M: Marc-André Lureau dump/dump.c Patches collected so far: Alberto Garcia (1): test-throttle: don't shadow 'index' variable in do_test_accounting() Alistair Francis (4): hw/riscv: opentitan: Fixup local variables shadowing target/riscv: cpu: Fixup local variables shadowing target/riscv: vector_helper: Fixup local variables shadowing softmmu/device_tree: Fixup local variables shadowing Ani Sinha (1): hw/acpi: changes towards enabling -Wshadow=local Cédric Le Goater (13): hw/ppc: Clean up local variable shadowing in _FDT helper routine pnv/psi: Clean up local variable shadowing spapr: Clean up local variable shadowing in spapr_dt_cpus() spapr: Clean up local variable shadowing in spapr_init_cpus() spapr: Clean up local variable shadowing in spapr_get_fw_dev_path() spapr/drc: Clean up local variable shadowing in rtas_ibm_configure_connector() spapr/pci: Clean up local variable shadowing in spapr_phb_realize() spapr/drc: Clean up local variable shadowing in prop_get_fdt() aspeed/i2c: Clean up local variable shadowing aspeed: Clean up local variable shadowing aspeed/i3c: Rename variable shadowing a local aspeed/timer: Clean up local variable shadowing target/ppc: Rename variables to avoid local variable shadowing in VUPKPX Daniel P. Berrangé (2): crypto: remove shadowed 'ret' variable seccomp: avoid shadowing of 'action' variable Eric Blake (1): qemu-nbd: changes towards enabling -Wshadow=local Klaus Jensen (1): hw/nvme: Clean up local variable shadowing in nvme_ns_init() Laurent Vivier (1): disas/m68k: clean up local variable shadowing Markus Armbruster (8): meson: Enable -Wshadow as a warning 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 Paolo Bonzini (8): mptsas: avoid shadowed local variables pm_smbus: rename variable to avoid shadowing vl: remove shadowed local variables target/i386/kvm: eliminate shadowed local variables target/i386/cpu: avoid shadowed local variables target/i386/translate: avoid shadowed local variables target/i386/svm_helper: eliminate duplicate local variable target/i386/seg_helper: remove shadowed variable Peter Maydell (4): hw/intc/arm_gicv3_its: Avoid shadowing variable in do_process_its_cmd() hw/misc/arm_sysctl.c: Avoid shadowing local variable
