Re: [PATCH 02/11] target/arm: Clean up local variable shadowing

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.