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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 9/10/23 22:53, Brian Cain wrote:
>> On 9/10/23 08:09, Philippe Mathieu-Daudé wrote:
>>> 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...):
> 
> No -- I generally build with gcc and only double-check the clang results to make sure I don't see any new failures there.
> 
> But I've not tested "-Wshadow" with clang yet.  I found these by adding "-Wshadow=global" to "-Wshadow=local".  I thought it might be useful to address these too while we're here.
> 
>> 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;
>>         |                  ^~~~~

If we have to clean that for -Wshadow=global, I'm tempted to rename
the typedef as 'vaddr_t' and keep the 'vaddr' variable names.

Richard, Anton, what do you think?

>> 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).
> 
> Hopefully it's helpful or at least benign if we address the shadowed globals under target/hexagon/ for now, even if "-Wshadow=global" is not enabled.
> 
>> See this thread:
>> https://lore.kernel.org/qemu-devel/11abc551-188e-85c0-fe55-
>> b2b58d35105d@redhat.com/
>>
>> Regards,
>>
>> Phil.