On 08.10.2015 21:40, Peter Maydell wrote:
> On 28 September 2015 at 11:07, Sergey Fedorov <serge.f...@gmail.com> wrote:
>> A QEMU breakpoint match is not definitely an architectural breakpoint
>> match. If an exception is generated unconditionally during translation,
>> it is hardly possible to ignore it in the debug exception handler.
>>
>> Generate a call to a helper to check CPU breakpoints and raise an
>> exception only if any breakpoint matches architecturally.
>>
>> Signed-off-by: Sergey Fedorov <serge.f...@gmail.com>
>> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
>> index ec0936c..426229f 100644
>> --- a/target-arm/translate-a64.c
>> +++ b/target-arm/translate-a64.c
>> @@ -11082,11 +11082,14 @@ 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);
>> + } else {
>> + gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> We shouldn't just continue here, because now we'll try to generate the
> code for the instruction even in the "we know this will always be a bp"
> case. Also, you've dropped the "advance PC" part which we need so this
> TB is not zero length.
Actually, I was going to do the same way as some architectures (e.g.
alpha) did: always translate one instruction so that TB size is
determined by actual instruction decoding. At least, it makes sense for
AArch32 where we can have 16/32-bit insns. If we advance PC incorrectly,
we will get "Disassembler disagrees with translator over instruction
decoding" warning messages when in_asm log enabled. I can rewrite it
with PC advancement, but at least, I would like to change the
advancement to 4 bytes for A64 translation.
Best regards,
Sergey
>
>> + }
>> + /* End the TB early; it's likely not going to be
>> executed */
>> + dc->is_jmp = DISAS_UPDATE;
> gen_exception_internal_insn sets is_jmp to DISAS_EXC, and then this
> overrides it; so this line should go inside the "is this a BP_CPU bp?"
> if clause. (I think the effect is just that we pointlessly generate some
> unreachable code after the exception generating call.)
>
>> + break;
>> }
>> }
>> }
>> @@ -11209,7 +11212,6 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>> }
>> }
>>
>> -done_generating:
>> gen_tb_end(tb, num_insns);
>>
>> #ifdef DEBUG_DISAS
>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>> index 84a21ac..405d6d0 100644
>> --- a/target-arm/translate.c
>> +++ b/target-arm/translate.c
>> @@ -11328,11 +11328,14 @@ static inline void
>> gen_intermediate_code_internal(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);
>> + } else {
>> + gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
>> + }
>> + /* End the TB early; it's likely not going to be
>> executed */
>> + dc->is_jmp = DISAS_UPDATE;
> Similar comments here.
>
> Annoying corner case which I don't think we need to handle necessarily:
> if you set a breakpoint on a 32-bit Thumb instruction which spans a page
> boundary, and the second page is not present, we will end up taking the
> page fault when I think we should take the breakpoint. I can't think
> of a way to get that right, so just commenting that it isn't handled
> right would do.
>
>> + break;
>> }
>> }
>> }
>> --
>> 1.9.1
>>
> Otherwise I think this is the right approach.
>
> thanks
> -- PMM
