[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH 66/67] target/arm: Move singlestep check from gen_
From: |
Richard Henderson |
Subject: |
Re: [Qemu-arm] [PATCH 66/67] target/arm: Move singlestep check from gen_jmp to gen_goto_tb |
Date: |
Fri, 26 Jul 2019 11:34:02 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
On 7/26/19 11:13 AM, Peter Maydell wrote:
> On Fri, 26 Jul 2019 at 18:51, Richard Henderson
> <address@hidden> wrote:
>>
>> We miss quite a number of single-step events by having
>> the check in the wrong place.
>>
>> Signed-off-by: Richard Henderson <address@hidden>
>> ---
>> 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~
- [Qemu-arm] [PATCH 57/67] target/arm: Convert T16, nop hints, (continued)
- [Qemu-arm] [PATCH 57/67] target/arm: Convert T16, nop hints, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 51/67] target/arm: Convert T16 branch and exchange, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 60/67] target/arm: Convert T16, Miscellaneous 16-bit instructions, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 53/67] target/arm: Convert T16 adjust sp (immediate), Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 56/67] target/arm: Convert T16, Reverse bytes, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 58/67] target/arm: Convert T16, push and pop, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 64/67] target/arm: Convert T16, long branches, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 61/67] target/arm: Convert T16, shift immediate, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 66/67] target/arm: Move singlestep check from gen_jmp to gen_goto_tb, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 59/67] target/arm: Convert T16, Conditional branches, Supervisor call, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 63/67] target/arm: Convert T16, Unconditional branch, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 62/67] target/arm: Convert T16, load (literal), Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 65/67] target/arm: Clean up disas_thumb_insn, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 55/67] target/arm: Convert T16, Change processor state, Richard Henderson, 2019/07/26
- [Qemu-arm] [PATCH 67/67] target/arm: Merge gen_bx_im into trans_BLX_i, Richard Henderson, 2019/07/26
- Re: [Qemu-arm] [Qemu-devel] [PATCH 00/67] target/arm: Convert aa32 base isa to decodetree, no-reply, 2019/07/26