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

[Philippe Mathieu-Daudé]

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.