Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 9/10/23 08:09, Philippe Mathieu-Daudé wrote:
>> Hi Brian,
>> On 6/10/23 00:22, Brian Cain wrote:
>>> The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the
>>> identifiers to avoid shadowing the type name.
>> This one surprises me, since we have other occurences:
>> include/exec/memory.h:751:bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
>>      include/qemu/plugin.h:199:void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
>> target/arm/internals.h:643:G_NORETURN void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>> target/i386/tcg/helper-tcg.h:70:G_NORETURN void handle_unaligned_access(CPUX86State *env, vaddr vaddr,
>> ...
>> $ git grep -w vaddr, | wc -l
>>       207
>> What is the error/warning like?
>
> OK I could reproduce, I suppose you are building with Clang which
> doesn't support shadow-local so you get global warnings too (as
> mentioned in this patch subject...):
>
> In file included from ../../gdbstub/trace.h:1,
>                  from ../../gdbstub/softmmu.c:30:
> trace/trace-gdbstub.h: In function '_nocheck__trace_gdbstub_hit_watchpoint':
> trace/trace-gdbstub.h:903:106: error: declaration of 'vaddr' shadows a global declaration [-Werror=shadow]
>   903 | static inline void _nocheck__trace_gdbstub_hit_watchpoint(const char * type, int cpu_gdb_index, uint64_t vaddr)
>       |                                  ~~~~~~~~~^~~~~
> In file included from include/sysemu/accel-ops.h:13,
>                  from include/sysemu/cpus.h:4,
>                  from ../../gdbstub/softmmu.c:21:
> include/exec/cpu-common.h:21:18: note: shadowed declaration is here
>    21 | typedef uint64_t vaddr;
>       |                  ^~~~~
> trace/trace-gdbstub.h: In function 'trace_gdbstub_hit_watchpoint':
> trace/trace-gdbstub.h:923:96: error: declaration of 'vaddr' shadows a global declaration [-Werror=shadow]
>   923 | static inline void trace_gdbstub_hit_watchpoint(const char * type, int cpu_gdb_index, uint64_t vaddr)
>       |                        ~~~~~~~~~^~~~~
> include/exec/cpu-common.h:21:18: note: shadowed declaration is here
>    21 | typedef uint64_t vaddr;
>       |                  ^~~~~
>
> Clang users got confused by this, IIUC Markus and Thomas idea is
> to only enable these warnings for GCC, enforcing them for Clang
> users via CI (until Clang get this option supported). Personally
> I'd rather enable the warning once for all, waiting for Clang
> support (or clean/enable global shadowing for GCC too).
>
> See this thread:
> https://lore.kernel.org/qemu-devel/11abc551-188e-85c0-fe55-b2b58d35105d@redhat.com/

The idea to enable some variation of -Wshadow goes back to this thread:

    Subject: Re: [RFC PATCH] docs/style: permit inline loop variables
    Date: Thu, 24 Aug 2023 14:18:53 +0100
    Message-ID: <CAFEAcA8wobO5F16vYhbQCjeadfN5Zwx5CQ7L4vQ3fh8c_6ngJg@mail.gmail.com>
    https://lore.kernel.org/qemu-devel/CAFEAcA8wobO5F16vYhbQCjeadfN5Zwx5CQ7L4vQ3fh8c_6ngJg@mail.gmail.com/

I've been aiming at -Wshadow=local because (1) it seemed more practical
(a lot less cleanup needed before we can enable the warning), and (2)
local shadowing is basically always foolish and fully our own fault.

No objection to additional -Wshadow work as long as I'm not the one
doing it :)