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(-)
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
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(-)
--
2.41.0Markus 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
qemu_rdma_save_page() reports polling error with error_report(), then
succeeds anyway. This is because the variable holding the polling
status *shadows* the variable the function returns. The latter
remains zero.
Broken since day one, and duplicated more recently.
Fixes: 2da776db4846 (rdma: core logic)
Fixes: b390afd8c50b (migration/rdma: Fix out of order wrid)
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
migration/rdma.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/migration/rdma.c b/migration/rdma.c
index ca430d319d..b2e869aced 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -3281,7 +3281,8 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
*/
while (1) {
uint64_t wr_id, wr_id_in;
- int ret = qemu_rdma_poll(rdma, rdma->recv_cq, &wr_id_in, NULL);
+ ret = qemu_rdma_poll(rdma, rdma->recv_cq, &wr_id_in, NULL);
+
if (ret < 0) {
error_report("rdma migration: polling error! %d", ret);
goto err;
@@ -3296,7 +3297,8 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
while (1) {
uint64_t wr_id, wr_id_in;
- int ret = qemu_rdma_poll(rdma, rdma->send_cq, &wr_id_in, NULL);
+ ret = qemu_rdma_poll(rdma, rdma->send_cq, &wr_id_in, NULL);
+
if (ret < 0) {
error_report("rdma migration: polling error! %d", ret);
goto err;
--
2.41.0
On 31/08/2023 21:25, Markus Armbruster wrote:
> qemu_rdma_save_page() reports polling error with error_report(), then
> succeeds anyway. This is because the variable holding the polling
> status *shadows* the variable the function returns. The latter
> remains zero.
>
> Broken since day one, and duplicated more recently.
>
> Fixes: 2da776db4846 (rdma: core logic)
> Fixes: b390afd8c50b (migration/rdma: Fix out of order wrid)
Thanks for the fixes
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> migration/rdma.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/migration/rdma.c b/migration/rdma.c
> index ca430d319d..b2e869aced 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -3281,7 +3281,8 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
> */
> while (1) {
> uint64_t wr_id, wr_id_in;
> - int ret = qemu_rdma_poll(rdma, rdma->recv_cq, &wr_id_in, NULL);
> + ret = qemu_rdma_poll(rdma, rdma->recv_cq, &wr_id_in, NULL);
> +
> if (ret < 0) {
> error_report("rdma migration: polling error! %d", ret);
> goto err;
> @@ -3296,7 +3297,8 @@ static size_t qemu_rdma_save_page(QEMUFile *f,
>
> while (1) {
> uint64_t wr_id, wr_id_in;
> - int ret = qemu_rdma_poll(rdma, rdma->send_cq, &wr_id_in, NULL);
> + ret = qemu_rdma_poll(rdma, rdma->send_cq, &wr_id_in, NULL);
> +
> if (ret < 0) {
> error_report("rdma migration: polling error! %d", ret);
> goto err;
On Thu, Aug 31, 2023 at 03:25:40PM +0200, Markus Armbruster wrote: > qemu_rdma_save_page() reports polling error with error_report(), then > succeeds anyway. This is because the variable holding the polling > status *shadows* the variable the function returns. The latter > remains zero. > > Broken since day one, and duplicated more recently. > > Fixes: 2da776db4846 (rdma: core logic) > Fixes: b390afd8c50b (migration/rdma: Fix out of order wrid) > Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Peter Xu <peterx@redhat.com> -- Peter Xu
On Thu, Aug 31, 2023 at 03:25:40PM +0200, Markus Armbruster wrote: > qemu_rdma_save_page() reports polling error with error_report(), then > succeeds anyway. This is because the variable holding the polling > status *shadows* the variable the function returns. The latter > remains zero. > > Broken since day one, and duplicated more recently. > > Fixes: 2da776db4846 (rdma: core logic) > Fixes: b390afd8c50b (migration/rdma: Fix out of order wrid) Alas, the curse of immutable git history preserving typos in commit subjects ;) The alternative of rewriting history and breaking SHA references is worse. > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > migration/rdma.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Eric Blake <eblake@redhat.com> writes: > On Thu, Aug 31, 2023 at 03:25:40PM +0200, Markus Armbruster wrote: >> qemu_rdma_save_page() reports polling error with error_report(), then >> succeeds anyway. This is because the variable holding the polling >> status *shadows* the variable the function returns. The latter >> remains zero. >> >> Broken since day one, and duplicated more recently. >> >> Fixes: 2da776db4846 (rdma: core logic) >> Fixes: b390afd8c50b (migration/rdma: Fix out of order wrid) > > Alas, the curse of immutable git history preserving typos in commit > subjects ;) "wrid" is short for "work request ID", actually. > The alternative of rewriting history and breaking SHA > references is worse. Rewriting master is a big no-no. git-note can be used to correct the record, but it has its usability issues. >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> migration/rdma.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
Local variables shadowing other local variables or parameters make the
code needlessly hard to understand. Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
migration/block.c | 4 ++--
migration/ram.c | 8 +++-----
migration/rdma.c | 8 +++++---
migration/vmstate.c | 2 +-
4 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/migration/block.c b/migration/block.c
index b9580a6c7e..33b3776198 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -438,8 +438,8 @@ static int init_blk_migration(QEMUFile *f)
/* Can only insert new BDSes now because doing so while iterating block
* devices may end up in a deadlock (iterating the new BDSes, too). */
for (i = 0; i < num_bs; i++) {
- BlkMigDevState *bmds = bmds_bs[i].bmds;
- BlockDriverState *bs = bmds_bs[i].bs;
+ bmds = bmds_bs[i].bmds;
+ bs = bmds_bs[i].bs;
if (bmds) {
ret = blk_insert_bs(bmds->blk, bs, &local_err);
diff --git a/migration/ram.c b/migration/ram.c
index 9040d66e61..0c202f8109 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3517,8 +3517,6 @@ int colo_init_ram_cache(void)
* we use the same name 'ram_bitmap' as for migration.
*/
if (ram_bytes_total()) {
- RAMBlock *block;
-
RAMBLOCK_FOREACH_NOT_IGNORED(block) {
unsigned long pages = block->max_length >> TARGET_PAGE_BITS;
block->bmap = bitmap_new(pages);
@@ -3998,12 +3996,12 @@ static int ram_load_precopy(QEMUFile *f)
}
}
if (migrate_ignore_shared()) {
- hwaddr addr = qemu_get_be64(f);
+ hwaddr addr2 = qemu_get_be64(f);
if (migrate_ram_is_ignored(block) &&
- block->mr->addr != addr) {
+ block->mr->addr != addr2) {
error_report("Mismatched GPAs for block %s "
"%" PRId64 "!= %" PRId64,
- id, (uint64_t)addr,
+ id, (uint64_t)addr2,
(uint64_t)block->mr->addr);
ret = -EINVAL;
}
diff --git a/migration/rdma.c b/migration/rdma.c
index b2e869aced..c6400cf641 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -1902,9 +1902,11 @@ static int qemu_rdma_exchange_send(RDMAContext *rdma, RDMAControlHeader *head,
* by waiting for a READY message.
*/
if (rdma->control_ready_expected) {
- RDMAControlHeader resp;
- ret = qemu_rdma_exchange_get_response(rdma,
- &resp, RDMA_CONTROL_READY, RDMA_WRID_READY);
+ RDMAControlHeader resp_ignored;
+
+ ret = qemu_rdma_exchange_get_response(rdma, &resp_ignored,
+ RDMA_CONTROL_READY,
+ RDMA_WRID_READY);
if (ret < 0) {
return ret;
}
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 31842c3afb..438ea77cfa 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -97,7 +97,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
return -EINVAL;
}
if (vmsd->pre_load) {
- int ret = vmsd->pre_load(opaque);
+ ret = vmsd->pre_load(opaque);
if (ret) {
return ret;
}
--
2.41.0On Thu, Aug 31, 2023 at 03:25:41PM +0200, Markus Armbruster wrote: > Local variables shadowing other local variables or parameters make the > code needlessly hard to understand. Tracked down with -Wshadow=local. > Clean up: delete inner declarations when they are actually redundant, > else rename variables. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Peter Xu <peterx@redhat.com> -- Peter Xu
Local variables shadowing other local variables or parameters make the
code needlessly hard to understand. Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
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 ++++-----
5 files changed, 22 insertions(+), 24 deletions(-)
diff --git a/ui/gtk.c b/ui/gtk.c
index 8ba41c8f13..24c78df79e 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -930,8 +930,8 @@ static gboolean gd_motion_event(GtkWidget *widget, GdkEventMotion *motion,
GdkMonitor *monitor = gdk_display_get_monitor_at_window(dpy, win);
GdkRectangle geometry;
- int x = (int)motion->x_root;
- int y = (int)motion->y_root;
+ int xr = (int)motion->x_root;
+ int yr = (int)motion->y_root;
gdk_monitor_get_geometry(monitor, &geometry);
@@ -942,13 +942,13 @@ static gboolean gd_motion_event(GtkWidget *widget, GdkEventMotion *motion,
* may still be only half way across the screen. Without
* this warp, the server pointer would thus appear to hit
* an invisible wall */
- if (x <= geometry.x || x - geometry.x >= geometry.width - 1 ||
- y <= geometry.y || y - geometry.y >= geometry.height - 1) {
+ if (xr <= geometry.x || xr - geometry.x >= geometry.width - 1 ||
+ yr <= geometry.y || yr - geometry.y >= geometry.height - 1) {
GdkDevice *dev = gdk_event_get_device((GdkEvent *)motion);
- x = geometry.x + geometry.width / 2;
- y = geometry.y + geometry.height / 2;
+ xr = geometry.x + geometry.width / 2;
+ yr = geometry.y + geometry.height / 2;
- gdk_device_warp(dev, screen, x, y);
+ gdk_device_warp(dev, screen, xr, yr);
s->last_set = FALSE;
return FALSE;
}
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 3f3f8013d8..cbaba3bbad 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1080,15 +1080,16 @@ static void qemu_spice_gl_update(DisplayChangeListener *dcl,
}
if (render_cursor) {
- int x, y;
+ int ptr_x, ptr_y;
+
qemu_mutex_lock(&ssd->lock);
- x = ssd->ptr_x;
- y = ssd->ptr_y;
+ ptr_x = ssd->ptr_x;
+ ptr_y = ssd->ptr_y;
qemu_mutex_unlock(&ssd->lock);
egl_texture_blit(ssd->gls, &ssd->blit_fb, &ssd->guest_fb,
!y_0_top);
egl_texture_blend(ssd->gls, &ssd->blit_fb, &ssd->cursor_fb,
- !y_0_top, x, y, 1.0, 1.0);
+ !y_0_top, ptr_x, ptr_y, 1.0, 1.0);
glFlush();
}
diff --git a/ui/vnc-palette.c b/ui/vnc-palette.c
index dc7c0ba997..4e88c412f0 100644
--- a/ui/vnc-palette.c
+++ b/ui/vnc-palette.c
@@ -86,8 +86,6 @@ int palette_put(VncPalette *palette, uint32_t color)
return 0;
}
if (!entry) {
- VncPaletteEntry *entry;
-
entry = &palette->pool[palette->size];
entry->color = color;
entry->idx = idx;
diff --git a/ui/vnc.c b/ui/vnc.c
index 92964dcc0c..3e1fccffa3 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1584,15 +1584,15 @@ static void vnc_jobs_bh(void *opaque)
*/
static int vnc_client_read(VncState *vs)
{
- size_t ret;
+ size_t sz;
#ifdef CONFIG_VNC_SASL
if (vs->sasl.conn && vs->sasl.runSSF)
- ret = vnc_client_read_sasl(vs);
+ sz = vnc_client_read_sasl(vs);
else
#endif /* CONFIG_VNC_SASL */
- ret = vnc_client_read_plain(vs);
- if (!ret) {
+ sz = vnc_client_read_plain(vs);
+ if (!sz) {
if (vs->disconnecting) {
vnc_disconnect_finish(vs);
return -1;
@@ -3118,8 +3118,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)
cmp_bytes = MIN(VNC_DIRTY_PIXELS_PER_BIT * VNC_SERVER_FB_BYTES,
server_stride);
if (vd->guest.format != VNC_SERVER_FB_FORMAT) {
- int width = pixman_image_get_width(vd->server);
- tmpbuf = qemu_pixman_linebuf_create(VNC_SERVER_FB_FORMAT, width);
+ int w = pixman_image_get_width(vd->server);
+ tmpbuf = qemu_pixman_linebuf_create(VNC_SERVER_FB_FORMAT, w);
} else {
int guest_bpp =
PIXMAN_FORMAT_BPP(pixman_image_get_format(vd->guest.fb));
diff --git a/ui/vnc-enc-zrle.c.inc b/ui/vnc-enc-zrle.c.inc
index c107d8affc..edf42d4a6a 100644
--- a/ui/vnc-enc-zrle.c.inc
+++ b/ui/vnc-enc-zrle.c.inc
@@ -153,11 +153,12 @@ static void ZRLE_ENCODE_TILE(VncState *vs, ZRLE_PIXEL *data, int w, int h,
}
if (use_rle) {
- ZRLE_PIXEL *ptr = data;
- ZRLE_PIXEL *end = ptr + w * h;
ZRLE_PIXEL *run_start;
ZRLE_PIXEL pix;
+ ptr = data;
+ end = ptr + w * h;
+
while (ptr < end) {
int len;
int index = 0;
@@ -198,7 +199,7 @@ static void ZRLE_ENCODE_TILE(VncState *vs, ZRLE_PIXEL *data, int w, int h,
}
} else if (use_palette) { /* no RLE */
int bppp;
- ZRLE_PIXEL *ptr = data;
+ ptr = data;
/* packed pixels */
@@ -241,8 +242,6 @@ static void ZRLE_ENCODE_TILE(VncState *vs, ZRLE_PIXEL *data, int w, int h,
#endif
{
#ifdef ZRLE_COMPACT_PIXEL
- ZRLE_PIXEL *ptr;
-
for (ptr = data; ptr < data + w * h; ptr++) {
ZRLE_WRITE_PIXEL(vs, *ptr);
}
--
2.41.0On Thu, 31 Aug 2023 at 14:25, Markus Armbruster <armbru@redhat.com> wrote:
>
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand. Tracked down with -Wshadow=local.
> Clean up: delete inner declarations when they are actually redundant,
> else rename variables.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> diff --git a/ui/vnc-enc-zrle.c.inc b/ui/vnc-enc-zrle.c.inc
> index c107d8affc..edf42d4a6a 100644
> --- a/ui/vnc-enc-zrle.c.inc
> +++ b/ui/vnc-enc-zrle.c.inc
> @@ -153,11 +153,12 @@ static void ZRLE_ENCODE_TILE(VncState *vs, ZRLE_PIXEL *data, int w, int h,
> }
>
> if (use_rle) {
> - ZRLE_PIXEL *ptr = data;
> - ZRLE_PIXEL *end = ptr + w * h;
> ZRLE_PIXEL *run_start;
> ZRLE_PIXEL pix;
>
> + ptr = data;
> + end = ptr + w * h;
> +
> while (ptr < end) {
> int len;
> int index = 0;
> @@ -198,7 +199,7 @@ static void ZRLE_ENCODE_TILE(VncState *vs, ZRLE_PIXEL *data, int w, int h,
> }
> } else if (use_palette) { /* no RLE */
> int bppp;
> - ZRLE_PIXEL *ptr = data;
> + ptr = data;
>
> /* packed pixels */
>
> @@ -241,8 +242,6 @@ static void ZRLE_ENCODE_TILE(VncState *vs, ZRLE_PIXEL *data, int w, int h,
> #endif
> {
> #ifdef ZRLE_COMPACT_PIXEL
> - ZRLE_PIXEL *ptr;
> -
> for (ptr = data; ptr < data + w * h; ptr++) {
> ZRLE_WRITE_PIXEL(vs, *ptr);
> }
For this one I'm tempted to suggest instead moving the
pix and end currently at whole-function scope into their
own block, so it's clear these are actually four
completely independent uses of ptr/end. But either way
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
Peter Maydell <peter.maydell@linaro.org> writes:
> On Thu, 31 Aug 2023 at 14:25, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Local variables shadowing other local variables or parameters make the
>> code needlessly hard to understand. Tracked down with -Wshadow=local.
>> Clean up: delete inner declarations when they are actually redundant,
>> else rename variables.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>
>> diff --git a/ui/vnc-enc-zrle.c.inc b/ui/vnc-enc-zrle.c.inc
>> index c107d8affc..edf42d4a6a 100644
>> --- a/ui/vnc-enc-zrle.c.inc
>> +++ b/ui/vnc-enc-zrle.c.inc
>> @@ -153,11 +153,12 @@ static void ZRLE_ENCODE_TILE(VncState *vs, ZRLE_PIXEL *data, int w, int h,
>> }
>>
>> if (use_rle) {
>> - ZRLE_PIXEL *ptr = data;
>> - ZRLE_PIXEL *end = ptr + w * h;
>> ZRLE_PIXEL *run_start;
>> ZRLE_PIXEL pix;
>>
>> + ptr = data;
>> + end = ptr + w * h;
>> +
>> while (ptr < end) {
>> int len;
>> int index = 0;
>> @@ -198,7 +199,7 @@ static void ZRLE_ENCODE_TILE(VncState *vs, ZRLE_PIXEL *data, int w, int h,
>> }
>> } else if (use_palette) { /* no RLE */
>> int bppp;
>> - ZRLE_PIXEL *ptr = data;
>> + ptr = data;
>>
>> /* packed pixels */
>>
>> @@ -241,8 +242,6 @@ static void ZRLE_ENCODE_TILE(VncState *vs, ZRLE_PIXEL *data, int w, int h,
>> #endif
>> {
>> #ifdef ZRLE_COMPACT_PIXEL
>> - ZRLE_PIXEL *ptr;
>> -
>> for (ptr = data; ptr < data + w * h; ptr++) {
>> ZRLE_WRITE_PIXEL(vs, *ptr);
>> }
>
> For this one I'm tempted to suggest instead moving the
> pix and end currently at whole-function scope into their
> own block, so it's clear these are actually four
> completely independent uses of ptr/end. But either way
You have a point. However, we'd need to splice in a block just for
that, which involved some reindenting. Can do if the assigned
maintainers (Gerd and Marc-André) prefer it. Else, I'll stay with
minimally invasive.
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Thanks!
Local variables shadowing other local variables or parameters make the
code needlessly hard to understand. Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block/monitor/bitmap-qmp-cmds.c | 2 +-
block/qcow2-bitmap.c | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
index 55f778f5af..4d018423d8 100644
--- a/block/monitor/bitmap-qmp-cmds.c
+++ b/block/monitor/bitmap-qmp-cmds.c
@@ -276,7 +276,7 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
for (lst = bms; lst; lst = lst->next) {
switch (lst->value->type) {
- const char *name, *node;
+ const char *name;
case QTYPE_QSTRING:
name = lst->value->u.local;
src = bdrv_find_dirty_bitmap(bs, name);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 037fa2d435..ffd5cd3b23 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1555,7 +1555,6 @@ bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
const char *name = bdrv_dirty_bitmap_name(bitmap);
uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
- Qcow2Bitmap *bm;
if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
bdrv_dirty_bitmap_inconsistent(bitmap)) {
@@ -1625,7 +1624,7 @@ bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
/* allocate clusters and store bitmaps */
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
- BdrvDirtyBitmap *bitmap = bm->dirty_bitmap;
+ bitmap = bm->dirty_bitmap;
if (bitmap == NULL || bdrv_dirty_bitmap_readonly(bitmap)) {
continue;
--
2.41.0Am 31.08.2023 um 15:25 hat Markus Armbruster geschrieben:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand. Tracked down with -Wshadow=local.
> Clean up: delete inner declarations when they are actually redundant,
> else rename variables.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/monitor/bitmap-qmp-cmds.c | 2 +-
> block/qcow2-bitmap.c | 3 +--
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
> index 55f778f5af..4d018423d8 100644
> --- a/block/monitor/bitmap-qmp-cmds.c
> +++ b/block/monitor/bitmap-qmp-cmds.c
> @@ -276,7 +276,7 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
>
> for (lst = bms; lst; lst = lst->next) {
> switch (lst->value->type) {
> - const char *name, *node;
> + const char *name;
> case QTYPE_QSTRING:
> name = lst->value->u.local;
> src = bdrv_find_dirty_bitmap(bs, name);
The names in this function are all over the place... A more ambitious
patch could rename the parameters to dst_node/dst_bitmap and these
variables to src_node/src_bitmap to get some more consistency (both with
each other and with the existing src/dst variables).
Preexisting, so I'm not insisting that you should do this.
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 037fa2d435..ffd5cd3b23 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1555,7 +1555,6 @@ bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
> const char *name = bdrv_dirty_bitmap_name(bitmap);
> uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
> - Qcow2Bitmap *bm;
>
> if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
> bdrv_dirty_bitmap_inconsistent(bitmap)) {
> @@ -1625,7 +1624,7 @@ bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>
> /* allocate clusters and store bitmaps */
> QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> - BdrvDirtyBitmap *bitmap = bm->dirty_bitmap;
> + bitmap = bm->dirty_bitmap;
>
> if (bitmap == NULL || bdrv_dirty_bitmap_readonly(bitmap)) {
> continue;
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Kevin Wolf <kwolf@redhat.com> writes:
> Am 31.08.2023 um 15:25 hat Markus Armbruster geschrieben:
>> Local variables shadowing other local variables or parameters make the
>> code needlessly hard to understand. Tracked down with -Wshadow=local.
>> Clean up: delete inner declarations when they are actually redundant,
>> else rename variables.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> block/monitor/bitmap-qmp-cmds.c | 2 +-
>> block/qcow2-bitmap.c | 3 +--
>> 2 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
>> index 55f778f5af..4d018423d8 100644
>> --- a/block/monitor/bitmap-qmp-cmds.c
>> +++ b/block/monitor/bitmap-qmp-cmds.c
>> @@ -276,7 +276,7 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
>>
>> for (lst = bms; lst; lst = lst->next) {
>> switch (lst->value->type) {
>> - const char *name, *node;
>> + const char *name;
>> case QTYPE_QSTRING:
>> name = lst->value->u.local;
>> src = bdrv_find_dirty_bitmap(bs, name);
>
> The names in this function are all over the place... A more ambitious
> patch could rename the parameters to dst_node/dst_bitmap and these
> variables to src_node/src_bitmap to get some more consistency (both with
> each other and with the existing src/dst variables).
What exactly would you like me to consider? Perhaps:
* Rename parameter @node to @dst_node
* Rename which parameter to @dst_bitmap?
* Rename nested local @node to @src_node
* Rename which local variable to @src_bitmap?
* Move nested locals to function scope
> Preexisting, so I'm not insisting that you should do this.
>
>> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
>> index 037fa2d435..ffd5cd3b23 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1555,7 +1555,6 @@ bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>> FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
>> const char *name = bdrv_dirty_bitmap_name(bitmap);
>> uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
>> - Qcow2Bitmap *bm;
>>
>> if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
>> bdrv_dirty_bitmap_inconsistent(bitmap)) {
>> @@ -1625,7 +1624,7 @@ bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>>
>> /* allocate clusters and store bitmaps */
>> QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> - BdrvDirtyBitmap *bitmap = bm->dirty_bitmap;
>> + bitmap = bm->dirty_bitmap;
>>
>> if (bitmap == NULL || bdrv_dirty_bitmap_readonly(bitmap)) {
>> continue;
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Thanks!
Am 19.09.2023 um 07:48 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 31.08.2023 um 15:25 hat Markus Armbruster geschrieben:
> >> Local variables shadowing other local variables or parameters make the
> >> code needlessly hard to understand. Tracked down with -Wshadow=local.
> >> Clean up: delete inner declarations when they are actually redundant,
> >> else rename variables.
> >>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >> block/monitor/bitmap-qmp-cmds.c | 2 +-
> >> block/qcow2-bitmap.c | 3 +--
> >> 2 files changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
> >> index 55f778f5af..4d018423d8 100644
> >> --- a/block/monitor/bitmap-qmp-cmds.c
> >> +++ b/block/monitor/bitmap-qmp-cmds.c
> >> @@ -276,7 +276,7 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
> >>
> >> for (lst = bms; lst; lst = lst->next) {
> >> switch (lst->value->type) {
> >> - const char *name, *node;
> >> + const char *name;
> >> case QTYPE_QSTRING:
> >> name = lst->value->u.local;
> >> src = bdrv_find_dirty_bitmap(bs, name);
> >
> > The names in this function are all over the place... A more ambitious
> > patch could rename the parameters to dst_node/dst_bitmap and these
> > variables to src_node/src_bitmap to get some more consistency (both with
> > each other and with the existing src/dst variables).
>
> What exactly would you like me to consider? Perhaps:
>
> * Rename parameter @node to @dst_node
>
> * Rename which parameter to @dst_bitmap?
Parameter @target to @dst_bitmap (it's the name of a bitmap in
@node/@dst_node)
> * Rename nested local @node to @src_node
>
> * Rename which local variable to @src_bitmap?
@name to @src_bitmap (it's the name of a bitmap in the local
@node/@src_node)
> * Move nested locals to function scope
I don't really mind either way, but yes, maybe that would be more
conventional.
That you couldn't tell for two of the variables what they actually are
probably supports the argument that they should be renamed. :-)
Kevin
Kevin Wolf <kwolf@redhat.com> writes:
> Am 19.09.2023 um 07:48 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > Am 31.08.2023 um 15:25 hat Markus Armbruster geschrieben:
>> >> Local variables shadowing other local variables or parameters make the
>> >> code needlessly hard to understand. Tracked down with -Wshadow=local.
>> >> Clean up: delete inner declarations when they are actually redundant,
>> >> else rename variables.
>> >>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >> block/monitor/bitmap-qmp-cmds.c | 2 +-
>> >> block/qcow2-bitmap.c | 3 +--
>> >> 2 files changed, 2 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
>> >> index 55f778f5af..4d018423d8 100644
>> >> --- a/block/monitor/bitmap-qmp-cmds.c
>> >> +++ b/block/monitor/bitmap-qmp-cmds.c
>> >> @@ -276,7 +276,7 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
>> >>
>> >> for (lst = bms; lst; lst = lst->next) {
>> >> switch (lst->value->type) {
>> >> - const char *name, *node;
>> >> + const char *name;
>> >> case QTYPE_QSTRING:
>> >> name = lst->value->u.local;
>> >> src = bdrv_find_dirty_bitmap(bs, name);
>> >
>> > The names in this function are all over the place... A more ambitious
>> > patch could rename the parameters to dst_node/dst_bitmap and these
>> > variables to src_node/src_bitmap to get some more consistency (both with
>> > each other and with the existing src/dst variables).
>>
>> What exactly would you like me to consider? Perhaps:
>>
>> * Rename parameter @node to @dst_node
>>
>> * Rename which parameter to @dst_bitmap?
>
> Parameter @target to @dst_bitmap (it's the name of a bitmap in
> @node/@dst_node)
>
>> * Rename nested local @node to @src_node
>>
>> * Rename which local variable to @src_bitmap?
>
> @name to @src_bitmap (it's the name of a bitmap in the local
> @node/@src_node)
>
>> * Move nested locals to function scope
>
> I don't really mind either way, but yes, maybe that would be more
> conventional.
Done for v2.
> That you couldn't tell for two of the variables what they actually are
> probably supports the argument that they should be renamed. :-)
Fair point!
On Thu, Aug 31, 2023 at 03:25:43PM +0200, Markus Armbruster wrote: > Local variables shadowing other local variables or parameters make the > code needlessly hard to understand. Tracked down with -Wshadow=local. > Clean up: delete inner declarations when they are actually redundant, > else rename variables. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/monitor/bitmap-qmp-cmds.c | 2 +- > block/qcow2-bitmap.c | 3 +-- > 2 files changed, 2 insertions(+), 3 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Local variables shadowing other local variables or parameters make the
code needlessly hard to understand. Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block/vdi.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/block/vdi.c b/block/vdi.c
index 6c35309e04..c084105b78 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -634,7 +634,6 @@ vdi_co_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes,
bmap_entry = le32_to_cpu(s->bmap[block_index]);
if (!VDI_IS_ALLOCATED(bmap_entry)) {
/* Allocate new block and write to it. */
- uint64_t data_offset;
qemu_co_rwlock_upgrade(&s->bmap_lock);
bmap_entry = le32_to_cpu(s->bmap[block_index]);
if (VDI_IS_ALLOCATED(bmap_entry)) {
@@ -700,7 +699,7 @@ nonallocating_write:
/* One or more new blocks were allocated. */
VdiHeader *header;
uint8_t *base;
- uint64_t offset;
+ uint64_t offs;
uint32_t n_sectors;
g_free(block);
@@ -723,11 +722,11 @@ nonallocating_write:
bmap_first /= (SECTOR_SIZE / sizeof(uint32_t));
bmap_last /= (SECTOR_SIZE / sizeof(uint32_t));
n_sectors = bmap_last - bmap_first + 1;
- offset = s->bmap_sector + bmap_first;
+ offs = s->bmap_sector + bmap_first;
base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
logout("will write %u block map sectors starting from entry %u\n",
n_sectors, bmap_first);
- ret = bdrv_co_pwrite(bs->file, offset * SECTOR_SIZE,
+ ret = bdrv_co_pwrite(bs->file, offs * SECTOR_SIZE,
n_sectors * SECTOR_SIZE, base, 0);
}
--
2.41.0Am 31.08.2023 um 15:25 hat Markus Armbruster geschrieben:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand. Tracked down with -Wshadow=local.
> Clean up: delete inner declarations when they are actually redundant,
> else rename variables.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> @@ -700,7 +699,7 @@ nonallocating_write:
> /* One or more new blocks were allocated. */
> VdiHeader *header;
> uint8_t *base;
> - uint64_t offset;
> + uint64_t offs;
> uint32_t n_sectors;
>
> g_free(block);
> @@ -723,11 +722,11 @@ nonallocating_write:
> bmap_first /= (SECTOR_SIZE / sizeof(uint32_t));
> bmap_last /= (SECTOR_SIZE / sizeof(uint32_t));
> n_sectors = bmap_last - bmap_first + 1;
> - offset = s->bmap_sector + bmap_first;
> + offs = s->bmap_sector + bmap_first;
> base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
> logout("will write %u block map sectors starting from entry %u\n",
> n_sectors, bmap_first);
> - ret = bdrv_co_pwrite(bs->file, offset * SECTOR_SIZE,
> + ret = bdrv_co_pwrite(bs->file, offs * SECTOR_SIZE,
> n_sectors * SECTOR_SIZE, base, 0);
> }
Having two variables 'offset' and 'offs' doesn't really help with
clarity either. Can we be more specific and use something like
'bmap_offset' here?
Kevin
Kevin Wolf <kwolf@redhat.com> writes:
> Am 31.08.2023 um 15:25 hat Markus Armbruster geschrieben:
>> Local variables shadowing other local variables or parameters make the
>> code needlessly hard to understand. Tracked down with -Wshadow=local.
>> Clean up: delete inner declarations when they are actually redundant,
>> else rename variables.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>> @@ -700,7 +699,7 @@ nonallocating_write:
>> /* One or more new blocks were allocated. */
>> VdiHeader *header;
>> uint8_t *base;
>> - uint64_t offset;
>> + uint64_t offs;
>> uint32_t n_sectors;
>>
>> g_free(block);
>> @@ -723,11 +722,11 @@ nonallocating_write:
>> bmap_first /= (SECTOR_SIZE / sizeof(uint32_t));
>> bmap_last /= (SECTOR_SIZE / sizeof(uint32_t));
>> n_sectors = bmap_last - bmap_first + 1;
>> - offset = s->bmap_sector + bmap_first;
>> + offs = s->bmap_sector + bmap_first;
>> base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
>> logout("will write %u block map sectors starting from entry %u\n",
>> n_sectors, bmap_first);
>> - ret = bdrv_co_pwrite(bs->file, offset * SECTOR_SIZE,
>> + ret = bdrv_co_pwrite(bs->file, offs * SECTOR_SIZE,
>> n_sectors * SECTOR_SIZE, base, 0);
>> }
>
> Having two variables 'offset' and 'offs' doesn't really help with
> clarity either. Can we be more specific and use something like
> 'bmap_offset' here?
Sure!
On Thu, Aug 31, 2023 at 03:25:44PM +0200, Markus Armbruster wrote: > Local variables shadowing other local variables or parameters make the > code needlessly hard to understand. Tracked down with -Wshadow=local. > Clean up: delete inner declarations when they are actually redundant, > else rename variables. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/vdi.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Local variables shadowing other local variables or parameters make the
code needlessly hard to understand. Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block.c | 7 ++++---
block/rbd.c | 2 +-
block/stream.c | 1 -
block/vvfat.c | 34 +++++++++++++++++-----------------
hw/block/xen-block.c | 6 +++---
5 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/block.c b/block.c
index a307c151a8..7f0003d8ac 100644
--- a/block.c
+++ b/block.c
@@ -3001,7 +3001,8 @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,
BdrvChildRole child_role,
uint64_t perm, uint64_t shared_perm,
void *opaque,
- Transaction *tran, Error **errp)
+ Transaction *transaction,
+ Error **errp)
{
BdrvChild *new_child;
AioContext *parent_ctx, *new_child_ctx;
@@ -3088,7 +3089,7 @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,
.old_parent_ctx = parent_ctx,
.old_child_ctx = child_ctx,
};
- tran_add(tran, &bdrv_attach_child_common_drv, s);
+ tran_add(transaction, &bdrv_attach_child_common_drv, s);
if (new_child_ctx != child_ctx) {
aio_context_release(new_child_ctx);
@@ -6073,12 +6074,12 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
QLIST_FOREACH(drv, &bdrv_drivers, list) {
if (drv->format_name) {
bool found = false;
- int i = count;
if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, read_only)) {
continue;
}
+ i = count;
while (formats && i && !found) {
found = !strcmp(formats[--i], drv->format_name);
}
diff --git a/block/rbd.c b/block/rbd.c
index 978671411e..472ca05cba 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1290,7 +1290,7 @@ static int coroutine_fn qemu_rbd_start_co(BlockDriverState *bs,
* operations that exceed the current size.
*/
if (offset + bytes > s->image_size) {
- int r = qemu_rbd_resize(bs, offset + bytes);
+ r = qemu_rbd_resize(bs, offset + bytes);
if (r < 0) {
return r;
}
diff --git a/block/stream.c b/block/stream.c
index e522bbdec5..007253880b 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -282,7 +282,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
/* Make sure that the image is opened in read-write mode */
bs_read_only = bdrv_is_read_only(bs);
if (bs_read_only) {
- int ret;
/* Hold the chain during reopen */
if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
return;
diff --git a/block/vvfat.c b/block/vvfat.c
index 0ddc91fc09..d7425ee602 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -777,7 +777,6 @@ static int read_directory(BDRVVVFATState* s, int mapping_index)
while((entry=readdir(dir))) {
unsigned int length=strlen(dirname)+2+strlen(entry->d_name);
char* buffer;
- direntry_t* direntry;
struct stat st;
int is_dot=!strcmp(entry->d_name,".");
int is_dotdot=!strcmp(entry->d_name,"..");
@@ -857,7 +856,7 @@ static int read_directory(BDRVVVFATState* s, int mapping_index)
/* fill with zeroes up to the end of the cluster */
while(s->directory.next%(0x10*s->sectors_per_cluster)) {
- direntry_t* direntry=array_get_next(&(s->directory));
+ direntry = array_get_next(&(s->directory));
memset(direntry,0,sizeof(direntry_t));
}
@@ -1962,24 +1961,24 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, direntry_t* direntry, const ch
* This is horribly inefficient, but that is okay, since
* it is rarely executed, if at all.
*/
- int64_t offset = cluster2sector(s, cluster_num);
+ int64_t offs = cluster2sector(s, cluster_num);
vvfat_close_current_file(s);
for (i = 0; i < s->sectors_per_cluster; i++) {
int res;
res = bdrv_is_allocated(s->qcow->bs,
- (offset + i) * BDRV_SECTOR_SIZE,
+ (offs + i) * BDRV_SECTOR_SIZE,
BDRV_SECTOR_SIZE, NULL);
if (res < 0) {
return -1;
}
if (!res) {
- res = vvfat_read(s->bs, offset, s->cluster_buffer, 1);
+ res = vvfat_read(s->bs, offs, s->cluster_buffer, 1);
if (res) {
return -1;
}
- res = bdrv_co_pwrite(s->qcow, offset * BDRV_SECTOR_SIZE,
+ res = bdrv_co_pwrite(s->qcow, offs * BDRV_SECTOR_SIZE,
BDRV_SECTOR_SIZE, s->cluster_buffer,
0);
if (res < 0) {
@@ -2467,8 +2466,9 @@ commit_direntries(BDRVVVFATState* s, int dir_index, int parent_mapping_index)
for (c = first_cluster; !fat_eof(s, c); c = modified_fat_get(s, c)) {
direntry_t *first_direntry;
- void* direntry = array_get(&(s->directory), current_dir_index);
- int ret = vvfat_read(s->bs, cluster2sector(s, c), direntry,
+
+ direntry = array_get(&(s->directory), current_dir_index);
+ ret = vvfat_read(s->bs, cluster2sector(s, c), (uint8_t *)direntry,
s->sectors_per_cluster);
if (ret)
return ret;
@@ -2690,12 +2690,12 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
direntry_t* direntry = array_get(&(s->directory),
mapping->info.dir.first_dir_index);
uint32_t c = mapping->begin;
- int i = 0;
+ int j = 0;
/* recurse */
while (!fat_eof(s, c)) {
do {
- direntry_t* d = direntry + i;
+ direntry_t* d = direntry + j;
if (is_file(d) || (is_directory(d) && !is_dot(d))) {
int l;
@@ -2716,8 +2716,8 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s)
schedule_rename(s, m->begin, new_path);
}
- i++;
- } while((i % (0x10 * s->sectors_per_cluster)) != 0);
+ j++;
+ } while((j % (0x10 * s->sectors_per_cluster)) != 0);
c = fat_get(s, c);
}
}
@@ -2804,16 +2804,16 @@ static int coroutine_fn GRAPH_RDLOCK handle_commits(BDRVVVFATState* s)
int begin = commit->param.new_file.first_cluster;
mapping_t* mapping = find_mapping_for_cluster(s, begin);
direntry_t* entry;
- int i;
+ int j;
/* find direntry */
- for (i = 0; i < s->directory.next; i++) {
- entry = array_get(&(s->directory), i);
+ for (j = 0; j < s->directory.next; j++) {
+ entry = array_get(&(s->directory), j);
if (is_file(entry) && begin_of_direntry(entry) == begin)
break;
}
- if (i >= s->directory.next) {
+ if (j >= s->directory.next) {
fail = -6;
continue;
}
@@ -2833,7 +2833,7 @@ static int coroutine_fn GRAPH_RDLOCK handle_commits(BDRVVVFATState* s)
mapping->mode = MODE_NORMAL;
mapping->info.file.offset = 0;
- if (commit_one_file(s, i, 0))
+ if (commit_one_file(s, j, 0))
fail = -7;
break;
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 3906b9058b..a07cd7eb5d 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -369,7 +369,7 @@ static void xen_block_get_vdev(Object *obj, Visitor *v, const char *name,
case XEN_BLOCK_VDEV_TYPE_XVD:
case XEN_BLOCK_VDEV_TYPE_HD:
case XEN_BLOCK_VDEV_TYPE_SD: {
- char *name = disk_to_vbd_name(vdev->disk);
+ char *vbd_name = disk_to_vbd_name(vdev->disk);
str = g_strdup_printf("%s%s%lu",
(vdev->type == XEN_BLOCK_VDEV_TYPE_XVD) ?
@@ -377,8 +377,8 @@ static void xen_block_get_vdev(Object *obj, Visitor *v, const char *name,
(vdev->type == XEN_BLOCK_VDEV_TYPE_HD) ?
"hd" :
"sd",
- name, vdev->partition);
- g_free(name);
+ vbd_name, vdev->partition);
+ g_free(vbd_name);
break;
}
default:
--
2.41.0Am 31.08.2023 um 15:25 hat Markus Armbruster geschrieben:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand. Tracked down with -Wshadow=local.
> Clean up: delete inner declarations when they are actually redundant,
> else rename variables.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block.c | 7 ++++---
> block/rbd.c | 2 +-
> block/stream.c | 1 -
> block/vvfat.c | 34 +++++++++++++++++-----------------
> hw/block/xen-block.c | 6 +++---
> 5 files changed, 25 insertions(+), 25 deletions(-)
I wonder why you made vdi a separate patch, but not vvfat, even though
that has more changes. (Of course, my selfish motivation for asking this
is that I could have given a R-b for it and wouldn't have to look at it
again in a v2 :-))
> diff --git a/block.c b/block.c
> index a307c151a8..7f0003d8ac 100644
> --- a/block.c
> +++ b/block.c
> @@ -3001,7 +3001,8 @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,
> BdrvChildRole child_role,
> uint64_t perm, uint64_t shared_perm,
> void *opaque,
> - Transaction *tran, Error **errp)
> + Transaction *transaction,
> + Error **errp)
> {
> BdrvChild *new_child;
> AioContext *parent_ctx, *new_child_ctx;
> @@ -3088,7 +3089,7 @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,
> .old_parent_ctx = parent_ctx,
> .old_child_ctx = child_ctx,
> };
> - tran_add(tran, &bdrv_attach_child_common_drv, s);
> + tran_add(transaction, &bdrv_attach_child_common_drv, s);
>
> if (new_child_ctx != child_ctx) {
> aio_context_release(new_child_ctx);
I think I would resolve this one the other way around. 'tran' is the
typical name for the parameter and it is the transaction that this
function should add things to.
The other one that shadows it is a local transaction that is completed
within the function. I think it's better if that one has a different
name.
As usual, being more specific than just 'tran' vs. 'transaction' would
be nice. Maybe 'aio_ctx_tran' for the nested one?
The rest looks okay.
Kevin
Kevin Wolf <kwolf@redhat.com> writes:
> Am 31.08.2023 um 15:25 hat Markus Armbruster geschrieben:
>> Local variables shadowing other local variables or parameters make the
>> code needlessly hard to understand. Tracked down with -Wshadow=local.
>> Clean up: delete inner declarations when they are actually redundant,
>> else rename variables.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> block.c | 7 ++++---
>> block/rbd.c | 2 +-
>> block/stream.c | 1 -
>> block/vvfat.c | 34 +++++++++++++++++-----------------
>> hw/block/xen-block.c | 6 +++---
>> 5 files changed, 25 insertions(+), 25 deletions(-)
>
> I wonder why you made vdi a separate patch, but not vvfat, even though
> that has more changes. (Of course, my selfish motivation for asking this
> is that I could have given a R-b for it and wouldn't have to look at it
> again in a v2 :-))
I split by maintainer. The files changed by this patch are only covered
by "Block layer core".
>> diff --git a/block.c b/block.c
>> index a307c151a8..7f0003d8ac 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3001,7 +3001,8 @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,
>> BdrvChildRole child_role,
>> uint64_t perm, uint64_t shared_perm,
>> void *opaque,
>> - Transaction *tran, Error **errp)
>> + Transaction *transaction,
>> + Error **errp)
>> {
>> BdrvChild *new_child;
>> AioContext *parent_ctx, *new_child_ctx;
>> @@ -3088,7 +3089,7 @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,
>> .old_parent_ctx = parent_ctx,
>> .old_child_ctx = child_ctx,
>> };
>> - tran_add(tran, &bdrv_attach_child_common_drv, s);
>> + tran_add(transaction, &bdrv_attach_child_common_drv, s);
>>
>> if (new_child_ctx != child_ctx) {
>> aio_context_release(new_child_ctx);
>
> I think I would resolve this one the other way around. 'tran' is the
> typical name for the parameter and it is the transaction that this
> function should add things to.
>
> The other one that shadows it is a local transaction that is completed
> within the function. I think it's better if that one has a different
> name.
>
> As usual, being more specific than just 'tran' vs. 'transaction' would
> be nice. Maybe 'aio_ctx_tran' for the nested one?
Can do.
> The rest looks okay.
Thanks!
On Thu, Aug 31, 2023 at 3:25 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand. Tracked down with -Wshadow=local.
> Clean up: delete inner declarations when they are actually redundant,
> else rename variables.
For rbd
> block/rbd.c | 2 +-
Acked-by: Ilya Dryomov <idryomov@gmail.com>
Thanks,
Ilya
On Thu, Aug 31, 2023 at 03:25:45PM +0200, Markus Armbruster wrote:
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 3906b9058b..a07cd7eb5d 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -369,7 +369,7 @@ static void xen_block_get_vdev(Object *obj, Visitor *v, const char *name,
> case XEN_BLOCK_VDEV_TYPE_XVD:
> case XEN_BLOCK_VDEV_TYPE_HD:
> case XEN_BLOCK_VDEV_TYPE_SD: {
> - char *name = disk_to_vbd_name(vdev->disk);
> + char *vbd_name = disk_to_vbd_name(vdev->disk);
>
> str = g_strdup_printf("%s%s%lu",
> (vdev->type == XEN_BLOCK_VDEV_TYPE_XVD) ?
> @@ -377,8 +377,8 @@ static void xen_block_get_vdev(Object *obj, Visitor *v, const char *name,
> (vdev->type == XEN_BLOCK_VDEV_TYPE_HD) ?
> "hd" :
> "sd",
> - name, vdev->partition);
> - g_free(name);
> + vbd_name, vdev->partition);
> + g_free(vbd_name);
> break;
> }
> default:
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Thanks,
--
Anthony PERARD
On Thu, Aug 31, 2023 at 03:25:45PM +0200, Markus Armbruster wrote: > Local variables shadowing other local variables or parameters make the > code needlessly hard to understand. Tracked down with -Wshadow=local. > Clean up: delete inner declarations when they are actually redundant, > else rename variables. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block.c | 7 ++++--- > block/rbd.c | 2 +- > block/stream.c | 1 - > block/vvfat.c | 34 +++++++++++++++++----------------- > hw/block/xen-block.c | 6 +++--- > 5 files changed, 25 insertions(+), 25 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Variables declared in macros can shadow other variables. Much of the
time, this is harmless, e.g.:
#define _FDT(exp) \
do { \
int ret = (exp); \
if (ret < 0) { \
error_report("error creating device tree: %s: %s", \
#exp, fdt_strerror(ret)); \
exit(1); \
} \
} while (0)
Harmless shadowing in h_client_architecture_support():
target_ulong ret;
[...]
ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize);
if (ret == H_SUCCESS) {
_FDT((fdt_pack(spapr->fdt_blob)));
[...]
}
return ret;
However, we can get in trouble when the shadowed variable is used in a
macro argument:
#define QOBJECT(obj) ({ \
typeof(obj) o = (obj); \
o ? container_of(&(o)->base, QObject, base) : NULL; \
})
QOBJECT(o) expands into
({
---> typeof(o) o = (o);
o ? container_of(&(o)->base, QObject, base) : NULL;
})
Unintended variable name capture at --->. We'd be saved by
-Winit-self. But I could certainly construct more elaborate death
traps that don't trigger it.
To reduce the risk of trapping ourselves, we use variable names in
macros that no sane person would use elsewhere. Here's our actual
definition of QOBJECT():
#define QOBJECT(obj) ({ \
typeof(obj) _obj = (obj); \
_obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
})
Works well enough until we nest macro calls. For instance, with
#define qobject_ref(obj) ({ \
typeof(obj) _obj = (obj); \
qobject_ref_impl(QOBJECT(_obj)); \
_obj; \
})
the expression qobject_ref(obj) expands into
({
typeof(obj) _obj = (obj);
qobject_ref_impl(
({
---> typeof(_obj) _obj = (_obj);
_obj ? container_of(&(_obj)->base, QObject, base) : NULL;
}));
_obj;
})
Unintended variable name capture at --->.
The only reliable way to prevent unintended variable name capture is
-Wshadow.
One blocker for enabling it is shadowing hiding in function-like
macros like
qdict_put(dict, "name", qobject_ref(...))
qdict_put() wraps its last argument in QOBJECT(), and the last
argument here contains another QOBJECT().
Use dark preprocessor sorcery to make the macros that give us this
problem use different variable names on every call.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
include/qapi/qmp/qobject.h | 8 +++++---
include/qemu/atomic.h | 11 ++++++-----
include/qemu/osdep.h | 34 +++++++++++++++++++---------------
3 files changed, 30 insertions(+), 23 deletions(-)
diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
index 9003b71fd3..7b50fc905d 100644
--- a/include/qapi/qmp/qobject.h
+++ b/include/qapi/qmp/qobject.h
@@ -45,10 +45,12 @@ struct QObject {
struct QObjectBase_ base;
};
-#define QOBJECT(obj) ({ \
- typeof(obj) _obj = (obj); \
- _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
+#define QOBJECT_INTERNAL(obj, l) ({ \
+ typeof(obj) PASTE(_obj, l) = (obj); \
+ PASTE(_obj, l) \
+ ? container_of(&(PASTE(_obj, l))->base, QObject, base) : NULL; \
})
+#define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__)
/* Required for qobject_to() */
#define QTYPE_CAST_TO_QNull QTYPE_QNULL
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index d95612f7a0..3f80ffac69 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -157,13 +157,14 @@
smp_read_barrier_depends();
#endif
-#define qatomic_rcu_read(ptr) \
- ({ \
+#define qatomic_rcu_read_internal(ptr, l) \
+ ({ \
qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
- typeof_strip_qual(*ptr) _val; \
- qatomic_rcu_read__nocheck(ptr, &_val); \
- _val; \
+ typeof_strip_qual(*ptr) PASTE(_val, l); \
+ qatomic_rcu_read__nocheck(ptr, &PASTE(_val, l)); \
+ PASTE(_val, l); \
})
+#define qatomic_rcu_read(ptr) qatomic_rcu_read_internal((ptr), __COUNTER__)
#define qatomic_rcu_set(ptr, i) do { \
qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 21ef8f1699..9c191ebe99 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -371,18 +371,21 @@ void QEMU_ERROR("code path is reachable")
* have to open-code it. Sadly, Coverity is severely confused by the
* constant variants, so we have to dumb things down there.
*/
+#define PASTE(a, b) a##b
+#define MIN_INTERNAL(a, b, l) \
+ ({ \
+ typeof(1 ? (a) : (b)) PASTE(_a, l) = (a), PASTE(_b, l) = (b); \
+ PASTE(_a, l) < PASTE(_b, l) ? PASTE(_a, l) : PASTE(_b, l); \
+ })
#undef MIN
-#define MIN(a, b) \
- ({ \
- typeof(1 ? (a) : (b)) _a = (a), _b = (b); \
- _a < _b ? _a : _b; \
+#define MIN(a, b) MIN_INTERNAL((a), (b), __COUNTER__)
+#define MAX_INTERNAL(a, b, l) \
+ ({ \
+ typeof(1 ? (a) : (b)) PASTE(_a, l) = (a), PASTE(_b, l) = (b); \
+ PASTE(_a, l) > PASTE(_b, l) ? PASTE(_a, l) : PASTE(_b, l); \
})
#undef MAX
-#define MAX(a, b) \
- ({ \
- typeof(1 ? (a) : (b)) _a = (a), _b = (b); \
- _a > _b ? _a : _b; \
- })
+#define MAX(a, b) MAX_INTERNAL((a), (b), __COUNTER__)
#ifdef __COVERITY__
# define MIN_CONST(a, b) ((a) < (b) ? (a) : (b))
@@ -404,13 +407,14 @@ void QEMU_ERROR("code path is reachable")
* Minimum function that returns zero only if both values are zero.
* Intended for use with unsigned values only.
*/
-#ifndef MIN_NON_ZERO
-#define MIN_NON_ZERO(a, b) \
- ({ \
- typeof(1 ? (a) : (b)) _a = (a), _b = (b); \
- _a == 0 ? _b : (_b == 0 || _b > _a) ? _a : _b; \
+#define MIN_NON_ZERO_INTERNAL(a, b, l) \
+ ({ \
+ typeof(1 ? (a) : (b)) PASTE(_a, l) = (a), PASTE(_b, l) = (b); \
+ PASTE(_a, l) == 0 ? PASTE(_b, l) \
+ : (PASTE(_b, l) == 0 || PASTE(_b, l) > PASTE(_a, l)) ? PASTE(_a, l) \
+ : PASTE(_b, l); \
})
-#endif
+#define MIN_NON_ZERO(a, b) MIN_NON_ZERO_INTERNAL((a), (b), __COUNTER__)
/*
* Round number down to multiple. Safe when m is not a power of 2 (see
--
2.41.0On 8/31/23 06:25, Markus Armbruster wrote: > +#define PASTE(a, b) a##b We already have glue() in qemu/compiler.h. The rest of it looks quite sensible. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 8/31/23 06:25, Markus Armbruster wrote: >> +#define PASTE(a, b) a##b > > We already have glue() in qemu/compiler.h. Missed it, will fix. > The rest of it looks quite sensible. Thanks!
On Thu, Aug 31, 2023 at 03:25:46PM +0200, Markus Armbruster wrote:
[This paragraph written last: Bear with my stream of consciousness
review below, where I end up duplicating some of the conslusions you
reached before the point where I saw where the patch was headed]
> Variables declared in macros can shadow other variables. Much of the
> time, this is harmless, e.g.:
>
> #define _FDT(exp) \
> do { \
> int ret = (exp); \
> if (ret < 0) { \
> error_report("error creating device tree: %s: %s", \
> #exp, fdt_strerror(ret)); \
> exit(1); \
> } \
> } while (0)
Which is why I've seen some projects require a strict namespace
separation: if all macro parameters and any identifiers declared in
macros use either a leading or a trailing _ (I prefer a trailing one,
to avoid risking conflicts with libc reserved namespace; but leading
is usually okay), and all other identifiers avoid that namespace, then
you will never have shadowing by calling a macro from normal code.
>
> Harmless shadowing in h_client_architecture_support():
>
> target_ulong ret;
>
> [...]
>
> ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize);
> if (ret == H_SUCCESS) {
> _FDT((fdt_pack(spapr->fdt_blob)));
> [...]
> }
>
> return ret;
>
> However, we can get in trouble when the shadowed variable is used in a
> macro argument:
>
> #define QOBJECT(obj) ({ \
> typeof(obj) o = (obj); \
> o ? container_of(&(o)->base, QObject, base) : NULL; \
> })
>
> QOBJECT(o) expands into
>
> ({
> ---> typeof(o) o = (o);
> o ? container_of(&(o)->base, QObject, base) : NULL;
> })
>
> Unintended variable name capture at --->. We'd be saved by
> -Winit-self. But I could certainly construct more elaborate death
> traps that don't trigger it.
Indeed, not fully understanding the preprocessor makes for some
interesting death traps.
>
> To reduce the risk of trapping ourselves, we use variable names in
> macros that no sane person would use elsewhere. Here's our actual
> definition of QOBJECT():
>
> #define QOBJECT(obj) ({ \
> typeof(obj) _obj = (obj); \
> _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
> })
Yep, goes back to the policy I've seen of enforcing naming conventions
that ensure macros can't shadow normal code.
>
> Works well enough until we nest macro calls. For instance, with
>
> #define qobject_ref(obj) ({ \
> typeof(obj) _obj = (obj); \
> qobject_ref_impl(QOBJECT(_obj)); \
> _obj; \
> })
>
> the expression qobject_ref(obj) expands into
>
> ({
> typeof(obj) _obj = (obj);
> qobject_ref_impl(
> ({
> ---> typeof(_obj) _obj = (_obj);
> _obj ? container_of(&(_obj)->base, QObject, base) : NULL;
> }));
> _obj;
> })
>
> Unintended variable name capture at --->.
Yep, you've just proven how macros calling macros requires care, as we
no longer have the namespace protections. If macro calls can only
form a DAG, it is possible to choose unique intermediate declarations
for every macro (although remembering to do that, and still keeping
the macro definition legible, may not be easy). But if you can have
macros that form any sort of nesting loop (and we WANT to allow things
like MIN(MIN(a, b), c) - which is such a loop), dynamic naming becomes
the only solution.
>
> The only reliable way to prevent unintended variable name capture is
> -Wshadow.
Yes, I would love to have that enabled eventually.
>
> One blocker for enabling it is shadowing hiding in function-like
> macros like
>
> qdict_put(dict, "name", qobject_ref(...))
>
> qdict_put() wraps its last argument in QOBJECT(), and the last
> argument here contains another QOBJECT().
>
> Use dark preprocessor sorcery to make the macros that give us this
> problem use different variable names on every call.
Sounds foreboding; hopefully not many macros are affected...
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> include/qapi/qmp/qobject.h | 8 +++++---
> include/qemu/atomic.h | 11 ++++++-----
> include/qemu/osdep.h | 34 +++++++++++++++++++---------------
> 3 files changed, 30 insertions(+), 23 deletions(-)
...okay, the size of the diffstat seems tolerable (good that we don't
have many macros that expand to a temporary variable declaration and
which are likely to be heavily reused from nested contexts).
>
> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
> index 9003b71fd3..7b50fc905d 100644
> --- a/include/qapi/qmp/qobject.h
> +++ b/include/qapi/qmp/qobject.h
> @@ -45,10 +45,12 @@ struct QObject {
> struct QObjectBase_ base;
> };
>
> -#define QOBJECT(obj) ({ \
> - typeof(obj) _obj = (obj); \
> - _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
> +#define QOBJECT_INTERNAL(obj, l) ({ \
> + typeof(obj) PASTE(_obj, l) = (obj); \
> + PASTE(_obj, l) \
> + ? container_of(&(PASTE(_obj, l))->base, QObject, base) : NULL; \
> })
> +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__)
Slick! Every call to QOBJECT() defines a unique temporary variable
name. But as written, QOBJECT(o) expands to this (when __COUNTER__ is
1):
({
typeof((o)) _obj1 = ((o));
_obj1 ? container_of(&(_obj1)->base, QObject, base) : NULL;
})
which has three sets of redundant parens that could be elided. Why do
you need to add () around obj when forwarding to QOBJECT_INTERNAL()?
The only way the expansion of the text passed through the 'obj'
parameter can contain a comma is if the user has already parenthesized
it on their end (not that I expect a comma expression to be a frequent
argument to QOBJECT(), but who knows). Arguing that it is to silence
a syntax checker is weak; since we must NOT add parens around the
parameters to QOBJECT_INTERNAL (calling PASTE((_obj), (l)) is
obviously wrong).
Meanwhile, the repetition of three calls to PASTE() is annoying. How
about:
#define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \
typeof _obj _tmp = _obj; \
_tmp ? container_of(&_tmp->base, QObject, base) : NULL; \
)}
#define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp)
#define QOBJECT_INTERNAL(_arg, _ctr) \
QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr))
#define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__)
or:
#define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \
typeof(_obj) _tmp = (_obj); \
_tmp ? container_of(&_tmp->base, QObject, base) : NULL; \
)}
#define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp)
#define QOBJECT_INTERNAL(_arg, _ctr) \
QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr))
#define QOBJECT(obj) QOBJECT_INTERNAL(obj, __COUNTER__)
where, in either case, QOBJECT(o) should then expand to
({
typeof (o) _obj1 = (o);
_obj1 ? container_of(&_obj1->base, QObject, base) : NULL;
})
>
> /* Required for qobject_to() */
> #define QTYPE_CAST_TO_QNull QTYPE_QNULL
> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
> index d95612f7a0..3f80ffac69 100644
> --- a/include/qemu/atomic.h
> +++ b/include/qemu/atomic.h
> @@ -157,13 +157,14 @@
> smp_read_barrier_depends();
> #endif
>
> -#define qatomic_rcu_read(ptr) \
> - ({ \
> +#define qatomic_rcu_read_internal(ptr, l) \
> + ({ \
> qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
> - typeof_strip_qual(*ptr) _val; \
> - qatomic_rcu_read__nocheck(ptr, &_val); \
> - _val; \
> + typeof_strip_qual(*ptr) PASTE(_val, l); \
> + qatomic_rcu_read__nocheck(ptr, &PASTE(_val, l)); \
> + PASTE(_val, l); \
> })
> +#define qatomic_rcu_read(ptr) qatomic_rcu_read_internal((ptr), __COUNTER__)
Same observation about being able to reduce the number of PASTE()
calls by adding yet another layer of macro invocations.
>
> #define qatomic_rcu_set(ptr, i) do { \
> qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 21ef8f1699..9c191ebe99 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -371,18 +371,21 @@ void QEMU_ERROR("code path is reachable")
> * have to open-code it. Sadly, Coverity is severely confused by the
> * constant variants, so we have to dumb things down there.
> */
> +#define PASTE(a, b) a##b
> +#define MIN_INTERNAL(a, b, l) \
> + ({ \
> + typeof(1 ? (a) : (b)) PASTE(_a, l) = (a), PASTE(_b, l) = (b); \
> + PASTE(_a, l) < PASTE(_b, l) ? PASTE(_a, l) : PASTE(_b, l); \
> + })
And again.
I think you are definitely on the right track to have all internal
variable declarations within a macro definition use multi-layered
expansion with the help of __COUNTER__ to ensure that the macro's
temporary variable is globally unique; so if you leave it as written,
I could live with:
Reviewed-by: Eric Blake <eblake@redhat.com>
But if you respin it to pick up any of my suggestions, I'll definitely
spend another review cycle on the result.
If it helps, here's what I ended up using in nbdkit:
https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/unique-name.h#L36
/* https://stackoverflow.com/a/1597129
* https://stackoverflow.com/a/12711226
*/
#define XXUNIQUE_NAME(name, counter) name ## counter
#define XUNIQUE_NAME(name, counter) XXUNIQUE_NAME (name, counter)
#define NBDKIT_UNIQUE_NAME(name) XUNIQUE_NAME (name, __COUNTER__)
https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/minmax.h#L47
#include "unique-name.h"
#undef MIN
#define MIN(x, y) \
MIN_1 ((x), (y), NBDKIT_UNIQUE_NAME (_x), NBDKIT_UNIQUE_NAME (_y))
...
#define MIN_1(x, y, _x, _y) ({ \
__auto_type _x = (x); \
__auto_type _y = (y); \
_x < _y ? _x : _y; \
})
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
On 8/31/23 16:30, Eric Blake wrote:
> On Thu, Aug 31, 2023 at 03:25:46PM +0200, Markus Armbruster wrote:
>
> [This paragraph written last: Bear with my stream of consciousness
> review below, where I end up duplicating some of the conslusions you
> reached before the point where I saw where the patch was headed]
>
>> Variables declared in macros can shadow other variables. Much of the
>> time, this is harmless, e.g.:
>>
>> #define _FDT(exp) \
>> do { \
>> int ret = (exp); \
>> if (ret < 0) { \
>> error_report("error creating device tree: %s: %s", \
>> #exp, fdt_strerror(ret)); \
>> exit(1); \
>> } \
>> } while (0)
>
> Which is why I've seen some projects require a strict namespace
> separation: if all macro parameters and any identifiers declared in
> macros use either a leading or a trailing _ (I prefer a trailing one,
> to avoid risking conflicts with libc reserved namespace; but leading
> is usually okay), and all other identifiers avoid that namespace, then
> you will never have shadowing by calling a macro from normal code.
I started fixing the _FDT() macro since it is quite noisy at compile.
Same for qemu_fdt_setprop_cells(). So are we ok with names like 'ret_'
and 'i_' ? I used a 'local_' prefix for now but I can change.
I also have a bunch of fixes for ppc.
C.
>
>>
>> Harmless shadowing in h_client_architecture_support():
>>
>> target_ulong ret;
>>
>> [...]
>>
>> ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize);
>> if (ret == H_SUCCESS) {
>> _FDT((fdt_pack(spapr->fdt_blob)));
>> [...]
>> }
>>
>> return ret;
>>
>> However, we can get in trouble when the shadowed variable is used in a
>> macro argument:
>>
>> #define QOBJECT(obj) ({ \
>> typeof(obj) o = (obj); \
>> o ? container_of(&(o)->base, QObject, base) : NULL; \
>> })
>>
>> QOBJECT(o) expands into
>>
>> ({
>> ---> typeof(o) o = (o);
>> o ? container_of(&(o)->base, QObject, base) : NULL;
>> })
>>
>> Unintended variable name capture at --->. We'd be saved by
>> -Winit-self. But I could certainly construct more elaborate death
>> traps that don't trigger it.
>
> Indeed, not fully understanding the preprocessor makes for some
> interesting death traps.
>
>>
>> To reduce the risk of trapping ourselves, we use variable names in
>> macros that no sane person would use elsewhere. Here's our actual
>> definition of QOBJECT():
>>
>> #define QOBJECT(obj) ({ \
>> typeof(obj) _obj = (obj); \
>> _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
>> })
>
> Yep, goes back to the policy I've seen of enforcing naming conventions
> that ensure macros can't shadow normal code.
>
>>
>> Works well enough until we nest macro calls. For instance, with
>>
>> #define qobject_ref(obj) ({ \
>> typeof(obj) _obj = (obj); \
>> qobject_ref_impl(QOBJECT(_obj)); \
>> _obj; \
>> })
>>
>> the expression qobject_ref(obj) expands into
>>
>> ({
>> typeof(obj) _obj = (obj);
>> qobject_ref_impl(
>> ({
>> ---> typeof(_obj) _obj = (_obj);
>> _obj ? container_of(&(_obj)->base, QObject, base) : NULL;
>> }));
>> _obj;
>> })
>>
>> Unintended variable name capture at --->.
>
> Yep, you've just proven how macros calling macros requires care, as we
> no longer have the namespace protections. If macro calls can only
> form a DAG, it is possible to choose unique intermediate declarations
> for every macro (although remembering to do that, and still keeping
> the macro definition legible, may not be easy). But if you can have
> macros that form any sort of nesting loop (and we WANT to allow things
> like MIN(MIN(a, b), c) - which is such a loop), dynamic naming becomes
> the only solution.
>
>>
>> The only reliable way to prevent unintended variable name capture is
>> -Wshadow.
>
> Yes, I would love to have that enabled eventually.
>
>>
>> One blocker for enabling it is shadowing hiding in function-like
>> macros like
>>
>> qdict_put(dict, "name", qobject_ref(...))
>>
>> qdict_put() wraps its last argument in QOBJECT(), and the last
>> argument here contains another QOBJECT().
>>
>> Use dark preprocessor sorcery to make the macros that give us this
>> problem use different variable names on every call.
>
> Sounds foreboding; hopefully not many macros are affected...
>
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> include/qapi/qmp/qobject.h | 8 +++++---
>> include/qemu/atomic.h | 11 ++++++-----
>> include/qemu/osdep.h | 34 +++++++++++++++++++---------------
>> 3 files changed, 30 insertions(+), 23 deletions(-)
>
> ...okay, the size of the diffstat seems tolerable (good that we don't
> have many macros that expand to a temporary variable declaration and
> which are likely to be heavily reused from nested contexts).
>
>>
>> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
>> index 9003b71fd3..7b50fc905d 100644
>> --- a/include/qapi/qmp/qobject.h
>> +++ b/include/qapi/qmp/qobject.h
>> @@ -45,10 +45,12 @@ struct QObject {
>> struct QObjectBase_ base;
>> };
>>
>> -#define QOBJECT(obj) ({ \
>> - typeof(obj) _obj = (obj); \
>> - _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
>> +#define QOBJECT_INTERNAL(obj, l) ({ \
>> + typeof(obj) PASTE(_obj, l) = (obj); \
>> + PASTE(_obj, l) \
>> + ? container_of(&(PASTE(_obj, l))->base, QObject, base) : NULL; \
>> })
>> +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__)
>
> Slick! Every call to QOBJECT() defines a unique temporary variable
> name. But as written, QOBJECT(o) expands to this (when __COUNTER__ is
> 1):
>
> ({
> typeof((o)) _obj1 = ((o));
> _obj1 ? container_of(&(_obj1)->base, QObject, base) : NULL;
> })
>
> which has three sets of redundant parens that could be elided. Why do
> you need to add () around obj when forwarding to QOBJECT_INTERNAL()?
> The only way the expansion of the text passed through the 'obj'
> parameter can contain a comma is if the user has already parenthesized
> it on their end (not that I expect a comma expression to be a frequent
> argument to QOBJECT(), but who knows). Arguing that it is to silence
> a syntax checker is weak; since we must NOT add parens around the
> parameters to QOBJECT_INTERNAL (calling PASTE((_obj), (l)) is
> obviously wrong).
>
> Meanwhile, the repetition of three calls to PASTE() is annoying. How
> about:
>
> #define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \
> typeof _obj _tmp = _obj; \
> _tmp ? container_of(&_tmp->base, QObject, base) : NULL; \
> )}
> #define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp)
> #define QOBJECT_INTERNAL(_arg, _ctr) \
> QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr))
> #define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__)
>
> or:
>
> #define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \
> typeof(_obj) _tmp = (_obj); \
> _tmp ? container_of(&_tmp->base, QObject, base) : NULL; \
> )}
> #define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp)
> #define QOBJECT_INTERNAL(_arg, _ctr) \
> QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr))
> #define QOBJECT(obj) QOBJECT_INTERNAL(obj, __COUNTER__)
>
> where, in either case, QOBJECT(o) should then expand to
>
> ({
> typeof (o) _obj1 = (o);
> _obj1 ? container_of(&_obj1->base, QObject, base) : NULL;
> })
>
>>
>> /* Required for qobject_to() */
>> #define QTYPE_CAST_TO_QNull QTYPE_QNULL
>> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
>> index d95612f7a0..3f80ffac69 100644
>> --- a/include/qemu/atomic.h
>> +++ b/include/qemu/atomic.h
>> @@ -157,13 +157,14 @@
>> smp_read_barrier_depends();
>> #endif
>>
>> -#define qatomic_rcu_read(ptr) \
>> - ({ \
>> +#define qatomic_rcu_read_internal(ptr, l) \
>> + ({ \
>> qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
>> - typeof_strip_qual(*ptr) _val; \
>> - qatomic_rcu_read__nocheck(ptr, &_val); \
>> - _val; \
>> + typeof_strip_qual(*ptr) PASTE(_val, l); \
>> + qatomic_rcu_read__nocheck(ptr, &PASTE(_val, l)); \
>> + PASTE(_val, l); \
>> })
>> +#define qatomic_rcu_read(ptr) qatomic_rcu_read_internal((ptr), __COUNTER__)
>
> Same observation about being able to reduce the number of PASTE()
> calls by adding yet another layer of macro invocations.
>
>>
>> #define qatomic_rcu_set(ptr, i) do { \
>> qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index 21ef8f1699..9c191ebe99 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -371,18 +371,21 @@ void QEMU_ERROR("code path is reachable")
>> * have to open-code it. Sadly, Coverity is severely confused by the
>> * constant variants, so we have to dumb things down there.
>> */
>> +#define PASTE(a, b) a##b
>> +#define MIN_INTERNAL(a, b, l) \
>> + ({ \
>> + typeof(1 ? (a) : (b)) PASTE(_a, l) = (a), PASTE(_b, l) = (b); \
>> + PASTE(_a, l) < PASTE(_b, l) ? PASTE(_a, l) : PASTE(_b, l); \
>> + })
>
> And again.
>
> I think you are definitely on the right track to have all internal
> variable declarations within a macro definition use multi-layered
> expansion with the help of __COUNTER__ to ensure that the macro's
> temporary variable is globally unique; so if you leave it as written,
> I could live with:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> But if you respin it to pick up any of my suggestions, I'll definitely
> spend another review cycle on the result.
>
> If it helps, here's what I ended up using in nbdkit:
>
> https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/unique-name.h#L36
> /* https://stackoverflow.com/a/1597129
> * https://stackoverflow.com/a/12711226
> */
> #define XXUNIQUE_NAME(name, counter) name ## counter
> #define XUNIQUE_NAME(name, counter) XXUNIQUE_NAME (name, counter)
> #define NBDKIT_UNIQUE_NAME(name) XUNIQUE_NAME (name, __COUNTER__)
>
> https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/minmax.h#L47
> #include "unique-name.h"
>
> #undef MIN
> #define MIN(x, y) \
> MIN_1 ((x), (y), NBDKIT_UNIQUE_NAME (_x), NBDKIT_UNIQUE_NAME (_y))
>
> ...
> #define MIN_1(x, y, _x, _y) ({ \
> __auto_type _x = (x); \
> __auto_type _y = (y); \
> _x < _y ? _x : _y; \
> })
>
Cédric Le Goater <clg@kaod.org> writes:
> On 8/31/23 16:30, Eric Blake wrote:
>> On Thu, Aug 31, 2023 at 03:25:46PM +0200, Markus Armbruster wrote:
>> [This paragraph written last: Bear with my stream of consciousness
>> review below, where I end up duplicating some of the conslusions you
>> reached before the point where I saw where the patch was headed]
>>
>>> Variables declared in macros can shadow other variables. Much of the
>>> time, this is harmless, e.g.:
>>>
>>> #define _FDT(exp) \
>>> do { \
>>> int ret = (exp); \
>>> if (ret < 0) { \
>>> error_report("error creating device tree: %s: %s", \
>>> #exp, fdt_strerror(ret)); \
>>> exit(1); \
>>> } \
>>> } while (0)
>> Which is why I've seen some projects require a strict namespace
>> separation: if all macro parameters and any identifiers declared in
>> macros use either a leading or a trailing _ (I prefer a trailing one,
>> to avoid risking conflicts with libc reserved namespace; but leading
>> is usually okay), and all other identifiers avoid that namespace, then
>> you will never have shadowing by calling a macro from normal code.
>
> I started fixing the _FDT() macro since it is quite noisy at compile.
> Same for qemu_fdt_setprop_cells(). So are we ok with names like 'ret_'
> and 'i_' ? I used a 'local_' prefix for now but I can change.
I believe identifiers with a '_' suffix are just fine in macros. We
have quite a few of them already.
> I also have a bunch of fixes for ppc.
Appreciated!
On 9/1/23 16:50, Markus Armbruster wrote:
> Cédric Le Goater <clg@kaod.org> writes:
>
>> On 8/31/23 16:30, Eric Blake wrote:
>>> On Thu, Aug 31, 2023 at 03:25:46PM +0200, Markus Armbruster wrote:
>>> [This paragraph written last: Bear with my stream of consciousness
>>> review below, where I end up duplicating some of the conslusions you
>>> reached before the point where I saw where the patch was headed]
>>>
>>>> Variables declared in macros can shadow other variables. Much of the
>>>> time, this is harmless, e.g.:
>>>>
>>>> #define _FDT(exp) \
>>>> do { \
>>>> int ret = (exp); \
>>>> if (ret < 0) { \
>>>> error_report("error creating device tree: %s: %s", \
>>>> #exp, fdt_strerror(ret)); \
>>>> exit(1); \
>>>> } \
>>>> } while (0)
>>> Which is why I've seen some projects require a strict namespace
>>> separation: if all macro parameters and any identifiers declared in
>>> macros use either a leading or a trailing _ (I prefer a trailing one,
>>> to avoid risking conflicts with libc reserved namespace; but leading
>>> is usually okay), and all other identifiers avoid that namespace, then
>>> you will never have shadowing by calling a macro from normal code.
>>
>> I started fixing the _FDT() macro since it is quite noisy at compile.
>> Same for qemu_fdt_setprop_cells(). So are we ok with names like 'ret_'
>> and 'i_' ? I used a 'local_' prefix for now but I can change.
>
> I believe identifiers with a '_' suffix are just fine in macros. We
> have quite a few of them already.
ok
>> I also have a bunch of fixes for ppc.
>
> Appreciated!
count me on for the ppc files :
hw/ppc/pnv_psi.c
hw/ppc/spapr.c
hw/ppc/spapr_drc.c
hw/ppc/spapr_pci.c
include/hw/ppc/fdt.h
and surely some other files under target/ppc/
This one was taken care of by Phil:
include/sysemu/device_tree.h
C.
On 1/9/23 14:59, Cédric Le Goater wrote:
> On 8/31/23 16:30, Eric Blake wrote:
>> On Thu, Aug 31, 2023 at 03:25:46PM +0200, Markus Armbruster wrote:
>>
>> [This paragraph written last: Bear with my stream of consciousness
>> review below, where I end up duplicating some of the conslusions you
>> reached before the point where I saw where the patch was headed]
>>
>>> Variables declared in macros can shadow other variables. Much of the
>>> time, this is harmless, e.g.:
>>>
>>> #define
>>> _FDT(exp) \
>>> do
>>> { \
>>> int ret =
>>> (exp); \
>>> if (ret < 0)
>>> { \
>>> error_report("error creating device tree: %s: %s", \
>>> #exp,
>>> fdt_strerror(ret)); \
>>>
>>> exit(1); \
>>>
>>> } \
>>> } while (0)
>>
>> Which is why I've seen some projects require a strict namespace
>> separation: if all macro parameters and any identifiers declared in
>> macros use either a leading or a trailing _ (I prefer a trailing one,
>> to avoid risking conflicts with libc reserved namespace; but leading
>> is usually okay), and all other identifiers avoid that namespace, then
>> you will never have shadowing by calling a macro from normal code.
>
> I started fixing the _FDT() macro since it is quite noisy at compile.
> Same for qemu_fdt_setprop_cells(). So are we ok with names like 'ret_'
> and 'i_' ? I used a 'local_' prefix for now but I can change.
See
https://lore.kernel.org/qemu-devel/20230831225607.30829-12-philmd@linaro.org/
Eric Blake <eblake@redhat.com> writes:
> On Thu, Aug 31, 2023 at 03:25:46PM +0200, Markus Armbruster wrote:
>
> [This paragraph written last: Bear with my stream of consciousness
> review below, where I end up duplicating some of the conslusions you
> reached before the point where I saw where the patch was headed]
>
>> Variables declared in macros can shadow other variables. Much of the
>> time, this is harmless, e.g.:
>>
>> #define _FDT(exp) \
>> do { \
>> int ret = (exp); \
>> if (ret < 0) { \
>> error_report("error creating device tree: %s: %s", \
>> #exp, fdt_strerror(ret)); \
>> exit(1); \
>> } \
>> } while (0)
>
> Which is why I've seen some projects require a strict namespace
> separation: if all macro parameters and any identifiers declared in
> macros use either a leading or a trailing _ (I prefer a trailing one,
> to avoid risking conflicts with libc reserved namespace; but leading
> is usually okay), and all other identifiers avoid that namespace, then
> you will never have shadowing by calling a macro from normal code.
>
>>
>> Harmless shadowing in h_client_architecture_support():
>>
>> target_ulong ret;
>>
>> [...]
>>
>> ret = do_client_architecture_support(cpu, spapr, vec, fdt_bufsize);
>> if (ret == H_SUCCESS) {
>> _FDT((fdt_pack(spapr->fdt_blob)));
>> [...]
>> }
>>
>> return ret;
>>
>> However, we can get in trouble when the shadowed variable is used in a
>> macro argument:
>>
>> #define QOBJECT(obj) ({ \
>> typeof(obj) o = (obj); \
>> o ? container_of(&(o)->base, QObject, base) : NULL; \
>> })
>>
>> QOBJECT(o) expands into
>>
>> ({
>> ---> typeof(o) o = (o);
>> o ? container_of(&(o)->base, QObject, base) : NULL;
>> })
>>
>> Unintended variable name capture at --->. We'd be saved by
>> -Winit-self. But I could certainly construct more elaborate death
>> traps that don't trigger it.
>
> Indeed, not fully understanding the preprocessor makes for some
> interesting death traps.
We use ALL_CAPS for macros to signal "watch out for traps".
>> To reduce the risk of trapping ourselves, we use variable names in
>> macros that no sane person would use elsewhere. Here's our actual
>> definition of QOBJECT():
>>
>> #define QOBJECT(obj) ({ \
>> typeof(obj) _obj = (obj); \
>> _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
>> })
>
> Yep, goes back to the policy I've seen of enforcing naming conventions
> that ensure macros can't shadow normal code.
>
>>
>> Works well enough until we nest macro calls. For instance, with
>>
>> #define qobject_ref(obj) ({ \
>> typeof(obj) _obj = (obj); \
>> qobject_ref_impl(QOBJECT(_obj)); \
>> _obj; \
>> })
>>
>> the expression qobject_ref(obj) expands into
>>
>> ({
>> typeof(obj) _obj = (obj);
>> qobject_ref_impl(
>> ({
>> ---> typeof(_obj) _obj = (_obj);
>> _obj ? container_of(&(_obj)->base, QObject, base) : NULL;
>> }));
>> _obj;
>> })
>>
>> Unintended variable name capture at --->.
>
> Yep, you've just proven how macros calling macros requires care, as we
> no longer have the namespace protections. If macro calls can only
> form a DAG, it is possible to choose unique intermediate declarations
> for every macro (although remembering to do that, and still keeping
> the macro definition legible, may not be easy). But if you can have
> macros that form any sort of nesting loop (and we WANT to allow things
> like MIN(MIN(a, b), c) - which is such a loop), dynamic naming becomes
> the only solution.
>
>>
>> The only reliable way to prevent unintended variable name capture is
>> -Wshadow.
>
> Yes, I would love to have that enabled eventually.
>
>>
>> One blocker for enabling it is shadowing hiding in function-like
>> macros like
>>
>> qdict_put(dict, "name", qobject_ref(...))
>>
>> qdict_put() wraps its last argument in QOBJECT(), and the last
>> argument here contains another QOBJECT().
>>
>> Use dark preprocessor sorcery to make the macros that give us this
>> problem use different variable names on every call.
>
> Sounds foreboding; hopefully not many macros are affected...
>
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> include/qapi/qmp/qobject.h | 8 +++++---
>> include/qemu/atomic.h | 11 ++++++-----
>> include/qemu/osdep.h | 34 +++++++++++++++++++---------------
>> 3 files changed, 30 insertions(+), 23 deletions(-)
>
> ...okay, the size of the diffstat seems tolerable (good that we don't
> have many macros that expand to a temporary variable declaration and
> which are likely to be heavily reused from nested contexts).
>
>> diff --git a/include/qapi/qmp/qobject.h b/include/qapi/qmp/qobject.h
>> index 9003b71fd3..7b50fc905d 100644
>> --- a/include/qapi/qmp/qobject.h
>> +++ b/include/qapi/qmp/qobject.h
>> @@ -45,10 +45,12 @@ struct QObject {
>> struct QObjectBase_ base;
>> };
>>
>> -#define QOBJECT(obj) ({ \
>> - typeof(obj) _obj = (obj); \
>> - _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
>> +#define QOBJECT_INTERNAL(obj, l) ({ \
>> + typeof(obj) PASTE(_obj, l) = (obj); \
>> + PASTE(_obj, l) \
>> + ? container_of(&(PASTE(_obj, l))->base, QObject, base) : NULL; \
>> })
>> +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__)
>
> Slick! Every call to QOBJECT() defines a unique temporary variable
> name. But as written, QOBJECT(o) expands to this (when __COUNTER__ is
> 1):
>
> ({
> typeof((o)) _obj1 = ((o));
> _obj1 ? container_of(&(_obj1)->base, QObject, base) : NULL;
> })
>
> which has three sets of redundant parens that could be elided. Why do
> you need to add () around obj when forwarding to QOBJECT_INTERNAL()?
Habit: enclose macro arguments in parenthesis unless there's a specific
need not to.
> The only way the expansion of the text passed through the 'obj'
> parameter can contain a comma is if the user has already parenthesized
> it on their end (not that I expect a comma expression to be a frequent
> argument to QOBJECT(), but who knows). Arguing that it is to silence
> a syntax checker is weak; since we must NOT add parens around the
> parameters to QOBJECT_INTERNAL (calling PASTE((_obj), (l)) is
> obviously wrong).
>
> Meanwhile, the repetition of three calls to PASTE() is annoying. How
> about:
>
> #define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \
> typeof _obj _tmp = _obj; \
> _tmp ? container_of(&_tmp->base, QObject, base) : NULL; \
> )}
> #define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp)
> #define QOBJECT_INTERNAL(_arg, _ctr) \
> QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr))
> #define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__)
>
> or:
>
> #define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \
> typeof(_obj) _tmp = (_obj); \
> _tmp ? container_of(&_tmp->base, QObject, base) : NULL; \
> )}
> #define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp)
> #define QOBJECT_INTERNAL(_arg, _ctr) \
> QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr))
> #define QOBJECT(obj) QOBJECT_INTERNAL(obj, __COUNTER__)
>
> where, in either case, QOBJECT(o) should then expand to
>
> ({
> typeof (o) _obj1 = (o);
> _obj1 ? container_of(&_obj1->base, QObject, base) : NULL;
> })
The idea is to have the innermost macro take the variable name instead
of the counter. "PASTE(_obj, l)" then becomes "_tmp" there, which is
more legible. However, we pay for it by going through two more macros.
Can you explain why you need two more?
Complication: the innermost macro needs to take all its variable names
as arguments. The macro above has just one. Macros below have two.
Not quite sure which version is easier to understand.
>> /* Required for qobject_to() */
>> #define QTYPE_CAST_TO_QNull QTYPE_QNULL
>> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
>> index d95612f7a0..3f80ffac69 100644
>> --- a/include/qemu/atomic.h
>> +++ b/include/qemu/atomic.h
>> @@ -157,13 +157,14 @@
>> smp_read_barrier_depends();
>> #endif
>>
>> -#define qatomic_rcu_read(ptr) \
>> - ({ \
>> +#define qatomic_rcu_read_internal(ptr, l) \
>> + ({ \
>> qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
>> - typeof_strip_qual(*ptr) _val; \
>> - qatomic_rcu_read__nocheck(ptr, &_val); \
>> - _val; \
>> + typeof_strip_qual(*ptr) PASTE(_val, l); \
>> + qatomic_rcu_read__nocheck(ptr, &PASTE(_val, l)); \
>> + PASTE(_val, l); \
>> })
>> +#define qatomic_rcu_read(ptr) qatomic_rcu_read_internal((ptr), __COUNTER__)
>
> Same observation about being able to reduce the number of PASTE()
> calls by adding yet another layer of macro invocations.
>
>>
>> #define qatomic_rcu_set(ptr, i) do { \
>> qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index 21ef8f1699..9c191ebe99 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -371,18 +371,21 @@ void QEMU_ERROR("code path is reachable")
>> * have to open-code it. Sadly, Coverity is severely confused by the
>> * constant variants, so we have to dumb things down there.
>> */
>> +#define PASTE(a, b) a##b
>> +#define MIN_INTERNAL(a, b, l) \
>> + ({ \
>> + typeof(1 ? (a) : (b)) PASTE(_a, l) = (a), PASTE(_b, l) = (b); \
>> + PASTE(_a, l) < PASTE(_b, l) ? PASTE(_a, l) : PASTE(_b, l); \
>> + })
>
> And again.
>
> I think you are definitely on the right track to have all internal
> variable declarations within a macro definition use multi-layered
> expansion with the help of __COUNTER__ to ensure that the macro's
> temporary variable is globally unique; so if you leave it as written,
> I could live with:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> But if you respin it to pick up any of my suggestions, I'll definitely
> spend another review cycle on the result.
>
> If it helps, here's what I ended up using in nbdkit:
>
> https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/unique-name.h#L36
> /* https://stackoverflow.com/a/1597129
> * https://stackoverflow.com/a/12711226
> */
> #define XXUNIQUE_NAME(name, counter) name ## counter
> #define XUNIQUE_NAME(name, counter) XXUNIQUE_NAME (name, counter)
> #define NBDKIT_UNIQUE_NAME(name) XUNIQUE_NAME (name, __COUNTER__)
>
> https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/minmax.h#L47
> #include "unique-name.h"
>
> #undef MIN
> #define MIN(x, y) \
> MIN_1 ((x), (y), NBDKIT_UNIQUE_NAME (_x), NBDKIT_UNIQUE_NAME (_y))
>
> ...
> #define MIN_1(x, y, _x, _y) ({ \
> __auto_type _x = (x); \
> __auto_type _y = (y); \
> _x < _y ? _x : _y; \
> })
Thanks!
On Fri, Sep 01, 2023 at 10:48:26AM +0200, Markus Armbruster wrote:
> > Indeed, not fully understanding the preprocessor makes for some
> > interesting death traps.
>
> We use ALL_CAPS for macros to signal "watch out for traps".
>
> >> -#define QOBJECT(obj) ({ \
> >> - typeof(obj) _obj = (obj); \
> >> - _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
> >> +#define QOBJECT_INTERNAL(obj, l) ({ \
> >> + typeof(obj) PASTE(_obj, l) = (obj); \
> >> + PASTE(_obj, l) \
> >> + ? container_of(&(PASTE(_obj, l))->base, QObject, base) : NULL; \
> >> })
> >> +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__)
> >
> > Slick! Every call to QOBJECT() defines a unique temporary variable
> > name. But as written, QOBJECT(o) expands to this (when __COUNTER__ is
> > 1):
> >
> > ({
> > typeof((o)) _obj1 = ((o));
> > _obj1 ? container_of(&(_obj1)->base, QObject, base) : NULL;
> > })
> >
> > which has three sets of redundant parens that could be elided. Why do
> > you need to add () around obj when forwarding to QOBJECT_INTERNAL()?
>
> Habit: enclose macro arguments in parenthesis unless there's a specific
> need not to.
>
> > The only way the expansion of the text passed through the 'obj'
> > parameter can contain a comma is if the user has already parenthesized
> > it on their end (not that I expect a comma expression to be a frequent
> > argument to QOBJECT(), but who knows). Arguing that it is to silence
> > a syntax checker is weak; since we must NOT add parens around the
> > parameters to QOBJECT_INTERNAL (calling PASTE((_obj), (l)) is
> > obviously wrong).
> >
> > Meanwhile, the repetition of three calls to PASTE() is annoying. How
> > about:
> >
> > #define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \
> > typeof _obj _tmp = _obj; \
> > _tmp ? container_of(&_tmp->base, QObject, base) : NULL; \
> > )}
> > #define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp)
> > #define QOBJECT_INTERNAL(_arg, _ctr) \
> > QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr))
> > #define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__)
> >
> > or:
> >
> > #define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \
> > typeof(_obj) _tmp = (_obj); \
> > _tmp ? container_of(&_tmp->base, QObject, base) : NULL; \
> > )}
> > #define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp)
> > #define QOBJECT_INTERNAL(_arg, _ctr) \
> > QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr))
> > #define QOBJECT(obj) QOBJECT_INTERNAL(obj, __COUNTER__)
> >
> > where, in either case, QOBJECT(o) should then expand to
> >
> > ({
> > typeof (o) _obj1 = (o);
> > _obj1 ? container_of(&_obj1->base, QObject, base) : NULL;
> > })
>
> The idea is to have the innermost macro take the variable name instead
> of the counter. "PASTE(_obj, l)" then becomes "_tmp" there, which is
> more legible. However, we pay for it by going through two more macros.
>
> Can you explain why you need two more?
Likewise habit, on my part (and laziness - I wrote the previous email
without testing anything). I've learned over the years that pasting is
hard (you can't mix ## with a macro that you want expanded without 2
layers of indirection), so I started out by adding two layers of
QOBJECT_INTERNAL, then wrote QOBJECT to forward to QOBJECT_INTERNAL.
But now that you asked, I actually spent the time to test with the
preprocessor, and your hunch is indeed correct: I over-compensated
becaues of my habit.
$cat foo.c
#define PASTE(_a, _b) _a##_b
#define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \
typeof _obj _tmp = _obj; \
_tmp ? container_of(&_tmp->base, QObject, base) : NULL; \
)}
#define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp)
#define QOBJECT_INTERNAL(_arg, _ctr) \
QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr))
#define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__)
QOBJECT(o)
#define Q_INTERNAL_1(_obj, _tmp) ({ \
typeof _obj _tmp = _obj; \
_tmp ? container_of(&_tmp->base, QObject, base) : NULL; \
)}
#define Q_INTERNAL(_arg, _ctr) \
Q_INTERNAL_1(_arg, PASTE(_obj, _ctr))
#define Q(obj) Q_INTERNAL((obj), __COUNTER__)
Q(o)
$ gcc -E foo.c
# 0 "foo.c"
# 0 "<built-in>"
# 0 "<command-line>"
# 1 "/usr/include/stdc-predef.h" 1 3 4
# 0 "<command-line>" 2
# 1 "foo.c"
# 12 "foo.c"
({ typeof (o) _obj0 = (o); _obj0 ? container_of(&_obj0->base, QObject, base) : NULL; )}
# 22 "foo.c"
({ typeof (o) _obj1 = (o); _obj1 ? container_of(&_obj1->base, QObject, base) : NULL; )}
So the important part was merely ensuring that __COUNTER__ has a
chance to be expanded PRIOR to the point where ## gets its argument,
and one less layer of indirection was still sufficient for that.
>
> Complication: the innermost macro needs to take all its variable names
> as arguments. The macro above has just one. Macros below have two.
>
> Not quite sure which version is easier to understand.
Proper comments may help; once you realize that you are passing in the
unique variable name(s) to be used as the temporary identifier(s),
rather than an integer that still needs to be glued, then separating
the task of generating name(s) (which is done once per name, instead
of repeated 3 times) makes sense to me. I also like minimizing the
number of times I have to spell out __COUNTER__; wrapping unique name
generation behind a well-named helper macro gives a better view of why
it is needed in the first place.
At any rate, this comment still applies:
> >
> > I think you are definitely on the right track to have all internal
> > variable declarations within a macro definition use multi-layered
> > expansion with the help of __COUNTER__ to ensure that the macro's
> > temporary variable is globally unique; so if you leave it as written,
> > I could live with:
> >
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> >
> > But if you respin it to pick up any of my suggestions, I'll definitely
> > spend another review cycle on the result.
> >
> > If it helps, here's what I ended up using in nbdkit:
> >
> > https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/unique-name.h#L36
> > /* https://stackoverflow.com/a/1597129
> > * https://stackoverflow.com/a/12711226
> > */
> > #define XXUNIQUE_NAME(name, counter) name ## counter
> > #define XUNIQUE_NAME(name, counter) XXUNIQUE_NAME (name, counter)
> > #define NBDKIT_UNIQUE_NAME(name) XUNIQUE_NAME (name, __COUNTER__)
> >
> > https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/minmax.h#L47
> > #include "unique-name.h"
> >
> > #undef MIN
> > #define MIN(x, y) \
> > MIN_1 ((x), (y), NBDKIT_UNIQUE_NAME (_x), NBDKIT_UNIQUE_NAME (_y))
> >
> > ...
> > #define MIN_1(x, y, _x, _y) ({ \
> > __auto_type _x = (x); \
> > __auto_type _y = (y); \
> > _x < _y ? _x : _y; \
> > })
>
> Thanks!
and so I'm fine leaving it up to you if you want to copy the paradigm
I've seen elsewhere, or if you want to leave it as you first proposed.
The end result is the same, and it is more of a judgment call on which
form is easier for you to reason about.
But as other commenters mentioned, we already have a glue() macro, so
use that instead of PASTE(), no matter what else you choose.
Looking forward to v2!
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Eric Blake <eblake@redhat.com> writes:
> On Fri, Sep 01, 2023 at 10:48:26AM +0200, Markus Armbruster wrote:
>> > Indeed, not fully understanding the preprocessor makes for some
>> > interesting death traps.
>>
>> We use ALL_CAPS for macros to signal "watch out for traps".
>>
>
>> >> -#define QOBJECT(obj) ({ \
>> >> - typeof(obj) _obj = (obj); \
>> >> - _obj ? container_of(&(_obj)->base, QObject, base) : NULL; \
>> >> +#define QOBJECT_INTERNAL(obj, l) ({ \
>> >> + typeof(obj) PASTE(_obj, l) = (obj); \
>> >> + PASTE(_obj, l) \
>> >> + ? container_of(&(PASTE(_obj, l))->base, QObject, base) : NULL; \
>> >> })
>> >> +#define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__)
>> >
>> > Slick! Every call to QOBJECT() defines a unique temporary variable
>> > name. But as written, QOBJECT(o) expands to this (when __COUNTER__ is
>> > 1):
>> >
>> > ({
>> > typeof((o)) _obj1 = ((o));
>> > _obj1 ? container_of(&(_obj1)->base, QObject, base) : NULL;
>> > })
>> >
>> > which has three sets of redundant parens that could be elided. Why do
>> > you need to add () around obj when forwarding to QOBJECT_INTERNAL()?
>>
>> Habit: enclose macro arguments in parenthesis unless there's a specific
>> need not to.
>>
>> > The only way the expansion of the text passed through the 'obj'
>> > parameter can contain a comma is if the user has already parenthesized
>> > it on their end (not that I expect a comma expression to be a frequent
>> > argument to QOBJECT(), but who knows). Arguing that it is to silence
>> > a syntax checker is weak; since we must NOT add parens around the
>> > parameters to QOBJECT_INTERNAL (calling PASTE((_obj), (l)) is
>> > obviously wrong).
>> >
>> > Meanwhile, the repetition of three calls to PASTE() is annoying. How
>> > about:
>> >
>> > #define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \
>> > typeof _obj _tmp = _obj; \
>> > _tmp ? container_of(&_tmp->base, QObject, base) : NULL; \
>> > )}
>> > #define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp)
>> > #define QOBJECT_INTERNAL(_arg, _ctr) \
>> > QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr))
>> > #define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__)
>> >
>> > or:
>> >
>> > #define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \
>> > typeof(_obj) _tmp = (_obj); \
>> > _tmp ? container_of(&_tmp->base, QObject, base) : NULL; \
>> > )}
>> > #define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp)
>> > #define QOBJECT_INTERNAL(_arg, _ctr) \
>> > QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr))
>> > #define QOBJECT(obj) QOBJECT_INTERNAL(obj, __COUNTER__)
>> >
>> > where, in either case, QOBJECT(o) should then expand to
>> >
>> > ({
>> > typeof (o) _obj1 = (o);
>> > _obj1 ? container_of(&_obj1->base, QObject, base) : NULL;
>> > })
>>
>> The idea is to have the innermost macro take the variable name instead
>> of the counter. "PASTE(_obj, l)" then becomes "_tmp" there, which is
>> more legible. However, we pay for it by going through two more macros.
>>
>> Can you explain why you need two more?
>
> Likewise habit, on my part (and laziness - I wrote the previous email
> without testing anything). I've learned over the years that pasting is
> hard (you can't mix ## with a macro that you want expanded without 2
> layers of indirection), so I started out by adding two layers of
> QOBJECT_INTERNAL, then wrote QOBJECT to forward to QOBJECT_INTERNAL.
> But now that you asked, I actually spent the time to test with the
> preprocessor, and your hunch is indeed correct: I over-compensated
> becaues of my habit.
>
> $cat foo.c
> #define PASTE(_a, _b) _a##_b
>
> #define QOBJECT_INTERNAL_2(_obj, _tmp) ({ \
> typeof _obj _tmp = _obj; \
> _tmp ? container_of(&_tmp->base, QObject, base) : NULL; \
> )}
> #define QOBJECT_INTERNAL_1(_arg, _tmp) QOBJECT_INTERNAL_2(_arg, _tmp)
> #define QOBJECT_INTERNAL(_arg, _ctr) \
> QOBJECT_INTERNAL_1(_arg, PASTE(_obj, _ctr))
> #define QOBJECT(obj) QOBJECT_INTERNAL((obj), __COUNTER__)
>
> QOBJECT(o)
>
> #define Q_INTERNAL_1(_obj, _tmp) ({ \
> typeof _obj _tmp = _obj; \
> _tmp ? container_of(&_tmp->base, QObject, base) : NULL; \
> )}
> #define Q_INTERNAL(_arg, _ctr) \
> Q_INTERNAL_1(_arg, PASTE(_obj, _ctr))
> #define Q(obj) Q_INTERNAL((obj), __COUNTER__)
>
> Q(o)
> $ gcc -E foo.c
> # 0 "foo.c"
> # 0 "<built-in>"
> # 0 "<command-line>"
> # 1 "/usr/include/stdc-predef.h" 1 3 4
> # 0 "<command-line>" 2
> # 1 "foo.c"
> # 12 "foo.c"
> ({ typeof (o) _obj0 = (o); _obj0 ? container_of(&_obj0->base, QObject, base) : NULL; )}
> # 22 "foo.c"
> ({ typeof (o) _obj1 = (o); _obj1 ? container_of(&_obj1->base, QObject, base) : NULL; )}
>
> So the important part was merely ensuring that __COUNTER__ has a
> chance to be expanded PRIOR to the point where ## gets its argument,
> and one less layer of indirection was still sufficient for that.
The version with less indirection is easier to understand for me.
>>
>> Complication: the innermost macro needs to take all its variable names
>> as arguments. The macro above has just one. Macros below have two.
>>
>> Not quite sure which version is easier to understand.
>
> Proper comments may help; once you realize that you are passing in the
> unique variable name(s) to be used as the temporary identifier(s),
> rather than an integer that still needs to be glued, then separating
> the task of generating name(s) (which is done once per name, instead
> of repeated 3 times) makes sense to me. I also like minimizing the
> number of times I have to spell out __COUNTER__; wrapping unique name
> generation behind a well-named helper macro gives a better view of why
> it is needed in the first place.
I can add this comment to every instance of the __COUNTER__ trick:
/*
* Preprocessor wizardry ahead: glue(_val, l) expands to a new
* identifier in each macro expansion. Helps avoid shadowing
* variables and the unwanted name captures that come with it.
*/
> At any rate, this comment still applies:
>
>> >
>> > I think you are definitely on the right track to have all internal
>> > variable declarations within a macro definition use multi-layered
>> > expansion with the help of __COUNTER__ to ensure that the macro's
>> > temporary variable is globally unique; so if you leave it as written,
>> > I could live with:
>> >
>> > Reviewed-by: Eric Blake <eblake@redhat.com>
>> >
>> > But if you respin it to pick up any of my suggestions, I'll definitely
>> > spend another review cycle on the result.
>> >
>> > If it helps, here's what I ended up using in nbdkit:
>> >
>> > https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/unique-name.h#L36
>> > /* https://stackoverflow.com/a/1597129
>> > * https://stackoverflow.com/a/12711226
>> > */
>> > #define XXUNIQUE_NAME(name, counter) name ## counter
>> > #define XUNIQUE_NAME(name, counter) XXUNIQUE_NAME (name, counter)
>> > #define NBDKIT_UNIQUE_NAME(name) XUNIQUE_NAME (name, __COUNTER__)
>> >
>> > https://gitlab.com/nbdkit/nbdkit/-/blame/master/common/include/minmax.h#L47
>> > #include "unique-name.h"
>> >
>> > #undef MIN
>> > #define MIN(x, y) \
>> > MIN_1 ((x), (y), NBDKIT_UNIQUE_NAME (_x), NBDKIT_UNIQUE_NAME (_y))
>> >
>> > ...
>> > #define MIN_1(x, y, _x, _y) ({ \
>> > __auto_type _x = (x); \
>> > __auto_type _y = (y); \
>> > _x < _y ? _x : _y; \
>> > })
>>
>> Thanks!
>
> and so I'm fine leaving it up to you if you want to copy the paradigm
> I've seen elsewhere, or if you want to leave it as you first proposed.
> The end result is the same, and it is more of a judgment call on which
> form is easier for you to reason about.
I need to review the two competing versions once more to decide which I
find easier to read. Or shall I say less hard; preprocessor wizardry is
never really easy.
> But as other commenters mentioned, we already have a glue() macro, so
> use that instead of PASTE(), no matter what else you choose.
>
> Looking forward to v2!
Thanks again!
© 2016 - 2024 Red Hat, Inc.