| Message ID | 20230831225607.30829-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | (few more) Steps towards enabling -Wshadow | expand |
On Thu, 31 Aug 2023 at 23:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > Fix: > > target/arm/tcg/translate-m-nocp.c:509:18: error: declaration shadows a local variable [-Werror,-Wshadow] > TCGv_i32 tmp = load_cpu_field(v7m.fpdscr[M_REG_NS]); > ^ > target/arm/tcg/translate-m-nocp.c:433:14: note: previous declaration is here > TCGv_i32 tmp; > ^ > target/arm/tcg/mve_helper.c:2463:17: error: declaration shadows a local variable [-Werror,-Wshadow] > int64_t extval = sextract64(src << shift, 0, 48); > ^ > target/arm/tcg/mve_helper.c:2443:18: note: previous declaration is here > int64_t val, extval; > ^ > target/arm/hvf/hvf.c:1936:13: error: declaration shadows a local variable [-Werror,-Wshadow] > int ret = 0; > ^ > target/arm/hvf/hvf.c:1807:9: note: previous declaration is here > int ret; > ^ > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > target/arm/hvf/hvf.c | 1 - > target/arm/tcg/mve_helper.c | 8 ++++---- > target/arm/tcg/translate-m-nocp.c | 2 +- > 3 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c > index 486f90be1d..20d534faef 100644 > --- a/target/arm/hvf/hvf.c > +++ b/target/arm/hvf/hvf.c > @@ -1933,7 +1933,6 @@ int hvf_vcpu_exec(CPUState *cpu) > uint32_t rt = (syndrome >> 5) & 0x1f; > uint32_t reg = syndrome & SYSREG_MASK; > uint64_t val; > - int ret = 0; > > if (isread) { > ret = hvf_sysreg_read(cpu, reg, rt); I'm not sure this is correct. The hvf_vcpu_exec() function is not documented, but in practice its caller expects it to return either EXCP_DEBUG (for "this was a guest debug exception you need to deal with") or something else (presumably the intention being 0 for OK). The hvf_sysreg_read() and hvf_sysreg_write() functions are also not documented, but they return 0 on success, or 1 for a completely unrecognized sysreg where we've raised the UNDEF exception (but not if we raised an UNDEF exception for an unrecognized GIC sysreg -- I think this is a bug). We use this return value to decide whether we need to advance the PC past the insn or not. It's not the same as the return value we want to return from hvf_vcpu_exec(). So I think the correct fix here is to retain the variable as locally scoped but give it a name that doesn't clash with the other function-scoped variable. > diff --git a/target/arm/tcg/mve_helper.c b/target/arm/tcg/mve_helper.c > index 403b345ea3..32087b6f0a 100644 > --- a/target/arm/tcg/mve_helper.c > +++ b/target/arm/tcg/mve_helper.c > @@ -924,8 +924,8 @@ DO_1OP_IMM(vorri, DO_ORRI) > bool qc = false; \ > for (e = 0; e < 16 / ESIZE; e++, mask >>= ESIZE) { \ > bool sat = false; \ > - TYPE r = FN(n[H##ESIZE(e)], m[H##ESIZE(e)], &sat); \ > - mergemask(&d[H##ESIZE(e)], r, mask); \ > + TYPE r_ = FN(n[H##ESIZE(e)], m[H##ESIZE(e)], &sat); \ > + mergemask(&d[H##ESIZE(e)], r_, mask); \ > qc |= sat & mask & 1; \ > } \ > if (qc) { \ The commit message doesn't list an error message relating to this change and it's not immediately obvious to me what 'r' would be shadowing here. > @@ -2460,7 +2460,7 @@ static inline int64_t do_sqrshl48_d(int64_t src, int64_t shift, > return extval; > } > } else if (shift < 48) { > - int64_t extval = sextract64(src << shift, 0, 48); > + extval = sextract64(src << shift, 0, 48); > if (!sat || src == (extval >> shift)) { > return extval; > } > @@ -2492,7 +2492,7 @@ static inline uint64_t do_uqrshl48_d(uint64_t src, int64_t shift, > return extval; > } > } else if (shift < 48) { > - uint64_t extval = extract64(src << shift, 0, 48); > + extval = extract64(src << shift, 0, 48); > if (!sat || src == (extval >> shift)) { > return extval; > } These two parts are good, but one of them is missing from the listed error messages in the commit message. > diff --git a/target/arm/tcg/translate-m-nocp.c b/target/arm/tcg/translate-m-nocp.c > index 33f6478bb9..42308c4db5 100644 > --- a/target/arm/tcg/translate-m-nocp.c > +++ b/target/arm/tcg/translate-m-nocp.c > @@ -506,7 +506,7 @@ static bool gen_M_fp_sysreg_read(DisasContext *s, int regno, > > gen_branch_fpInactive(s, TCG_COND_EQ, lab_active); > /* fpInactive case: reads as FPDSCR_NS */ > - TCGv_i32 tmp = load_cpu_field(v7m.fpdscr[M_REG_NS]); > + tmp = load_cpu_field(v7m.fpdscr[M_REG_NS]); > storefn(s, opaque, tmp, true); > lab_end = gen_new_label(); > tcg_gen_br(lab_end); This one's right too. thanks -- PMM
On 1/9/23 12:46, Peter Maydell wrote: > On Thu, 31 Aug 2023 at 23:56, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: >> >> Fix: >> >> target/arm/tcg/translate-m-nocp.c:509:18: error: declaration shadows a local variable [-Werror,-Wshadow] >> TCGv_i32 tmp = load_cpu_field(v7m.fpdscr[M_REG_NS]); >> ^ >> target/arm/tcg/translate-m-nocp.c:433:14: note: previous declaration is here >> TCGv_i32 tmp; >> ^ >> target/arm/tcg/mve_helper.c:2463:17: error: declaration shadows a local variable [-Werror,-Wshadow] >> int64_t extval = sextract64(src << shift, 0, 48); >> ^ >> target/arm/tcg/mve_helper.c:2443:18: note: previous declaration is here >> int64_t val, extval; >> ^ >> target/arm/hvf/hvf.c:1936:13: error: declaration shadows a local variable [-Werror,-Wshadow] >> int ret = 0; >> ^ >> target/arm/hvf/hvf.c:1807:9: note: previous declaration is here >> int ret; >> ^ >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> target/arm/hvf/hvf.c | 1 - >> target/arm/tcg/mve_helper.c | 8 ++++---- >> target/arm/tcg/translate-m-nocp.c | 2 +- >> 3 files changed, 5 insertions(+), 6 deletions(-) >> >> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c >> index 486f90be1d..20d534faef 100644 >> --- a/target/arm/hvf/hvf.c >> +++ b/target/arm/hvf/hvf.c >> @@ -1933,7 +1933,6 @@ int hvf_vcpu_exec(CPUState *cpu) >> uint32_t rt = (syndrome >> 5) & 0x1f; >> uint32_t reg = syndrome & SYSREG_MASK; >> uint64_t val; >> - int ret = 0; >> >> if (isread) { >> ret = hvf_sysreg_read(cpu, reg, rt); > > I'm not sure this is correct. > > The hvf_vcpu_exec() function is not documented, but in practice > its caller expects it to return either EXCP_DEBUG (for "this was > a guest debug exception you need to deal with") or something else > (presumably the intention being 0 for OK). > > The hvf_sysreg_read() and hvf_sysreg_write() functions are also not > documented, but they return 0 on success, or 1 for a completely > unrecognized sysreg where we've raised the UNDEF exception (but > not if we raised an UNDEF exception for an unrecognized GIC sysreg -- > I think this is a bug). We use this return value to decide whether > we need to advance the PC past the insn or not. It's not the same > as the return value we want to return from hvf_vcpu_exec(). Indeed. > So I think the correct fix here is to retain the variable as > locally scoped but give it a name that doesn't clash with the > other function-scoped variable. OK. >> diff --git a/target/arm/tcg/mve_helper.c b/target/arm/tcg/mve_helper.c >> index 403b345ea3..32087b6f0a 100644 >> --- a/target/arm/tcg/mve_helper.c >> +++ b/target/arm/tcg/mve_helper.c >> @@ -924,8 +924,8 @@ DO_1OP_IMM(vorri, DO_ORRI) >> bool qc = false; \ >> for (e = 0; e < 16 / ESIZE; e++, mask >>= ESIZE) { \ >> bool sat = false; \ >> - TYPE r = FN(n[H##ESIZE(e)], m[H##ESIZE(e)], &sat); \ >> - mergemask(&d[H##ESIZE(e)], r, mask); \ >> + TYPE r_ = FN(n[H##ESIZE(e)], m[H##ESIZE(e)], &sat); \ >> + mergemask(&d[H##ESIZE(e)], r_, mask); \ >> qc |= sat & mask & 1; \ >> } \ >> if (qc) { \ > > The commit message doesn't list an error message relating to > this change and it's not immediately obvious to me what 'r' > would be shadowing here. Full error: target/arm/tcg/mve_helper.c: In function ‘helper_mve_vqshlsb’: target/arm/tcg/mve_helper.c:1259:19: warning: declaration of ‘r’ shadows a previous local [-Wshadow=compatible-local] 1259 | typeof(N) r = FN(N, (int8_t)(M), sizeof(N) * 8, ROUND, &su32); \ | ^ target/arm/tcg/mve_helper.c:1267:5: note: in expansion of macro ‘WRAP_QRSHL_HELPER’ 1267 | WRAP_QRSHL_HELPER(do_sqrshl_bhs, N, M, false, satp) | ^~~~~~~~~~~~~~~~~ target/arm/tcg/mve_helper.c:927:22: note: in expansion of macro ‘DO_SQSHL_OP’ 927 | TYPE r = FN(n[H##ESIZE(e)], m[H##ESIZE(e)], &sat); \ | ^~ target/arm/tcg/mve_helper.c:945:5: note: in expansion of macro ‘DO_2OP_SAT’ 945 | DO_2OP_SAT(OP##b, 1, int8_t, FN) \ | ^~~~~~~~~~ target/arm/tcg/mve_helper.c:1277:1: note: in expansion of macro ‘DO_2OP_SAT_S’ 1277 | DO_2OP_SAT_S(vqshls, DO_SQSHL_OP) | ^~~~~~~~~~~~ So 'r' comes from: #define WRAP_QRSHL_HELPER(FN, N, M, ROUND, satp) \ ({ \ uint32_t su32 = 0; \ typeof(N) r = FN(N, (int8_t)(M), sizeof(N) * 8, ROUND, &su32); \ if (su32) { \ *satp = true; \ } \ r; \ }) I'll rename this one as 'qrshl_ret', and the previous one 'vqdmladh_ret'. >> @@ -2460,7 +2460,7 @@ static inline int64_t do_sqrshl48_d(int64_t src, int64_t shift, >> return extval; >> } >> } else if (shift < 48) { >> - int64_t extval = sextract64(src << shift, 0, 48); >> + extval = sextract64(src << shift, 0, 48); >> if (!sat || src == (extval >> shift)) { >> return extval; >> } >> @@ -2492,7 +2492,7 @@ static inline uint64_t do_uqrshl48_d(uint64_t src, int64_t shift, >> return extval; >> } >> } else if (shift < 48) { >> - uint64_t extval = extract64(src << shift, 0, 48); >> + extval = extract64(src << shift, 0, 48); >> if (!sat || src == (extval >> shift)) { >> return extval; >> } > > These two parts are good, but one of them is missing from the > listed error messages in the commit message. I'll record both. Thanks, Phil.
diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c index 486f90be1d..20d534faef 100644 --- a/target/arm/hvf/hvf.c +++ b/target/arm/hvf/hvf.c @@ -1933,7 +1933,6 @@ int hvf_vcpu_exec(CPUState *cpu) uint32_t rt = (syndrome >> 5) & 0x1f; uint32_t reg = syndrome & SYSREG_MASK; uint64_t val; - int ret = 0; if (isread) { ret = hvf_sysreg_read(cpu, reg, rt); diff --git a/target/arm/tcg/mve_helper.c b/target/arm/tcg/mve_helper.c index 403b345ea3..32087b6f0a 100644 --- a/target/arm/tcg/mve_helper.c +++ b/target/arm/tcg/mve_helper.c @@ -924,8 +924,8 @@ DO_1OP_IMM(vorri, DO_ORRI) bool qc = false; \ for (e = 0; e < 16 / ESIZE; e++, mask >>= ESIZE) { \ bool sat = false; \ - TYPE r = FN(n[H##ESIZE(e)], m[H##ESIZE(e)], &sat); \ - mergemask(&d[H##ESIZE(e)], r, mask); \ + TYPE r_ = FN(n[H##ESIZE(e)], m[H##ESIZE(e)], &sat); \ + mergemask(&d[H##ESIZE(e)], r_, mask); \ qc |= sat & mask & 1; \ } \ if (qc) { \ @@ -2460,7 +2460,7 @@ static inline int64_t do_sqrshl48_d(int64_t src, int64_t shift, return extval; } } else if (shift < 48) { - int64_t extval = sextract64(src << shift, 0, 48); + extval = sextract64(src << shift, 0, 48); if (!sat || src == (extval >> shift)) { return extval; } @@ -2492,7 +2492,7 @@ static inline uint64_t do_uqrshl48_d(uint64_t src, int64_t shift, return extval; } } else if (shift < 48) { - uint64_t extval = extract64(src << shift, 0, 48); + extval = extract64(src << shift, 0, 48); if (!sat || src == (extval >> shift)) { return extval; } diff --git a/target/arm/tcg/translate-m-nocp.c b/target/arm/tcg/translate-m-nocp.c index 33f6478bb9..42308c4db5 100644 --- a/target/arm/tcg/translate-m-nocp.c +++ b/target/arm/tcg/translate-m-nocp.c @@ -506,7 +506,7 @@ static bool gen_M_fp_sysreg_read(DisasContext *s, int regno, gen_branch_fpInactive(s, TCG_COND_EQ, lab_active); /* fpInactive case: reads as FPDSCR_NS */ - TCGv_i32 tmp = load_cpu_field(v7m.fpdscr[M_REG_NS]); + tmp = load_cpu_field(v7m.fpdscr[M_REG_NS]); storefn(s, opaque, tmp, true); lab_end = gen_new_label(); tcg_gen_br(lab_end);
Fix: target/arm/tcg/translate-m-nocp.c:509:18: error: declaration shadows a local variable [-Werror,-Wshadow] TCGv_i32 tmp = load_cpu_field(v7m.fpdscr[M_REG_NS]); ^ target/arm/tcg/translate-m-nocp.c:433:14: note: previous declaration is here TCGv_i32 tmp; ^ target/arm/tcg/mve_helper.c:2463:17: error: declaration shadows a local variable [-Werror,-Wshadow] int64_t extval = sextract64(src << shift, 0, 48); ^ target/arm/tcg/mve_helper.c:2443:18: note: previous declaration is here int64_t val, extval; ^ target/arm/hvf/hvf.c:1936:13: error: declaration shadows a local variable [-Werror,-Wshadow] int ret = 0; ^ target/arm/hvf/hvf.c:1807:9: note: previous declaration is here int ret; ^ Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- target/arm/hvf/hvf.c | 1 - target/arm/tcg/mve_helper.c | 8 ++++---- target/arm/tcg/translate-m-nocp.c | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-)