[PATCH v2 0/3] hexagon: GETPC() and shadowing fixes

[PATCH v2 0/3] hexagon: GETPC() and shadowing fixes

[Brian Cain]

In v2: reworked with suggestions from Philippe and added a new patch
to cover -Wshadow=global.

Brian Cain (2):
  target/hexagon: fix some occurrences of -Wshadow=local
  target/hexagon: avoid shadowing globals

Matheus Tavares Bernardino (1):
  target/hexagon: move GETPC() calls to top level helpers

 target/hexagon/genptr.c                 | 56 ++++++++---------
 target/hexagon/genptr.h                 | 18 +++---
 target/hexagon/imported/alu.idef        |  6 +-
 target/hexagon/macros.h                 | 19 +++---
 target/hexagon/mmvec/macros.h           |  6 +-
 target/hexagon/mmvec/system_ext_mmvec.c |  4 +-
 target/hexagon/mmvec/system_ext_mmvec.h |  2 +-
 target/hexagon/op_helper.c              | 84 ++++++++++---------------
 target/hexagon/op_helper.h              |  9 ---
 target/hexagon/translate.c              |  9 ++-
 10 files changed, 91 insertions(+), 122 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] target/hexagon: move GETPC() calls to top level helpers

[Brian Cain]

From: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>

As docs/devel/loads-stores.rst states:

  ``GETPC()`` should be used with great care: calling
  it in other functions that are *not* the top level
  ``HELPER(foo)`` will cause unexpected behavior. Instead, the
  value of ``GETPC()`` should be read from the helper and passed
  if needed to the functions that the helper calls.

Let's fix the GETPC() usage in Hexagon, making sure it's always called
from top level helpers and passed down to the places where it's
needed. There are a few snippets where that is not currently the case:

- probe_store(), which is only called from two helpers, so it's easy to
  move GETPC() up.

- mem_load*() functions, which are also called directly from helpers,
  but through the MEM_LOAD*() set of macros. Note that this are only
  used when compiling with --disable-hexagon-idef-parser.

  In this case, we also take this opportunity to simplify the code,
  unifying the mem_load*() functions.

- HELPER(probe_hvx_stores), when called from another helper, ends up
  using its own GETPC() expansion instead of the top level caller.

Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com>
Message-Id: <2c74c3696946edba7cc5b2942cf296a5af532052.1689070412.git.quic_mathbern@quicinc.com>-ne
Reviewed-by: Brian Cain <bcain@quicinc.com>
Signed-off-by: Brian Cain <bcain@quicinc.com>
---
 target/hexagon/macros.h    | 19 +++++-----
 target/hexagon/op_helper.c | 75 +++++++++++++++-----------------------
 target/hexagon/op_helper.h |  9 -----
 3 files changed, 38 insertions(+), 65 deletions(-)

diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 5451b061ee..dafa0df6ed 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -173,15 +173,6 @@
 #define MEM_STORE8(VA, DATA, SLOT) \
     MEM_STORE8_FUNC(DATA)(cpu_env, VA, DATA, SLOT)
 #else
-#define MEM_LOAD1s(VA) ((int8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD1u(VA) ((uint8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2s(VA) ((int16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2u(VA) ((uint16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4s(VA) ((int32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4u(VA) ((uint32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8s(VA) ((int64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8u(VA) ((uint64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-
 #define MEM_STORE1(VA, DATA, SLOT) log_store32(env, VA, DATA, 1, SLOT)
 #define MEM_STORE2(VA, DATA, SLOT) log_store32(env, VA, DATA, 2, SLOT)
 #define MEM_STORE4(VA, DATA, SLOT) log_store32(env, VA, DATA, 4, SLOT)
@@ -530,8 +521,16 @@ static inline TCGv gen_read_ireg(TCGv result, TCGv val, int shift)
 #ifdef QEMU_GENERATE
 #define fLOAD(NUM, SIZE, SIGN, EA, DST) MEM_LOAD##SIZE##SIGN(DST, EA)
 #else
+#define MEM_LOAD1 cpu_ldub_data_ra
+#define MEM_LOAD2 cpu_lduw_data_ra
+#define MEM_LOAD4 cpu_ldl_data_ra
+#define MEM_LOAD8 cpu_ldq_data_ra
+
 #define fLOAD(NUM, SIZE, SIGN, EA, DST) \
-    DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA)
+    do { \
+        check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \
+        DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE(env, EA, GETPC()); \
+    } while (0)
 #endif
 
 #define fMEMOP(NUM, SIZE, SIGN, EA, FNTYPE, VALUE)
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 12967ac21e..8ca3976a65 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -95,9 +95,8 @@ void HELPER(debug_check_store_width)(CPUHexagonState *env, int slot, int check)
     }
 }
 
-void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
+static void commit_store(CPUHexagonState *env, int slot_num, uintptr_t ra)
 {
-    uintptr_t ra = GETPC();
     uint8_t width = env->mem_log_stores[slot_num].width;
     target_ulong va = env->mem_log_stores[slot_num].va;
 
@@ -119,6 +118,12 @@ void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
     }
 }
 
+void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
+{
+    uintptr_t ra = GETPC();
+    commit_store(env, slot_num, ra);
+}
+
 void HELPER(gather_store)(CPUHexagonState *env, uint32_t addr, int slot)
 {
     mem_gather_store(env, addr, slot);
@@ -467,13 +472,12 @@ int32_t HELPER(cabacdecbin_pred)(int64_t RssV, int64_t RttV)
 }
 
 static void probe_store(CPUHexagonState *env, int slot, int mmu_idx,
-                        bool is_predicated)
+                        bool is_predicated, uintptr_t retaddr)
 {
     if (!is_predicated || !(env->slot_cancelled & (1 << slot))) {
         size1u_t width = env->mem_log_stores[slot].width;
         target_ulong va = env->mem_log_stores[slot].va;
-        uintptr_t ra = GETPC();
-        probe_write(env, va, width, mmu_idx, ra);
+        probe_write(env, va, width, mmu_idx, retaddr);
     }
 }
 
@@ -494,12 +498,13 @@ void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState *env, int args)
     int mmu_idx = FIELD_EX32(args, PROBE_PKT_SCALAR_STORE_S0, MMU_IDX);
     bool is_predicated =
         FIELD_EX32(args, PROBE_PKT_SCALAR_STORE_S0, IS_PREDICATED);
-    probe_store(env, 0, mmu_idx, is_predicated);
+    uintptr_t ra = GETPC();
+    probe_store(env, 0, mmu_idx, is_predicated, ra);
 }
 
-void HELPER(probe_hvx_stores)(CPUHexagonState *env, int mmu_idx)
+static void probe_hvx_stores(CPUHexagonState *env, int mmu_idx,
+                                    uintptr_t retaddr)
 {
-    uintptr_t retaddr = GETPC();
     int i;
 
     /* Normal (possibly masked) vector store */
@@ -538,6 +543,12 @@ void HELPER(probe_hvx_stores)(CPUHexagonState *env, int mmu_idx)
     }
 }
 
+void HELPER(probe_hvx_stores)(CPUHexagonState *env, int mmu_idx)
+{
+    uintptr_t retaddr = GETPC();
+    probe_hvx_stores(env, mmu_idx, retaddr);
+}
+
 void HELPER(probe_pkt_scalar_hvx_stores)(CPUHexagonState *env, int mask)
 {
     bool has_st0 = FIELD_EX32(mask, PROBE_PKT_SCALAR_HVX_STORES, HAS_ST0);
@@ -547,18 +558,20 @@ void HELPER(probe_pkt_scalar_hvx_stores)(CPUHexagonState *env, int mask)
     bool s0_is_pred = FIELD_EX32(mask, PROBE_PKT_SCALAR_HVX_STORES, S0_IS_PRED);
     bool s1_is_pred = FIELD_EX32(mask, PROBE_PKT_SCALAR_HVX_STORES, S1_IS_PRED);
     int mmu_idx = FIELD_EX32(mask, PROBE_PKT_SCALAR_HVX_STORES, MMU_IDX);
+    uintptr_t ra = GETPC();
 
     if (has_st0) {
-        probe_store(env, 0, mmu_idx, s0_is_pred);
+        probe_store(env, 0, mmu_idx, s0_is_pred, ra);
     }
     if (has_st1) {
-        probe_store(env, 1, mmu_idx, s1_is_pred);
+        probe_store(env, 1, mmu_idx, s1_is_pred, ra);
     }
     if (has_hvx_stores) {
-        HELPER(probe_hvx_stores)(env, mmu_idx);
+        probe_hvx_stores(env, mmu_idx, ra);
     }
 }
 
+#ifndef CONFIG_HEXAGON_IDEF_PARSER
 /*
  * mem_noshuf
  * Section 5.5 of the Hexagon V67 Programmer's Reference Manual
@@ -567,46 +580,16 @@ void HELPER(probe_pkt_scalar_hvx_stores)(CPUHexagonState *env, int mask)
  * wasn't cancelled), we have to do the store first.
  */
 static void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
