[PATCH v2 0/2] SIGSEGV fixes

[PATCH v2 0/2] SIGSEGV fixes

[Taylor Simpson]

The Hexagon target was silently failing the SIGSEGV test because
the signal handler was not called.

Patch 1/2 fixes the Hexagon target
Patch 2/2 adds a check that the signal handler is called

**** Changes in v2 ****
Address feedback from Richard Henderson <richard.henderson@linaro.org>
- Replace put_user_* with cpu_st*_data_ra
- Replace get_user_* with cpu_ld*_data_ra
- Treat sig_segv_called as a counter


Taylor Simpson (2):
  Hexagon (target/hexagon) remove put_user_*/get_user_*
  linux-test (tests/tcg/multiarch/linux-test.c) add check

 target/hexagon/op_helper.c       | 39 ++++++++++++++++++---------------------
 tests/tcg/multiarch/linux-test.c |  8 ++++++++
 target/hexagon/hex_common.py     |  2 ++
 3 files changed, 28 insertions(+), 21 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] Hexagon (target/hexagon) remove put_user_*/get_user_*

[Taylor Simpson]

Replace put_user_* with cpu_st*_data_ra
Replace get_user_* with cpu_ld*_data_ra

Since these functions need the PC, we mark load/store instructions
with the A_IMPLICIT_READS_PC attribute in hex_common.py

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
---
 target/hexagon/op_helper.c   | 39 ++++++++++++++++++---------------------
 target/hexagon/hex_common.py |  2 ++
 2 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 4595559..c5b708d 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -17,6 +17,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu.h"
+#include "exec/cpu_ldst.h"
 #include "exec/helper-proto.h"
 #include "fpu/softfloat.h"
 #include "cpu.h"
