> -----Original Message----- > From: Philippe Mathieu-Daudé <phi...@linaro.org> > Sent: Tuesday, October 10, 2023 12:23 AM > To: Brian Cain <bc...@quicinc.com>; richard.hender...@linaro.org; > a...@rev.ng > Cc: arm...@redhat.com; peter.mayd...@linaro.org; Matheus Bernardino > (QUIC) <quic_mathb...@quicinc.com>; stefa...@redhat.com; a...@rev.ng; > Marco Liebel (QUIC) <quic_mlie...@quicinc.com>; ltaylorsimp...@gmail.com; > Thomas Huth <th...@redhat.com>; Daniel P. Berrangé > <berra...@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- > >> b2b58d351...@redhat.com/ > >> > >> Regards, > >> > >> Phil.
