[PATCH v2 00/22] (few more) Steps towards enabling -Wshadow

[PATCH v2 00/22] (few more) Steps towards enabling -Wshadow

[Philippe Mathieu-Daudé]

Since v1:
- Addressed review comments
- Added R-b tags
- More patches

For rational see Markus cover on
https://lore.kernel.org/qemu-devel/20230831132546.3525721-1-armbru@redhat.com/

This series contains few more, my take.

Based-on: <20230831132546.3525721-1-armbru@redhat.com>

Philippe Mathieu-Daudé (22):
  tcg: Clean up local variable shadowing
  target/arm/tcg: Clean up local variable shadowing
  target/arm/hvf: Clean up local variable shadowing
  target/mips: Clean up local variable shadowing
  target/m68k: Clean up local variable shadowing
  target/tricore: Clean up local variable shadowing
  hw/arm/armv7m: Clean up local variable shadowing
  hw/arm/virt: Clean up local variable shadowing
  hw/arm/allwinner: Clean up local variable shadowing
  hw/arm/aspeed: Clean up local variable shadowing
  hw/ide/ahci: Clean up local variable shadowing
  hw/m68k: Clean up local variable shadowing
  hw/microblaze: Clean up local variable shadowing
  hw/nios2: Clean up local variable shadowing
  net/eth: Clean up local variable shadowing
  crypto/cipher-gnutls.c: Clean up local variable shadowing
  util/vhost-user-server: Clean up local variable shadowing
  semihosting/arm-compat: Clean up local variable shadowing
  linux-user/strace: Clean up local variable shadowing
  sysemu/device_tree: Clean up local variable shadowing
  softmmu/memory: Clean up local variable shadowing
  RFC softmmu/physmem: Clean up local variable shadowing

 hw/m68k/bootinfo.h                       | 10 ++++------
 include/sysemu/device_tree.h             |  6 ++----
 accel/tcg/tb-maint.c                     |  3 +--
 hw/arm/allwinner-r40.c                   |  7 +++----
 hw/arm/armsse.c                          | 16 ++++++----------
 hw/arm/armv7m.c                          |  2 +-
 hw/arm/aspeed_ast2600.c                  |  2 +-
 hw/arm/virt.c                            |  3 +--
 hw/ide/ahci.c                            |  6 ++----
 hw/microblaze/petalogix_ml605_mmu.c      |  2 +-
 hw/nios2/10m50_devboard.c                |  4 ++--
 linux-user/strace.c                      |  1 -
 net/eth.c                                |  2 --
 semihosting/arm-compat-semi.c            |  9 +++------
 softmmu/memory.c                         |  1 -
 softmmu/physmem.c                        | 10 +++++-----
 target/arm/hvf/hvf.c                     |  8 ++++----
 target/arm/tcg/mve_helper.c              | 16 ++++++++--------
 target/arm/tcg/translate-m-nocp.c        |  2 +-
 target/m68k/translate.c                  |  2 +-
 target/mips/tcg/msa_helper.c             |  8 ++++----
 target/mips/tcg/translate.c              |  8 +++-----
 target/tricore/translate.c               |  6 +++---
 tcg/tcg.c                                | 16 ++++++++--------
 util/vhost-user-server.c                 |  2 +-
 crypto/cipher-gnutls.c.inc               |  4 ++--
 target/mips/tcg/nanomips_translate.c.inc |  6 +++---
 27 files changed, 70 insertions(+), 92 deletions(-)

-- 
2.41.0

