On 18.09.2015 19:36, Peter Maydell wrote:
> On 18 September 2015 at 17:33, Sergey Fedorov <serge.f...@gmail.com> wrote:
>> 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...
> Well, typically we'll take the BP exception in the helper function.
> There's nothing inherently wrong with translating further code
> after this insn, but it's usually not going to be executed, so
> we might as well end the TB early.
>
Thank, it is clear now. What about getting rid of "goto done generating"
and always translate one insn to update PC accordingly? Sometimes in_asm
log complains about that when disassembler and translator disagree about
the instruction size.
Best,
Sergey
