[PATCH v2 0/6] Two -Wclobbered fixes, plus other cleanup

[PATCH v2 0/6] Two -Wclobbered fixes, plus other cleanup

[Richard Henderson]

Stefan reported for accel/tcg, and I reproduced on Ubuntu 22.04.

Changes for v2:
  * Incorporate suggested changes to nanomips.c (phil, balaton).


r~


Richard Henderson (6):
  disas/nanomips: Move setjmp into nanomips_dis
  disas/nanomips: Merge insn{1,2,3} into words[3]
  disas/nanomips: Split out read_u16
  disas/nanomips: Tidy read for 48-bit opcodes
  tcg: Move TCG_TARGET_HAS_direct_jump init to tb_gen_code
  accel/tcg: Split out setjmp_gen_code

 accel/tcg/translate-all.c |  68 ++++++++++++-----------
 disas/nanomips.c          | 110 ++++++++++++++++----------------------
 tcg/tcg.c                 |  12 +++++
 3 files changed, 90 insertions(+), 100 deletions(-)

-- 
2.34.1

[PATCH v2 1/6] disas/nanomips: Move setjmp into nanomips_dis

[Richard Henderson]

Reduce the number of local variables within the scope of the
setjmp by moving it to the existing helper.  The actual length
returned from Disassemble is not used, because we have already
determined the length while reading bytes.  Fixes:

nanomips.c: In function ‘print_insn_nanomips’:
nanomips.c:21925:14: error: variable ‘insn1’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
nanomips.c:21925:25: error: variable ‘insn2’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
nanomips.c:21925:36: error: variable ‘insn3’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
nanomips.c:21926:22: error: variable ‘buf’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas/nanomips.c | 42 +++++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/disas/nanomips.c b/disas/nanomips.c
index 9647f1a8e3..ea3e9202ac 100644
--- a/disas/nanomips.c
+++ b/disas/nanomips.c
@@ -21905,22 +21905,24 @@ static const Pool MAJOR[2] = {
        0x0                 },        /* P16 */
 };
 
-static int nanomips_dis(char **buf,
-                 Dis_info *info,
-                 unsigned short one,
-                 unsigned short two,
-                 unsigned short three)
+static bool nanomips_dis(char **buf, Dis_info *info,
+                         unsigned short one,
+                         unsigned short two,
+                         unsigned short three)
 {
     uint16 bits[3] = {one, two, three};
-
     TABLE_ENTRY_TYPE type;
-    int size = Disassemble(bits, buf, &type, MAJOR, 2, info);
-    return size;
+
+    /* Handle runtime errors. */
+    if (unlikely(sigsetjmp(info->buf, 0) != 0)) {
+        return false;
+    }
+    return Disassemble(bits, buf, &type, MAJOR, ARRAY_SIZE(MAJOR), info) >= 0;
 }
 
 int print_insn_nanomips(bfd_vma memaddr, struct disassemble_info *info)
 {
-    int status;
+    int status, length;
     bfd_byte buffer[2];
     uint16_t insn1 = 0, insn2 = 0, insn3 = 0;
     g_autofree char *buf = NULL;
@@ -21950,6 +21952,7 @@ int print_insn_nanomips(bfd_vma memaddr, struct disassemble_info *info)
     } else {
         insn1 = bfd_getl16(buffer);
     }
+    length = 2;
     (*info->fprintf_func)(info->stream, "%04x ", insn1);
 
     /* Handle 32-bit opcodes.  */
@@ -21965,6 +21968,7 @@ int print_insn_nanomips(bfd_vma memaddr, struct disassemble_info *info)
         } else {
             insn2 = bfd_getl16(buffer);
         }
+        length = 4;
         (*info->fprintf_func)(info->stream, "%04x ", insn2);
     } else {
         (*info->fprintf_func)(info->stream, "     ");
@@ -21982,27 +21986,15 @@ int print_insn_nanomips(bfd_vma memaddr, struct disassemble_info *info)
         } else {
             insn3 = bfd_getl16(buffer);
         }
+        length = 6;
         (*info->fprintf_func)(info->stream, "%04x ", insn3);
     } else {
         (*info->fprintf_func)(info->stream, "     ");
     }
 