[PATCH v2 01/22] tcg: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  tcg/tcg.c:2551:27: error: declaration shadows a local variable [-Werror,-Wshadow]
                    MemOp op = get_memop(oi);
                          ^
  tcg/tcg.c:2437:12: note: previous declaration is here
    TCGOp *op;
           ^
  accel/tcg/tb-maint.c:245:18: error: declaration shadows a local variable [-Werror,-Wshadow]
        for (int i = 0; i < V_L2_SIZE; i++) {
                 ^
  accel/tcg/tb-maint.c:210:9: note: previous declaration is here
    int i;
        ^

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/tb-maint.c |  3 +--
 tcg/tcg.c            | 16 ++++++++--------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
index c406b2f7b7..f1cf3ad736 100644
--- a/accel/tcg/tb-maint.c
+++ b/accel/tcg/tb-maint.c
@@ -207,13 +207,12 @@ static PageDesc *page_find_alloc(tb_page_addr_t index, bool alloc)
 {
     PageDesc *pd;
     void **lp;
-    int i;
 
     /* Level 1.  Always allocated.  */
     lp = l1_map + ((index >> v_l1_shift) & (v_l1_size - 1));
 
     /* Level 2..N-1.  */
-    for (i = v_l2_levels; i > 0; i--) {
+    for (int i = v_l2_levels; i > 0; i--) {
         void **p = qatomic_rcu_read(lp);
 
         if (p == NULL) {
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 620dbe08da..6e2cf52802 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2548,21 +2548,21 @@ static void tcg_dump_ops(TCGContext *s, FILE *f, bool have_prefs)
                 {
                     const char *s_al, *s_op, *s_at;
                     MemOpIdx oi = op->args[k++];
-                    MemOp op = get_memop(oi);
+                    MemOp mop = get_memop(oi);
                     unsigned ix = get_mmuidx(oi);
 
-                    s_al = alignment_name[(op & MO_AMASK) >> MO_ASHIFT];
-                    s_op = ldst_name[op & (MO_BSWAP | MO_SSIZE)];
-                    s_at = atom_name[(op & MO_ATOM_MASK) >> MO_ATOM_SHIFT];
-                    op &= ~(MO_AMASK | MO_BSWAP | MO_SSIZE | MO_ATOM_MASK);
+                    s_al = alignment_name[(mop & MO_AMASK) >> MO_ASHIFT];
+                    s_op = ldst_name[mop & (MO_BSWAP | MO_SSIZE)];
+                    s_at = atom_name[(mop & MO_ATOM_MASK) >> MO_ATOM_SHIFT];
+                    mop &= ~(MO_AMASK | MO_BSWAP | MO_SSIZE | MO_ATOM_MASK);
 
                     /* If all fields are accounted for, print symbolically. */
-                    if (!op && s_al && s_op && s_at) {
+                    if (!mop && s_al && s_op && s_at) {
                         col += ne_fprintf(f, ",%s%s%s,%u",
                                           s_at, s_al, s_op, ix);
                     } else {
-                        op = get_memop(oi);
-                        col += ne_fprintf(f, ",$0x%x,%u", op, ix);
+                        mop = get_memop(oi);
+                        col += ne_fprintf(f, ",$0x%x,%u", mop, ix);
                     }
                     i = 1;
                 }
-- 
2.41.0

[PATCH v2 02/22] target/arm/tcg: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  target/arm/tcg/translate-m-nocp.c: In function ‘gen_M_fp_sysreg_read’:
  target/arm/tcg/translate-m-nocp.c:509:18: warning: declaration of ‘tmp’ shadows a previous local [-Wshadow=compatible-local]
    509 |         TCGv_i32 tmp = load_cpu_field(v7m.fpdscr[M_REG_NS]);
        |                  ^~~
  target/arm/tcg/translate-m-nocp.c:433:14: note: shadowed declaration is here
    433 |     TCGv_i32 tmp;
        |              ^~~
       ---

  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)
        | ^~~~~~~~~~~~
       ---

  target/arm/tcg/mve_helper.c: In function ‘do_sqrshl48_d’:
  target/arm/tcg/mve_helper.c:2463:17: warning: declaration of ‘extval’ shadows a previous local [-Wshadow=compatible-local]
   2463 |         int64_t extval = sextract64(src << shift, 0, 48);
        |                 ^~~~~~
  target/arm/tcg/mve_helper.c:2443:18: note: shadowed declaration is here
   2443 |     int64_t val, extval;
        |                  ^~~~~~
       ---

  target/arm/tcg/mve_helper.c: In function ‘do_uqrshl48_d’:
  target/arm/tcg/mve_helper.c:2495:18: warning: declaration of ‘extval’ shadows a previous local [-Wshadow=compatible-local]
   2495 |         uint64_t extval = extract64(src << shift, 0, 48);
        |                  ^~~~~~
  target/arm/tcg/mve_helper.c:2479:19: note: shadowed declaration is here
   2479 |     uint64_t val, extval;
        |                   ^~~~~~

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/arm/tcg/mve_helper.c       | 16 ++++++++--------
 target/arm/tcg/translate-m-nocp.c |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/target/arm/tcg/mve_helper.c b/target/arm/tcg/mve_helper.c
index 403b345ea3..7f5b6533cd 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) {                                                       \
@@ -1256,11 +1256,11 @@ DO_2OP_SAT(vqsubsw, 4, int32_t, DO_SQSUB_W)
 #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);  \
+        typeof(N) qrshl_ret = FN(N, (int8_t)(M), sizeof(N) * 8, ROUND, &su32); \
         if (su32) {                                                     \
             *satp = true;                                               \
         }                                                               \
-        r;                                                              \
+        qrshl_ret;                                                      \
     })
 
 #define DO_SQSHL_OP(N, M, satp) \
@@ -1298,12 +1298,12 @@ DO_2OP_SAT_U(vqrshlu, DO_UQRSHL_OP)
         for (e = 0; e < 16 / ESIZE; e++, mask >>= ESIZE) {              \
             bool sat = false;                                           \
             if ((e & 1) == XCHG) {                                      \
-                TYPE r = FN(n[H##ESIZE(e)],                             \
+                TYPE vqdmladh_ret = FN(n[H##ESIZE(e)],                  \
                             m[H##ESIZE(e - XCHG)],                      \
                             n[H##ESIZE(e + (1 - 2 * XCHG))],            \
                             m[H##ESIZE(e + (1 - XCHG))],                \
                             ROUND, &sat);                               \
-                mergemask(&d[H##ESIZE(e)], r, mask);                    \
+                mergemask(&d[H##ESIZE(e)], vqdmladh_ret, mask);         \
                 qc |= sat & mask & 1;                                   \
             }                                                           \
         }                                                               \
@@ -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);
-- 
2.41.0

Re: [PATCH v2 02/22] target/arm/tcg: Clean up local variable shadowing

[Peter Maydell]

On Mon, 4 Sept 2023 at 17:12, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Fix:
>
>   target/arm/tcg/translate-m-nocp.c: In function ‘gen_M_fp_sysreg_read’:
>   target/arm/tcg/translate-m-nocp.c:509:18: warning: declaration of ‘tmp’ shadows a previous local [-Wshadow=compatible-local]
>     509 |         TCGv_i32 tmp = load_cpu_field(v7m.fpdscr[M_REG_NS]);
>         |                  ^~~
>   target/arm/tcg/translate-m-nocp.c:433:14: note: shadowed declaration is here
>     433 |     TCGv_i32 tmp;
>         |              ^~~
>        ---
>
>   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)
>         | ^~~~~~~~~~~~
>        ---
>
>   target/arm/tcg/mve_helper.c: In function ‘do_sqrshl48_d’:
>   target/arm/tcg/mve_helper.c:2463:17: warning: declaration of ‘extval’ shadows a previous local [-Wshadow=compatible-local]
>    2463 |         int64_t extval = sextract64(src << shift, 0, 48);
>         |                 ^~~~~~
>   target/arm/tcg/mve_helper.c:2443:18: note: shadowed declaration is here
>    2443 |     int64_t val, extval;
>         |                  ^~~~~~
>        ---
>
>   target/arm/tcg/mve_helper.c: In function ‘do_uqrshl48_d’:
>   target/arm/tcg/mve_helper.c:2495:18: warning: declaration of ‘extval’ shadows a previous local [-Wshadow=compatible-local]
>    2495 |         uint64_t extval = extract64(src << shift, 0, 48);
>         |                  ^~~~~~
>   target/arm/tcg/mve_helper.c:2479:19: note: shadowed declaration is here
>    2479 |     uint64_t val, extval;
>         |                   ^~~~~~
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 03/22] target/arm/hvf: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Per Peter Maydell analysis [*]:

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

  Retain the variable as locally scoped but give it a name that
  doesn't clash with the other function-scoped variable.

This fixes:

  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;
        ^
[*] https://lore.kernel.org/qemu-devel/CAFEAcA_e+fU6JKtS+W63wr9cCJ6btu_hT_ydZWOwC0kBkDYYYQ@mail.gmail.com/

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
Peter, feel free to alter the commit description if it doesn't
sound right.
---
 target/arm/hvf/hvf.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 486f90be1d..0715f8a01c 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -1933,16 +1933,16 @@ int hvf_vcpu_exec(CPUState *cpu)
         uint32_t rt = (syndrome >> 5) & 0x1f;
         uint32_t reg = syndrome & SYSREG_MASK;
         uint64_t val;
-        int ret = 0;
+        int sysreg_ret = 0;
 
         if (isread) {
-            ret = hvf_sysreg_read(cpu, reg, rt);
+            sysreg_ret = hvf_sysreg_read(cpu, reg, rt);
         } else {
             val = hvf_get_reg(cpu, rt);
-            ret = hvf_sysreg_write(cpu, reg, val);
+            sysreg_ret = hvf_sysreg_write(cpu, reg, val);
         }
 
-        advance_pc = !ret;
+        advance_pc = !sysreg_ret;
         break;
     }
     case EC_WFX_TRAP:
-- 
2.41.0

Re: [PATCH v2 03/22] target/arm/hvf: Clean up local variable shadowing

[Peter Maydell]

On Mon, 4 Sept 2023 at 17:12, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Per Peter Maydell analysis [*]:
>
>   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().
>
>   Retain the variable as locally scoped but give it a name that
>   doesn't clash with the other function-scoped variable.
>
> This fixes:
>
>   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;
>         ^
> [*] https://lore.kernel.org/qemu-devel/CAFEAcA_e+fU6JKtS+W63wr9cCJ6btu_hT_ydZWOwC0kBkDYYYQ@mail.gmail.com/
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 04/22] target/mips: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  target/mips/tcg/nanomips_translate.c.inc:4410:33: error: declaration shadows a local variable [-Werror,-Wshadow]
                        int32_t imm = extract32(ctx->opcode, 1, 13) |
                                ^
  target/mips/tcg/nanomips_translate.c.inc:3577:9: note: previous declaration is here
    int imm;
        ^
  target/mips/tcg/translate.c:15578:19: error: declaration shadows a local variable [-Werror,-Wshadow]
    for (unsigned i = 1; i < 32; i++) {
                  ^
  target/mips/tcg/translate.c:15567:9: note: previous declaration is here
    int i;
        ^
  target/mips/tcg/msa_helper.c:7478:13: error: declaration shadows a local variable [-Werror,-Wshadow]
            MSA_FLOAT_MAXOP(pwx->w[0], min, pws->w[0], pws->w[0], 32);
            ^
  target/mips/tcg/msa_helper.c:7434:23: note: expanded from macro 'MSA_FLOAT_MAXOP'
        float_status *status = &env->active_tc.msa_fp_status;
                      ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/mips/tcg/msa_helper.c             | 8 ++++----
 target/mips/tcg/translate.c              | 8 +++-----
 target/mips/tcg/nanomips_translate.c.inc | 6 +++---
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/target/mips/tcg/msa_helper.c b/target/mips/tcg/msa_helper.c
index 29b31d70fe..391a5fca26 100644
--- a/target/mips/tcg/msa_helper.c
+++ b/target/mips/tcg/msa_helper.c
@@ -7431,15 +7431,15 @@ void helper_msa_ftq_df(CPUMIPSState *env, uint32_t df, uint32_t wd,
 
 #define MSA_FLOAT_MAXOP(DEST, OP, ARG1, ARG2, BITS)                         \
     do {                                                                    \
-        float_status *status = &env->active_tc.msa_fp_status;               \
+        float_status *status_ = &env->active_tc.msa_fp_status;              \
         int c;                                                              \
                                                                             \
-        set_float_exception_flags(0, status);                               \
-        DEST = float ## BITS ## _ ## OP(ARG1, ARG2, status);                \
+        set_float_exception_flags(0, status_);                              \
+        DEST = float ## BITS ## _ ## OP(ARG1, ARG2, status_);               \
         c = update_msacsr(env, 0, 0);                                       \
                                                                             \
         if (get_enabled_exceptions(env, c)) {                               \
-            DEST = ((FLOAT_SNAN ## BITS(status) >> 6) << 6) | c;            \
+            DEST = ((FLOAT_SNAN ## BITS(status_) >> 6) << 6) | c;           \
         }                                                                   \
     } while (0)
 
diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index 9bb40f1849..26d741d960 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -15564,10 +15564,8 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int *max_insns,
 
 void mips_tcg_init(void)
 {
-    int i;
-
     cpu_gpr[0] = NULL;
-    for (i = 1; i < 32; i++)
+    for (unsigned i = 1; i < 32; i++)
         cpu_gpr[i] = tcg_global_mem_new(cpu_env,
                                         offsetof(CPUMIPSState,
                                                  active_tc.gpr[i]),
@@ -15584,7 +15582,7 @@ void mips_tcg_init(void)
                                                rname);
     }
 #endif /* !TARGET_MIPS64 */
-    for (i = 0; i < 32; i++) {
+    for (unsigned i = 0; i < 32; i++) {
         int off = offsetof(CPUMIPSState, active_fpu.fpr[i].wr.d[0]);
 
         fpu_f64[i] = tcg_global_mem_new_i64(cpu_env, off, fregnames[i]);
@@ -15592,7 +15590,7 @@ void mips_tcg_init(void)
     msa_translate_init();
     cpu_PC = tcg_global_mem_new(cpu_env,
                                 offsetof(CPUMIPSState, active_tc.PC), "PC");
-    for (i = 0; i < MIPS_DSP_ACC; i++) {
+    for (unsigned i = 0; i < MIPS_DSP_ACC; i++) {
         cpu_HI[i] = tcg_global_mem_new(cpu_env,
                                        offsetof(CPUMIPSState, active_tc.HI[i]),
                                        regnames_HI[i]);
diff --git a/target/mips/tcg/nanomips_translate.c.inc b/target/mips/tcg/nanomips_translate.c.inc
index a98dde0d2e..d81a7c2d11 100644
--- a/target/mips/tcg/nanomips_translate.c.inc
+++ b/target/mips/tcg/nanomips_translate.c.inc
@@ -4407,8 +4407,8 @@ static int decode_nanomips_32_48_opc(CPUMIPSState *env, DisasContext *ctx)
                 case NM_BPOSGE32C:
                     check_dsp_r3(ctx);
                     {
-                        int32_t imm = extract32(ctx->opcode, 1, 13) |
-                                      extract32(ctx->opcode, 0, 1) << 13;
+                        imm = extract32(ctx->opcode, 1, 13)
+                            | extract32(ctx->opcode, 0, 1) << 13;
 
                         gen_compute_branch_nm(ctx, OPC_BPOSGE32, 4, -1, -2,
                                               imm << 1);
@@ -4635,7 +4635,7 @@ static int decode_isa_nanomips(CPUMIPSState *env, DisasContext *ctx)
         break;
     case NM_LI16:
         {
-            int imm = extract32(ctx->opcode, 0, 7);
+            imm = extract32(ctx->opcode, 0, 7);
             imm = (imm == 0x7f ? -1 : imm);
             if (rt != 0) {
                 tcg_gen_movi_tl(cpu_gpr[rt], imm);
-- 
2.41.0

[PATCH v2 05/22] target/m68k: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  target/m68k/translate.c:828:18: error: declaration shadows a local variable [-Werror,-Wshadow]
            TCGv tmp = tcg_temp_new();
                 ^
  target/m68k/translate.c:801:15: note: previous declaration is here
    TCGv reg, tmp, result;
              ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/m68k/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 15b3701b8f..f69ae0e3b0 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -825,7 +825,7 @@ static TCGv gen_ea_mode(CPUM68KState *env, DisasContext *s, int mode, int reg0,
         reg = get_areg(s, reg0);
         result = gen_ldst(s, opsize, reg, val, what, index);
         if (what == EA_STORE || !addrp) {
-            TCGv tmp = tcg_temp_new();
+            tmp = tcg_temp_new();
             if (reg0 == 7 && opsize == OS_BYTE &&
                 m68k_feature(s->env, M68K_FEATURE_M68K)) {
                 tcg_gen_addi_i32(tmp, reg, 2);
-- 
2.41.0

[PATCH v2 06/22] target/tricore: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  target/tricore/translate.c:5016:18: warning: declaration of ‘temp’ shadows a previous local [-Wshadow=compatible-local]
   5016 |             TCGv temp = tcg_constant_i32(const9);
        |                  ^~~~
  target/tricore/translate.c:4958:10: note: shadowed declaration is here
   4958 |     TCGv temp;
        |          ^~~~

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/tricore/translate.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 6ae5ccbf72..9ca211b2a8 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -4962,8 +4962,6 @@ static void decode_rc_logical_shift(DisasContext *ctx)
     const9 = MASK_OP_RC_CONST9(ctx->opcode);
     op2 = MASK_OP_RC_OP2(ctx->opcode);
 
-    temp = tcg_temp_new();
-
     switch (op2) {
     case OPC2_32_RC_AND:
         tcg_gen_andi_tl(cpu_gpr_d[r2], cpu_gpr_d[r1], const9);
@@ -4972,10 +4970,12 @@ static void decode_rc_logical_shift(DisasContext *ctx)
         tcg_gen_andi_tl(cpu_gpr_d[r2], cpu_gpr_d[r1], ~const9);
         break;
     case OPC2_32_RC_NAND:
+        temp = tcg_temp_new();
         tcg_gen_movi_tl(temp, const9);
         tcg_gen_nand_tl(cpu_gpr_d[r2], cpu_gpr_d[r1], temp);
         break;
     case OPC2_32_RC_NOR:
+        temp = tcg_temp_new();
         tcg_gen_movi_tl(temp, const9);
         tcg_gen_nor_tl(cpu_gpr_d[r2], cpu_gpr_d[r1], temp);
         break;
@@ -5013,7 +5013,7 @@ static void decode_rc_logical_shift(DisasContext *ctx)
         break;
     case OPC2_32_RC_SHUFFLE:
         if (has_feature(ctx, TRICORE_FEATURE_162)) {
-            TCGv temp = tcg_constant_i32(const9);
+            temp = tcg_constant_i32(const9);
             gen_helper_shuffle(cpu_gpr_d[r2], cpu_gpr_d[r1], temp);
         } else {
             generate_trap(ctx, TRAPC_INSN_ERR, TIN2_IOPC);
-- 
2.41.0

Re: [PATCH v2 06/22] target/tricore: Clean up local variable shadowing

[Bastian Koppelmann]

On Mon, Sep 04, 2023 at 06:12:18PM +0200, Philippe Mathieu-Daudé wrote:
> Fix:
> 
>   target/tricore/translate.c:5016:18: warning: declaration of ‘temp’ shadows a previous local [-Wshadow=compatible-local]
>    5016 |             TCGv temp = tcg_constant_i32(const9);
>         |                  ^~~~
>   target/tricore/translate.c:4958:10: note: shadowed declaration is here
>    4958 |     TCGv temp;
>         |          ^~~~
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  target/tricore/translate.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>

Cheers,
Bastian

[PATCH v2 07/22] hw/arm/armv7m: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  hw/arm/armv7m.c: In function ‘armv7m_realize’:
  hw/arm/armv7m.c:520:27: warning: declaration of ‘sbd’ shadows a previous local [-Wshadow=compatible-local]
    520 |             SysBusDevice *sbd = SYS_BUS_DEVICE(&s->bitband[i]);
        |                           ^~~
  hw/arm/armv7m.c:278:19: note: shadowed declaration is here
    278 |     SysBusDevice *sbd;
        |                   ^~~
       ---

  hw/arm/armsse.c: In function ‘armsse_realize’:
  hw/arm/armsse.c:1471:27: warning: declaration of ‘mr’ shadows a previous local [-Wshadow=compatible-local]
   1471 |             MemoryRegion *mr;
        |                           ^~
  hw/arm/armsse.c:917:19: note: shadowed declaration is here
    917 |     MemoryRegion *mr;
        |                   ^~
       ---

  hw/arm/armsse.c:1608:22: warning: declaration of ‘dev_splitter’ shadows a previous local [-Wshadow=compatible-local]
   1608 |         DeviceState *dev_splitter = DEVICE(splitter);
        |                      ^~~~~~~~~~~~
  hw/arm/armsse.c:923:18: note: shadowed declaration is here
    923 |     DeviceState *dev_splitter;
        |                  ^~~~~~~~~~~~

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/arm/armsse.c | 16 ++++++----------
 hw/arm/armv7m.c |  2 +-
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
index 11cd08b6c1..31acbf7347 100644
--- a/hw/arm/armsse.c
+++ b/hw/arm/armsse.c
@@ -1468,7 +1468,6 @@ static void armsse_realize(DeviceState *dev, Error **errp)
     if (info->has_cachectrl) {
         for (i = 0; i < info->num_cpus; i++) {
             char *name = g_strdup_printf("cachectrl%d", i);
-            MemoryRegion *mr;
 
             qdev_prop_set_string(DEVICE(&s->cachectrl[i]), "name", name);
             g_free(name);
@@ -1484,7 +1483,6 @@ static void armsse_realize(DeviceState *dev, Error **errp)
     if (info->has_cpusecctrl) {
         for (i = 0; i < info->num_cpus; i++) {
             char *name = g_strdup_printf("CPUSECCTRL%d", i);
-            MemoryRegion *mr;
 
             qdev_prop_set_string(DEVICE(&s->cpusecctrl[i]), "name", name);
             g_free(name);
@@ -1499,7 +1497,6 @@ static void armsse_realize(DeviceState *dev, Error **errp)
     }
     if (info->has_cpuid) {
         for (i = 0; i < info->num_cpus; i++) {
-            MemoryRegion *mr;
 
             qdev_prop_set_uint32(DEVICE(&s->cpuid[i]), "CPUID", i);
             if (!sysbus_realize(SYS_BUS_DEVICE(&s->cpuid[i]), errp)) {
@@ -1512,7 +1509,6 @@ static void armsse_realize(DeviceState *dev, Error **errp)
     }
     if (info->has_cpu_pwrctrl) {
         for (i = 0; i < info->num_cpus; i++) {
-            MemoryRegion *mr;
 
             if (!sysbus_realize(SYS_BUS_DEVICE(&s->cpu_pwrctrl[i]), errp)) {
                 return;
@@ -1605,7 +1601,7 @@ static void armsse_realize(DeviceState *dev, Error **errp)
     /* Wire up the splitters for the MPC IRQs */
     for (i = 0; i < IOTS_NUM_EXP_MPC + info->sram_banks; i++) {
         SplitIRQ *splitter = &s->mpc_irq_splitter[i];
-        DeviceState *dev_splitter = DEVICE(splitter);
+        DeviceState *devs = DEVICE(splitter);
 
         if (!object_property_set_int(OBJECT(splitter), "num-lines", 2,
                                      errp)) {
@@ -1617,22 +1613,22 @@ static void armsse_realize(DeviceState *dev, Error **errp)
 
         if (i < IOTS_NUM_EXP_MPC) {
             /* Splitter input is from GPIO input line */
-            s->mpcexp_status_in[i] = qdev_get_gpio_in(dev_splitter, 0);
-            qdev_connect_gpio_out(dev_splitter, 0,
+            s->mpcexp_status_in[i] = qdev_get_gpio_in(devs, 0);
+            qdev_connect_gpio_out(devs, 0,
                                   qdev_get_gpio_in_named(dev_secctl,
                                                          "mpcexp_status", i));
         } else {
             /* Splitter input is from our own MPC */
             qdev_connect_gpio_out_named(DEVICE(&s->mpc[i - IOTS_NUM_EXP_MPC]),
                                         "irq", 0,
-                                        qdev_get_gpio_in(dev_splitter, 0));
-            qdev_connect_gpio_out(dev_splitter, 0,
+                                        qdev_get_gpio_in(devs, 0));
+            qdev_connect_gpio_out(devs, 0,
                                   qdev_get_gpio_in_named(dev_secctl,
                                                          "mpc_status",
                                                          i - IOTS_NUM_EXP_MPC));
         }
 
-        qdev_connect_gpio_out(dev_splitter, 1,
+        qdev_connect_gpio_out(devs, 1,
                               qdev_get_gpio_in(DEVICE(&s->mpc_irq_orgate), i));
     }
     /* Create GPIO inputs which will pass the line state for our
diff --git a/hw/arm/armv7m.c b/hw/arm/armv7m.c
index bf173b10b8..1f78e18872 100644
--- a/hw/arm/armv7m.c
+++ b/hw/arm/armv7m.c
@@ -517,7 +517,7 @@ static void armv7m_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < ARRAY_SIZE(s->bitband); i++) {
         if (s->enable_bitband) {
             Object *obj = OBJECT(&s->bitband[i]);
-            SysBusDevice *sbd = SYS_BUS_DEVICE(&s->bitband[i]);
+            sbd = SYS_BUS_DEVICE(&s->bitband[i]);
 
             if (!object_property_set_int(obj, "base",
                                          bitband_input_addr[i], errp)) {
-- 
2.41.0

Re: [PATCH v2 07/22] hw/arm/armv7m: Clean up local variable shadowing

[Peter Maydell]

On Mon, 4 Sept 2023 at 17:13, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Fix:
>
>   hw/arm/armv7m.c: In function ‘armv7m_realize’:
>   hw/arm/armv7m.c:520:27: warning: declaration of ‘sbd’ shadows a previous local [-Wshadow=compatible-local]
>     520 |             SysBusDevice *sbd = SYS_BUS_DEVICE(&s->bitband[i]);
>         |                           ^~~
>   hw/arm/armv7m.c:278:19: note: shadowed declaration is here
>     278 |     SysBusDevice *sbd;
>         |                   ^~~
>        ---
>
>   hw/arm/armsse.c: In function ‘armsse_realize’:
>   hw/arm/armsse.c:1471:27: warning: declaration of ‘mr’ shadows a previous local [-Wshadow=compatible-local]
>    1471 |             MemoryRegion *mr;
>         |                           ^~
>   hw/arm/armsse.c:917:19: note: shadowed declaration is here
>     917 |     MemoryRegion *mr;
>         |                   ^~
>        ---
>
>   hw/arm/armsse.c:1608:22: warning: declaration of ‘dev_splitter’ shadows a previous local [-Wshadow=compatible-local]
>    1608 |         DeviceState *dev_splitter = DEVICE(splitter);
>         |                      ^~~~~~~~~~~~
>   hw/arm/armsse.c:923:18: note: shadowed declaration is here
>     923 |     DeviceState *dev_splitter;
>         |                  ^~~~~~~~~~~~
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 08/22] hw/arm/virt: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  hw/arm/virt.c:821:22: error: declaration shadows a local variable [-Werror,-Wshadow]
            qemu_irq irq = qdev_get_gpio_in(vms->gic,
                     ^
  hw/arm/virt.c:803:13: note: previous declaration is here
        int irq;
            ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a13c658bbf..32a964b572 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -800,7 +800,6 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
     for (i = 0; i < smp_cpus; i++) {
         DeviceState *cpudev = DEVICE(qemu_get_cpu(i));
         int ppibase = NUM_IRQS + i * GIC_INTERNAL + GIC_NR_SGIS;
-        int irq;
         /* Mapping from the output timer irq lines from the CPU to the
          * GIC PPI inputs we use for the virt board.
          */
@@ -811,7 +810,7 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem)
             [GTIMER_SEC]  = ARCH_TIMER_S_EL1_IRQ,
         };
 
-        for (irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
+        for (unsigned irq = 0; irq < ARRAY_SIZE(timer_irq); irq++) {
             qdev_connect_gpio_out(cpudev, irq,
                                   qdev_get_gpio_in(vms->gic,
                                                    ppibase + timer_irq[irq]));
-- 
2.41.0

[PATCH v2 09/22] hw/arm/allwinner: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  hw/arm/allwinner-r40.c:412:14: error: declaration shadows a local variable [-Werror,-Wshadow]
    for (int i = 0; i < AW_R40_NUM_MMCS; i++) {
             ^
  hw/arm/allwinner-r40.c:299:14: note: previous declaration is here
    unsigned i;
             ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/allwinner-r40.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
index 7d29eb224f..a0d367c60d 100644
--- a/hw/arm/allwinner-r40.c
+++ b/hw/arm/allwinner-r40.c
@@ -296,10 +296,9 @@ static void allwinner_r40_realize(DeviceState *dev, Error **errp)
 {
     const char *r40_nic_models[] = { "gmac", "emac", NULL };
     AwR40State *s = AW_R40(dev);
-    unsigned i;
 
     /* CPUs */
-    for (i = 0; i < AW_R40_NUM_CPUS; i++) {
+    for (unsigned i = 0; i < AW_R40_NUM_CPUS; i++) {
 
         /*
          * Disable secondary CPUs. Guest EL3 firmware will start
@@ -335,7 +334,7 @@ static void allwinner_r40_realize(DeviceState *dev, Error **errp)
      * maintenance interrupt signal to the appropriate GIC PPI inputs,
      * and the GIC's IRQ/FIQ/VIRQ/VFIQ interrupt outputs to the CPU's inputs.
      */
-    for (i = 0; i < AW_R40_NUM_CPUS; i++) {
+    for (unsigned i = 0; i < AW_R40_NUM_CPUS; i++) {
         DeviceState *cpudev = DEVICE(&s->cpus[i]);
         int ppibase = AW_R40_GIC_NUM_SPI + i * GIC_INTERNAL + GIC_NR_SGIS;
         int irq;
@@ -494,7 +493,7 @@ static void allwinner_r40_realize(DeviceState *dev, Error **errp)
                        qdev_get_gpio_in(DEVICE(&s->gic), AW_R40_GIC_SPI_EMAC));
 
     /* Unimplemented devices */
-    for (i = 0; i < ARRAY_SIZE(r40_unimplemented); i++) {
+    for (unsigned i = 0; i < ARRAY_SIZE(r40_unimplemented); i++) {
         create_unimplemented_device(r40_unimplemented[i].device_name,
                                     r40_unimplemented[i].base,
                                     r40_unimplemented[i].size);
-- 
2.41.0

[PATCH v2 10/22] hw/arm/aspeed: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  hw/arm/aspeed_ast2600.c:391:18: error: declaration shadows a local variable [-Werror,-Wshadow]
        qemu_irq irq = aspeed_soc_get_irq(s, ASPEED_DEV_TIMER1 + i);
                 ^
  hw/arm/aspeed_ast2600.c:283:14: note: previous declaration is here
    qemu_irq irq;
             ^

  hw/arm/aspeed_ast2600.c:416:18: error: declaration shadows a local variable [-Werror,-Wshadow]
        qemu_irq irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore),
                 ^
  hw/arm/aspeed_ast2600.c:283:14: note: previous declaration is here
    qemu_irq irq;
             ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
---
 hw/arm/aspeed_ast2600.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index a8b3a8065a..f54063445d 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -280,7 +280,6 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     AspeedSoCState *s = ASPEED_SOC(dev);
     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
     Error *err = NULL;
-    qemu_irq irq;
     g_autofree char *sram_name = NULL;
 
     /* Default boot region (SPI memory or ROMs) */
@@ -339,6 +338,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < sc->num_cpus; i++) {
         SysBusDevice *sbd = SYS_BUS_DEVICE(&s->a7mpcore);
         DeviceState  *d   = DEVICE(&s->cpu[i]);
+        qemu_irq irq;
 
         irq = qdev_get_gpio_in(d, ARM_CPU_IRQ);
         sysbus_connect_irq(sbd, i, irq);
-- 
2.41.0

[PATCH v2 11/22] hw/ide/ahci: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

  hw/ide/ahci.c:1577:23: error: declaration shadows a local variable [-Werror,-Wshadow]
            IDEState *s = &ad->port.ifs[j];
                      ^
  hw/ide/ahci.c:1569:29: note: previous declaration is here
    void ahci_uninit(AHCIState *s)
                                ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/ide/ahci.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 48d550f633..8c9a7c2117 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1573,10 +1573,8 @@ void ahci_uninit(AHCIState *s)
     for (i = 0; i < s->ports; i++) {
         AHCIDevice *ad = &s->dev[i];
 
-        for (j = 0; j < 2; j++) {
-            IDEState *s = &ad->port.ifs[j];
-
-            ide_exit(s);
+        for (j = 0; j < ARRAY_SIZE(ad->port.ifs); j++) {
+            ide_exit(&ad->port.ifs[j]);
         }
         object_unparent(OBJECT(&ad->port));
     }
-- 
2.41.0

Re: [PATCH v2 11/22] hw/ide/ahci: Clean up local variable shadowing

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

>   hw/ide/ahci.c:1577:23: error: declaration shadows a local variable [-Werror,-Wshadow]
>             IDEState *s = &ad->port.ifs[j];
>                       ^
>   hw/ide/ahci.c:1569:29: note: previous declaration is here
>     void ahci_uninit(AHCIState *s)
>                                 ^
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/ide/ahci.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 48d550f633..8c9a7c2117 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1573,10 +1573,8 @@ void ahci_uninit(AHCIState *s)
>      for (i = 0; i < s->ports; i++) {
>          AHCIDevice *ad = &s->dev[i];
>  
> -        for (j = 0; j < 2; j++) {
> -            IDEState *s = &ad->port.ifs[j];
> -
> -            ide_exit(s);
> +        for (j = 0; j < ARRAY_SIZE(ad->port.ifs); j++) {

This line's change is unrelated.

When we're dealing with at a huge changeset like the tree-wide
-Wshadow=local cleanup, I prefer to keep changes minimal to ease review
as much as possible.  But it's sunk cost now.

ad->port.ifs is IDEBus member IDEState ifs[2].  .ifs[0] corresponds to
.master, and .ifs[1] corresponds to .slave.  Size 2 is fundamental, and
will not ever change.  Whether we count to 2 or to ARRAY_SIZE(.ifs) is a
matter of taste.  Taste is up to the maintainer, not me.  John?

> +            ide_exit(&ad->port.ifs[j]);
>          }
>          object_unparent(OBJECT(&ad->port));
>      }

[PATCH v2 12/22] hw/m68k: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  hw/m68k/virt.c:263:13: error: declaration shadows a local variable [-Werror,-Wshadow]
            BOOTINFOSTR(param_ptr, BI_COMMAND_LINE,
            ^
  hw/m68k/bootinfo.h:47:13: note: expanded from macro 'BOOTINFOSTR'
        int i; \
            ^
  hw/m68k/virt.c:130:9: note: previous declaration is here
    int i;
        ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/m68k/bootinfo.h | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/m68k/bootinfo.h b/hw/m68k/bootinfo.h
index a3d37e3c80..0e6e3eea87 100644
--- a/hw/m68k/bootinfo.h
+++ b/hw/m68k/bootinfo.h
@@ -44,15 +44,14 @@
 
 #define BOOTINFOSTR(base, id, string) \
     do { \
-        int i; \
         stw_p(base, id); \
         base += 2; \
         stw_p(base, \
                  (sizeof(struct bi_record) + strlen(string) + \
                   1 /* null termination */ + 3 /* padding */) & ~3); \
         base += 2; \
-        for (i = 0; string[i]; i++) { \
-            stb_p(base++, string[i]); \
+        for (unsigned i_ = 0; string[i_]; i_++) { \
+            stb_p(base++, string[i_]); \
         } \
         stb_p(base++, 0); \
         base = QEMU_ALIGN_PTR_UP(base, 4); \
@@ -60,7 +59,6 @@
 
 #define BOOTINFODATA(base, id, data, len) \
     do { \
-        int i; \
         stw_p(base, id); \
         base += 2; \
         stw_p(base, \
@@ -69,8 +67,8 @@
         base += 2; \
         stw_p(base, len); \
         base += 2; \
-        for (i = 0; i < len; ++i) { \
-            stb_p(base++, data[i]); \
+        for (unsigned i_ = 0; i_ < len; ++i_) { \
+            stb_p(base++, data[i_]); \
         } \
         base = QEMU_ALIGN_PTR_UP(base, 4); \
     } while (0)
-- 
2.41.0

Re: [PATCH v2 12/22] hw/m68k: Clean up local variable shadowing

[Thomas Huth]

On 04/09/2023 18.12, Philippe Mathieu-Daudé wrote:
> Fix:
> 
>    hw/m68k/virt.c:263:13: error: declaration shadows a local variable [-Werror,-Wshadow]
>              BOOTINFOSTR(param_ptr, BI_COMMAND_LINE,
>              ^
>    hw/m68k/bootinfo.h:47:13: note: expanded from macro 'BOOTINFOSTR'
>          int i; \
>              ^
>    hw/m68k/virt.c:130:9: note: previous declaration is here
>      int i;
>          ^
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/m68k/bootinfo.h | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH v2 13/22] hw/microblaze: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  hw/microblaze/petalogix_ml605_mmu.c: In function ‘petalogix_ml605_init’:
  hw/microblaze/petalogix_ml605_mmu.c:186:24: warning: declaration of ‘dinfo’ shadows a previous local [-Wshadow=compatible-local]
    186 |             DriveInfo *dinfo = drive_get(IF_MTD, 0, i);
        |                        ^~~~~
  hw/microblaze/petalogix_ml605_mmu.c:78:16: note: shadowed declaration is here
     78 |     DriveInfo *dinfo;
        |                ^~~~~

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/microblaze/petalogix_ml605_mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/microblaze/petalogix_ml605_mmu.c b/hw/microblaze/petalogix_ml605_mmu.c
index babb053035..445509f699 100644
--- a/hw/microblaze/petalogix_ml605_mmu.c
+++ b/hw/microblaze/petalogix_ml605_mmu.c
@@ -183,7 +183,7 @@ petalogix_ml605_init(MachineState *machine)
         spi = (SSIBus *)qdev_get_child_bus(dev, "spi");
 
         for (i = 0; i < NUM_SPI_FLASHES; i++) {
-            DriveInfo *dinfo = drive_get(IF_MTD, 0, i);
+            dinfo = drive_get(IF_MTD, 0, i);
             qemu_irq cs_line;
 
             dev = qdev_new("n25q128");
-- 
2.41.0

[PATCH v2 14/22] hw/nios2: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  hw/nios2/10m50_devboard.c: In function ‘nios2_10m50_ghrd_init’:
  hw/nios2/10m50_devboard.c:101:22: warning: declaration of ‘dev’ shadows a previous local [-Wshadow=compatible-local]
    101 |         DeviceState *dev = qdev_new(TYPE_NIOS2_VIC);
        |                      ^~~
  hw/nios2/10m50_devboard.c:60:18: note: shadowed declaration is here
     60 |     DeviceState *dev;
        |                  ^~~

  hw/nios2/10m50_devboard.c:110:18: warning: declaration of ‘i’ shadows a previous local [-Wshadow=compatible-local]
    110 |         for (int i = 0; i < 32; i++) {
        |                  ^
  hw/nios2/10m50_devboard.c:67:9: note: shadowed declaration is here
     67 |     int i;
        |         ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/nios2/10m50_devboard.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/nios2/10m50_devboard.c b/hw/nios2/10m50_devboard.c
index 91383fb097..952a0dc33e 100644
--- a/hw/nios2/10m50_devboard.c
+++ b/hw/nios2/10m50_devboard.c
@@ -98,7 +98,7 @@ static void nios2_10m50_ghrd_init(MachineState *machine)
     qdev_realize_and_unref(DEVICE(cpu), NULL, &error_fatal);
 
     if (nms->vic) {
-        DeviceState *dev = qdev_new(TYPE_NIOS2_VIC);
+        dev = qdev_new(TYPE_NIOS2_VIC);
         MemoryRegion *dev_mr;
         qemu_irq cpu_irq;
 
@@ -107,7 +107,7 @@ static void nios2_10m50_ghrd_init(MachineState *machine)
 
         cpu_irq = qdev_get_gpio_in_named(DEVICE(cpu), "EIC", 0);
         sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, cpu_irq);
-        for (int i = 0; i < 32; i++) {
+        for (i = 0; i < 32; i++) {
             irq[i] = qdev_get_gpio_in(dev, i);
         }
 
-- 
2.41.0

[PATCH v2 15/22] net/eth: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  net/eth.c:435:20: error: declaration shadows a local variable [-Werror,-Wshadow]
            size_t input_size = iov_size(pkt, pkt_frags);
                   ^
  net/eth.c:413:16: note: previous declaration is here
        size_t input_size = iov_size(pkt, pkt_frags);
               ^

Suggested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 net/eth.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/eth.c b/net/eth.c
index 649e66bb1f..3f680cc033 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -432,8 +432,6 @@ _eth_get_rss_ex_src_addr(const struct iovec *pkt, int pkt_frags,
         }
 
         if (opthdr.type == IP6_OPT_HOME) {
-            size_t input_size = iov_size(pkt, pkt_frags);
-
             if (input_size < opt_offset + sizeof(opthdr)) {
                 return false;
             }
-- 
2.41.0

Re: [PATCH v2 15/22] net/eth: Clean up local variable shadowing

[Eric Blake]

On Mon, Sep 04, 2023 at 06:12:27PM +0200, Philippe Mathieu-Daudé wrote:
> Fix:
> 
>   net/eth.c:435:20: error: declaration shadows a local variable [-Werror,-Wshadow]
>             size_t input_size = iov_size(pkt, pkt_frags);
>                    ^
>   net/eth.c:413:16: note: previous declaration is here
>         size_t input_size = iov_size(pkt, pkt_frags);
>                ^
> 
> Suggested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  net/eth.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH v2 15/22] net/eth: Clean up local variable shadowing

[Akihiko Odaki]

On 2023/09/05 1:12, Philippe Mathieu-Daudé wrote:
> Fix:
> 
>    net/eth.c:435:20: error: declaration shadows a local variable [-Werror,-Wshadow]
>              size_t input_size = iov_size(pkt, pkt_frags);
>                     ^
>    net/eth.c:413:16: note: previous declaration is here
>          size_t input_size = iov_size(pkt, pkt_frags);
>                 ^
> 
> Suggested-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   net/eth.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/net/eth.c b/net/eth.c
> index 649e66bb1f..3f680cc033 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -432,8 +432,6 @@ _eth_get_rss_ex_src_addr(const struct iovec *pkt, int pkt_frags,
>           }
>   
>           if (opthdr.type == IP6_OPT_HOME) {
> -            size_t input_size = iov_size(pkt, pkt_frags);
> -
>               if (input_size < opt_offset + sizeof(opthdr)) {
>                   return false;
>               }

Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>

[PATCH v2 16/22] crypto/cipher-gnutls.c: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  In file included from crypto/cipher.c:140:
  crypto/cipher-gnutls.c.inc: In function ‘qcrypto_gnutls_cipher_encrypt’:
  crypto/cipher-gnutls.c.inc:116:17: warning: declaration of ‘err’ shadows a previous local [-Wshadow=compatible-local]
    116 |             int err = gnutls_cipher_init(&handle, ctx->galg, &gkey, NULL);
        |                 ^~~
  crypto/cipher-gnutls.c.inc:94:9: note: shadowed declaration is here
     94 |     int err;
        |         ^~~
       ---

  crypto/cipher-gnutls.c.inc: In function ‘qcrypto_gnutls_cipher_decrypt’:
  crypto/cipher-gnutls.c.inc:177:17: warning: declaration of ‘err’ shadows a previous local [-Wshadow=compatible-local]
    177 |             int err = gnutls_cipher_init(&handle, ctx->galg, &gkey, NULL);
        |                 ^~~
  crypto/cipher-gnutls.c.inc:154:9: note: shadowed declaration is here
    154 |     int err;
        |         ^~~

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 crypto/cipher-gnutls.c.inc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/cipher-gnutls.c.inc b/crypto/cipher-gnutls.c.inc
index 501e4e07a5..d3e231c13c 100644
--- a/crypto/cipher-gnutls.c.inc
+++ b/crypto/cipher-gnutls.c.inc
@@ -113,7 +113,7 @@ qcrypto_gnutls_cipher_encrypt(QCryptoCipher *cipher,
         while (len) {
             gnutls_cipher_hd_t handle;
             gnutls_datum_t gkey = { (unsigned char *)ctx->key, ctx->nkey };
-            int err = gnutls_cipher_init(&handle, ctx->galg, &gkey, NULL);
+            err = gnutls_cipher_init(&handle, ctx->galg, &gkey, NULL);
             if (err != 0) {
                 error_setg(errp, "Cannot initialize cipher: %s",
                            gnutls_strerror(err));
@@ -174,7 +174,7 @@ qcrypto_gnutls_cipher_decrypt(QCryptoCipher *cipher,
         while (len) {
             gnutls_cipher_hd_t handle;
             gnutls_datum_t gkey = { (unsigned char *)ctx->key, ctx->nkey };
-            int err = gnutls_cipher_init(&handle, ctx->galg, &gkey, NULL);
+            err = gnutls_cipher_init(&handle, ctx->galg, &gkey, NULL);
             if (err != 0) {
                 error_setg(errp, "Cannot initialize cipher: %s",
                            gnutls_strerror(err));
-- 
2.41.0

Re: [PATCH v2 16/22] crypto/cipher-gnutls.c: Clean up local variable shadowing

[Daniel P. Berrangé]

On Mon, Sep 04, 2023 at 06:12:28PM +0200, Philippe Mathieu-Daudé wrote:
> Fix:
> 
>   In file included from crypto/cipher.c:140:
>   crypto/cipher-gnutls.c.inc: In function ‘qcrypto_gnutls_cipher_encrypt’:
>   crypto/cipher-gnutls.c.inc:116:17: warning: declaration of ‘err’ shadows a previous local [-Wshadow=compatible-local]
>     116 |             int err = gnutls_cipher_init(&handle, ctx->galg, &gkey, NULL);
>         |                 ^~~
>   crypto/cipher-gnutls.c.inc:94:9: note: shadowed declaration is here
>      94 |     int err;
>         |         ^~~
>        ---
> 
>   crypto/cipher-gnutls.c.inc: In function ‘qcrypto_gnutls_cipher_decrypt’:
>   crypto/cipher-gnutls.c.inc:177:17: warning: declaration of ‘err’ shadows a previous local [-Wshadow=compatible-local]
>     177 |             int err = gnutls_cipher_init(&handle, ctx->galg, &gkey, NULL);
>         |                 ^~~
>   crypto/cipher-gnutls.c.inc:154:9: note: shadowed declaration is here
>     154 |     int err;
>         |         ^~~
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  crypto/cipher-gnutls.c.inc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

and if you want to include it in general pull request

Acked-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 16/22] crypto/cipher-gnutls.c: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

On 4/9/23 18:27, Daniel P. Berrangé wrote:
> On Mon, Sep 04, 2023 at 06:12:28PM +0200, Philippe Mathieu-Daudé wrote:
>> Fix:
>>
>>    In file included from crypto/cipher.c:140:
>>    crypto/cipher-gnutls.c.inc: In function ‘qcrypto_gnutls_cipher_encrypt’:
>>    crypto/cipher-gnutls.c.inc:116:17: warning: declaration of ‘err’ shadows a previous local [-Wshadow=compatible-local]
>>      116 |             int err = gnutls_cipher_init(&handle, ctx->galg, &gkey, NULL);
>>          |                 ^~~
>>    crypto/cipher-gnutls.c.inc:94:9: note: shadowed declaration is here
>>       94 |     int err;
>>          |         ^~~
>>         ---
>>
>>    crypto/cipher-gnutls.c.inc: In function ‘qcrypto_gnutls_cipher_decrypt’:
>>    crypto/cipher-gnutls.c.inc:177:17: warning: declaration of ‘err’ shadows a previous local [-Wshadow=compatible-local]
>>      177 |             int err = gnutls_cipher_init(&handle, ctx->galg, &gkey, NULL);
>>          |                 ^~~
>>    crypto/cipher-gnutls.c.inc:154:9: note: shadowed declaration is here
>>      154 |     int err;
>>          |         ^~~
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   crypto/cipher-gnutls.c.inc | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> and if you want to include it in general pull request

Sure,

> Acked-by: Daniel P. Berrangé <berrange@redhat.com>

Thank you!

[PATCH v2 17/22] util/vhost-user-server: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  util/vhost-user-server.c: In function ‘set_watch’:
  util/vhost-user-server.c:274:20: warning: declaration of ‘vu_fd_watch’ shadows a previous local [-Wshadow=compatible-local]
    274 |         VuFdWatch *vu_fd_watch = g_new0(VuFdWatch, 1);
        |                    ^~~~~~~~~~~
  util/vhost-user-server.c:271:16: note: shadowed declaration is here
    271 |     VuFdWatch *vu_fd_watch = find_vu_fd_watch(server, fd);
        |                ^~~~~~~~~~~

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 util/vhost-user-server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index cd17fb5326..5073f775ed 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -271,7 +271,7 @@ set_watch(VuDev *vu_dev, int fd, int vu_evt,
     VuFdWatch *vu_fd_watch = find_vu_fd_watch(server, fd);
 
     if (!vu_fd_watch) {
-        VuFdWatch *vu_fd_watch = g_new0(VuFdWatch, 1);
+        vu_fd_watch = g_new0(VuFdWatch, 1);
 
         QTAILQ_INSERT_TAIL(&server->vu_fd_watches, vu_fd_watch, next);
 
-- 
2.41.0

Re: [PATCH v2 17/22] util/vhost-user-server: Clean up local variable shadowing

[Michael S. Tsirkin]

On Mon, Sep 04, 2023 at 06:12:29PM +0200, Philippe Mathieu-Daudé wrote:
> Fix:
> 
>   util/vhost-user-server.c: In function ‘set_watch’:
>   util/vhost-user-server.c:274:20: warning: declaration of ‘vu_fd_watch’ shadows a previous local [-Wshadow=compatible-local]
>     274 |         VuFdWatch *vu_fd_watch = g_new0(VuFdWatch, 1);
>         |                    ^~~~~~~~~~~
>   util/vhost-user-server.c:271:16: note: shadowed declaration is here
>     271 |     VuFdWatch *vu_fd_watch = find_vu_fd_watch(server, fd);
>         |                ^~~~~~~~~~~
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>


Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

feel free to merge.

> ---
>  util/vhost-user-server.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
> index cd17fb5326..5073f775ed 100644
> --- a/util/vhost-user-server.c
> +++ b/util/vhost-user-server.c
> @@ -271,7 +271,7 @@ set_watch(VuDev *vu_dev, int fd, int vu_evt,
>      VuFdWatch *vu_fd_watch = find_vu_fd_watch(server, fd);
>  
>      if (!vu_fd_watch) {
> -        VuFdWatch *vu_fd_watch = g_new0(VuFdWatch, 1);
> +        vu_fd_watch = g_new0(VuFdWatch, 1);
>  
>          QTAILQ_INSERT_TAIL(&server->vu_fd_watches, vu_fd_watch, next);
>  
> -- 
> 2.41.0
> 
> 

[PATCH v2 18/22] semihosting/arm-compat: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  semihosting/arm-compat-semi.c: In function ‘do_common_semihosting’:
  semihosting/arm-compat-semi.c:379:13: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=local]
    379 |         int ret, err = 0;
        |             ^~~
  semihosting/arm-compat-semi.c:370:14: note: shadowed declaration is here
    370 |     uint32_t ret;
        |              ^~~
  semihosting/arm-compat-semi.c:682:27: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=local]
    682 |                 abi_ulong ret;
        |                           ^~~
  semihosting/arm-compat-semi.c:370:9: note: shadowed declaration is here
    370 |     int ret;
        |         ^~~

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 semihosting/arm-compat-semi.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index 564fe17f75..85852a15b8 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -367,7 +367,7 @@ void do_common_semihosting(CPUState *cs)
     target_ulong ul_ret;
     char * s;
     int nr;
-    uint32_t ret;
+    int ret;
     int64_t elapsed;
 
     nr = common_semi_arg(cs, 0) & 0xffffffffU;
@@ -376,7 +376,7 @@ void do_common_semihosting(CPUState *cs)
     switch (nr) {
     case TARGET_SYS_OPEN:
     {
-        int ret, err = 0;
+        int err = 0;
         int hostfd;
 
         GET_ARG(0);
@@ -679,14 +679,11 @@ void do_common_semihosting(CPUState *cs)
              * allocate it using sbrk.
              */
             if (!ts->heap_limit) {
-                abi_ulong ret;
-
                 ts->heap_base = do_brk(0);
                 limit = ts->heap_base + COMMON_SEMI_HEAP_SIZE;
                 /* Try a big heap, and reduce the size if that fails.  */
                 for (;;) {
-                    ret = do_brk(limit);
-                    if (ret >= limit) {
+                    if (do_brk(limit) >= limit) {
                         break;
                     }
                     limit = (ts->heap_base >> 1) + (limit >> 1);
-- 
2.41.0

Re: [PATCH v2 18/22] semihosting/arm-compat: Clean up local variable shadowing

[Peter Maydell]

On Mon, 4 Sept 2023 at 17:15, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Fix:
>
>   semihosting/arm-compat-semi.c: In function ‘do_common_semihosting’:
>   semihosting/arm-compat-semi.c:379:13: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=local]
>     379 |         int ret, err = 0;
>         |             ^~~
>   semihosting/arm-compat-semi.c:370:14: note: shadowed declaration is here
>     370 |     uint32_t ret;
>         |              ^~~
>   semihosting/arm-compat-semi.c:682:27: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=local]
>     682 |                 abi_ulong ret;
>         |                           ^~~
>   semihosting/arm-compat-semi.c:370:9: note: shadowed declaration is here
>     370 |     int ret;
>         |         ^~~
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  semihosting/arm-compat-semi.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

If I'm reading the code correctly, the top level 'ret' variable
is only used by the SYS_EXIT case currently. So rather than
changing the type of it I think it would be better to remove
it and have a variable at tighter scope for SYS_EXIT. I
think that's easier to read than this kind of "single variable
used for multiple purposes at different places within a
long function".

> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index 564fe17f75..85852a15b8 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -367,7 +367,7 @@ void do_common_semihosting(CPUState *cs)
>      target_ulong ul_ret;
>      char * s;
>      int nr;
> -    uint32_t ret;
> +    int ret;
>      int64_t elapsed;
>
>      nr = common_semi_arg(cs, 0) & 0xffffffffU;
> @@ -376,7 +376,7 @@ void do_common_semihosting(CPUState *cs)
>      switch (nr) {
>      case TARGET_SYS_OPEN:
>      {
> -        int ret, err = 0;
> +        int err = 0;
>          int hostfd;
>
>          GET_ARG(0);
> @@ -679,14 +679,11 @@ void do_common_semihosting(CPUState *cs)
>               * allocate it using sbrk.
>               */
>              if (!ts->heap_limit) {
> -                abi_ulong ret;
> -
>                  ts->heap_base = do_brk(0);
>                  limit = ts->heap_base + COMMON_SEMI_HEAP_SIZE;
>                  /* Try a big heap, and reduce the size if that fails.  */
>                  for (;;) {
> -                    ret = do_brk(limit);
> -                    if (ret >= limit) {
> +                    if (do_brk(limit) >= limit) {
>                          break;
>                      }
>                      limit = (ts->heap_base >> 1) + (limit >> 1);
> -

thanks
-- PMM

Re: [PATCH v2 18/22] semihosting/arm-compat: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Hi Peter, Alex,

On 8/9/23 14:36, Peter Maydell wrote:
> On Mon, 4 Sept 2023 at 17:15, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Fix:
>>
>>    semihosting/arm-compat-semi.c: In function ‘do_common_semihosting’:
>>    semihosting/arm-compat-semi.c:379:13: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=local]
>>      379 |         int ret, err = 0;
>>          |             ^~~
>>    semihosting/arm-compat-semi.c:370:14: note: shadowed declaration is here
>>      370 |     uint32_t ret;
>>          |              ^~~
>>    semihosting/arm-compat-semi.c:682:27: warning: declaration of ‘ret’ shadows a previous local [-Wshadow=local]
>>      682 |                 abi_ulong ret;
>>          |                           ^~~
>>    semihosting/arm-compat-semi.c:370:9: note: shadowed declaration is here
>>      370 |     int ret;
>>          |         ^~~
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   semihosting/arm-compat-semi.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> If I'm reading the code correctly, the top level 'ret' variable
> is only used by the SYS_EXIT case currently. So rather than
> changing the type of it I think it would be better to remove
> it and have a variable at tighter scope for SYS_EXIT. I
> think that's easier to read than this kind of "single variable
> used for multiple purposes at different places within a
> long function".

Yes you are right.

Now looking at it, we currently use a uint32_t type, but per
https://github.com/ARM-software/abi-aa/blob/main/semihosting/semihosting.rst#652entry-64-bit:

   In particular, if field 1 is ADP_Stopped_ApplicationExit then
   field 2 is an exit status code, as passed to the C standard library
   exit() function. A simulator receiving this request must notify a
   connected debugger, if present, and then exit with the specified
   status.

exit() is declared as:

LIBRARY
      Standard C Library (libc, -lc)

SYNOPSIS

      void
      exit(int status);

So it expects a signed status code, but we convert it to unsigned...
Alex, shouldn't we use a 'int' type here instead of 'uint32_t'?

>> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
>> index 564fe17f75..85852a15b8 100644
>> --- a/semihosting/arm-compat-semi.c
>> +++ b/semihosting/arm-compat-semi.c
>> @@ -367,7 +367,7 @@ void do_common_semihosting(CPUState *cs)
>>       target_ulong ul_ret;
>>       char * s;
>>       int nr;
>> -    uint32_t ret;
>> +    int ret;
>>       int64_t elapsed;

[PATCH v2 19/22] linux-user/strace: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  linux-user/strace.c: In function ‘print_sockaddr’:
  linux-user/strace.c:370:17: warning: declaration of ‘i’ shadows a previous local [-Wshadow=compatible-local]
    370 |             int i;
        |                 ^
  linux-user/strace.c:361:9: note: shadowed declaration is here
    361 |     int i;
        |         ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 linux-user/strace.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/linux-user/strace.c b/linux-user/strace.c
index e0ab8046ec..cf26e55264 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -367,7 +367,6 @@ print_sockaddr(abi_ulong addr, abi_long addrlen, int last)
         switch (sa_family) {
         case AF_UNIX: {
             struct target_sockaddr_un *un = (struct target_sockaddr_un *)sa;
-            int i;
             qemu_log("{sun_family=AF_UNIX,sun_path=\"");
             for (i = 0; i < addrlen -
                             offsetof(struct target_sockaddr_un, sun_path) &&
-- 
2.41.0

[PATCH v2 20/22] sysemu/device_tree: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  hw/mips/boston.c:472:5: error: declaration shadows a local variable [-Werror,-Wshadow]
    qemu_fdt_setprop_cells(fdt, name, "reg", reg_base, reg_size);
    ^
  include/sysemu/device_tree.h:129:13: note: expanded from macro 'qemu_fdt_setprop_cells'
        int i;
            ^
  hw/mips/boston.c:461:9: note: previous declaration is here
    int i;
        ^

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/sysemu/device_tree.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index ca5339beae..8eab395934 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -126,10 +126,8 @@ int qemu_fdt_add_path(void *fdt, const char *path);
 #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
     do {                                                                      \
         uint32_t qdt_tmp[] = { __VA_ARGS__ };                                 \
-        int i;                                                                \
-                                                                              \
-        for (i = 0; i < ARRAY_SIZE(qdt_tmp); i++) {                           \
-            qdt_tmp[i] = cpu_to_be32(qdt_tmp[i]);                             \
+        for (unsigned i_ = 0; i_ < ARRAY_SIZE(qdt_tmp); i_++) {               \
+            qdt_tmp[i_] = cpu_to_be32(qdt_tmp[i_]);                           \
         }                                                                     \
         qemu_fdt_setprop(fdt, node_path, property, qdt_tmp,                   \
                          sizeof(qdt_tmp));                                    \
-- 
2.41.0

Re: [PATCH v2 20/22] sysemu/device_tree: Clean up local variable shadowing

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Fix:
>
>   hw/mips/boston.c:472:5: error: declaration shadows a local variable [-Werror,-Wshadow]
>     qemu_fdt_setprop_cells(fdt, name, "reg", reg_base, reg_size);
>     ^
>   include/sysemu/device_tree.h:129:13: note: expanded from macro 'qemu_fdt_setprop_cells'
>         int i;
>             ^
>   hw/mips/boston.c:461:9: note: previous declaration is here
>     int i;
>         ^
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/sysemu/device_tree.h | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index ca5339beae..8eab395934 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -126,10 +126,8 @@ int qemu_fdt_add_path(void *fdt, const char *path);
>  #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
>      do {                                                                      \
>          uint32_t qdt_tmp[] = { __VA_ARGS__ };                                 \
> -        int i;                                                                \
> -                                                                              \
> -        for (i = 0; i < ARRAY_SIZE(qdt_tmp); i++) {                           \
> -            qdt_tmp[i] = cpu_to_be32(qdt_tmp[i]);                             \
> +        for (unsigned i_ = 0; i_ < ARRAY_SIZE(qdt_tmp); i_++) {               \
> +            qdt_tmp[i_] = cpu_to_be32(qdt_tmp[i_]);                           \
>          }                                                                     \
>          qemu_fdt_setprop(fdt, node_path, property, qdt_tmp,                   \
>                           sizeof(qdt_tmp));                                    \

You don't just rename the variable, but also adjust its type.  When
we're dealing with at a huge changeset like the tree-wide -Wshadow=local
cleanup, I prefer to keep changes minimal to ease review as much as
possible.  But it's sunk cost now, so

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH v2 21/22] softmmu/memory: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  softmmu/memory.c: In function ‘mtree_print_mr’:
  softmmu/memory.c:3236:27: warning: declaration of ‘ml’ shadows a previous local [-Wshadow=compatible-local]
   3236 |         MemoryRegionList *ml;
        |                           ^~
  softmmu/memory.c:3213:32: note: shadowed declaration is here
   3213 |     MemoryRegionList *new_ml, *ml, *next_ml;
        |                                ^~

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 softmmu/memory.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/softmmu/memory.c b/softmmu/memory.c
index 7d9494ce70..bf48350438 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -3233,7 +3233,6 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
     }
 
     if (mr->alias) {
-        MemoryRegionList *ml;
         bool found = false;
 
         /* check if the alias is already in the queue */
-- 
2.41.0

Re: [PATCH v2 21/22] softmmu/memory: Clean up local variable shadowing

[Peter Xu]

On Mon, Sep 04, 2023 at 06:12:33PM +0200, Philippe Mathieu-Daudé wrote:
> Fix:
> 
>   softmmu/memory.c: In function ‘mtree_print_mr’:
>   softmmu/memory.c:3236:27: warning: declaration of ‘ml’ shadows a previous local [-Wshadow=compatible-local]
>    3236 |         MemoryRegionList *ml;
>         |                           ^~
>   softmmu/memory.c:3213:32: note: shadowed declaration is here
>    3213 |     MemoryRegionList *new_ml, *ml, *next_ml;
>         |                                ^~
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[RFC PATCH v2 22/22] softmmu/physmem: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Fix:

  softmmu/physmem.c: In function ‘cpu_physical_memory_snapshot_and_clear_dirty’:
  softmmu/physmem.c:916:27: warning: declaration of ‘offset’ shadows a parameter [-Wshadow=compatible-local]
    916 |             unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
        |                           ^~~~~~
  softmmu/physmem.c:892:31: note: shadowed declaration is here
    892 |     (MemoryRegion *mr, hwaddr offset, hwaddr length, unsigned client)
        |                        ~~~~~~~^~~~~~

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC: Please double-check how 'offset' is used few lines later.
---
 softmmu/physmem.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 18277ddd67..db5b628a60 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -913,16 +913,16 @@ DirtyBitmapSnapshot *cpu_physical_memory_snapshot_and_clear_dirty
 
         while (page < end) {
             unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
-            unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
+            unsigned long ofs = page % DIRTY_MEMORY_BLOCK_SIZE;
             unsigned long num = MIN(end - page,
-                                    DIRTY_MEMORY_BLOCK_SIZE - offset);
+                                    DIRTY_MEMORY_BLOCK_SIZE - ofs);
 
-            assert(QEMU_IS_ALIGNED(offset, (1 << BITS_PER_LEVEL)));
+            assert(QEMU_IS_ALIGNED(ofs, (1 << BITS_PER_LEVEL)));
             assert(QEMU_IS_ALIGNED(num,    (1 << BITS_PER_LEVEL)));
-            offset >>= BITS_PER_LEVEL;
+            ofs >>= BITS_PER_LEVEL;
 
             bitmap_copy_and_clear_atomic(snap->dirty + dest,
-                                         blocks->blocks[idx] + offset,
+                                         blocks->blocks[idx] + ofs,
                                          num);
             page += num;
             dest += num >> BITS_PER_LEVEL;
-- 
2.41.0

Re: [RFC PATCH v2 22/22] softmmu/physmem: Clean up local variable shadowing

[Daniel P. Berrangé]

On Mon, Sep 04, 2023 at 06:12:34PM +0200, Philippe Mathieu-Daudé wrote:
> Fix:
> 
>   softmmu/physmem.c: In function ‘cpu_physical_memory_snapshot_and_clear_dirty’:
>   softmmu/physmem.c:916:27: warning: declaration of ‘offset’ shadows a parameter [-Wshadow=compatible-local]
>     916 |             unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
>         |                           ^~~~~~
>   softmmu/physmem.c:892:31: note: shadowed declaration is here
>     892 |     (MemoryRegion *mr, hwaddr offset, hwaddr length, unsigned client)
>         |                        ~~~~~~~^~~~~~
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> RFC: Please double-check how 'offset' is used few lines later.

I don't see an issue - those lines are in an outer scope, so won't
be accessing the 'offset' you've changed, they'll be the parameter
instead. If you want to sanity check though, presumably the asm
dissassembly for this method should be the same before/after this
change

> ---
>  softmmu/physmem.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 18277ddd67..db5b628a60 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -913,16 +913,16 @@ DirtyBitmapSnapshot *cpu_physical_memory_snapshot_and_clear_dirty
>  
>          while (page < end) {
>              unsigned long idx = page / DIRTY_MEMORY_BLOCK_SIZE;
> -            unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
> +            unsigned long ofs = page % DIRTY_MEMORY_BLOCK_SIZE;
>              unsigned long num = MIN(end - page,
> -                                    DIRTY_MEMORY_BLOCK_SIZE - offset);
> +                                    DIRTY_MEMORY_BLOCK_SIZE - ofs);
>  
> -            assert(QEMU_IS_ALIGNED(offset, (1 << BITS_PER_LEVEL)));
> +            assert(QEMU_IS_ALIGNED(ofs, (1 << BITS_PER_LEVEL)));
>              assert(QEMU_IS_ALIGNED(num,    (1 << BITS_PER_LEVEL)));
> -            offset >>= BITS_PER_LEVEL;
> +            ofs >>= BITS_PER_LEVEL;
>  
>              bitmap_copy_and_clear_atomic(snap->dirty + dest,
> -                                         blocks->blocks[idx] + offset,
> +                                         blocks->blocks[idx] + ofs,
>                                           num);
>              page += num;
>              dest += num >> BITS_PER_LEVEL;
> -- 
> 2.41.0
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v2 22/22] softmmu/physmem: Clean up local variable shadowing

[Peter Xu]

On Mon, Sep 04, 2023 at 05:31:30PM +0100, Daniel P. Berrangé wrote:
> On Mon, Sep 04, 2023 at 06:12:34PM +0200, Philippe Mathieu-Daudé wrote:
> > Fix:
> > 
> >   softmmu/physmem.c: In function ‘cpu_physical_memory_snapshot_and_clear_dirty’:
> >   softmmu/physmem.c:916:27: warning: declaration of ‘offset’ shadows a parameter [-Wshadow=compatible-local]
> >     916 |             unsigned long offset = page % DIRTY_MEMORY_BLOCK_SIZE;
> >         |                           ^~~~~~
> >   softmmu/physmem.c:892:31: note: shadowed declaration is here
> >     892 |     (MemoryRegion *mr, hwaddr offset, hwaddr length, unsigned client)
> >         |                        ~~~~~~~^~~~~~
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > RFC: Please double-check how 'offset' is used few lines later.
> 
> I don't see an issue - those lines are in an outer scope, so won't
> be accessing the 'offset' you've changed, they'll be the parameter
> instead. If you want to sanity check though, presumably the asm
> dissassembly for this method should be the same before/after this
> change

(and if it didn't do so then it's a bug..)

> 
> > ---
> >  softmmu/physmem.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH v2 00/22] (few more) Steps towards enabling -Wshadow

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Since v1:
> - Addressed review comments
> - Added R-b tags
> - More patches
>
> For rational see Markus cover on
> https://lore.kernel.org/qemu-devel/20230831132546.3525721-1-armbru@redhat.com/
>
> This series contains few more, my take.
>
> Based-on: <20230831132546.3525721-1-armbru@redhat.com>

Queued except for:

PATCH 11: I asked for John Snow's opinion on a matter of taste.
PATCH 18: Review comment from Peter Maydell is pending.

Thanks!

Re: [PATCH v2 00/22] (few more) Steps towards enabling -Wshadow

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>
>> Since v1:
>> - Addressed review comments
>> - Added R-b tags
>> - More patches
>>
>> For rational see Markus cover on
>> https://lore.kernel.org/qemu-devel/20230831132546.3525721-1-armbru@redhat.com/
>>
>> This series contains few more, my take.
>>
>> Based-on: <20230831132546.3525721-1-armbru@redhat.com>

Correction...

> Queued except for:

  PATCH 10: Clashes with a patch from Cédric, picking Cédric's

> PATCH 11: I asked for John Snow's opinion on a matter of taste.
> PATCH 18: Review comment from Peter Maydell is pending.
>
> Thanks!