> On 29-Sep-2023, at 2:20 PM, Markus Armbruster <arm...@redhat.com> wrote:
>
> From: Ani Sinha <anisi...@redhat.com>
>
> Code changes in acpi 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. See also
>
> Subject: Help wanted for enabling -Wshadow=local
> Message-Id: <87r0mqlf9x....@pond.sub.org>
> https://lore.kernel.org/qemu-devel/87r0mqlf9x....@pond.sub.org
>
> The code is tested to build with and without the flag turned on.
>
> CC: Markus Armbruster <arm...@redhat.com>
> CC: Philippe Mathieu-Daude <phi...@linaro.org>
> CC: m...@redhat.com
> CC: imamm...@redhat.com
> Signed-off-by: Ani Sinha <anisi...@redhat.com>
> Message-ID: <20230922124203.127110-1-anisi...@redhat.com>
> Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
> [Commit message tweaked]
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
Thanks!
Are you not going to pick up "hw/i386: changes towards enabling -Wshadow=local”
?
> ---
> hw/acpi/cpu_hotplug.c | 25 +++++++++++++------------
> hw/i386/acpi-build.c | 24 ++++++++++++------------
> hw/smbios/smbios.c | 37 +++++++++++++++++++------------------
> 3 files changed, 44 insertions(+), 42 deletions(-)
>
> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> index ff14c3f410..634bbecb31 100644
> --- a/hw/acpi/cpu_hotplug.c
> +++ b/hw/acpi/cpu_hotplug.c
> @@ -265,26 +265,27 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx,
> MachineState *machine,
>
> /* build Processor object for each processor */
> for (i = 0; i < apic_ids->len; i++) {
> - int apic_id = apic_ids->cpus[i].arch_id;
> + int cpu_apic_id = apic_ids->cpus[i].arch_id;
>
> - assert(apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
> + assert(cpu_apic_id < ACPI_CPU_HOTPLUG_ID_LIMIT);
>
> - dev = aml_processor(i, 0, 0, "CP%.02X", apic_id);
> + dev = aml_processor(i, 0, 0, "CP%.02X", cpu_apic_id);
>
> method = aml_method("_MAT", 0, AML_NOTSERIALIZED);
> aml_append(method,
> - aml_return(aml_call2(CPU_MAT_METHOD, aml_int(apic_id),
> aml_int(i))
> + aml_return(aml_call2(CPU_MAT_METHOD,
> + aml_int(cpu_apic_id), aml_int(i))
> ));
> aml_append(dev, method);
>
> method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> aml_append(method,
> - aml_return(aml_call1(CPU_STATUS_METHOD, aml_int(apic_id))));
> + aml_return(aml_call1(CPU_STATUS_METHOD, aml_int(cpu_apic_id))));
> aml_append(dev, method);
>
> method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
> aml_append(method,
> - aml_return(aml_call2(CPU_EJECT_METHOD, aml_int(apic_id),
> + aml_return(aml_call2(CPU_EJECT_METHOD, aml_int(cpu_apic_id),
> aml_arg(0)))
> );
> aml_append(dev, method);
> @@ -298,11 +299,11 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx,
> MachineState *machine,
> /* Arg0 = APIC ID */
> method = aml_method(AML_NOTIFY_METHOD, 2, AML_NOTSERIALIZED);
> for (i = 0; i < apic_ids->len; i++) {
> - int apic_id = apic_ids->cpus[i].arch_id;
> + int cpu_apic_id = apic_ids->cpus[i].arch_id;
>
> - if_ctx = aml_if(aml_equal(aml_arg(0), aml_int(apic_id)));
> + if_ctx = aml_if(aml_equal(aml_arg(0), aml_int(cpu_apic_id)));
> aml_append(if_ctx,
> - aml_notify(aml_name("CP%.02X", apic_id), aml_arg(1))
> + aml_notify(aml_name("CP%.02X", cpu_apic_id), aml_arg(1))
> );
> aml_append(method, if_ctx);
> }
> @@ -319,13 +320,13 @@ void build_legacy_cpu_hotplug_aml(Aml *ctx,
> MachineState *machine,
> aml_varpackage(x86ms->apic_id_limit);
>
> for (i = 0, apic_idx = 0; i < apic_ids->len; i++) {
> - int apic_id = apic_ids->cpus[i].arch_id;
> + int cpu_apic_id = apic_ids->cpus[i].arch_id;
>
> - for (; apic_idx < apic_id; apic_idx++) {
> + for (; apic_idx < cpu_apic_id; apic_idx++) {
> aml_append(pkg, aml_int(0));
> }
> aml_append(pkg, aml_int(apic_ids->cpus[i].cpu ? 1 : 0));
> - apic_idx = apic_id + 1;
> + apic_idx = cpu_apic_id + 1;
> }
> aml_append(sb_scope, aml_name_decl(CPU_ON_BITMAP, pkg));
> aml_append(ctx, sb_scope);
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4d2d40bab5..95199c8900 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1585,12 +1585,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> aml_append(dev, aml_name_decl("_UID", aml_int(bus_num)));
> aml_append(dev, aml_name_decl("_BBN", aml_int(bus_num)));
> if (pci_bus_is_cxl(bus)) {
> - struct Aml *pkg = aml_package(2);
> + struct Aml *aml_pkg = aml_package(2);
>
> aml_append(dev, aml_name_decl("_HID",
> aml_string("ACPI0016")));
> - aml_append(pkg, aml_eisaid("PNP0A08"));
> - aml_append(pkg, aml_eisaid("PNP0A03"));
> - aml_append(dev, aml_name_decl("_CID", pkg));
> + aml_append(aml_pkg, aml_eisaid("PNP0A08"));
> + aml_append(aml_pkg, aml_eisaid("PNP0A03"));
> + aml_append(dev, aml_name_decl("_CID", aml_pkg));
> build_cxl_osc_method(dev);
> } else if (pci_bus_is_express(bus)) {
> aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> @@ -1783,14 +1783,14 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> Object *pci_host = acpi_get_i386_pci_host();
>
> if (pci_host) {
> - PCIBus *bus = PCI_HOST_BRIDGE(pci_host)->bus;
> - Aml *scope = aml_scope("PCI0");
> + PCIBus *pbus = PCI_HOST_BRIDGE(pci_host)->bus;
> + Aml *ascope = aml_scope("PCI0");
> /* Scan all PCI buses. Generate tables to support hotplug. */
> - build_append_pci_bus_devices(scope, bus);
> - if (object_property_find(OBJECT(bus), ACPI_PCIHP_PROP_BSEL)) {
> - build_append_pcihp_slots(scope, bus);
> + build_append_pci_bus_devices(ascope, pbus);
> + if (object_property_find(OBJECT(pbus), ACPI_PCIHP_PROP_BSEL)) {
> + build_append_pcihp_slots(ascope, pbus);
> }
> - aml_append(sb_scope, scope);
> + aml_append(sb_scope, ascope);
> }
> }
>
> @@ -1842,10 +1842,10 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> bool has_pcnt;
>
> Object *pci_host = acpi_get_i386_pci_host();
> - PCIBus *bus = PCI_HOST_BRIDGE(pci_host)->bus;
> + PCIBus *b = PCI_HOST_BRIDGE(pci_host)->bus;
>
> scope = aml_scope("\\_SB.PCI0");
> - has_pcnt = build_append_notfication_callback(scope, bus);
> + has_pcnt = build_append_notfication_callback(scope, b);
> if (has_pcnt) {
> aml_append(dsdt, scope);
> }
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index b753705856..2a90601ac5 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -1423,13 +1423,14 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> if (!qemu_opts_validate(opts, qemu_smbios_type8_opts, errp)) {
> return;
> }
> - struct type8_instance *t;
> - t = g_new0(struct type8_instance, 1);
> - save_opt(&t->internal_reference, opts, "internal_reference");
> - save_opt(&t->external_reference, opts, "external_reference");
> - t->connector_type = qemu_opt_get_number(opts, "connector_type",
> 0);
> - t->port_type = qemu_opt_get_number(opts, "port_type", 0);
> - QTAILQ_INSERT_TAIL(&type8, t, next);
> + struct type8_instance *t8_i;
> + t8_i = g_new0(struct type8_instance, 1);
> + save_opt(&t8_i->internal_reference, opts, "internal_reference");
> + save_opt(&t8_i->external_reference, opts, "external_reference");
> + t8_i->connector_type = qemu_opt_get_number(opts,
> + "connector_type", 0);
> + t8_i->port_type = qemu_opt_get_number(opts, "port_type", 0);
> + QTAILQ_INSERT_TAIL(&type8, t8_i, next);
> return;
> case 11:
> if (!qemu_opts_validate(opts, qemu_smbios_type11_opts, errp)) {
> @@ -1452,27 +1453,27 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> type17.speed = qemu_opt_get_number(opts, "speed", 0);
> return;
> case 41: {
> - struct type41_instance *t;
> + struct type41_instance *t41_i;
> Error *local_err = NULL;
>
> if (!qemu_opts_validate(opts, qemu_smbios_type41_opts, errp)) {
> return;
> }
> - t = g_new0(struct type41_instance, 1);
> - save_opt(&t->designation, opts, "designation");
> - t->kind = qapi_enum_parse(&type41_kind_lookup,
> - qemu_opt_get(opts, "kind"),
> - 0, &local_err) + 1;
> - t->kind |= 0x80; /* enabled */
> + t41_i = g_new0(struct type41_instance, 1);
> + save_opt(&t41_i->designation, opts, "designation");
> + t41_i->kind = qapi_enum_parse(&type41_kind_lookup,
> + qemu_opt_get(opts, "kind"),
> + 0, &local_err) + 1;
> + t41_i->kind |= 0x80; /* enabled */
> if (local_err != NULL) {
> error_propagate(errp, local_err);
> - g_free(t);
> + g_free(t41_i);
> return;
> }
> - t->instance = qemu_opt_get_number(opts, "instance", 1);
> - save_opt(&t->pcidev, opts, "pcidev");
> + t41_i->instance = qemu_opt_get_number(opts, "instance", 1);
> + save_opt(&t41_i->pcidev, opts, "pcidev");
>
> - QTAILQ_INSERT_TAIL(&type41, t, next);
> + QTAILQ_INSERT_TAIL(&type41, t41_i, next);
> return;
> }
> default:
> --
> 2.41.0
>