| Message ID | 20190726175032.6769-67-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Convert aa32 base isa to decodetree | expand |
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
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~
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)
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