[v2] Help wanted for enabling -Wshadow=local

[v2] Help wanted for enabling -Wshadow=local

[Markus Armbruster]

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 <bcain@quicinc.com>
    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 <pbonzini@redhat.com>
M: Richard Henderson <richard.henderson@linaro.org>
M: Eduardo Habkost <eduardo@habkost.net>
    target/i386/tcg/seg_helper.c

Devices
-------
Network devices
M: Jason Wang <jasowang@redhat.com>
    hw/net/vhost_net.c(*2*)

USB
M: Gerd Hoffmann <kraxel@redhat.com>
    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 <mst@redhat.com>
    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 <mst@redhat.com>
    hw/virtio/virtio-pci.c
    include/hw/virtio/virtio-gpu.h(*2*)

virtio-gpu
M: Gerd Hoffmann <kraxel@redhat.com>
    include/hw/virtio/virtio-gpu.h(*2*)

vhost-user-gpu
M: Marc-André Lureau <marcandre.lureau@redhat.com>
R: Gerd Hoffmann <kraxel@redhat.com>
    contrib/vhost-user-gpu/vhost-user-gpu.c(*2*)
    contrib/vhost-user-gpu/vugpu.h(*2*)

Subsystems
----------
Overall Audio backends
M: Gerd Hoffmann <kraxel@redhat.com>
M: Marc-André Lureau <marcandre.lureau@redhat.com>
    audio/audio.c

Open Sound System (OSS) Audio backend
M: Gerd Hoffmann <kraxel@redhat.com>
    audio/ossaudio.c

Dump
M: Marc-André Lureau <marcandre.lureau@redhat.com>
    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
      hw/arm/smmuv3.c: Avoid shadowing variable
      hw/arm/smmuv3-internal.h: Don't use locals in statement macros

Peter Xu (1):
      intel_iommu: Fix shadow local variables on "size"

Philippe Mathieu-Daudé (23):
      tcg: Clean up local variable shadowing
      target/arm/tcg: Clean up local variable shadowing
      target/arm/hvf: Clean up local variable shadowing
      target/mips: Clean up local variable shadowing
      target/m68k: Clean up local variable shadowing
      target/tricore: Clean up local variable shadowing
      hw/arm/armv7m: Clean up local variable shadowing
      hw/arm/virt: Clean up local variable shadowing
      hw/arm/allwinner: Clean up local variable shadowing
      hw/ide/ahci: Clean up local variable shadowing
      hw/m68k: Clean up local variable shadowing
      hw/microblaze: Clean up local variable shadowing
      hw/nios2: Clean up local variable shadowing
      net/eth: Clean up local variable shadowing
      crypto/cipher-gnutls.c: Clean up local variable shadowing
      util/vhost-user-server: Clean up local variable shadowing
      semihosting/arm-compat: Clean up local variable shadowing
      linux-user/strace: Clean up local variable shadowing
      sysemu/device_tree: Clean up local variable shadowing
      softmmu/memory: Clean up local variable shadowing
      softmmu/physmem: Clean up local variable shadowing
      hw/core/machine: Clean up local variable shadowing
      hw/intc/openpic: Clean up local variable shadowing

Re: [v2] Help wanted for enabling -Wshadow=local

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> 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.

[...]

Re: [v2] Help wanted for enabling -Wshadow=local

[Paolo Bonzini]

On 9/26/23 16:42, Markus Armbruster wrote:
> Overall Audio backends
> M: Gerd Hoffmann<kraxel@redhat.com>
> M: Marc-André Lureau<marcandre.lureau@redhat.com>
>      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

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 7872 bytes --]

On Tue, Sep 26, 2023 at 8:43 AM Markus Armbruster <armbru@redhat.com> 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 <bcain@quicinc.com>
>     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 <pbonzini@redhat.com>
> M: Richard Henderson <richard.henderson@linaro.org>
> M: Eduardo Habkost <eduardo@habkost.net>
>     target/i386/tcg/seg_helper.c
>
> Devices
> -------
> Network devices
> M: Jason Wang <jasowang@redhat.com>
>     hw/net/vhost_net.c(*2*)
>
> USB
> M: Gerd Hoffmann <kraxel@redhat.com>
>     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 <mst@redhat.com>
>     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 <mst@redhat.com>
>     hw/virtio/virtio-pci.c
>     include/hw/virtio/virtio-gpu.h(*2*)
>
> virtio-gpu
> M: Gerd Hoffmann <kraxel@redhat.com>
>     include/hw/virtio/virtio-gpu.h(*2*)
>
> vhost-user-gpu
> M: Marc-André Lureau <marcandre.lureau@redhat.com>
> R: Gerd Hoffmann <kraxel@redhat.com>
>     contrib/vhost-user-gpu/vhost-user-gpu.c(*2*)
>     contrib/vhost-user-gpu/vugpu.h(*2*)
>
> Subsystems
> ----------
> Overall Audio backends
> M: Gerd Hoffmann <kraxel@redhat.com>
> M: Marc-André Lureau <marcandre.lureau@redhat.com>
>     audio/audio.c
>
> Open Sound System (OSS) Audio backend
> M: Gerd Hoffmann <kraxel@redhat.com>
>     audio/ossaudio.c
>
> Dump
> M: Marc-André Lureau <marcandre.lureau@redhat.com>
>     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
>       hw/arm/smmuv3.c: Avoid shadowing variable
>       hw/arm/smmuv3-internal.h: Don't use locals in statement macros
>
> Peter Xu (1):
>       intel_iommu: Fix shadow local variables on "size"
>
> Philippe Mathieu-Daudé (23):
>       tcg: Clean up local variable shadowing
>       target/arm/tcg: Clean up local variable shadowing
>       target/arm/hvf: Clean up local variable shadowing
>       target/mips: Clean up local variable shadowing
>       target/m68k: Clean up local variable shadowing
>       target/tricore: Clean up local variable shadowing
>       hw/arm/armv7m: Clean up local variable shadowing
>       hw/arm/virt: Clean up local variable shadowing
>       hw/arm/allwinner: Clean up local variable shadowing
>       hw/ide/ahci: Clean up local variable shadowing
>       hw/m68k: Clean up local variable shadowing
>       hw/microblaze: Clean up local variable shadowing
>       hw/nios2: Clean up local variable shadowing
>       net/eth: Clean up local variable shadowing
>       crypto/cipher-gnutls.c: Clean up local variable shadowing
>       util/vhost-user-server: Clean up local variable shadowing
>       semihosting/arm-compat: Clean up local variable shadowing
>       linux-user/strace: Clean up local variable shadowing
>       sysemu/device_tree: Clean up local variable shadowing
>       softmmu/memory: Clean up local variable shadowing
>       softmmu/physmem: Clean up local variable shadowing
>       hw/core/machine: Clean up local variable shadowing
>       hw/intc/openpic: Clean up local variable shadowing
>
>
>

[-- Attachment #2: Type: text/html, Size: 10196 bytes --]

Re: [v2] Help wanted for enabling -Wshadow=local

[Markus Armbruster]

Warner Losh <imp@bsdimp.com> writes:

> On Tue, Sep 26, 2023 at 8:43 AM Markus Armbruster <armbru@redhat.com> 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!

[...]