Re: [Qemu-devel] [PULL 04/26] target-*: Introduce and use cpu_breakpoint_test

[Richard Henderson]

On 10/10/2015 12:34 AM, Sergey Fedorov wrote:
> > @@ -2936,6 +2927,10 @@ static inline void 
> > gen_intermediate_code_internal(AlphaCPU *cpu,
> >           tcg_gen_insn_start(ctx.pc);
> >           num_insns++;
> >
> > +        if (unlikely(cpu_breakpoint_test(cs, ctx.pc, BP_ANY))) {
> > +            gen_excp(&ctx, EXCP_DEBUG, 0);
> > +            break;
> > +        }
>
> Actually, control logic has changed here. The old code used a break
> statement to exit from QTAILQ_FOREACH loop and continue with instruction
> translation thus translating at least one instruction. The break
> statement in the new code makes exit from the translation loop itself,
> effectively producing zero-length TB which won't get invalidated when
> clearing the breakpoint. Seems like we should remove the break statement
> here and in similar cases below, right?

Why do you believe that a zero-length TB won't be cleared?
The TB still has a start address, which is contained within
a given page, which is invalidated.

Some target-*/translate.c takes care to advance the PC, but I believe that this 
is only required when the breakpoint instruction *itself* could span a page 
boundary.  I.e. the TB needs to be marked to span two pages.  This situation 
can never be true for many RISC targets.

We did discuss this exact situation during review of the patch set, though it's 
probably true that there are outstanding errors in some translators.


r~