[PATCH] hw/i386: changes towards enabling -Wshadow=local

[PATCH] hw/i386: changes towards enabling -Wshadow=local

[Ani Sinha]

Code changes that addresses all compiler complaints coming from enabling
-Wshadow flags. Enabling -Wshadow catches cases of local variables shadowing
other local variables or parameters. These makes the code confusing and/or adds
bugs that are difficult to catch.

CC: Markus Armbruster <armbru@redhat.com>
CC: Philippe Mathieu-Daude <philmd@linaro.org>
CC: mst@redhat.com
Message-Id: <87r0mqlf9x.fsf@pond.sub.org>
Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 hw/i386/acpi-microvm.c | 12 ++++++------
 hw/i386/intel_iommu.c  |  8 ++++----
 hw/i386/pc.c           |  1 -
 hw/i386/x86.c          |  2 --
 4 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index a075360d85..6e4f8061eb 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -78,18 +78,18 @@ static void acpi_dsdt_add_virtio(Aml *scope,
             hwaddr base = VIRTIO_MMIO_BASE + index * 512;
             hwaddr size = 512;
 
-            Aml *dev = aml_device("VR%02u", (unsigned)index);
-            aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0005")));
-            aml_append(dev, aml_name_decl("_UID", aml_int(index)));
-            aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
+            Aml *adev = aml_device("VR%02u", (unsigned)index);
+            aml_append(adev, aml_name_decl("_HID", aml_string("LNRO0005")));
+            aml_append(adev, aml_name_decl("_UID", aml_int(index)));
+            aml_append(adev, aml_name_decl("_CCA", aml_int(1)));
 
             Aml *crs = aml_resource_template();
             aml_append(crs, aml_memory32_fixed(base, size, AML_READ_WRITE));
             aml_append(crs,
                        aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
                                      AML_EXCLUSIVE, &irq, 1));
-            aml_append(dev, aml_name_decl("_CRS", crs));
-            aml_append(scope, dev);
+            aml_append(adev, aml_name_decl("_CRS", crs));
+            aml_append(scope, adev);
         }
     }
 }
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c0ce896668..c1fb69170f 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3770,9 +3770,9 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
     while (remain >= VTD_PAGE_SIZE) {
         IOMMUTLBEvent event;
         uint64_t mask = dma_aligned_pow2_mask(start, end, s->aw_bits);
-        uint64_t size = mask + 1;
+        uint64_t sz = mask + 1;
 
-        assert(size);
+        assert(sz);
 
         event.type = IOMMU_NOTIFIER_UNMAP;
         event.entry.iova = start;
@@ -3784,8 +3784,8 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
 
         memory_region_notify_iommu_one(n, &event);
 
-        start += size;
-        remain -= size;
+        start += sz;
+        remain -= sz;
     }
 
     assert(!remain);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3db0743f31..e7a233e886 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1116,7 +1116,6 @@ void pc_memory_init(PCMachineState *pcms,
 
     if (machine->device_memory) {
         uint64_t *val = g_malloc(sizeof(*val));
-        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
         uint64_t res_mem_end = machine->device_memory->base;
 
         if (!pcmc->broken_reserved_end) {
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index f034df8bf6..b3d054889b 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -365,8 +365,6 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
 
     cpu_slot = x86_find_cpu_slot(MACHINE(x86ms), cpu->apic_id, &idx);
     if (!cpu_slot) {
-        MachineState *ms = MACHINE(x86ms);
-
         x86_topo_ids_from_apicid(cpu->apic_id, &topo_info, &topo_ids);
         error_setg(errp,
             "Invalid CPU [socket: %u, die: %u, core: %u, thread: %u] with"
-- 
2.39.1

Re: [PATCH] hw/i386: changes towards enabling -Wshadow=local

[Daniel P. Berrangé]

On Sat, Sep 23, 2023 at 08:03:34AM +0530, Ani Sinha wrote:
> Code changes that addresses all compiler complaints coming from enabling
> -Wshadow flags. Enabling -Wshadow catches cases of local variables shadowing
> other local variables or parameters. These makes the code confusing and/or adds
> bugs that are difficult to catch.
> 
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Philippe Mathieu-Daude <philmd@linaro.org>
> CC: mst@redhat.com
> Message-Id: <87r0mqlf9x.fsf@pond.sub.org>
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>  hw/i386/acpi-microvm.c | 12 ++++++------
>  hw/i386/intel_iommu.c  |  8 ++++----
>  hw/i386/pc.c           |  1 -
>  hw/i386/x86.c          |  2 --
>  4 files changed, 10 insertions(+), 13 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] hw/i386: changes towards enabling -Wshadow=local

[Peter Xu]

On Sat, Sep 23, 2023 at 08:03:34AM +0530, Ani Sinha wrote:
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index c0ce896668..c1fb69170f 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3770,9 +3770,9 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>      while (remain >= VTD_PAGE_SIZE) {
>          IOMMUTLBEvent event;
>          uint64_t mask = dma_aligned_pow2_mask(start, end, s->aw_bits);
> -        uint64_t size = mask + 1;
> +        uint64_t sz = mask + 1;
>  
> -        assert(size);
> +        assert(sz);
>  
>          event.type = IOMMU_NOTIFIER_UNMAP;
>          event.entry.iova = start;
> @@ -3784,8 +3784,8 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>  
>          memory_region_notify_iommu_one(n, &event);
>  
> -        start += size;
> -        remain -= size;
> +        start += sz;
> +        remain -= sz;
>      }
>  
>      assert(!remain);

Ani,

I've got a small patch for this hunk already:

https://lore.kernel.org/r/20230922160410.138786-1-peterx@redhat.com

Wouldn't hurt to merge both, though.. or just drop the other one.

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu

Re: [PATCH] hw/i386: changes towards enabling -Wshadow=local

[Ani Sinha]

> On 25-Sep-2023, at 8:42 PM, Peter Xu <peterx@redhat.com> wrote:
> 
> On Sat, Sep 23, 2023 at 08:03:34AM +0530, Ani Sinha wrote:
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index c0ce896668..c1fb69170f 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -3770,9 +3770,9 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>>     while (remain >= VTD_PAGE_SIZE) {
>>         IOMMUTLBEvent event;
>>         uint64_t mask = dma_aligned_pow2_mask(start, end, s->aw_bits);
>> -        uint64_t size = mask + 1;
>> +        uint64_t sz = mask + 1;
>> 
>> -        assert(size);
>> +        assert(sz);
>> 
>>         event.type = IOMMU_NOTIFIER_UNMAP;
>>         event.entry.iova = start;
>> @@ -3784,8 +3784,8 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>> 
>>         memory_region_notify_iommu_one(n, &event);
>> 
>> -        start += size;
>> -        remain -= size;
>> +        start += sz;
>> +        remain -= sz;
>>     }
>> 
>>     assert(!remain);
> 
> Ani,
> 
> I've got a small patch for this hunk already:
> 
> https://lore.kernel.org/r/20230922160410.138786-1-peterx@redhat.com
> 
> Wouldn't hurt to merge both, though.. or just drop the other one.

I liked your change so kept it and removed mine. See v2.

> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> Thanks,
> 
> -- 
> Peter Xu
>