-                         uint32_t slot, target_ulong vaddr, int size)
+                         uint32_t slot, target_ulong vaddr, int size,
+                         uintptr_t ra)
 {
     if (slot == 0 && pkt_has_store_s1 &&
         ((env->slot_cancelled & (1 << 1)) == 0)) {
-        HELPER(probe_noshuf_load)(env, vaddr, size, MMU_USER_IDX);
-        HELPER(commit_store)(env, 1);
+        probe_read(env, vaddr, size, MMU_USER_IDX, ra);
+        commit_store(env, 1, ra);
     }
 }
-
-uint8_t mem_load1(CPUHexagonState *env, bool pkt_has_store_s1,
-                  uint32_t slot, target_ulong vaddr)
-{
-    uintptr_t ra = GETPC();
-    check_noshuf(env, pkt_has_store_s1, slot, vaddr, 1);
-    return cpu_ldub_data_ra(env, vaddr, ra);
-}
-
-uint16_t mem_load2(CPUHexagonState *env, bool pkt_has_store_s1,
-                   uint32_t slot, target_ulong vaddr)
-{
-    uintptr_t ra = GETPC();
-    check_noshuf(env, pkt_has_store_s1, slot, vaddr, 2);
-    return cpu_lduw_data_ra(env, vaddr, ra);
-}
-
-uint32_t mem_load4(CPUHexagonState *env, bool pkt_has_store_s1,
-                   uint32_t slot, target_ulong vaddr)
-{
-    uintptr_t ra = GETPC();
-    check_noshuf(env, pkt_has_store_s1, slot, vaddr, 4);
-    return cpu_ldl_data_ra(env, vaddr, ra);
-}
-
-uint64_t mem_load8(CPUHexagonState *env, bool pkt_has_store_s1,
-                   uint32_t slot, target_ulong vaddr)
-{
-    uintptr_t ra = GETPC();
-    check_noshuf(env, pkt_has_store_s1, slot, vaddr, 8);
-    return cpu_ldq_data_ra(env, vaddr, ra);
-}
+#endif
 
 /* Floating point */
 float64 HELPER(conv_sf2df)(CPUHexagonState *env, float32 RsV)
diff --git a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h
index 8f3764d15e..66119cf3d4 100644
--- a/target/hexagon/op_helper.h
+++ b/target/hexagon/op_helper.h
@@ -19,15 +19,6 @@
 #define HEXAGON_OP_HELPER_H
 
 /* Misc functions */
