On 18.09.2015 17:14, Peter Maydell wrote:
> On 18 September 2015 at 15:07, Sergey Fedorov <serge.f...@gmail.com> wrote:
>> On 18.09.2015 16:50, Peter Maydell wrote:
>>> On 14 September 2015 at 11:51, Sergey Fedorov <serge.f...@gmail.com> wrote:
>>>> --- a/target-arm/translate-a64.c
>>>> +++ b/target-arm/translate-a64.c
>>>> @@ -11000,11 +11000,13 @@ void gen_intermediate_code_internal_a64(ARMCPU
>>>> *cpu,
>>>> if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>>>> QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
>>>> if (bp->pc == dc->pc) {
>>>> - gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>>>> - /* Advance PC so that clearing the breakpoint will
>>>> - invalidate this TB. */
>>>> - dc->pc += 2;
>>>> - goto done_generating;
>>>> + if (bp->flags & BP_CPU) {
>>>> + gen_helper_check_breakpoints(cpu_env);
>>>> + break;
>>>> + } else {
>>>> + gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>>>> + goto done_generating;
>>>> + }
>>> You seem to have dropped the "advance the PC" code -- why is that ok?
>>>
>> I also dropped the immediately following goto statement. With these
>> changes PC is advanced in the same way as it happens during normal
>> translation. That is because we actually have to do the instruction
>> translation process here to support the case when a breakpoint with
>> matching PC is architecturally mismatched. As I understand, that
>> "advance the PC" code was necessary to produce a TB with non-zero size
>> so that it can be invalidated later when we clear the breakpoint.
> OK, that makes sense for the BP_CPU case but you still have the
> "goto done_generating;" in the else clause...
>
> Also, should we maybe make this TB be only one insn long even for
> the BP_CPU case? It seems like in the common case we will only
> execute one insn.
>
Right, I have to fix this PC advancement. But I can't think of why we
will only execute one insn...
Best,
Sergey
