RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> -Original Message- > From: Philippe Mathieu-Daudé > Sent: Tuesday, October 10, 2023 12:23 AM > To: Brian Cain ; richard.hender...@linaro.org; > a...@rev.ng > Cc: arm...@redhat.com; peter.mayd...@linaro.org; Matheus Bernardino > (QUIC) ; stefa...@redhat.com; a...@rev.ng; > Marco Liebel (QUIC) ; ltaylorsimp...@gmail.com; > Thomas Huth ; Daniel P. Berrangé > ; 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.
Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
On 10/10/2023 09.34, Philippe Mathieu-Daudé wrote: On 10/10/23 08:04, Markus Armbruster wrote: Philippe Mathieu-Daudé writes: [...] 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. POSIX reserves suffix _t, see https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02_02 Alternatives: tvaddr, VAddr, TargetVirtualAddress. Naming is hard. Do we care? According to our docs/devel/style.rst : "Scalar type names are lower_case_with_underscores_ending_with_a_t, like the POSIX uint64_t and family. Note that this last convention contradicts POSIX and is therefore likely to be changed." Maybe this would be a good point in time now to revisit our coding style, update it and then to change the "vaddr" type accordingly? My 0.02 €: If enum types should already be in CamelCase, why not also use that for scalar types? So I'd vote for VirtAddr or VAddr here. Thomas
Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
On 10/10/23 08:04, Markus Armbruster wrote: Philippe Mathieu-Daudé writes: [...] 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. POSIX reserves suffix _t, see https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02_02 Alternatives: tvaddr, VAddr, TargetVirtualAddress. Naming is hard. Do we care? Richard, Anton, what do you think? [...]
Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
Philippe Mathieu-Daudé writes: [...] > 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. POSIX reserves suffix _t, see https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02_02 Do we care? > Richard, Anton, what do you think? [...]
Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
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- b2b58d351...@redhat.com/ Regards, Phil.
Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
Philippe Mathieu-Daudé 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-b2b58d351...@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: https://lore.kernel.org/qemu-devel/cafeaca8wobo5f16vyhbqcjeadfn5zwx5cq7l4vq3fh8c_6n...@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 :)
RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> -Original Message- > From: Philippe Mathieu-Daudé > Sent: Monday, October 9, 2023 1:43 AM > To: Brian Cain ; qemu-devel@nongnu.org > Cc: arm...@redhat.com; richard.hender...@linaro.org; > peter.mayd...@linaro.org; Matheus Bernardino (QUIC) > ; stefa...@redhat.com; a...@rev.ng; > a...@rev.ng; Marco Liebel (QUIC) ; > ltaylorsimp...@gmail.com; Thomas Huth ; Daniel P. > Berrangé > 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 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...): 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; >| ^ > > 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.
RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> -Original Message- > From: Brian Cain > Sent: Sunday, October 8, 2023 7:50 AM > To: ltaylorsimp...@gmail.com; qemu-devel@nongnu.org > Cc: arm...@redhat.com; richard.hender...@linaro.org; phi...@linaro.org; > peter.mayd...@linaro.org; Matheus Bernardino (QUIC) > ; stefa...@redhat.com; a...@rev.ng; > a...@rev.ng; Marco Liebel (QUIC) > Subject: RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals > > > > > -Original Message- > > From: ltaylorsimp...@gmail.com > > Sent: Friday, October 6, 2023 11:01 AM > > To: Brian Cain ; qemu-devel@nongnu.org > > Cc: arm...@redhat.com; richard.hender...@linaro.org; > > phi...@linaro.org; peter.mayd...@linaro.org; Matheus Bernardino (QUIC) > > ; stefa...@redhat.com; a...@rev.ng; > > a...@rev.ng; Marco Liebel (QUIC) > > 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. > > > > > -Original Message- > > > From: Brian Cain > > > Sent: Thursday, October 5, 2023 4:22 PM > > > To: qemu-devel@nongnu.org > > > Cc: bc...@quicinc.com; arm...@redhat.com; > > > richard.hender...@linaro.org; phi...@linaro.org; > > > peter.mayd...@linaro.org; quic_mathb...@quicinc.com; > > > stefa...@redhat.com; a...@rev.ng; a...@rev.ng; > > > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com > > > Subject: [PATCH v2 3/3] target/hexagon: avoid shadowing globals > > > > > > The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename > > > the identifiers to avoid shadowing the type name. > > > > > > The global `cpu_env` is shadowed by local `cpu_env` arguments, so we > > > rename the function arguments to avoid shadowing the global. > > > > > > Signed-off-by: Brian Cain > > > --- > > > target/hexagon/genptr.c | 56 - > > > target/hexagon/genptr.h | 18 > > > target/hexagon/mmvec/system_ext_mmvec.c | 4 +- > > > target/hexagon/mmvec/system_ext_mmvec.h | 2 +- > > > target/hexagon/op_helper.c | 4 +- > > > 5 files changed, 42 insertions(+), 42 deletions(-) > > > > > > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index > > > 217bc7bb5a..11377ac92b 100644 > > > --- a/target/hexagon/genptr.c > > > +++ b/target/hexagon/genptr.c > > > @@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result, > > TCGv > > > src) > > > tcg_gen_deposit_i64(result, result, src64, N * 8, 8); } > > > > > > -static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int > > > mem_index) > > > +static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int > > > +mem_index) > > > > I'd recommend moving both the type and the arg name to the new line, > > also indent the new line. > > static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, > > int > > mem_index) > > > > > I could be mistaken but AFAICT none of these lines are wrapped in the way > they're quoted above in my patch (nor the baseline). I don't think any of > these lines exceed 80 columns, so they shouldn't need wrapping, either. > > I double checked how it's displayed at the archive > https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg01667.html to > make sure that it wasn't a misconfiguration of my mailer. For another > perspective - refer to the commit used to create this patch: > https://github.com/quic/qemu/commit/7f20565d403d16337ab6d69ee663121 > a3eef71e6 > > Is your review comment that "these lines should be wrapped and when you > do, make sure you do it like this"? Or "if you are going to wrap them, wrap > them like this"? Or something else? Yes. It looked like some adding the v_ would sometimes put the line over the 80 character size. If so, wrap them as described. If not, no wrapping is needed. > > > Otherwise, > > Reviewed-by: Taylor Simpson > >
Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
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-b2b58d351...@redhat.com/ Regards, Phil.
Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
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?
The global `cpu_env` is shadowed by local `cpu_env` arguments, so we
rename the function arguments to avoid shadowing the global.
Signed-off-by: Brian Cain
---
target/hexagon/genptr.c | 56 -
target/hexagon/genptr.h | 18
target/hexagon/mmvec/system_ext_mmvec.c | 4 +-
target/hexagon/mmvec/system_ext_mmvec.h | 2 +-
target/hexagon/op_helper.c | 4 +-
5 files changed, 42 insertions(+), 42 deletions(-)
diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
index 217bc7bb5a..11377ac92b 100644
--- a/target/hexagon/genptr.c
+++ b/target/hexagon/genptr.c
@@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result, TCGv src)
tcg_gen_deposit_i64(result, result, src64, N * 8, 8);
}
-static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int mem_index)
+static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int mem_index)
{
-tcg_gen_qemu_ld_tl(dest, vaddr, mem_index, MO_TEUL);
-tcg_gen_mov_tl(hex_llsc_addr, vaddr);
+tcg_gen_qemu_ld_tl(dest, v_addr, mem_index, MO_TEUL);
+tcg_gen_mov_tl(hex_llsc_addr, v_addr);
tcg_gen_mov_tl(hex_llsc_val, dest);
}
RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> -Original Message- > From: ltaylorsimp...@gmail.com > Sent: Friday, October 6, 2023 11:01 AM > To: Brian Cain ; qemu-devel@nongnu.org > Cc: arm...@redhat.com; richard.hender...@linaro.org; phi...@linaro.org; > peter.mayd...@linaro.org; Matheus Bernardino (QUIC) > ; stefa...@redhat.com; a...@rev.ng; > a...@rev.ng; Marco Liebel (QUIC) > 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. > > > -Original Message- > > From: Brian Cain > > Sent: Thursday, October 5, 2023 4:22 PM > > To: qemu-devel@nongnu.org > > Cc: bc...@quicinc.com; arm...@redhat.com; richard.hender...@linaro.org; > > phi...@linaro.org; peter.mayd...@linaro.org; quic_mathb...@quicinc.com; > > stefa...@redhat.com; a...@rev.ng; a...@rev.ng; > > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com > > Subject: [PATCH v2 3/3] target/hexagon: avoid shadowing globals > > > > The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the > > identifiers to avoid shadowing the type name. > > > > The global `cpu_env` is shadowed by local `cpu_env` arguments, so we > > rename the function arguments to avoid shadowing the global. > > > > Signed-off-by: Brian Cain > > --- > > target/hexagon/genptr.c | 56 - > > target/hexagon/genptr.h | 18 > > target/hexagon/mmvec/system_ext_mmvec.c | 4 +- > > target/hexagon/mmvec/system_ext_mmvec.h | 2 +- > > target/hexagon/op_helper.c | 4 +- > > 5 files changed, 42 insertions(+), 42 deletions(-) > > > > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index > > 217bc7bb5a..11377ac92b 100644 > > --- a/target/hexagon/genptr.c > > +++ b/target/hexagon/genptr.c > > @@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result, > TCGv > > src) > > tcg_gen_deposit_i64(result, result, src64, N * 8, 8); } > > > > -static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int > > mem_index) > > +static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int > > +mem_index) > > I'd recommend moving both the type and the arg name to the new line, also > indent the new line. > static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, > int > mem_index) > > > > > > -static inline void gen_load_locked8u(TCGv_i64 dest, TCGv vaddr, int > > mem_index) > > +static inline void gen_load_locked8u(TCGv_i64 dest, TCGv v_addr, int > > +mem_index) > > Ditto > > > static inline void gen_store_conditional4(DisasContext *ctx, > > - TCGv pred, TCGv vaddr, TCGv src) > > + TCGv pred, TCGv v_addr, TCGv > > + src) > > Ditto > > > zero = tcg_constant_tl(0); > > @@ -374,13 +374,13 @@ static inline void > > gen_store_conditional4(DisasContext *ctx, } > > > > static inline void gen_store_conditional8(DisasContext *ctx, > > - TCGv pred, TCGv vaddr, TCGv_i64 > > src) > > + TCGv pred, TCGv v_addr, > > + TCGv_i64 src) > > Indent > > > -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int > > slot) > > +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int > > +slot) > > Ditto > > > -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int > > slot); > > +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int > > +slot); > > Ditto I could be mistaken but AFAICT none of these lines are wrapped in the way they're quoted above in my patch (nor the baseline). I don't think any of these lines exceed 80 columns, so they shouldn't need wrapping, either. I double checked how it's displayed at the archive https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg01667.html to make sure that it wasn't a misconfiguration of my mailer. For another perspective - refer to the commit used to create this patch: https://github.com/quic/qemu/commit/7f20565d403d16337ab6d69ee663121a3eef71e6 Is your review comment that "these lines should be wrapped and when you do, make sure you do it like this"? Or "if you are going to wrap them, wrap them like this"? Or something else? > Otherwise, > Reviewed-by: Taylor Simpson >
RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> -Original Message- > From: Brian Cain > Sent: Thursday, October 5, 2023 4:22 PM > To: qemu-devel@nongnu.org > Cc: bc...@quicinc.com; arm...@redhat.com; richard.hender...@linaro.org; > phi...@linaro.org; peter.mayd...@linaro.org; quic_mathb...@quicinc.com; > stefa...@redhat.com; a...@rev.ng; a...@rev.ng; > quic_mlie...@quicinc.com; ltaylorsimp...@gmail.com > Subject: [PATCH v2 3/3] target/hexagon: avoid shadowing globals > > The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the > identifiers to avoid shadowing the type name. > > The global `cpu_env` is shadowed by local `cpu_env` arguments, so we > rename the function arguments to avoid shadowing the global. > > Signed-off-by: Brian Cain > --- > target/hexagon/genptr.c | 56 - > target/hexagon/genptr.h | 18 > target/hexagon/mmvec/system_ext_mmvec.c | 4 +- > target/hexagon/mmvec/system_ext_mmvec.h | 2 +- > target/hexagon/op_helper.c | 4 +- > 5 files changed, 42 insertions(+), 42 deletions(-) > > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index > 217bc7bb5a..11377ac92b 100644 > --- a/target/hexagon/genptr.c > +++ b/target/hexagon/genptr.c > @@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result, TCGv > src) > tcg_gen_deposit_i64(result, result, src64, N * 8, 8); } > > -static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int > mem_index) > +static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int > +mem_index) I'd recommend moving both the type and the arg name to the new line, also indent the new line. static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int mem_index) > > -static inline void gen_load_locked8u(TCGv_i64 dest, TCGv vaddr, int > mem_index) > +static inline void gen_load_locked8u(TCGv_i64 dest, TCGv v_addr, int > +mem_index) Ditto > static inline void gen_store_conditional4(DisasContext *ctx, > - TCGv pred, TCGv vaddr, TCGv src) > + TCGv pred, TCGv v_addr, TCGv > + src) Ditto > zero = tcg_constant_tl(0); > @@ -374,13 +374,13 @@ static inline void > gen_store_conditional4(DisasContext *ctx, } > > static inline void gen_store_conditional8(DisasContext *ctx, > - TCGv pred, TCGv vaddr, TCGv_i64 > src) > + TCGv pred, TCGv v_addr, > + TCGv_i64 src) Indent > -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int > slot) > +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int > +slot) Ditto > -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int > slot); > +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int > +slot); Ditto Otherwise, Reviewed-by: Taylor Simpson
