RISC-V: fix single stepping over ret and other branching instructions

Commit Message

Fabien Chouteau March 22, 2019, 11:22 a.m. UTC

This patch introduces wrappers around the tcg_gen_exit_tb() and
tcg_gen_lookup_and_goto_ptr() functions that handle single stepping,
i.e. call gen_exception_debug() when single stepping is enabled.

Theses functions are then used instead of the originals, bringing single
stepping handling in places where it was previously ignored such as jalr
and system branch instructions (ecall, mret, sret, etc.).

Signed-off-by: Fabien Chouteau <[e-mail]>
---
 .../riscv/insn_trans/trans_privileged.inc.c   |  8 +++---
 target/riscv/insn_trans/trans_rvi.inc.c       |  6 ++--
 target/riscv/translate.c                      | 28 +++++++++++++++----
 3 files changed, 29 insertions(+), 13 deletions(-)

Comments

Richard Henderson March 22, 2019, 3:24 p.m. UTC | #1

On 3/22/19 4:22 AM, Fabien Chouteau wrote:
> +/* Wrapper around tcg_gen_exit_tb that handles single stepping */
> +static void exit_tb(DisasContext *ctx, TranslationBlock *tb, unsigned idx)
> +{
> +    if (ctx->base.singlestep_enabled) {
> +        gen_exception_debug();
> +    } else {
> +        tcg_gen_exit_tb(tb, idx);
> +    }
> +}

You should remove the TB and idx parameters here and pass NULL, 0 to
tcg_gen_exit_tb.

> @@ -138,14 +158,10 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
>          /* chaining is only allowed when the jump is to the same page */
>          tcg_gen_goto_tb(n);
>          tcg_gen_movi_tl(cpu_pc, dest);
> -        tcg_gen_exit_tb(ctx->base.tb, n);
> +        exit_tb(ctx, ctx->base.tb, n);

Because this is the only non-zero use, and it is already protected by
use_goto_tb, which includes the single-step check.

Because goto_tb(n) must be paired with exit_tb(tb, n), and vice-versa.


r~

Fabien Chouteau March 25, 2019, 11:48 a.m. UTC | #2

Thanks Richard,

I sent a version 2.

On 22/03/2019 16:24, Richard Henderson wrote:
> On 3/22/19 4:22 AM, Fabien Chouteau wrote:
>> +/* Wrapper around tcg_gen_exit_tb that handles single stepping */
>> +static void exit_tb(DisasContext *ctx, TranslationBlock *tb, unsigned idx)
>> +{
>> +    if (ctx->base.singlestep_enabled) {
>> +        gen_exception_debug();
>> +    } else {
>> +        tcg_gen_exit_tb(tb, idx);
>> +    }
>> +}
> 
> You should remove the TB and idx parameters here and pass NULL, 0 to
> tcg_gen_exit_tb.
> 
>> @@ -138,14 +158,10 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
>>          /* chaining is only allowed when the jump is to the same page */
>>          tcg_gen_goto_tb(n);
>>          tcg_gen_movi_tl(cpu_pc, dest);
>> -        tcg_gen_exit_tb(ctx->base.tb, n);
>> +        exit_tb(ctx, ctx->base.tb, n);
> 
> Because this is the only non-zero use, and it is already protected by
> use_goto_tb, which includes the single-step check.
> 
> Because goto_tb(n) must be paired with exit_tb(tb, n), and vice-versa.
> 
> 
> r~
>

Patch

diff --git a/target/riscv/insn_trans/trans_privileged.inc.c b/target/riscv/insn_trans/trans_privileged.inc.c
index acb605923e..fe660ab3dd 100644
--- a/target/riscv/insn_trans/trans_privileged.inc.c
+++ b/target/riscv/insn_trans/trans_privileged.inc.c
@@ -22,7 +22,7 @@  static bool trans_ecall(DisasContext *ctx, arg_ecall *a)
 {
     /* always generates U-level ECALL, fixed in do_interrupt handler */
     generate_exception(ctx, RISCV_EXCP_U_ECALL);
-    tcg_gen_exit_tb(NULL, 0); /* no chaining */
+    exit_tb(ctx, NULL, 0); /* no chaining */
     ctx->base.is_jmp = DISAS_NORETURN;
     return true;
 }
@@ -30,7 +30,7 @@  static bool trans_ecall(DisasContext *ctx, arg_ecall *a)
 static bool trans_ebreak(DisasContext *ctx, arg_ebreak *a)
 {
     generate_exception(ctx, RISCV_EXCP_BREAKPOINT);
-    tcg_gen_exit_tb(NULL, 0); /* no chaining */
+    exit_tb(ctx, NULL, 0); /* no chaining */
     ctx->base.is_jmp = DISAS_NORETURN;
     return true;
 }