-    /* Handle runtime errors. */
-    if (sigsetjmp(disassm_info.buf, 0) != 0) {
-        info->insn_type = dis_noninsn;
-        return insn3 ? 6 : insn2 ? 4 : 2;
+    if (nanomips_dis(&buf, &disassm_info, insn1, insn2, insn3)) {
+        (*info->fprintf_func) (info->stream, "%s", buf);
     }
 
-    int length = nanomips_dis(&buf, &disassm_info, insn1, insn2, insn3);
-
-    /* FIXME: Should probably use a hash table on the major opcode here.  */
-
-    (*info->fprintf_func) (info->stream, "%s", buf);
-    if (length > 0) {
-        return length / 8;
-    }
-
-    info->insn_type = dis_noninsn;
-
-    return insn3 ? 6 : insn2 ? 4 : 2;
+    return length;
 }
-- 
2.34.1

[PATCH v2 2/6] disas/nanomips: Merge insn{1,2,3} into words[3]

[Richard Henderson]

Since Disassemble wants the data in this format, collect
it that way.  This allows using a loop to print the bytes.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas/nanomips.c | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/disas/nanomips.c b/disas/nanomips.c
index ea3e9202ac..1645d6d7aa 100644
--- a/disas/nanomips.c
+++ b/disas/nanomips.c
@@ -21905,26 +21905,22 @@ static const Pool MAJOR[2] = {
        0x0                 },        /* P16 */
 };
 
-static bool nanomips_dis(char **buf, Dis_info *info,
-                         unsigned short one,
-                         unsigned short two,
-                         unsigned short three)
+static bool nanomips_dis(const uint16_t *data, char **buf, Dis_info *info)
 {
-    uint16 bits[3] = {one, two, three};
     TABLE_ENTRY_TYPE type;
 
     /* Handle runtime errors. */
     if (unlikely(sigsetjmp(info->buf, 0) != 0)) {
         return false;
     }
-    return Disassemble(bits, buf, &type, MAJOR, ARRAY_SIZE(MAJOR), info) >= 0;
+    return Disassemble(data, buf, &type, MAJOR, ARRAY_SIZE(MAJOR), info) >= 0;
 }
 
 int print_insn_nanomips(bfd_vma memaddr, struct disassemble_info *info)
 {
     int status, length;
     bfd_byte buffer[2];
-    uint16_t insn1 = 0, insn2 = 0, insn3 = 0;
+    uint16_t words[3] = { };
     g_autofree char *buf = NULL;
 
     info->bytes_per_chunk = 2;
@@ -21948,15 +21944,14 @@ int print_insn_nanomips(bfd_vma memaddr, struct disassemble_info *info)
     }
 
     if (info->endian == BFD_ENDIAN_BIG) {
-        insn1 = bfd_getb16(buffer);
+        words[0] = bfd_getb16(buffer);
     } else {
-        insn1 = bfd_getl16(buffer);
+        words[0] = bfd_getl16(buffer);
     }
     length = 2;
-    (*info->fprintf_func)(info->stream, "%04x ", insn1);
 
     /* Handle 32-bit opcodes.  */