-uint8_t mem_load1(CPUHexagonState *env, bool pkt_has_store_s1,
-                  uint32_t slot, target_ulong vaddr);
-uint16_t mem_load2(CPUHexagonState *env, bool pkt_has_store_s1,
-                   uint32_t slot, target_ulong vaddr);
-uint32_t mem_load4(CPUHexagonState *env, bool pkt_has_store_s1,
-                   uint32_t slot, target_ulong vaddr);
-uint64_t mem_load8(CPUHexagonState *env, bool pkt_has_store_s1,
-                   uint32_t slot, target_ulong vaddr);
-
 void log_store64(CPUHexagonState *env, target_ulong addr,
                  int64_t val, int width, int slot);
 void log_store32(CPUHexagonState *env, target_ulong addr,
-- 
2.25.1

[PATCH v2 2/3] target/hexagon: fix some occurrences of -Wshadow=local

[Brian Cain]

Of the changes in this commit, the changes in `HELPER(commit_hvx_stores)()`
are less obvious.  They are required because of some macro invocations like
SCATTER_OP_WRITE_TO_MEM().

e.g.:

    In file included from ../target/hexagon/op_helper.c:31:
    ../target/hexagon/mmvec/macros.h:205:18: error: declaration of ‘i’ shadows a previous local [-Werror=shadow=compatible-local]
      205 |         for (int i = 0; i < sizeof(MMVector); i += sizeof(TYPE)) { \
          |                  ^
    ../target/hexagon/op_helper.c:157:17: note: in expansion of macro ‘SCATTER_OP_WRITE_TO_MEM’
      157 |                 SCATTER_OP_WRITE_TO_MEM(uint16_t);
          |                 ^~~~~~~~~~~~~~~~~~~~~~~
    ../target/hexagon/op_helper.c:135:9: note: shadowed declaration is here
      135 |     int i;
          |         ^
    In file included from ../target/hexagon/op_helper.c:31:
    ../target/hexagon/mmvec/macros.h:204:19: error: declaration of ‘ra’ shadows a previous local [-Werror=shadow=compatible-local]
      204 |         uintptr_t ra = GETPC(); \
          |                   ^~
    ../target/hexagon/op_helper.c:160:17: note: in expansion of macro ‘SCATTER_OP_WRITE_TO_MEM’
      160 |                 SCATTER_OP_WRITE_TO_MEM(uint32_t);
          |                 ^~~~~~~~~~~~~~~~~~~~~~~
    ../target/hexagon/op_helper.c:134:15: note: shadowed declaration is here
      134 |     uintptr_t ra = GETPC();
          |               ^~

Reviewed-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
Signed-off-by: Brian Cain <bcain@quicinc.com>
---
 target/hexagon/imported/alu.idef | 6 +++---
 target/hexagon/mmvec/macros.h    | 6 +++---
 target/hexagon/op_helper.c       | 9 +++------
 target/hexagon/translate.c       | 9 ++++-----
 4 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/target/hexagon/imported/alu.idef b/target/hexagon/imported/alu.idef
index 12d2aac5d4..b855676989 100644
--- a/target/hexagon/imported/alu.idef
+++ b/target/hexagon/imported/alu.idef
@@ -1142,9 +1142,9 @@ Q6INSN(A4_cround_rr,"Rd32=cround(Rs32,Rt32)",ATTRIBS(),"Convergent Round", {RdV
             tmp128 = fSHIFTR128(tmp128, SHIFT);\
             DST =  fCAST16S_8S(tmp128);\
         } else {\
-            size16s_t rndbit_128 =  fCAST8S_16S((1LL << (SHIFT - 1))); \
-            size16s_t src_128 =  fCAST8S_16S(SRC); \
-            size16s_t tmp128 = fADD128(src_128, rndbit_128);\
+            rndbit_128 =  fCAST8S_16S((1LL << (SHIFT - 1))); \
+            src_128 =  fCAST8S_16S(SRC); \
+            tmp128 = fADD128(src_128, rndbit_128);\
             tmp128 = fSHIFTR128(tmp128, SHIFT);\
             DST =  fCAST16S_8S(tmp128);\
         }
diff --git a/target/hexagon/mmvec/macros.h b/target/hexagon/mmvec/macros.h
index a655634fd1..728a63d35f 100644
--- a/target/hexagon/mmvec/macros.h
+++ b/target/hexagon/mmvec/macros.h
@@ -201,14 +201,14 @@
     } while (0)
 #define SCATTER_OP_WRITE_TO_MEM(TYPE) \
     do { \
-        uintptr_t ra = GETPC(); \
+        uintptr_t ra_ = GETPC(); \
         for (int i = 0; i < sizeof(MMVector); i += sizeof(TYPE)) { \
             if (test_bit(i, env->vtcm_log.mask)) { \
                 TYPE dst = 0; \
                 TYPE inc = 0; \
                 for (int j = 0; j < sizeof(TYPE); j++) { \
                     uint8_t val; \
-                    val = cpu_ldub_data_ra(env, env->vtcm_log.va[i + j], ra); \
+                    val = cpu_ldub_data_ra(env, env->vtcm_log.va[i + j], ra_); \
                     dst |= val << (8 * j); \
                     inc |= env->vtcm_log.data.ub[j + i] << (8 * j); \
                     clear_bit(j + i, env->vtcm_log.mask); \
@@ -217,7 +217,7 @@
                 dst += inc; \
                 for (int j = 0; j < sizeof(TYPE); j++) { \
                     cpu_stb_data_ra(env, env->vtcm_log.va[i + j], \
-                                    (dst >> (8 * j)) & 0xFF, ra); \
+                                    (dst >> (8 * j)) & 0xFF, ra_); \
                 } \
             } \
         } \
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 8ca3976a65..da10ac5847 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -132,10 +132,9 @@ void HELPER(gather_store)(CPUHexagonState *env, uint32_t addr, int slot)
 void HELPER(commit_hvx_stores)(CPUHexagonState *env)
 {
     uintptr_t ra = GETPC();
-    int i;
 
     /* Normal (possibly masked) vector store */
-    for (i = 0; i < VSTORES_MAX; i++) {
+    for (int i = 0; i < VSTORES_MAX; i++) {
         if (env->vstore_pending[i]) {
             env->vstore_pending[i] = 0;
             target_ulong va = env->vstore[i].va;
@@ -162,7 +161,7 @@ void HELPER(commit_hvx_stores)(CPUHexagonState *env)
                 g_assert_not_reached();
             }
         } else {
-            for (i = 0; i < sizeof(MMVector); i++) {
+            for (int i = 0; i < sizeof(MMVector); i++) {
                 if (test_bit(i, env->vtcm_log.mask)) {
                     cpu_stb_data_ra(env, env->vtcm_log.va[i],
                                     env->vtcm_log.data.ub[i], ra);
@@ -505,10 +504,8 @@ void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState *env, int args)
 static void probe_hvx_stores(CPUHexagonState *env, int mmu_idx,
                                     uintptr_t retaddr)
 {
-    int i;
-
     /* Normal (possibly masked) vector store */
-    for (i = 0; i < VSTORES_MAX; i++) {
+    for (int i = 0; i < VSTORES_MAX; i++) {
         if (env->vstore_pending[i]) {
             target_ulong va = env->vstore[i].va;
             int size = env->vstore[i].size;
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index c00254e4d5..ab708bf823 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -484,7 +484,6 @@ static void gen_start_packet(DisasContext *ctx)
 {
     Packet *pkt = ctx->pkt;
     target_ulong next_PC = ctx->base.pc_next + pkt->encod_pkt_size_in_bytes;
-    int i;
 
     /* Clear out the disassembly context */
     ctx->next_PC = next_PC;
@@ -506,15 +505,15 @@ static void gen_start_packet(DisasContext *ctx)
     bitmap_zero(ctx->vregs_read, NUM_VREGS);
     bitmap_zero(ctx->qregs_read, NUM_QREGS);
     ctx->qreg_log_idx = 0;
-    for (i = 0; i < STORES_MAX; i++) {
+    for (int i = 0; i < STORES_MAX; i++) {
         ctx->store_width[i] = 0;
     }
     ctx->s1_store_processed = false;
     ctx->pre_commit = true;
-    for (i = 0; i < TOTAL_PER_THREAD_REGS; i++) {
+    for (int i = 0; i < TOTAL_PER_THREAD_REGS; i++) {
         ctx->new_value[i] = NULL;
     }
-    for (i = 0; i < NUM_PREGS; i++) {
+    for (int i = 0; i < NUM_PREGS; i++) {
         ctx->new_pred_value[i] = NULL;
     }
 
@@ -1228,7 +1227,7 @@ void hexagon_translate_init(void)
             offsetof(CPUHexagonState, mem_log_stores[i].data64),
             store_val64_names[i]);
     }
-    for (int i = 0; i < VSTORES_MAX; i++) {
+    for (i = 0; i < VSTORES_MAX; i++) {
         snprintf(vstore_addr_names[i], NAME_LEN, "vstore_addr_%d", i);
         hex_vstore_addr[i] = tcg_global_mem_new(cpu_env,
             offsetof(CPUHexagonState, vstore[i].va),
-- 
2.25.1

[PATCH v2 3/3] target/hexagon: avoid shadowing globals

[Brian Cain]

The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the
identifiers to avoid shadowing the type name.

The global `cpu_env` is shadowed by local `cpu_env` arguments, so we
rename the function arguments to avoid shadowing the global.

Signed-off-by: Brian Cain <bcain@quicinc.com>
---
 target/hexagon/genptr.c                 | 56 ++++++++++++-------------
 target/hexagon/genptr.h                 | 18 ++++----
 target/hexagon/mmvec/system_ext_mmvec.c |  4 +-
 target/hexagon/mmvec/system_ext_mmvec.h |  2 +-
 target/hexagon/op_helper.c              |  4 +-
 5 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
index 217bc7bb5a..11377ac92b 100644
--- a/target/hexagon/genptr.c
+++ b/target/hexagon/genptr.c
@@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result, TCGv src)
     tcg_gen_deposit_i64(result, result, src64, N * 8, 8);
 }
 
-static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int mem_index)
+static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int mem_index)
 {
-    tcg_gen_qemu_ld_tl(dest, vaddr, mem_index, MO_TEUL);
-    tcg_gen_mov_tl(hex_llsc_addr, vaddr);
+    tcg_gen_qemu_ld_tl(dest, v_addr, mem_index, MO_TEUL);
+    tcg_gen_mov_tl(hex_llsc_addr, v_addr);
     tcg_gen_mov_tl(hex_llsc_val, dest);
 }
 
-static inline void gen_load_locked8u(TCGv_i64 dest, TCGv vaddr, int mem_index)
+static inline void gen_load_locked8u(TCGv_i64 dest, TCGv v_addr, int mem_index)
 {
-    tcg_gen_qemu_ld_i64(dest, vaddr, mem_index, MO_TEUQ);
-    tcg_gen_mov_tl(hex_llsc_addr, vaddr);
+    tcg_gen_qemu_ld_i64(dest, v_addr, mem_index, MO_TEUQ);
+    tcg_gen_mov_tl(hex_llsc_addr, v_addr);
     tcg_gen_mov_i64(hex_llsc_val_i64, dest);
 }
 
 static inline void gen_store_conditional4(DisasContext *ctx,
-                                          TCGv pred, TCGv vaddr, TCGv src)
+                                          TCGv pred, TCGv v_addr, TCGv src)
 {
     TCGLabel *fail = gen_new_label();
     TCGLabel *done = gen_new_label();
     TCGv one, zero, tmp;
 
-    tcg_gen_brcond_tl(TCG_COND_NE, vaddr, hex_llsc_addr, fail);
+    tcg_gen_brcond_tl(TCG_COND_NE, v_addr, hex_llsc_addr, fail);
 
     one = tcg_constant_tl(0xff);
     zero = tcg_constant_tl(0);
@@ -374,13 +374,13 @@ static inline void gen_store_conditional4(DisasContext *ctx,
 }
 
 static inline void gen_store_conditional8(DisasContext *ctx,
-                                          TCGv pred, TCGv vaddr, TCGv_i64 src)
+                                          TCGv pred, TCGv v_addr, TCGv_i64 src)
 {
     TCGLabel *fail = gen_new_label();
     TCGLabel *done = gen_new_label();
     TCGv_i64 one, zero, tmp;
 
-    tcg_gen_brcond_tl(TCG_COND_NE, vaddr, hex_llsc_addr, fail);
+    tcg_gen_brcond_tl(TCG_COND_NE, v_addr, hex_llsc_addr, fail);
 
     one = tcg_constant_i64(0xff);
     zero = tcg_constant_i64(0);
@@ -407,57 +407,57 @@ static TCGv gen_slotval(DisasContext *ctx)
 }
 #endif
 
-void gen_store32(TCGv vaddr, TCGv src, int width, uint32_t slot)
+void gen_store32(TCGv v_addr, TCGv src, int width, uint32_t slot)
 {
-    tcg_gen_mov_tl(hex_store_addr[slot], vaddr);
+    tcg_gen_mov_tl(hex_store_addr[slot], v_addr);
     tcg_gen_movi_tl(hex_store_width[slot], width);
     tcg_gen_mov_tl(hex_store_val32[slot], src);
 }
 
-void gen_store1(TCGv_env cpu_env, TCGv vaddr, TCGv src, uint32_t slot)
+void gen_store1(TCGv_env cpu_env_, TCGv v_addr, TCGv src, uint32_t slot)
 {
-    gen_store32(vaddr, src, 1, slot);
+    gen_store32(v_addr, src, 1, slot);
 }
 
-void gen_store1i(TCGv_env cpu_env, TCGv vaddr, int32_t src, uint32_t slot)
+void gen_store1i(TCGv_env cpu_env_, TCGv v_addr, int32_t src, uint32_t slot)
 {
     TCGv tmp = tcg_constant_tl(src);
-    gen_store1(cpu_env, vaddr, tmp, slot);
+    gen_store1(cpu_env_, v_addr, tmp, slot);
 }
 
-void gen_store2(TCGv_env cpu_env, TCGv vaddr, TCGv src, uint32_t slot)
+void gen_store2(TCGv_env cpu_env_, TCGv v_addr, TCGv src, uint32_t slot)
 {
-    gen_store32(vaddr, src, 2, slot);
+    gen_store32(v_addr, src, 2, slot);
 }
 
-void gen_store2i(TCGv_env cpu_env, TCGv vaddr, int32_t src, uint32_t slot)
+void gen_store2i(TCGv_env cpu_env_, TCGv v_addr, int32_t src, uint32_t slot)
 {
     TCGv tmp = tcg_constant_tl(src);
-    gen_store2(cpu_env, vaddr, tmp, slot);
+    gen_store2(cpu_env_, v_addr, tmp, slot);
 }
 
-void gen_store4(TCGv_env cpu_env, TCGv vaddr, TCGv src, uint32_t slot)
+void gen_store4(TCGv_env cpu_env_, TCGv v_addr, TCGv src, uint32_t slot)
 {
-    gen_store32(vaddr, src, 4, slot);
+    gen_store32(v_addr, src, 4, slot);
 }
 
-void gen_store4i(TCGv_env cpu_env, TCGv vaddr, int32_t src, uint32_t slot)
+void gen_store4i(TCGv_env cpu_env_, TCGv v_addr, int32_t src, uint32_t slot)
 {
     TCGv tmp = tcg_constant_tl(src);
-    gen_store4(cpu_env, vaddr, tmp, slot);
+    gen_store4(cpu_env_, v_addr, tmp, slot);
 }
 
-void gen_store8(TCGv_env cpu_env, TCGv vaddr, TCGv_i64 src, uint32_t slot)
+void gen_store8(TCGv_env cpu_env_, TCGv v_addr, TCGv_i64 src, uint32_t slot)
 {
-    tcg_gen_mov_tl(hex_store_addr[slot], vaddr);
+    tcg_gen_mov_tl(hex_store_addr[slot], v_addr);
     tcg_gen_movi_tl(hex_store_width[slot], 8);
     tcg_gen_mov_i64(hex_store_val64[slot], src);
 }
 
-void gen_store8i(TCGv_env cpu_env, TCGv vaddr, int64_t src, uint32_t slot)
+void gen_store8i(TCGv_env cpu_env_, TCGv v_addr, int64_t src, uint32_t slot)
 {
     TCGv_i64 tmp = tcg_constant_i64(src);
-    gen_store8(cpu_env, vaddr, tmp, slot);
+    gen_store8(cpu_env_, v_addr, tmp, slot);
 }
 
 TCGv gen_8bitsof(TCGv result, TCGv value)
diff --git a/target/hexagon/genptr.h b/target/hexagon/genptr.h
index a4b43c2910..b1289a3e78 100644
--- a/target/hexagon/genptr.h
+++ b/target/hexagon/genptr.h
@@ -24,15 +24,15 @@
 
 extern const SemanticInsn opcode_genptr[];
 
-void gen_store32(TCGv vaddr, TCGv src, int width, uint32_t slot);
-void gen_store1(TCGv_env cpu_env, TCGv vaddr, TCGv src, uint32_t slot);
-void gen_store2(TCGv_env cpu_env, TCGv vaddr, TCGv src, uint32_t slot);
-void gen_store4(TCGv_env cpu_env, TCGv vaddr, TCGv src, uint32_t slot);
-void gen_store8(TCGv_env cpu_env, TCGv vaddr, TCGv_i64 src, uint32_t slot);
-void gen_store1i(TCGv_env cpu_env, TCGv vaddr, int32_t src, uint32_t slot);
-void gen_store2i(TCGv_env cpu_env, TCGv vaddr, int32_t src, uint32_t slot);
-void gen_store4i(TCGv_env cpu_env, TCGv vaddr, int32_t src, uint32_t slot);
-void gen_store8i(TCGv_env cpu_env, TCGv vaddr, int64_t src, uint32_t slot);
+void gen_store32(TCGv v_addr, TCGv src, int width, uint32_t slot);
+void gen_store1(TCGv_env cpu_env_, TCGv v_addr, TCGv src, uint32_t slot);
+void gen_store2(TCGv_env cpu_env_, TCGv v_addr, TCGv src, uint32_t slot);
+void gen_store4(TCGv_env cpu_env_, TCGv v_addr, TCGv src, uint32_t slot);
+void gen_store8(TCGv_env cpu_env_, TCGv v_addr, TCGv_i64 src, uint32_t slot);
+void gen_store1i(TCGv_env cpu_env_, TCGv v_addr, int32_t src, uint32_t slot);
+void gen_store2i(TCGv_env cpu_env_, TCGv v_addr, int32_t src, uint32_t slot);
+void gen_store4i(TCGv_env cpu_env_, TCGv v_addr, int32_t src, uint32_t slot);
+void gen_store8i(TCGv_env cpu_env_, TCGv v_addr, int64_t src, uint32_t slot);
 TCGv gen_read_reg(TCGv result, int num);
 TCGv gen_read_preg(TCGv pred, uint8_t num);
 TCGv get_result_gpr(DisasContext *ctx, int rnum);
diff --git a/target/hexagon/mmvec/system_ext_mmvec.c b/target/hexagon/mmvec/system_ext_mmvec.c
index 8351f2cc01..c339eee38b 100644
--- a/target/hexagon/mmvec/system_ext_mmvec.c
+++ b/target/hexagon/mmvec/system_ext_mmvec.c
@@ -19,12 +19,12 @@
 #include "cpu.h"
 #include "mmvec/system_ext_mmvec.h"
 
-void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int slot)
+void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int slot)
 {
     size_t size = sizeof(MMVector);
 
     env->vstore_pending[slot] = 1;
-    env->vstore[slot].va   = vaddr;
+    env->vstore[slot].va   = v_addr;
     env->vstore[slot].size = size;
     memcpy(&env->vstore[slot].data.ub[0], &env->tmp_VRegs[0], size);
 
diff --git a/target/hexagon/mmvec/system_ext_mmvec.h b/target/hexagon/mmvec/system_ext_mmvec.h
index bcefbffdf2..6a711fcb69 100644
--- a/target/hexagon/mmvec/system_ext_mmvec.h
+++ b/target/hexagon/mmvec/system_ext_mmvec.h
@@ -18,7 +18,7 @@
 #ifndef HEXAGON_SYSTEM_EXT_MMVEC_H
 #define HEXAGON_SYSTEM_EXT_MMVEC_H
 
-void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int slot);
+void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int slot);
 void mem_vector_scatter_init(CPUHexagonState *env);
 void mem_vector_gather_init(CPUHexagonState *env);
 
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index da10ac5847..aeafd78887 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -577,12 +577,12 @@ void HELPER(probe_pkt_scalar_hvx_stores)(CPUHexagonState *env, int mask)
  * wasn't cancelled), we have to do the store first.
  */
 static void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
-                         uint32_t slot, target_ulong vaddr, int size,
+                         uint32_t slot, target_ulong v_addr, int size,
                          uintptr_t ra)
 {
     if (slot == 0 && pkt_has_store_s1 &&
         ((env->slot_cancelled & (1 << 1)) == 0)) {
-        probe_read(env, vaddr, size, MMU_USER_IDX, ra);
+        probe_read(env, v_addr, size, MMU_USER_IDX, ra);
         commit_store(env, 1, ra);
     }
 }
-- 
2.25.1

Re: [PATCH v2 2/3] target/hexagon: fix some occurrences of -Wshadow=local

[Philippe Mathieu-Daudé]

On 6/10/23 00:22, Brian Cain wrote:
> Of the changes in this commit, the changes in `HELPER(commit_hvx_stores)()`
> are less obvious.  They are required because of some macro invocations like
> SCATTER_OP_WRITE_TO_MEM().
> 
> e.g.:
> 
>      In file included from ../target/hexagon/op_helper.c:31:
>      ../target/hexagon/mmvec/macros.h:205:18: error: declaration of ‘i’ shadows a previous local [-Werror=shadow=compatible-local]
>        205 |         for (int i = 0; i < sizeof(MMVector); i += sizeof(TYPE)) { \
>            |                  ^
>      ../target/hexagon/op_helper.c:157:17: note: in expansion of macro ‘SCATTER_OP_WRITE_TO_MEM’
>        157 |                 SCATTER_OP_WRITE_TO_MEM(uint16_t);
>            |                 ^~~~~~~~~~~~~~~~~~~~~~~
>      ../target/hexagon/op_helper.c:135:9: note: shadowed declaration is here
>        135 |     int i;
>            |         ^
>      In file included from ../target/hexagon/op_helper.c:31:
>      ../target/hexagon/mmvec/macros.h:204:19: error: declaration of ‘ra’ shadows a previous local [-Werror=shadow=compatible-local]
>        204 |         uintptr_t ra = GETPC(); \
>            |                   ^~
>      ../target/hexagon/op_helper.c:160:17: note: in expansion of macro ‘SCATTER_OP_WRITE_TO_MEM’
>        160 |                 SCATTER_OP_WRITE_TO_MEM(uint32_t);
>            |                 ^~~~~~~~~~~~~~~~~~~~~~~
>      ../target/hexagon/op_helper.c:134:15: note: shadowed declaration is here
>        134 |     uintptr_t ra = GETPC();
>            |               ^~
> 
> Reviewed-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Signed-off-by: Brian Cain <bcain@quicinc.com>
> ---
>   target/hexagon/imported/alu.idef | 6 +++---
>   target/hexagon/mmvec/macros.h    | 6 +++---
>   target/hexagon/op_helper.c       | 9 +++------
>   target/hexagon/translate.c       | 9 ++++-----
>   4 files changed, 13 insertions(+), 17 deletions(-)

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

Re: [PATCH v2 0/3] hexagon: GETPC() and shadowing fixes

[Markus Armbruster]

Conflicts with recent commit ad75a51e84a (tcg: Rename cpu_env to
tcg_env).  Please rebase.

RE: [PATCH v2 2/3] target/hexagon: fix some occurrences of -Wshadow=local

[ltaylorsimpson]

> -----Original Message-----
> From: Brian Cain <bcain@quicinc.com>
> Sent: Thursday, October 5, 2023 4:22 PM
> To: qemu-devel@nongnu.org
> Cc: bcain@quicinc.com; armbru@redhat.com; richard.henderson@linaro.org;
> philmd@linaro.org; peter.maydell@linaro.org; quic_mathbern@quicinc.com;
> stefanha@redhat.com; ale@rev.ng; anjo@rev.ng;
> quic_mliebel@quicinc.com; ltaylorsimpson@gmail.com
> Subject: [PATCH v2 2/3] target/hexagon: fix some occurrences of -
> Wshadow=local
> 
> Of the changes in this commit, the changes in
> `HELPER(commit_hvx_stores)()` are less obvious.  They are required because
> of some macro invocations like SCATTER_OP_WRITE_TO_MEM().
> 
> e.g.:
> 
>     In file included from ../target/hexagon/op_helper.c:31:
>     ../target/hexagon/mmvec/macros.h:205:18: error: declaration of ‘i’
> shadows a previous local [-Werror=shadow=compatible-local]
>       205 |         for (int i = 0; i < sizeof(MMVector); i += sizeof(TYPE)) { \
>           |                  ^
>     ../target/hexagon/op_helper.c:157:17: note: in expansion of macro
> ‘SCATTER_OP_WRITE_TO_MEM’
>       157 |                 SCATTER_OP_WRITE_TO_MEM(uint16_t);
>           |                 ^~~~~~~~~~~~~~~~~~~~~~~
>     ../target/hexagon/op_helper.c:135:9: note: shadowed declaration is here
>       135 |     int i;
>           |         ^
>     In file included from ../target/hexagon/op_helper.c:31:
>     ../target/hexagon/mmvec/macros.h:204:19: error: declaration of ‘ra’
> shadows a previous local [-Werror=shadow=compatible-local]
>       204 |         uintptr_t ra = GETPC(); \
>           |                   ^~
>     ../target/hexagon/op_helper.c:160:17: note: in expansion of macro
> ‘SCATTER_OP_WRITE_TO_MEM’
>       160 |                 SCATTER_OP_WRITE_TO_MEM(uint32_t);
>           |                 ^~~~~~~~~~~~~~~~~~~~~~~
>     ../target/hexagon/op_helper.c:134:15: note: shadowed declaration is here
>       134 |     uintptr_t ra = GETPC();
>           |               ^~
> 
> Reviewed-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> Signed-off-by: Brian Cain <bcain@quicinc.com>
> ---
>  target/hexagon/imported/alu.idef | 6 +++---
>  target/hexagon/mmvec/macros.h    | 6 +++---
>  target/hexagon/op_helper.c       | 9 +++------
>  target/hexagon/translate.c       | 9 ++++-----
>  4 files changed, 13 insertions(+), 17 deletions(-)

Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com>

RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals

[ltaylorsimpson]

> -----Original Message-----
> From: Brian Cain <bcain@quicinc.com>
> Sent: Thursday, October 5, 2023 4:22 PM
> To: qemu-devel@nongnu.org
> Cc: bcain@quicinc.com; armbru@redhat.com; richard.henderson@linaro.org;
> philmd@linaro.org; peter.maydell@linaro.org; quic_mathbern@quicinc.com;
> stefanha@redhat.com; ale@rev.ng; anjo@rev.ng;
> quic_mliebel@quicinc.com; ltaylorsimpson@gmail.com
> Subject: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> 
> The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the
> identifiers to avoid shadowing the type name.
> 
> The global `cpu_env` is shadowed by local `cpu_env` arguments, so we
> rename the function arguments to avoid shadowing the global.
> 
> Signed-off-by: Brian Cain <bcain@quicinc.com>
> ---
>  target/hexagon/genptr.c                 | 56 ++++++++++++-------------
>  target/hexagon/genptr.h                 | 18 ++++----
>  target/hexagon/mmvec/system_ext_mmvec.c |  4 +-
> target/hexagon/mmvec/system_ext_mmvec.h |  2 +-
>  target/hexagon/op_helper.c              |  4 +-
>  5 files changed, 42 insertions(+), 42 deletions(-)
> 
> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
> 217bc7bb5a..11377ac92b 100644
> --- a/target/hexagon/genptr.c
> +++ b/target/hexagon/genptr.c
> @@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result, TCGv
> src)
>      tcg_gen_deposit_i64(result, result, src64, N * 8, 8);  }
> 
> -static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int
> mem_index)
> +static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int
> +mem_index)

I'd recommend moving both the type and the arg name to the new line, also indent the new line.
static inline void gen_load_locked4u(TCGv dest, TCGv v_addr,
                                                                  int mem_index)


> 
> -static inline void gen_load_locked8u(TCGv_i64 dest, TCGv vaddr, int
> mem_index)
> +static inline void gen_load_locked8u(TCGv_i64 dest, TCGv v_addr, int
> +mem_index)

Ditto

>  static inline void gen_store_conditional4(DisasContext *ctx,
> -                                          TCGv pred, TCGv vaddr, TCGv src)
> +                                          TCGv pred, TCGv v_addr, TCGv
> + src)

Ditto

>      zero = tcg_constant_tl(0);
> @@ -374,13 +374,13 @@ static inline void
> gen_store_conditional4(DisasContext *ctx,  }
> 
>  static inline void gen_store_conditional8(DisasContext *ctx,
> -                                          TCGv pred, TCGv vaddr, TCGv_i64 src)
> +                                          TCGv pred, TCGv v_addr,
> + TCGv_i64 src)

Indent

> -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int
> slot)
> +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int
> +slot)

Ditto

> -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int
> slot);
> +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int
> +slot);

