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

[Brian Cain]

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Sent: Tuesday, October 10, 2023 12:23 AM
> To: Brian Cain <bcain@quicinc.com>; richard.henderson@linaro.org;
> anjo@rev.ng
> Cc: armbru@redhat.com; peter.maydell@linaro.org; Matheus Bernardino
> (QUIC) <quic_mathbern@quicinc.com>; stefanha@redhat.com; ale@rev.ng;
> Marco Liebel (QUIC) <quic_mliebel@quicinc.com>; ltaylorsimpson@gmail.com;
> Thomas Huth <thuth@redhat.com>; Daniel P. Berrangé
> <berrange@redhat.com>; qemu-devel@nongnu.org
> Subject: Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> 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?

Feels like I may have strolled into uncharted territory.  I'll just drop the 
patch that is intended to address -Wshadow=global and resurrect it if/when we 
decide to take that on in general.

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