-    if ((insn1 & 0x1000) == 0) {
+    if ((words[0] & 0x1000) == 0) {
         status = (*info->read_memory_func)(memaddr + 2, buffer, 2, info);
         if (status != 0) {
             (*info->memory_error_func)(status, memaddr + 2, info);
@@ -21964,17 +21959,15 @@ int print_insn_nanomips(bfd_vma memaddr, struct disassemble_info *info)
         }
 
         if (info->endian == BFD_ENDIAN_BIG) {
-            insn2 = bfd_getb16(buffer);
+            words[1] = bfd_getb16(buffer);
         } else {
-            insn2 = bfd_getl16(buffer);
+            words[1] = bfd_getl16(buffer);
         }
         length = 4;
-        (*info->fprintf_func)(info->stream, "%04x ", insn2);
-    } else {
-        (*info->fprintf_func)(info->stream, "     ");
     }
+
     /* Handle 48-bit opcodes.  */
-    if ((insn1 >> 10) == 0x18) {
+    if ((words[0] >> 10) == 0x18) {
         status = (*info->read_memory_func)(memaddr + 4, buffer, 2, info);
         if (status != 0) {
             (*info->memory_error_func)(status, memaddr + 4, info);
@@ -21982,17 +21975,22 @@ int print_insn_nanomips(bfd_vma memaddr, struct disassemble_info *info)
         }
 
         if (info->endian == BFD_ENDIAN_BIG) {
-            insn3 = bfd_getb16(buffer);
+            words[2] = bfd_getb16(buffer);
         } else {
-            insn3 = bfd_getl16(buffer);
+            words[2] = bfd_getl16(buffer);
         }
         length = 6;
-        (*info->fprintf_func)(info->stream, "%04x ", insn3);
-    } else {
-        (*info->fprintf_func)(info->stream, "     ");
     }
 
-    if (nanomips_dis(&buf, &disassm_info, insn1, insn2, insn3)) {
+    for (int i = 0; i < ARRAY_SIZE(words); i++) {
+        if (i * 2 < length) {
+            (*info->fprintf_func)(info->stream, "%04x ", words[i]);
+        } else {
+            (*info->fprintf_func)(info->stream, "     ");
+        }
+    }
+
+    if (nanomips_dis(words, &buf, &disassm_info)) {
         (*info->fprintf_func) (info->stream, "%s", buf);
     }
 
-- 
2.34.1

[PATCH v2 3/6] disas/nanomips: Split out read_u16

[Richard Henderson]

Split out a helper function for reading a uint16_t
with the correct endianness.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas/nanomips.c | 48 +++++++++++++++++++-----------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/disas/nanomips.c b/disas/nanomips.c
index 1645d6d7aa..717fafb739 100644
--- a/disas/nanomips.c
+++ b/disas/nanomips.c
@@ -21916,10 +21916,24 @@ static bool nanomips_dis(const uint16_t *data, char **buf, Dis_info *info)
     return Disassemble(data, buf, &type, MAJOR, ARRAY_SIZE(MAJOR), info) >= 0;
 }
 
+static bool read_u16(uint16_t *ret, bfd_vma memaddr,
+                     struct disassemble_info *info)
+{
+    int status = (*info->read_memory_func)(memaddr, (bfd_byte *)ret, 2, info);
+    if (status != 0) {
+        (*info->memory_error_func)(status, memaddr, info);
+        return false;
+    }
+
+    if ((info->endian == BFD_ENDIAN_BIG) != HOST_BIG_ENDIAN) {
+        bswap16s(ret);
+    }
+    return true;
+}
+
 int print_insn_nanomips(bfd_vma memaddr, struct disassemble_info *info)
 {
-    int status, length;
-    bfd_byte buffer[2];
+    int length;
     uint16_t words[3] = { };
     g_autofree char *buf = NULL;
 
@@ -21937,48 +21951,24 @@ int print_insn_nanomips(bfd_vma memaddr, struct disassemble_info *info)
     disassm_info.fprintf_func = info->fprintf_func;
     disassm_info.stream = info->stream;
 
-    status = (*info->read_memory_func)(memaddr, buffer, 2, info);
-    if (status != 0) {
-        (*info->memory_error_func)(status, memaddr, info);
+    if (!read_u16(&words[0], memaddr, info)) {
         return -1;
     }
-
-    if (info->endian == BFD_ENDIAN_BIG) {
-        words[0] = bfd_getb16(buffer);
-    } else {
-        words[0] = bfd_getl16(buffer);
-    }
     length = 2;
 
     /* Handle 32-bit opcodes.  */
     if ((words[0] & 0x1000) == 0) {
-        status = (*info->read_memory_func)(memaddr + 2, buffer, 2, info);
-        if (status != 0) {
-            (*info->memory_error_func)(status, memaddr + 2, info);
+        if (!read_u16(&words[1], memaddr + 2, info)) {
             return -1;
         }
-
-        if (info->endian == BFD_ENDIAN_BIG) {
-            words[1] = bfd_getb16(buffer);
-        } else {
-            words[1] = bfd_getl16(buffer);
-        }
         length = 4;
     }
 
     /* Handle 48-bit opcodes.  */
     if ((words[0] >> 10) == 0x18) {
-        status = (*info->read_memory_func)(memaddr + 4, buffer, 2, info);
-        if (status != 0) {
-            (*info->memory_error_func)(status, memaddr + 4, info);
+        if (!read_u16(&words[1], memaddr + 4, info)) {
             return -1;
         }
-
-        if (info->endian == BFD_ENDIAN_BIG) {
-            words[2] = bfd_getb16(buffer);
-        } else {
-            words[2] = bfd_getl16(buffer);
-        }
         length = 6;
     }
 
-- 
2.34.1

[PATCH v2 4/6] disas/nanomips: Tidy read for 48-bit opcodes

[Richard Henderson]

There is no point in looking for a 48-bit opcode if we've
not read the second word for a 32-bit opcode.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 disas/nanomips.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/disas/nanomips.c b/disas/nanomips.c
index 717fafb739..abc78ae165 100644
--- a/disas/nanomips.c
+++ b/disas/nanomips.c
@@ -21962,14 +21962,14 @@ int print_insn_nanomips(bfd_vma memaddr, struct disassemble_info *info)
             return -1;
         }
         length = 4;
-    }
 
-    /* Handle 48-bit opcodes.  */
-    if ((words[0] >> 10) == 0x18) {
-        if (!read_u16(&words[1], memaddr + 4, info)) {
-            return -1;
+        /* Handle 48-bit opcodes.  */
+        if ((words[0] >> 10) == 0x18) {
+            if (!read_u16(&words[1], memaddr + 4, info)) {
+                return -1;
+            }
+            length = 6;
         }
-        length = 6;
     }
 
     for (int i = 0; i < ARRAY_SIZE(words); i++) {
-- 
2.34.1

[PATCH v2 5/6] tcg: Move TCG_TARGET_HAS_direct_jump init to tb_gen_code

[Richard Henderson]

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

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 921944a5ab..9ee21f7f52 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -821,16 +821,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
     trace_translate_block(tb, pc, tb->tc.ptr);
 
     /* generate machine code */
-    tb->jmp_reset_offset[0] = TB_JMP_RESET_OFFSET_INVALID;
-    tb->jmp_reset_offset[1] = TB_JMP_RESET_OFFSET_INVALID;
-    tcg_ctx->tb_jmp_reset_offset = tb->jmp_reset_offset;
-    if (TCG_TARGET_HAS_direct_jump) {
-        tcg_ctx->tb_jmp_insn_offset = tb->jmp_target_arg;
-        tcg_ctx->tb_jmp_target_addr = NULL;
-    } else {
-        tcg_ctx->tb_jmp_insn_offset = NULL;
-        tcg_ctx->tb_jmp_target_addr = tb->jmp_target_arg;
-    }
 
 #ifdef CONFIG_PROFILER
     qatomic_set(&prof->tb_count, prof->tb_count + 1);
diff --git a/tcg/tcg.c b/tcg/tcg.c
index b43b6a7981..436fcf6ebd 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -4228,6 +4228,18 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb, target_ulong pc_start)
     }
 #endif
 
+    /* Initialize goto_tb jump offsets. */
+    tb->jmp_reset_offset[0] = TB_JMP_RESET_OFFSET_INVALID;
+    tb->jmp_reset_offset[1] = TB_JMP_RESET_OFFSET_INVALID;
+    tcg_ctx->tb_jmp_reset_offset = tb->jmp_reset_offset;
+    if (TCG_TARGET_HAS_direct_jump) {
+        tcg_ctx->tb_jmp_insn_offset = tb->jmp_target_arg;
+        tcg_ctx->tb_jmp_target_addr = NULL;
+    } else {
+        tcg_ctx->tb_jmp_insn_offset = NULL;
+        tcg_ctx->tb_jmp_target_addr = tb->jmp_target_arg;
+    }
+
     tcg_reg_alloc_start(s);
 
     /*
-- 
2.34.1

[PATCH v2 6/6] accel/tcg: Split out setjmp_gen_code

[Richard Henderson]

Isolate the code protected by setjmp.  Fixes:

translate-all.c: In function ‘tb_gen_code’:
translate-all.c:748:51: error: argument ‘cflags’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/translate-all.c | 58 ++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 9ee21f7f52..ac3ee3740c 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -742,6 +742,37 @@ void page_collection_unlock(struct page_collection *set)
 
 #endif /* !CONFIG_USER_ONLY */
 
+/*
+ * Isolate the portion of code gen which can setjmp/longjmp.
+ * Return the size of the generated code, or negative on error.
+ */
+static int setjmp_gen_code(CPUArchState *env, TranslationBlock *tb,
+                           target_ulong pc, void *host_pc,
+                           int *max_insns, int64_t *ti)
+{
+    int ret = sigsetjmp(tcg_ctx->jmp_trans, 0);
+    if (unlikely(ret != 0)) {
+        return ret;
+    }
+
+    tcg_func_start(tcg_ctx);
+
+    tcg_ctx->cpu = env_cpu(env);
+    gen_intermediate_code(env_cpu(env), tb, *max_insns, pc, host_pc);
+    assert(tb->size != 0);
+    tcg_ctx->cpu = NULL;
+    *max_insns = tb->icount;
+
+#ifdef CONFIG_PROFILER
+    qatomic_set(&tcg_ctx->prof.tb_count, tcg_ctx->prof.tb_count + 1);
+    qatomic_set(&tcg_ctx->prof.interm_time,
+                tcg_ctx->prof.interm_time + profile_getclock() - *ti);
+    *ti = profile_getclock();
+#endif
+
+    return tcg_gen_code(tcg_ctx, tb, pc);
+}
+
 /* Called with mmap_lock held for user mode emulation.  */
 TranslationBlock *tb_gen_code(CPUState *cpu,
                               target_ulong pc, target_ulong cs_base,
@@ -754,8 +785,8 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
     int gen_code_size, search_size, max_insns;
 #ifdef CONFIG_PROFILER
     TCGProfile *prof = &tcg_ctx->prof;
-    int64_t ti;
 #endif
+    int64_t ti;
     void *host_pc;
 
     assert_memory_lock();
@@ -805,33 +836,10 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
     ti = profile_getclock();
 #endif
 
-    gen_code_size = sigsetjmp(tcg_ctx->jmp_trans, 0);
-    if (unlikely(gen_code_size != 0)) {
-        goto error_return;
-    }
-
-    tcg_func_start(tcg_ctx);
-
-    tcg_ctx->cpu = env_cpu(env);
-    gen_intermediate_code(cpu, tb, max_insns, pc, host_pc);
-    assert(tb->size != 0);
-    tcg_ctx->cpu = NULL;
-    max_insns = tb->icount;
-
     trace_translate_block(tb, pc, tb->tc.ptr);
 
-    /* generate machine code */
-
-#ifdef CONFIG_PROFILER
-    qatomic_set(&prof->tb_count, prof->tb_count + 1);
-    qatomic_set(&prof->interm_time,
-                prof->interm_time + profile_getclock() - ti);
-    ti = profile_getclock();
-#endif
-
-    gen_code_size = tcg_gen_code(tcg_ctx, tb, pc);
+    gen_code_size = setjmp_gen_code(env, tb, pc, host_pc, &max_insns, &ti);
     if (unlikely(gen_code_size < 0)) {
- error_return:
         switch (gen_code_size) {
         case -1:
             /*
-- 
2.34.1

Re: [PATCH v2 0/6] Two -Wclobbered fixes, plus other cleanup

[Philippe Mathieu-Daudé]

On 6/11/22 22:28, Richard Henderson wrote:
> Stefan reported for accel/tcg, and I reproduced on Ubuntu 22.04.
> 
> Changes for v2:
>    * Incorporate suggested changes to nanomips.c (phil, balaton).

Thanks for v2, it was an optional review change request from my
side, but I appreciate the change, the result is cleaner.

> Richard Henderson (6):
>    disas/nanomips: Move setjmp into nanomips_dis
>    disas/nanomips: Merge insn{1,2,3} into words[3]
>    disas/nanomips: Split out read_u16
>    disas/nanomips: Tidy read for 48-bit opcodes

I'm queuing the nanomips patches via mips-fixes.

Regards,

Phil.