@@ -140,22 +141,22 @@ void HELPER(debug_check_store_width)(CPUHexagonState *env, int slot, int check)
 
 void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
 {
-    switch (env->mem_log_stores[slot_num].width) {
+    target_ulong pc = env->gpr[HEX_REG_PC];
+    uint8_t width = env->mem_log_stores[slot_num].width;
+    target_ulong va = env->mem_log_stores[slot_num].va;
+
+    switch (width) {
     case 1:
-        put_user_u8(env->mem_log_stores[slot_num].data32,
-                    env->mem_log_stores[slot_num].va);
+        cpu_stb_data_ra(env, va, env->mem_log_stores[slot_num].data32, pc);
         break;
     case 2:
-        put_user_u16(env->mem_log_stores[slot_num].data32,
-                     env->mem_log_stores[slot_num].va);
+        cpu_stw_data_ra(env, va, env->mem_log_stores[slot_num].data32, pc);
         break;
     case 4:
-        put_user_u32(env->mem_log_stores[slot_num].data32,
-                     env->mem_log_stores[slot_num].va);
+        cpu_stl_data_ra(env, va, env->mem_log_stores[slot_num].data32, pc);
         break;
     case 8:
-        put_user_u64(env->mem_log_stores[slot_num].data64,
-                     env->mem_log_stores[slot_num].va);
+        cpu_stq_data_ra(env, va, env->mem_log_stores[slot_num].data64, pc);
         break;
     default:
         g_assert_not_reached();
@@ -393,37 +394,33 @@ static void check_noshuf(CPUHexagonState *env, uint32_t slot)
 static uint8_t mem_load1(CPUHexagonState *env, uint32_t slot,
                          target_ulong vaddr)
 {
-    uint8_t retval;
+    target_ulong pc = env->gpr[HEX_REG_PC];
     check_noshuf(env, slot);
-    get_user_u8(retval, vaddr);
-    return retval;
+    return cpu_ldub_data_ra(env, vaddr, pc);
 }
 
 static uint16_t mem_load2(CPUHexagonState *env, uint32_t slot,
                           target_ulong vaddr)
 {
-    uint16_t retval;
+    target_ulong pc = env->gpr[HEX_REG_PC];
     check_noshuf(env, slot);
-    get_user_u16(retval, vaddr);
-    return retval;
+    return cpu_lduw_data_ra(env, vaddr, pc);
 }
 
 static uint32_t mem_load4(CPUHexagonState *env, uint32_t slot,
                           target_ulong vaddr)
 {
-    uint32_t retval;
+    target_ulong pc = env->gpr[HEX_REG_PC];
     check_noshuf(env, slot);
-    get_user_u32(retval, vaddr);
-    return retval;
+    return cpu_ldl_data_ra(env, vaddr, pc);
 }
 
 static uint64_t mem_load8(CPUHexagonState *env, uint32_t slot,
                           target_ulong vaddr)
 {
-    uint64_t retval;
+    target_ulong pc = env->gpr[HEX_REG_PC];
     check_noshuf(env, slot);
-    get_user_u64(retval, vaddr);
-    return retval;
+    return cpu_ldq_data_ra(env, vaddr, pc);
 }
 
 /* Floating point */
diff --git a/target/hexagon/hex_common.py b/target/hexagon/hex_common.py
index b3b5340..16872a2 100755
--- a/target/hexagon/hex_common.py
+++ b/target/hexagon/hex_common.py
@@ -69,6 +69,8 @@ def add_qemu_macro_attrib(name, attrib):
 def calculate_attribs():
     add_qemu_macro_attrib('fREAD_PC', 'A_IMPLICIT_READS_PC')
     add_qemu_macro_attrib('fTRAP', 'A_IMPLICIT_READS_PC')
+    add_qemu_macro_attrib('fLOAD', 'A_IMPLICIT_READS_PC')
+    add_qemu_macro_attrib('fSTORE', 'A_IMPLICIT_READS_PC')
     add_qemu_macro_attrib('fWRITE_P0', 'A_WRITES_PRED_REG')
     add_qemu_macro_attrib('fWRITE_P1', 'A_WRITES_PRED_REG')
     add_qemu_macro_attrib('fWRITE_P2', 'A_WRITES_PRED_REG')
-- 
2.7.4

[PATCH v2 2/2] linux-test (tests/tcg/multiarch/linux-test.c) add check

[Taylor Simpson]

Add a check that the SIGSEGV handler is called

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
---
 tests/tcg/multiarch/linux-test.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/tcg/multiarch/linux-test.c b/tests/tcg/multiarch/linux-test.c
index c8c6aed..e2d88f8 100644
--- a/tests/tcg/multiarch/linux-test.c
+++ b/tests/tcg/multiarch/linux-test.c
@@ -439,10 +439,14 @@ static void sig_alarm(int sig)
     alarm_count++;
 }
 
+/* Count the number of times handler is called */
+static int sig_segv_called;
+
 static void sig_segv(int sig, siginfo_t *info, void *puc)
 {
     if (sig != SIGSEGV)
         error("signal");
+    sig_segv_called++;
     longjmp(jmp_env, 1);
 }
 
@@ -492,6 +496,10 @@ static void test_signal(void)
         *(volatile uint8_t *)0 = 0;
     }
 
+    if (sig_segv_called == 0) {
+        error("SIGSEGV handler not called");
+    }
+
     act.sa_handler = SIG_DFL;
     sigemptyset(&act.sa_mask);
     act.sa_flags = 0;
-- 
2.7.4

Re: [PATCH v2 1/2] Hexagon (target/hexagon) remove put_user_*/get_user_*

[Richard Henderson]

On 7/14/21 3:55 PM, Taylor Simpson wrote:
> +    target_ulong pc = env->gpr[HEX_REG_PC];
> +    uint8_t width = env->mem_log_stores[slot_num].width;
> +    target_ulong va = env->mem_log_stores[slot_num].va;
> +
> +    switch (width) {
>       case 1:
> -        put_user_u8(env->mem_log_stores[slot_num].data32,
> -                    env->mem_log_stores[slot_num].va);
> +        cpu_stb_data_ra(env, va, env->mem_log_stores[slot_num].data32, pc);

No, you need to pass the host return address, not the guest.
This should be

     uintptr_t ra = GETPC();


r~