@@ -47,7 +47,7 @@  static bool trans_sret(DisasContext *ctx, arg_sret *a)
 
     if (has_ext(ctx, RVS)) {
         gen_helper_sret(cpu_pc, cpu_env, cpu_pc);
-        tcg_gen_exit_tb(NULL, 0); /* no chaining */
+        exit_tb(ctx, NULL, 0); /* no chaining */
         ctx->base.is_jmp = DISAS_NORETURN;
     } else {
         return false;
@@ -68,7 +68,7 @@  static bool trans_mret(DisasContext *ctx, arg_mret *a)
 #ifndef CONFIG_USER_ONLY
     tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next);
     gen_helper_mret(cpu_pc, cpu_env, cpu_pc);
-    tcg_gen_exit_tb(NULL, 0); /* no chaining */
+    exit_tb(ctx, NULL, 0); /* no chaining */
     ctx->base.is_jmp = DISAS_NORETURN;
     return true;
 #else
diff --git a/target/riscv/insn_trans/trans_rvi.inc.c b/target/riscv/insn_trans/trans_rvi.inc.c
index d420a4d8b2..44ae573768 100644
--- a/target/riscv/insn_trans/trans_rvi.inc.c
+++ b/target/riscv/insn_trans/trans_rvi.inc.c
@@ -60,7 +60,7 @@  static bool trans_jalr(DisasContext *ctx, arg_jalr *a)
     if (a->rd != 0) {
         tcg_gen_movi_tl(cpu_gpr[a->rd], ctx->pc_succ_insn);
     }
-    tcg_gen_lookup_and_goto_ptr();
+    lookup_and_goto_ptr(ctx);
 
     if (misaligned) {
         gen_set_label(misaligned);
@@ -483,7 +483,7 @@  static bool trans_fence_i(DisasContext *ctx, arg_fence_i *a)
      * however we need to end the translation block
      */
     tcg_gen_movi_tl(cpu_pc, ctx->pc_succ_insn);
-    tcg_gen_exit_tb(NULL, 0);
+    exit_tb(ctx, NULL, 0);
     ctx->base.is_jmp = DISAS_NORETURN;
     return true;
 }
@@ -504,7 +504,7 @@  static bool trans_fence_i(DisasContext *ctx, arg_fence_i *a)
     gen_io_end(); \
     gen_set_gpr(a->rd, dest); \
     tcg_gen_movi_tl(cpu_pc, ctx->pc_succ_insn); \
-    tcg_gen_exit_tb(NULL, 0); \
+    exit_tb(ctx, NULL, 0); \
     ctx->base.is_jmp = DISAS_NORETURN; \
     tcg_temp_free(source1); \
     tcg_temp_free(csr_store); \
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 049fa65c66..c438400b19 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -109,6 +109,26 @@  static void gen_exception_debug(void)
     tcg_temp_free_i32(helper_tmp);
 }
 
+/* Wrapper around tcg_gen_exit_tb that handles single stepping */
+static void exit_tb(DisasContext *ctx, TranslationBlock *tb, unsigned idx)
+{
+    if (ctx->base.singlestep_enabled) {
+        gen_exception_debug();
+    } else {
+        tcg_gen_exit_tb(tb, idx);
+    }
+}
+
+/* Wrapper around tcg_gen_lookup_and_goto_ptr that handles single stepping */
+static void lookup_and_goto_ptr(DisasContext *ctx)
+{
+    if (ctx->base.singlestep_enabled) {
+        gen_exception_debug();
+    } else {
+        tcg_gen_lookup_and_goto_ptr();
+    }
+}
+
 static void gen_exception_illegal(DisasContext *ctx)
 {
     generate_exception(ctx, RISCV_EXCP_ILLEGAL_INST);
@@ -138,14 +158,10 @@  static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
         /* chaining is only allowed when the jump is to the same page */
         tcg_gen_goto_tb(n);
         tcg_gen_movi_tl(cpu_pc, dest);
-        tcg_gen_exit_tb(ctx->base.tb, n);
+        exit_tb(ctx, ctx->base.tb, n);
     } else {
         tcg_gen_movi_tl(cpu_pc, dest);
-        if (ctx->base.singlestep_enabled) {
-            gen_exception_debug();
-        } else {
-            tcg_gen_lookup_and_goto_ptr();
-        }
+        lookup_and_goto_ptr(ctx);
     }
 }