[66/67] target/arm: Move singlestep check from gen_jmp to gen_goto_tb

Message ID 20190726175032.6769-67-richard.henderson@linaro.org
State New
Headers show
Series
  • target/arm: Convert aa32 base isa to decodetree
Related show

Commit Message

Richard Henderson July 26, 2019, 5:50 p.m.
We miss quite a number of single-step events by having
the check in the wrong place.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/arm/translate.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

-- 
2.17.1

Comments

Peter Maydell July 26, 2019, 6:13 p.m. | #1
On Fri, 26 Jul 2019 at 18:51, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> We miss quite a number of single-step events by having

> the check in the wrong place.

>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  target/arm/translate.c | 16 ++++++----------

>  1 file changed, 6 insertions(+), 10 deletions(-)

>

> diff --git a/target/arm/translate.c b/target/arm/translate.c

> index c2b8b86fd2..9ae9b23823 100644

> --- a/target/arm/translate.c

> +++ b/target/arm/translate.c

> @@ -2740,7 +2740,10 @@ static void gen_goto_ptr(void)

>   */

>  static void gen_goto_tb(DisasContext *s, int n, target_ulong dest)

>  {

> -    if (use_goto_tb(s, dest)) {

> +    if (unlikely(is_singlestepping(s))) {

> +        gen_set_pc_im(s, dest);

> +        gen_singlestep_exception(s);

> +    } else if (use_goto_tb(s, dest)) {

>          tcg_gen_goto_tb(n);

>          gen_set_pc_im(s, dest);

>          tcg_gen_exit_tb(s->base.tb, n);

> @@ -2751,16 +2754,9 @@ static void gen_goto_tb(DisasContext *s, int n, target_ulong dest)

>      s->base.is_jmp = DISAS_NORETURN;

>  }

>

> -static inline void gen_jmp (DisasContext *s, uint32_t dest)

> +static inline void gen_jmp(DisasContext *s, uint32_t dest)

>  {

> -    if (unlikely(is_singlestepping(s))) {

> -        /* An indirect jump so that we still trigger the debug exception.  */

> -        if (s->thumb)

> -            dest |= 1;

> -        gen_bx_im(s, dest);

> -    } else {

> -        gen_goto_tb(s, 0, dest);

> -    }

> +    gen_goto_tb(s, 0, dest);

>  }

>

>  static inline void gen_mulxy(TCGv_i32 t0, TCGv_i32 t1, int x, int y)


I haven't tested but I'm not sure this change is right.
The idea of the current code is that we handle generating
the actual singlestep exception in arm_tr_tb_stop() at the
end, rather than in the process of generating code for the
insn. This change means we now do a gen_singlestep_exception()
in the gen_jmp()/gen_goto_tb() part of the code, but we haven't
removed the handling of it in arm_tr_tb_stop(), so we're now
doing this in two places. Why doesn't the current design work?
And if we need to do something different for the route to
"change the PC via gen_jmp()" why don't we need to do it
also when we change the PC via eg gen_bx_im() ?

(Incidentally, the only places other than gen_jmp()
which call gen_goto_tb() are:
 * the end-of-TB handling of DISAS_NEXT/DISAS_TOO_MANY
 * four places for barrier insns where we use a
   gen_goto_tb to end the TB -- this isn't consistent
   with how we end the TB for other situations...)

thanks
-- PMM
Richard Henderson July 26, 2019, 6:34 p.m. | #2
On 7/26/19 11:13 AM, Peter Maydell wrote:
> On Fri, 26 Jul 2019 at 18:51, Richard Henderson

> <richard.henderson@linaro.org> wrote:

>>

>> We miss quite a number of single-step events by having

>> the check in the wrong place.

>>

>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>> ---

>>  target/arm/translate.c | 16 ++++++----------

>>  1 file changed, 6 insertions(+), 10 deletions(-)

>>

>> diff --git a/target/arm/translate.c b/target/arm/translate.c

>> index c2b8b86fd2..9ae9b23823 100644

>> --- a/target/arm/translate.c

>> +++ b/target/arm/translate.c

>> @@ -2740,7 +2740,10 @@ static void gen_goto_ptr(void)

>>   */

>>  static void gen_goto_tb(DisasContext *s, int n, target_ulong dest)

>>  {

>> -    if (use_goto_tb(s, dest)) {

>> +    if (unlikely(is_singlestepping(s))) {

>> +        gen_set_pc_im(s, dest);

>> +        gen_singlestep_exception(s);

>> +    } else if (use_goto_tb(s, dest)) {

>>          tcg_gen_goto_tb(n);

>>          gen_set_pc_im(s, dest);

>>          tcg_gen_exit_tb(s->base.tb, n);

>> @@ -2751,16 +2754,9 @@ static void gen_goto_tb(DisasContext *s, int n, target_ulong dest)

>>      s->base.is_jmp = DISAS_NORETURN;

>>  }

>>

>> -static inline void gen_jmp (DisasContext *s, uint32_t dest)

>> +static inline void gen_jmp(DisasContext *s, uint32_t dest)

>>  {

>> -    if (unlikely(is_singlestepping(s))) {

>> -        /* An indirect jump so that we still trigger the debug exception.  */

>> -        if (s->thumb)

>> -            dest |= 1;

>> -        gen_bx_im(s, dest);

>> -    } else {

>> -        gen_goto_tb(s, 0, dest);

>> -    }

>> +    gen_goto_tb(s, 0, dest);

>>  }

>>

>>  static inline void gen_mulxy(TCGv_i32 t0, TCGv_i32 t1, int x, int y)

> 

> I haven't tested but I'm not sure this change is right.

> The idea of the current code is that we handle generating

> the actual singlestep exception in arm_tr_tb_stop() at the

> end, rather than in the process of generating code for the

> insn. This change means we now do a gen_singlestep_exception()

> in the gen_jmp()/gen_goto_tb() part of the code, but we haven't

> removed the handling of it in arm_tr_tb_stop(), so we're now

> doing this in two places. Why doesn't the current design work?


Note that the existing gen_goto_tb does not test for single-step, and always
emits NORETURN code, either via tcg_gen_goto_tb() or
tcg_gen_lookup_and_goto_ptr().  The singlestep exception is neither generated
nor would it be reachable.

Another way to do handle this would be to test single-step but set DISAS_JUMP
instead of actually generate the singlestep exception in gen_goto_tb.  Do you
prefer that method?

> And if we need to do something different for the route to

> "change the PC via gen_jmp()" why don't we need to do it

> also when we change the PC via eg gen_bx_im() ?


See patch 67.

>  * four places for barrier insns where we use a

>    gen_goto_tb to end the TB -- this isn't consistent

>    with how we end the TB for other situations...)


Fixing this sounds like a good cleanup.


r~

Patch

diff --git a/target/arm/translate.c b/target/arm/translate.c
index c2b8b86fd2..9ae9b23823 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -2740,7 +2740,10 @@  static void gen_goto_ptr(void)
  */
 static void gen_goto_tb(DisasContext *s, int n, target_ulong dest)
 {
-    if (use_goto_tb(s, dest)) {
+    if (unlikely(is_singlestepping(s))) {
+        gen_set_pc_im(s, dest);
+        gen_singlestep_exception(s);
+    } else if (use_goto_tb(s, dest)) {
         tcg_gen_goto_tb(n);
         gen_set_pc_im(s, dest);
         tcg_gen_exit_tb(s->base.tb, n);
@@ -2751,16 +2754,9 @@  static void gen_goto_tb(DisasContext *s, int n, target_ulong dest)
     s->base.is_jmp = DISAS_NORETURN;
 }
 
-static inline void gen_jmp (DisasContext *s, uint32_t dest)
+static inline void gen_jmp(DisasContext *s, uint32_t dest)
 {
-    if (unlikely(is_singlestepping(s))) {
-        /* An indirect jump so that we still trigger the debug exception.  */
-        if (s->thumb)
-            dest |= 1;
-        gen_bx_im(s, dest);
-    } else {
-        gen_goto_tb(s, 0, dest);
-    }
+    gen_goto_tb(s, 0, dest);
 }
 
 static inline void gen_mulxy(TCGv_i32 t0, TCGv_i32 t1, int x, int y)