Re: [PATCH v2 08/10] accel/tcg: Adjust interface of TranslatorOps.breakpoint_check

[Richard Henderson <richard.henderson@linaro.org>]

On 7/17/21 10:52 AM, Peter Maydell wrote:
> On Mon, 12 Jul 2021 at 16:48, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> We don't need the whole CPUBreakpoint structure in the check,
>> only the flags.  Return the instruction length to consolidate
>> the adjustment of db->pc_next.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
>> diff --git a/target/avr/translate.c b/target/avr/translate.c
>> index 8237a03c23..73ff467926 100644
>> --- a/target/avr/translate.c
>> +++ b/target/avr/translate.c
>> @@ -2944,13 +2944,13 @@ static void avr_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
>>       tcg_gen_insn_start(ctx->npc);
>>   }
>>
>> -static bool avr_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
>> -                                    const CPUBreakpoint *bp)
>> +static int avr_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
>> +                                   int bp_flags)
>>   {
>>       DisasContext *ctx = container_of(dcbase, DisasContext, base);
>>
>>       gen_breakpoint(ctx);
>> -    return true;
>> +    return 2; /* minimum instruction length */
> 
> Here we weren't advancing pc_next at all, and now we do.
> 
>> diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
>> index 47c967acbf..c7b9d813c2 100644
>> --- a/target/mips/tcg/translate.c
>> +++ b/target/mips/tcg/translate.c
>> @@ -16190,22 +16190,16 @@ static void mips_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
>>                          ctx->btarget);
>>   }
>>
>> -static bool mips_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
>> -                                     const CPUBreakpoint *bp)
>> +static int mips_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
>> +                                    int bp_flags)
>>   {
>>       DisasContext *ctx = container_of(dcbase, DisasContext, base);
>>
>>       save_cpu_state(ctx, 1);
>>       ctx->base.is_jmp = DISAS_NORETURN;
>>       gen_helper_raise_exception_debug(cpu_env);
>> -    /*
>> -     * The address covered by the breakpoint must be included in
>> -     * [tb->pc, tb->pc + tb->size) in order to for it to be
>> -     * properly cleared -- thus we increment the PC here so that
>> -     * the logic setting tb->size below does the right thing.
>> -     */
>> -    ctx->base.pc_next += 4;
>> -    return true;
>> +
>> +    return 2; /* minimum instruction length */
>>   }
> 
> Here we were advancing by 4 and now advance by 2.
> 
>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
>> index deda0c8a44..8a6bc58572 100644
>> --- a/target/riscv/translate.c
>> +++ b/target/riscv/translate.c
>> @@ -961,20 +961,15 @@ static void riscv_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
>>       tcg_gen_insn_start(ctx->base.pc_next);
>>   }
>>
>> -static bool riscv_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
>> -                                      const CPUBreakpoint *bp)
>> +static int riscv_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
>> +                                     int bp_flags)
>>   {
>>       DisasContext *ctx = container_of(dcbase, DisasContext, base);
>>
>>       tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next);
>>       ctx->base.is_jmp = DISAS_NORETURN;
>>       gen_exception_debug();
>> -    /* The address covered by the breakpoint must be included in
>> -       [tb->pc, tb->pc + tb->size) in order to for it to be
>> -       properly cleared -- thus we increment the PC here so that
>> -       the logic setting tb->size below does the right thing.  */
>> -    ctx->base.pc_next += 4;
>> -    return true;
>> +    return 2; /* minimum instruction length */
>>   }
> 
> Ditto.
> 
> If these are intentional changes (are they bugfixes?) they should be in a
> separate patch.

Yes, they are bug fixes.

r~

> 
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> thanks
> -- PMM
>