Re: [Qemu-devel] [PATCH v13 18/26] target/arm: [tcg] Port to breakpoint_check

["Lluís Vilanova" <vilanova@ac.upc.edu>]

Richard Henderson writes:

> On 07/13/2017 11:26 PM, Lluís Vilanova wrote:
>> Incrementally paves the way towards using the generic instruction translation
>> loop.
>> 
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> Reviewed-by: Richard Henderson <rth@twiddle.net>
>> ---
>> target/arm/translate.c |   53 +++++++++++++++++++++++++++++++-----------------
>> 1 file changed, 34 insertions(+), 19 deletions(-)
>> 
>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>> index b9183fc511..55bef09739 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -11917,6 +11917,33 @@ static void arm_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
>> #endif
>> }
>> +static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState
>> *cpu,
>> +                                    const CPUBreakpoint *bp)
>> +{
>> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>> +
>> +    if (bp->flags & BP_CPU) {
>> +        gen_set_condexec(dc);
>> +        gen_set_pc_im(dc, dc->pc);
>> +        gen_helper_check_breakpoints(cpu_env);
>> +        /* End the TB early; it's likely not going to be executed */
>> +        dc->base.is_jmp = DISAS_UPDATE;
>> +    } else {
>> +        gen_exception_internal_insn(dc, 0, EXCP_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.  */
>> +        /* TODO: Advance PC by correct instruction length to
>> +         * avoid disassembler error messages */
>> +        dc->pc += 2;
>> +        dc->base.is_jmp = DISAS_NORETURN;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> /* generate intermediate code for basic block 'tb'.  */
>> void gen_intermediate_code(CPUState *cs, TranslationBlock *tb)
>> {
>> @@ -11965,28 +11992,16 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb)
>> if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>> CPUBreakpoint *bp;
>> QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
>> -                if (bp->pc == dc->pc) {
>> -                    if (bp->flags & BP_CPU) {
>> -                        gen_set_condexec(dc);
>> -                        gen_set_pc_im(dc, dc->pc);
>> -                        gen_helper_check_breakpoints(cpu_env);
>> -                        /* End the TB early; it's likely not going to be executed */
>> -                        dc->base.is_jmp = DISAS_UPDATE;

> Oh I see what you're doing there in the main loop.
> And I see that you're copying existing behaviour.

> That said, I do wonder if there's a better way.

> Looking back at the original patch (5d98bf8f), there do not
> seem to have been any other side effects intended; simply
> "single step" any insn for which this bp condition is met.

> Another way to handle this would be if we could adjust max_insns = num_insns.
> That would cause the loop to exit after the current insn, with DISAS_TOO_MANY if
> nothing else.

But adjusting num_insns (I guess passed as a pointer to breakpoint_check()) will
stop *after* translating the instructions, instead of before as some targets do
currently.

I'd really prefer to keep the current behaviour, and if in the end we see that
the use of DISAS_NORETURN in all targets is safe to switch into DISAS_TOO_MANY,
we can do so in a future patch.

Cheers,
  Lluis