Ditto


Otherwise,
Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com>

RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals

[Brian Cain]

> -----Original Message-----
> From: ltaylorsimpson@gmail.com <ltaylorsimpson@gmail.com>
> Sent: Friday, October 6, 2023 11:01 AM
> To: Brian Cain <bcain@quicinc.com>; qemu-devel@nongnu.org
> Cc: armbru@redhat.com; richard.henderson@linaro.org; philmd@linaro.org;
> peter.maydell@linaro.org; Matheus Bernardino (QUIC)
> <quic_mathbern@quicinc.com>; stefanha@redhat.com; ale@rev.ng;
> anjo@rev.ng; Marco Liebel (QUIC) <quic_mliebel@quicinc.com>
> Subject: RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> > -----Original Message-----
> > From: Brian Cain <bcain@quicinc.com>
> > Sent: Thursday, October 5, 2023 4:22 PM
> > To: qemu-devel@nongnu.org
> > Cc: bcain@quicinc.com; armbru@redhat.com; richard.henderson@linaro.org;
> > philmd@linaro.org; peter.maydell@linaro.org; quic_mathbern@quicinc.com;
> > stefanha@redhat.com; ale@rev.ng; anjo@rev.ng;
> > quic_mliebel@quicinc.com; ltaylorsimpson@gmail.com
> > Subject: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> >
> > The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the
> > identifiers to avoid shadowing the type name.
> >
> > The global `cpu_env` is shadowed by local `cpu_env` arguments, so we
> > rename the function arguments to avoid shadowing the global.
> >
> > Signed-off-by: Brian Cain <bcain@quicinc.com>
> > ---
> >  target/hexagon/genptr.c                 | 56 ++++++++++++-------------
> >  target/hexagon/genptr.h                 | 18 ++++----
> >  target/hexagon/mmvec/system_ext_mmvec.c |  4 +-
> > target/hexagon/mmvec/system_ext_mmvec.h |  2 +-
> >  target/hexagon/op_helper.c              |  4 +-
> >  5 files changed, 42 insertions(+), 42 deletions(-)
> >
> > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
> > 217bc7bb5a..11377ac92b 100644
> > --- a/target/hexagon/genptr.c
> > +++ b/target/hexagon/genptr.c
> > @@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result,
> TCGv
> > src)
> >      tcg_gen_deposit_i64(result, result, src64, N * 8, 8);  }
> >
> > -static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int
> > mem_index)
> > +static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int
> > +mem_index)
> 
> I'd recommend moving both the type and the arg name to the new line, also
> indent the new line.
> static inline void gen_load_locked4u(TCGv dest, TCGv v_addr,
>                                                                   int mem_index)
> 
> 
> >
> > -static inline void gen_load_locked8u(TCGv_i64 dest, TCGv vaddr, int
> > mem_index)
> > +static inline void gen_load_locked8u(TCGv_i64 dest, TCGv v_addr, int
> > +mem_index)
> 
> Ditto
> 
> >  static inline void gen_store_conditional4(DisasContext *ctx,
> > -                                          TCGv pred, TCGv vaddr, TCGv src)
> > +                                          TCGv pred, TCGv v_addr, TCGv
> > + src)
> 
> Ditto
> 
> >      zero = tcg_constant_tl(0);
> > @@ -374,13 +374,13 @@ static inline void
> > gen_store_conditional4(DisasContext *ctx,  }
> >
> >  static inline void gen_store_conditional8(DisasContext *ctx,
> > -                                          TCGv pred, TCGv vaddr, TCGv_i64 src)
> > +                                          TCGv pred, TCGv v_addr,
> > + TCGv_i64 src)
> 
> Indent
> 
> > -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int
> > slot)
> > +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int
> > +slot)
> 
> Ditto
> 
> > -void mem_gather_store(CPUHexagonState *env, target_ulong vaddr, int
> > slot);
> > +void mem_gather_store(CPUHexagonState *env, target_ulong v_addr, int
> > +slot);
> 
> Ditto

