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

[Sergey Fedorov]

On 13.10.2015 03:13, Richard Henderson wrote:
> 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.

I see your point, but what I am actually concerned about is the
following scenario.

Lets suppose we are going to remove a breakpoint. Eventually we can get
the following function call stack:

#0  tb_invalidate_phys_page_range()
#1  tb_invalidate_phys_addr()
#2  breakpoint_invalidate()
#3  cpu_breakpoint_remove_by_ref()
...

Then, if we come across a zero-sized TB then 'tb_start' would be equal
to 'tb_end'. That is not a big deal unless 'start' is not equal to
'tb_start'. Otherwise, "!(tb_end <= start || tb_start >= end)" condition
check will fail and that TB won't get invalidated as it should be. As I
can see this is a case when we try to remove a breakpoint which is
happened to be set at the very first instruction of a TB. So we either
need to change the condition in tb_invalidate_phys_page_range() or do
the PC advancement trick during translation, no matter can instructions
cross a page boundary or not.

Best regards,
Sergey