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

[Richard Henderson]

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 <address@hidden>
> Reviewed-by: Richard Henderson <address@hidden>
> ---
>   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.


r~