I could be mistaken but AFAICT none of these lines are wrapped in the way they're quoted above  in my patch (nor the baseline).  I don't think any of these lines exceed 80 columns, so they shouldn't need wrapping, either.

I double checked how it's displayed at the archive https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg01667.html to make sure that it wasn't a misconfiguration of my mailer.  For another perspective - refer to the commit used to create this patch: https://github.com/quic/qemu/commit/7f20565d403d16337ab6d69ee663121a3eef71e6

Is your review comment that "these lines should be wrapped and when you do, make sure you do it like this"?  Or "if you are going to wrap them, wrap them like this"?  Or something else?

> Otherwise,
> Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com>
> 

Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals

[Philippe Mathieu-Daudé]

Hi Brian,

On 6/10/23 00:22, Brian Cain wrote:
> The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the
> identifiers to avoid shadowing the type name.

This one surprises me, since we have other occurences:

include/exec/memory.h:751:bool memory_get_xlat_addr(IOMMUTLBEntry 
*iotlb, void **vaddr,
	include/qemu/plugin.h:199:void qemu_plugin_vcpu_mem_cb(CPUState *cpu, 
uint64_t vaddr,
target/arm/internals.h:643:G_NORETURN void 
arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
target/i386/tcg/helper-tcg.h:70:G_NORETURN void 
handle_unaligned_access(CPUX86State *env, vaddr vaddr,
...

$ git grep -w vaddr, | wc -l
      207

What is the error/warning like?

> The global `cpu_env` is shadowed by local `cpu_env` arguments, so we
> rename the function arguments to avoid shadowing the global.
> 
> Signed-off-by: Brian Cain <bcain@quicinc.com>
> ---
>   target/hexagon/genptr.c                 | 56 ++++++++++++-------------
>   target/hexagon/genptr.h                 | 18 ++++----
>   target/hexagon/mmvec/system_ext_mmvec.c |  4 +-
>   target/hexagon/mmvec/system_ext_mmvec.h |  2 +-
>   target/hexagon/op_helper.c              |  4 +-
>   5 files changed, 42 insertions(+), 42 deletions(-)
> 
> diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
> index 217bc7bb5a..11377ac92b 100644
> --- a/target/hexagon/genptr.c
> +++ b/target/hexagon/genptr.c
> @@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result, TCGv src)
>       tcg_gen_deposit_i64(result, result, src64, N * 8, 8);
>   }
>   
> -static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int mem_index)
> +static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int mem_index)
>   {
> -    tcg_gen_qemu_ld_tl(dest, vaddr, mem_index, MO_TEUL);
> -    tcg_gen_mov_tl(hex_llsc_addr, vaddr);
> +    tcg_gen_qemu_ld_tl(dest, v_addr, mem_index, MO_TEUL);
> +    tcg_gen_mov_tl(hex_llsc_addr, v_addr);
>       tcg_gen_mov_tl(hex_llsc_val, dest);
>   }

Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals

