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

[Michael S. Tsirkin]

On Tue, Sep 26, 2023 at 11:22:35AM +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

These make

> 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>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> ---


chunks seem unrelated. why not split them up?

>  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(-)
> 
> changelog:
> v2: kept Peter's changes from 
> 20230922160410.138786-1-peterx@redhat.com">https://lore.kernel.org/r/20230922160410.138786-1-peterx@redhat.com
> and removed mine.
> 
> 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);
>          }
>      }
>  }

I would prefer to just drop the devicestate dev pointer, use kid->child inside 
the
macro.

> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index c0ce896668..2c832ab68b 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3744,7 +3744,7 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, 
> PCIBus *bus,
>  /* Unmap the whole range in the notifier's scope. */
>  static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>  {
> -    hwaddr size, remain;
> +    hwaddr total, remain;
>      hwaddr start = n->start;
>      hwaddr end = n->end;
>      IntelIOMMUState *s = as->iommu_state;
> @@ -3765,7 +3765,7 @@ static void vtd_address_space_unmap(VTDAddressSpace 
> *as, IOMMUNotifier *n)
>      }
>  
>      assert(start <= end);
> -    size = remain = end - start + 1;
> +    total = remain = end - start + 1;
>  
>      while (remain >= VTD_PAGE_SIZE) {
>          IOMMUTLBEvent event;
> @@ -3793,10 +3793,10 @@ static void vtd_address_space_unmap(VTDAddressSpace 
> *as, IOMMUNotifier *n)
>      trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
>                               VTD_PCI_SLOT(as->devfn),
>                               VTD_PCI_FUNC(as->devfn),
> -                             n->start, size);
> +                             n->start, total);
>  
>      map.iova = n->start;
> -    map.size = size - 1; /* Inclusive */
> +    map.size = total - 1; /* Inclusive */
>      iova_tree_remove(as->iova_tree, map);
>  }
>


arguably an improvement

  
> 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"


killing dead code, nice

> -- 
> 2.39.3