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