[Philippe Mathieu-Daudé]

On 9/10/23 08:09, Philippe Mathieu-Daudé wrote:
> Hi Brian,
> 
> On 6/10/23 00:22, Brian Cain wrote:
>> The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the
>> identifiers to avoid shadowing the type name.
> 
> This one surprises me, since we have other occurences:
> 
> include/exec/memory.h:751:bool memory_get_xlat_addr(IOMMUTLBEntry 
> *iotlb, void **vaddr,
>      include/qemu/plugin.h:199:void qemu_plugin_vcpu_mem_cb(CPUState 
> *cpu, uint64_t vaddr,
> target/arm/internals.h:643:G_NORETURN void 
> arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
> target/i386/tcg/helper-tcg.h:70:G_NORETURN void 
> handle_unaligned_access(CPUX86State *env, vaddr vaddr,
> ...
> 
> $ git grep -w vaddr, | wc -l
>       207
> 
> What is the error/warning like?

OK I could reproduce, I suppose you are building with Clang which
doesn't support shadow-local so you get global warnings too (as
mentioned in this patch subject...):

In file included from ../../gdbstub/trace.h:1,
                  from ../../gdbstub/softmmu.c:30:
trace/trace-gdbstub.h: In function '_nocheck__trace_gdbstub_hit_watchpoint':
trace/trace-gdbstub.h:903:106: error: declaration of 'vaddr' shadows a 
global declaration [-Werror=shadow]
   903 | static inline void _nocheck__trace_gdbstub_hit_watchpoint(const 
char * type, int cpu_gdb_index, uint64_t vaddr)
       | 
                                 ~~~~~~~~~^~~~~
In file included from include/sysemu/accel-ops.h:13,
                  from include/sysemu/cpus.h:4,
                  from ../../gdbstub/softmmu.c:21:
include/exec/cpu-common.h:21:18: note: shadowed declaration is here
    21 | typedef uint64_t vaddr;
       |                  ^~~~~
trace/trace-gdbstub.h: In function 'trace_gdbstub_hit_watchpoint':
trace/trace-gdbstub.h:923:96: error: declaration of 'vaddr' shadows a 
global declaration [-Werror=shadow]
   923 | static inline void trace_gdbstub_hit_watchpoint(const char * 
type, int cpu_gdb_index, uint64_t vaddr)
       | 
                       ~~~~~~~~~^~~~~
include/exec/cpu-common.h:21:18: note: shadowed declaration is here
    21 | typedef uint64_t vaddr;
       |                  ^~~~~

Clang users got confused by this, IIUC Markus and Thomas idea is
to only enable these warnings for GCC, enforcing them for Clang
users via CI (until Clang get this option supported). Personally
I'd rather enable the warning once for all, waiting for Clang
support (or clean/enable global shadowing for GCC too).

See this thread:
https://lore.kernel.org/qemu-devel/11abc551-188e-85c0-fe55-b2b58d35105d@redhat.com/

Regards,

Phil.

RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals

[ltaylorsimpson]

> -----Original Message-----
> From: Brian Cain <bcain@quicinc.com>
> Sent: Sunday, October 8, 2023 7:50 AM
> To: ltaylorsimpson@gmail.com; qemu-devel@nongnu.org
> Cc: armbru@redhat.com; richard.henderson@linaro.org; philmd@linaro.org;
> peter.maydell@linaro.org; Matheus Bernardino (QUIC)
> <quic_mathbern@quicinc.com>; stefanha@redhat.com; ale@rev.ng;
> anjo@rev.ng; Marco Liebel (QUIC) <quic_mliebel@quicinc.com>
> Subject: RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> 
> 
> 
> > -----Original Message-----
> > From: ltaylorsimpson@gmail.com <ltaylorsimpson@gmail.com>
> > Sent: Friday, October 6, 2023 11:01 AM
> > To: Brian Cain <bcain@quicinc.com>; qemu-devel@nongnu.org
> > Cc: armbru@redhat.com; richard.henderson@linaro.org;
> > philmd@linaro.org; peter.maydell@linaro.org; Matheus Bernardino (QUIC)
> > <quic_mathbern@quicinc.com>; stefanha@redhat.com; ale@rev.ng;
> > anjo@rev.ng; Marco Liebel (QUIC) <quic_mliebel@quicinc.com>
> > Subject: RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> >
> > WARNING: This email originated from outside of Qualcomm. Please be
> > wary of any links or attachments, and do not enable macros.
> >
> > > -----Original Message-----
> > > From: Brian Cain <bcain@quicinc.com>
> > > Sent: Thursday, October 5, 2023 4:22 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: bcain@quicinc.com; armbru@redhat.com;
> > > richard.henderson@linaro.org; philmd@linaro.org;
> > > peter.maydell@linaro.org; quic_mathbern@quicinc.com;
> > > stefanha@redhat.com; ale@rev.ng; anjo@rev.ng;
> > > quic_mliebel@quicinc.com; ltaylorsimpson@gmail.com
> > > Subject: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> > >
> > > The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename
> > > the identifiers to avoid shadowing the type name.
> > >
> > > The global `cpu_env` is shadowed by local `cpu_env` arguments, so we
> > > rename the function arguments to avoid shadowing the global.
> > >
> > > Signed-off-by: Brian Cain <bcain@quicinc.com>
> > > ---
> > >  target/hexagon/genptr.c                 | 56 ++++++++++++-------------
> > >  target/hexagon/genptr.h                 | 18 ++++----
> > >  target/hexagon/mmvec/system_ext_mmvec.c |  4 +-
> > > target/hexagon/mmvec/system_ext_mmvec.h |  2 +-
> > >  target/hexagon/op_helper.c              |  4 +-
> > >  5 files changed, 42 insertions(+), 42 deletions(-)
> > >
> > > diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c index
> > > 217bc7bb5a..11377ac92b 100644
> > > --- a/target/hexagon/genptr.c
> > > +++ b/target/hexagon/genptr.c
> > > @@ -334,28 +334,28 @@ void gen_set_byte_i64(int N, TCGv_i64 result,
> > TCGv
> > > src)
> > >      tcg_gen_deposit_i64(result, result, src64, N * 8, 8);  }
> > >
> > > -static inline void gen_load_locked4u(TCGv dest, TCGv vaddr, int
> > > mem_index)
> > > +static inline void gen_load_locked4u(TCGv dest, TCGv v_addr, int
> > > +mem_index)
> >
> > I'd recommend moving both the type and the arg name to the new line,
> > also indent the new line.
> > static inline void gen_load_locked4u(TCGv dest, TCGv v_addr,
> >                                                                   int
> > mem_index)
> >
> >
> I could be mistaken but AFAICT none of these lines are wrapped in the way
> they're quoted above  in my patch (nor the baseline).  I don't think any of
> these lines exceed 80 columns, so they shouldn't need wrapping, either.
> 
> I double checked how it's displayed at the archive
> https://lists.gnu.org/archive/html/qemu-devel/2023-10/msg01667.html to
> make sure that it wasn't a misconfiguration of my mailer.  For another
> perspective - refer to the commit used to create this patch:
> https://github.com/quic/qemu/commit/7f20565d403d16337ab6d69ee663121
> a3eef71e6
> 
> Is your review comment that "these lines should be wrapped and when you
> do, make sure you do it like this"?  Or "if you are going to wrap them, wrap
> them like this"?  Or something else?

Yes.  It looked like some adding the v_ would sometimes put the line over the 80 character size.
If so, wrap them as described.  If not, no wrapping is needed.


> 
> > Otherwise,
> > Reviewed-by: Taylor Simpson <ltaylorsimpson@gmail.com>
> >

RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals

[Brian Cain]

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Sent: Monday, October 9, 2023 1:43 AM
> To: Brian Cain <bcain@quicinc.com>; qemu-devel@nongnu.org
> Cc: armbru@redhat.com; richard.henderson@linaro.org;
> peter.maydell@linaro.org; Matheus Bernardino (QUIC)
> <quic_mathbern@quicinc.com>; stefanha@redhat.com; ale@rev.ng;
> anjo@rev.ng; Marco Liebel (QUIC) <quic_mliebel@quicinc.com>;
> ltaylorsimpson@gmail.com; Thomas Huth <thuth@redhat.com>; Daniel P.
> Berrangé <berrange@redhat.com>
> Subject: Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> On 9/10/23 08:09, Philippe Mathieu-Daudé wrote:
> > Hi Brian,
> >
> > On 6/10/23 00:22, Brian Cain wrote:
> >> The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the
> >> identifiers to avoid shadowing the type name.
> >
> > This one surprises me, since we have other occurences:
> >
> > include/exec/memory.h:751:bool memory_get_xlat_addr(IOMMUTLBEntry
> > *iotlb, void **vaddr,
> >      include/qemu/plugin.h:199:void qemu_plugin_vcpu_mem_cb(CPUState
> > *cpu, uint64_t vaddr,
> > target/arm/internals.h:643:G_NORETURN void
> > arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
> > target/i386/tcg/helper-tcg.h:70:G_NORETURN void
> > handle_unaligned_access(CPUX86State *env, vaddr vaddr,
> > ...
> >
> > $ git grep -w vaddr, | wc -l
> >       207
> >
> > What is the error/warning like?
> 
> OK I could reproduce, I suppose you are building with Clang which
> doesn't support shadow-local so you get global warnings too (as
> mentioned in this patch subject...):

No -- I generally build with gcc and only double-check the clang results to make sure I don't see any new failures there.

But I've not tested "-Wshadow" with clang yet.  I found these by adding "-Wshadow=global" to "-Wshadow=local".  I thought it might be useful to address these too while we're here.

> In file included from ../../gdbstub/trace.h:1,
>                   from ../../gdbstub/softmmu.c:30:
> trace/trace-gdbstub.h: In function '_nocheck__trace_gdbstub_hit_watchpoint':
> trace/trace-gdbstub.h:903:106: error: declaration of 'vaddr' shadows a
> global declaration [-Werror=shadow]
>    903 | static inline void _nocheck__trace_gdbstub_hit_watchpoint(const
> char * type, int cpu_gdb_index, uint64_t vaddr)
>        |
>                                  ~~~~~~~~~^~~~~
> In file included from include/sysemu/accel-ops.h:13,
>                   from include/sysemu/cpus.h:4,
>                   from ../../gdbstub/softmmu.c:21:
> include/exec/cpu-common.h:21:18: note: shadowed declaration is here
>     21 | typedef uint64_t vaddr;
>        |                  ^~~~~
> trace/trace-gdbstub.h: In function 'trace_gdbstub_hit_watchpoint':
> trace/trace-gdbstub.h:923:96: error: declaration of 'vaddr' shadows a
> global declaration [-Werror=shadow]
>    923 | static inline void trace_gdbstub_hit_watchpoint(const char *
> type, int cpu_gdb_index, uint64_t vaddr)
>        |
>                        ~~~~~~~~~^~~~~
> include/exec/cpu-common.h:21:18: note: shadowed declaration is here
>     21 | typedef uint64_t vaddr;
>        |                  ^~~~~
> 
> Clang users got confused by this, IIUC Markus and Thomas idea is
> to only enable these warnings for GCC, enforcing them for Clang
> users via CI (until Clang get this option supported). Personally
> I'd rather enable the warning once for all, waiting for Clang
> support (or clean/enable global shadowing for GCC too).

Hopefully it's helpful or at least benign if we address the shadowed globals under target/hexagon/ for now, even if "-Wshadow=global" is not enabled.

> See this thread:
> https://lore.kernel.org/qemu-devel/11abc551-188e-85c0-fe55-
> b2b58d35105d@redhat.com/
> 
> Regards,
> 
> Phil.

Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals

[Markus Armbruster]

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

> On 9/10/23 08:09, Philippe Mathieu-Daudé wrote:
>> Hi Brian,
>> On 6/10/23 00:22, Brian Cain wrote:
>>> The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the
>>> identifiers to avoid shadowing the type name.
>> This one surprises me, since we have other occurences:
>> include/exec/memory.h:751:bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
>>      include/qemu/plugin.h:199:void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
>> target/arm/internals.h:643:G_NORETURN void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>> target/i386/tcg/helper-tcg.h:70:G_NORETURN void handle_unaligned_access(CPUX86State *env, vaddr vaddr,
>> ...
>> $ git grep -w vaddr, | wc -l
>>       207
>> What is the error/warning like?
>
> OK I could reproduce, I suppose you are building with Clang which
> doesn't support shadow-local so you get global warnings too (as
> mentioned in this patch subject...):
>
> In file included from ../../gdbstub/trace.h:1,
>                  from ../../gdbstub/softmmu.c:30:
> trace/trace-gdbstub.h: In function '_nocheck__trace_gdbstub_hit_watchpoint':
> trace/trace-gdbstub.h:903:106: error: declaration of 'vaddr' shadows a global declaration [-Werror=shadow]
>   903 | static inline void _nocheck__trace_gdbstub_hit_watchpoint(const char * type, int cpu_gdb_index, uint64_t vaddr)
>       |                                  ~~~~~~~~~^~~~~
> In file included from include/sysemu/accel-ops.h:13,
>                  from include/sysemu/cpus.h:4,
>                  from ../../gdbstub/softmmu.c:21:
> include/exec/cpu-common.h:21:18: note: shadowed declaration is here
>    21 | typedef uint64_t vaddr;
>       |                  ^~~~~
> trace/trace-gdbstub.h: In function 'trace_gdbstub_hit_watchpoint':
> trace/trace-gdbstub.h:923:96: error: declaration of 'vaddr' shadows a global declaration [-Werror=shadow]
>   923 | static inline void trace_gdbstub_hit_watchpoint(const char * type, int cpu_gdb_index, uint64_t vaddr)
>       |                        ~~~~~~~~~^~~~~
> include/exec/cpu-common.h:21:18: note: shadowed declaration is here
>    21 | typedef uint64_t vaddr;
>       |                  ^~~~~
>
> Clang users got confused by this, IIUC Markus and Thomas idea is
> to only enable these warnings for GCC, enforcing them for Clang
> users via CI (until Clang get this option supported). Personally
> I'd rather enable the warning once for all, waiting for Clang
> support (or clean/enable global shadowing for GCC too).
>
> See this thread:
> https://lore.kernel.org/qemu-devel/11abc551-188e-85c0-fe55-b2b58d35105d@redhat.com/

The idea to enable some variation of -Wshadow goes back to this thread:

    Subject: Re: [RFC PATCH] docs/style: permit inline loop variables
    Date: Thu, 24 Aug 2023 14:18:53 +0100
    Message-ID: <CAFEAcA8wobO5F16vYhbQCjeadfN5Zwx5CQ7L4vQ3fh8c_6ngJg@mail.gmail.com>
    https://lore.kernel.org/qemu-devel/CAFEAcA8wobO5F16vYhbQCjeadfN5Zwx5CQ7L4vQ3fh8c_6ngJg@mail.gmail.com/

I've been aiming at -Wshadow=local because (1) it seemed more practical
(a lot less cleanup needed before we can enable the warning), and (2)
local shadowing is basically always foolish and fully our own fault.

No objection to additional -Wshadow work as long as I'm not the one
doing it :)

Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals

[Philippe Mathieu-Daudé]

On 9/10/23 22:53, Brian Cain wrote:
>> On 9/10/23 08:09, Philippe Mathieu-Daudé wrote:
>>> On 6/10/23 00:22, Brian Cain wrote:
>>>> The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the
>>>> identifiers to avoid shadowing the type name.
>>>
>>> This one surprises me, since we have other occurences:
>>>
>>> include/exec/memory.h:751:bool memory_get_xlat_addr(IOMMUTLBEntry
>>> *iotlb, void **vaddr,
>>>       include/qemu/plugin.h:199:void qemu_plugin_vcpu_mem_cb(CPUState
>>> *cpu, uint64_t vaddr,
>>> target/arm/internals.h:643:G_NORETURN void
>>> arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
>>> target/i386/tcg/helper-tcg.h:70:G_NORETURN void
>>> handle_unaligned_access(CPUX86State *env, vaddr vaddr,
>>> ...
>>>
>>> $ git grep -w vaddr, | wc -l
>>>        207
>>>
>>> What is the error/warning like?
>>
>> OK I could reproduce, I suppose you are building with Clang which
>> doesn't support shadow-local so you get global warnings too (as
>> mentioned in this patch subject...):
> 
> No -- I generally build with gcc and only double-check the clang results to make sure I don't see any new failures there.
> 
> But I've not tested "-Wshadow" with clang yet.  I found these by adding "-Wshadow=global" to "-Wshadow=local".  I thought it might be useful to address these too while we're here.
> 
>> In file included from ../../gdbstub/trace.h:1,
>>                    from ../../gdbstub/softmmu.c:30:
>> trace/trace-gdbstub.h: In function '_nocheck__trace_gdbstub_hit_watchpoint':
>> trace/trace-gdbstub.h:903:106: error: declaration of 'vaddr' shadows a
>> global declaration [-Werror=shadow]
>>     903 | static inline void _nocheck__trace_gdbstub_hit_watchpoint(const
>> char * type, int cpu_gdb_index, uint64_t vaddr)
>>         |
>>                                   ~~~~~~~~~^~~~~
>> In file included from include/sysemu/accel-ops.h:13,
>>                    from include/sysemu/cpus.h:4,
>>                    from ../../gdbstub/softmmu.c:21:
>> include/exec/cpu-common.h:21:18: note: shadowed declaration is here
>>      21 | typedef uint64_t vaddr;
>>         |                  ^~~~~
>> trace/trace-gdbstub.h: In function 'trace_gdbstub_hit_watchpoint':
>> trace/trace-gdbstub.h:923:96: error: declaration of 'vaddr' shadows a
>> global declaration [-Werror=shadow]
>>     923 | static inline void trace_gdbstub_hit_watchpoint(const char *
>> type, int cpu_gdb_index, uint64_t vaddr)
>>         |
>>                         ~~~~~~~~~^~~~~
>> include/exec/cpu-common.h:21:18: note: shadowed declaration is here
>>      21 | typedef uint64_t vaddr;
>>         |                  ^~~~~

If we have to clean that for -Wshadow=global, I'm tempted to rename
the typedef as 'vaddr_t' and keep the 'vaddr' variable names.

Richard, Anton, what do you think?

>> Clang users got confused by this, IIUC Markus and Thomas idea is
>> to only enable these warnings for GCC, enforcing them for Clang
>> users via CI (until Clang get this option supported). Personally
>> I'd rather enable the warning once for all, waiting for Clang
>> support (or clean/enable global shadowing for GCC too).
> 
> Hopefully it's helpful or at least benign if we address the shadowed globals under target/hexagon/ for now, even if "-Wshadow=global" is not enabled.
> 
>> See this thread:
>> https://lore.kernel.org/qemu-devel/11abc551-188e-85c0-fe55-
>> b2b58d35105d@redhat.com/
>>
>> Regards,
>>
>> Phil.

Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals

[Markus Armbruster]

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

[...]

> If we have to clean that for -Wshadow=global, I'm tempted to rename
> the typedef as 'vaddr_t' and keep the 'vaddr' variable names.

POSIX reserves suffix _t, see
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02_02

Do we care?

> Richard, Anton, what do you think?

[...]

Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals

[Philippe Mathieu-Daudé]

On 10/10/23 08:04, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
> [...]
> 
>> If we have to clean that for -Wshadow=global, I'm tempted to rename
>> the typedef as 'vaddr_t' and keep the 'vaddr' variable names.
> 
> POSIX reserves suffix _t, see
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02_02

Alternatives: tvaddr, VAddr, TargetVirtualAddress.

Naming is hard.

> Do we care?
> 
>> Richard, Anton, what do you think?
> 
> [...]
> 

Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals

[Thomas Huth]

On 10/10/2023 09.34, Philippe Mathieu-Daudé wrote:
> On 10/10/23 08:04, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>>
>> [...]
>>
>>> If we have to clean that for -Wshadow=global, I'm tempted to rename
>>> the typedef as 'vaddr_t' and keep the 'vaddr' variable names.
>>
>> POSIX reserves suffix _t, see
>> https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02_02
> 
> Alternatives: tvaddr, VAddr, TargetVirtualAddress.
> 
> Naming is hard.
> 
>> Do we care?

According to our docs/devel/style.rst :

"Scalar type
names are lower_case_with_underscores_ending_with_a_t, like the POSIX
uint64_t and family.  Note that this last convention contradicts POSIX
and is therefore likely to be changed."

Maybe this would be a good point in time now to revisit our coding style, 
update it and then to change the "vaddr" type accordingly?

My 0.02 €: If enum types should already be in CamelCase, why not also use 
that for scalar types? So I'd vote for VirtAddr or VAddr here.

  Thomas

RE: [PATCH v2 3/3] target/hexagon: avoid shadowing globals

[Brian Cain]

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Sent: Tuesday, October 10, 2023 12:23 AM
> To: Brian Cain <bcain@quicinc.com>; richard.henderson@linaro.org;
> anjo@rev.ng
> Cc: armbru@redhat.com; peter.maydell@linaro.org; Matheus Bernardino
> (QUIC) <quic_mathbern@quicinc.com>; stefanha@redhat.com; ale@rev.ng;
> Marco Liebel (QUIC) <quic_mliebel@quicinc.com>; ltaylorsimpson@gmail.com;
> Thomas Huth <thuth@redhat.com>; Daniel P. Berrangé
> <berrange@redhat.com>; qemu-devel@nongnu.org
> Subject: Re: [PATCH v2 3/3] target/hexagon: avoid shadowing globals
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> On 9/10/23 22:53, Brian Cain wrote:
> >> On 9/10/23 08:09, Philippe Mathieu-Daudé wrote:
> >>> On 6/10/23 00:22, Brian Cain wrote:
> >>>> The typedef `vaddr` is shadowed by `vaddr` identifiers, so we rename the
> >>>> identifiers to avoid shadowing the type name.
> >>>
> >>> This one surprises me, since we have other occurences:
> >>>
> >>> include/exec/memory.h:751:bool memory_get_xlat_addr(IOMMUTLBEntry
> >>> *iotlb, void **vaddr,
> >>>       include/qemu/plugin.h:199:void qemu_plugin_vcpu_mem_cb(CPUState
> >>> *cpu, uint64_t vaddr,
> >>> target/arm/internals.h:643:G_NORETURN void
> >>> arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr,
> >>> target/i386/tcg/helper-tcg.h:70:G_NORETURN void
> >>> handle_unaligned_access(CPUX86State *env, vaddr vaddr,
> >>> ...
> >>>
> >>> $ git grep -w vaddr, | wc -l
> >>>        207
> >>>
> >>> What is the error/warning like?
> >>
> >> OK I could reproduce, I suppose you are building with Clang which
> >> doesn't support shadow-local so you get global warnings too (as
> >> mentioned in this patch subject...):
> >
> > No -- I generally build with gcc and only double-check the clang results to
> make sure I don't see any new failures there.
> >
> > But I've not tested "-Wshadow" with clang yet.  I found these by adding "-
> Wshadow=global" to "-Wshadow=local".  I thought it might be useful to
> address these too while we're here.
> >
> >> In file included from ../../gdbstub/trace.h:1,
> >>                    from ../../gdbstub/softmmu.c:30:
> >> trace/trace-gdbstub.h: In function
> '_nocheck__trace_gdbstub_hit_watchpoint':
> >> trace/trace-gdbstub.h:903:106: error: declaration of 'vaddr' shadows a
> >> global declaration [-Werror=shadow]
> >>     903 | static inline void _nocheck__trace_gdbstub_hit_watchpoint(const
> >> char * type, int cpu_gdb_index, uint64_t vaddr)
> >>         |
> >>                                   ~~~~~~~~~^~~~~
> >> In file included from include/sysemu/accel-ops.h:13,
> >>                    from include/sysemu/cpus.h:4,
> >>                    from ../../gdbstub/softmmu.c:21:
> >> include/exec/cpu-common.h:21:18: note: shadowed declaration is here
> >>      21 | typedef uint64_t vaddr;
> >>         |                  ^~~~~
> >> trace/trace-gdbstub.h: In function 'trace_gdbstub_hit_watchpoint':
> >> trace/trace-gdbstub.h:923:96: error: declaration of 'vaddr' shadows a
> >> global declaration [-Werror=shadow]
> >>     923 | static inline void trace_gdbstub_hit_watchpoint(const char *
> >> type, int cpu_gdb_index, uint64_t vaddr)
> >>         |
> >>                         ~~~~~~~~~^~~~~
> >> include/exec/cpu-common.h:21:18: note: shadowed declaration is here
> >>      21 | typedef uint64_t vaddr;
> >>         |                  ^~~~~
> 
> If we have to clean that for -Wshadow=global, I'm tempted to rename
> the typedef as 'vaddr_t' and keep the 'vaddr' variable names.
> 
> Richard, Anton, what do you think?

Feels like I may have strolled into uncharted territory.  I'll just drop the patch that is intended to address -Wshadow=global and resurrect it if/when we decide to take that on in general.

> >> Clang users got confused by this, IIUC Markus and Thomas idea is
> >> to only enable these warnings for GCC, enforcing them for Clang
> >> users via CI (until Clang get this option supported). Personally
> >> I'd rather enable the warning once for all, waiting for Clang
> >> support (or clean/enable global shadowing for GCC too).
> >
> > Hopefully it's helpful or at least benign if we address the shadowed globals
> under target/hexagon/ for now, even if "-Wshadow=global" is not enabled.
> >
> >> See this thread:
> >> https://lore.kernel.org/qemu-devel/11abc551-188e-85c0-fe55-
> >> b2b58d35105d@redhat.com/
> >>
> >> Regards,
> >>
> >> Phil.