[Qemu-devel] [PATCH v2] target-arm: Fix non-CPU breakpoint handling in arm_debug_excp_handler()

[Qemu-devel] [PATCH v2] target-arm: Fix non-CPU breakpoint handling in arm_debug_excp_handler()

[Sergey Fedorov]

Do not raise a CPU exception if no CPU breakpoint has fired, since
singlestep is also done by generating a debug internal exception. This
fixes a bug with singlestepping in gdbstub.

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
---
This is a v2 of 'target-arm: Fix arm_debug_excp_handler() for singlestep
enabled.'

Changes in v2:
 * Commit subject and body changed
 * Instead of checking for singlestep enabled, CPU breakpoint match checked

 target-arm/op_helper.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index b5db345..6cd54c8 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -917,7 +917,13 @@ void arm_debug_excp_handler(CPUState *cs)
         uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
         bool same_el = (arm_debug_target_el(env) == arm_current_el(env));
 
-        if (cpu_breakpoint_test(cs, pc, BP_GDB)) {
+        /* (1) GDB breakpoints should be handled first.
+         * (2) Do not raise a CPU exception if no CPU breakpoint has fired,
+         * since singlestep is also done by generating a debug internal
+         * exception.
+         */
+        if (cpu_breakpoint_test(cs, pc, BP_GDB)
+            || !cpu_breakpoint_test(cs, pc, BP_CPU)) {
             return;
         }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2] target-arm: Fix non-CPU breakpoint handling in arm_debug_excp_handler()

[Peter Maydell]

On 5 November 2015 at 12:26, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> Do not raise a CPU exception if no CPU breakpoint has fired, since
> singlestep is also done by generating a debug internal exception. This
> fixes a bug with singlestepping in gdbstub.
>
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> ---
> This is a v2 of 'target-arm: Fix arm_debug_excp_handler() for singlestep
> enabled.'
>
> Changes in v2:
>  * Commit subject and body changed
>  * Instead of checking for singlestep enabled, CPU breakpoint match checked

Isn't this fixing singlestep, not non-CPU breakpoints?
(GDB breakpoints were already being checked for and early-returned.)

I'll tweak the commit subject line and apply to target-arm.next.

Incidentally, this only affects gdbstub singlestep for aarch64
in practice, because gdb for 32-bit ARM uses set-breakpoint-and-continue
rather than the singlestep gdbstub protocol command.

>  target-arm/op_helper.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index b5db345..6cd54c8 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -917,7 +917,13 @@ void arm_debug_excp_handler(CPUState *cs)
>          uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
>          bool same_el = (arm_debug_target_el(env) == arm_current_el(env));
>
> -        if (cpu_breakpoint_test(cs, pc, BP_GDB)) {
> +        /* (1) GDB breakpoints should be handled first.
> +         * (2) Do not raise a CPU exception if no CPU breakpoint has fired,
> +         * since singlestep is also done by generating a debug internal
> +         * exception.
> +         */
> +        if (cpu_breakpoint_test(cs, pc, BP_GDB)
> +            || !cpu_breakpoint_test(cs, pc, BP_CPU)) {
>              return;
>          }
>
> --
> 1.9.1
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] target-arm: Fix non-CPU breakpoint handling in arm_debug_excp_handler()

[Sergey Fedorov]

On 06.11.2015 14:57, Peter Maydell wrote:
> On 5 November 2015 at 12:26, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> Do not raise a CPU exception if no CPU breakpoint has fired, since
>> singlestep is also done by generating a debug internal exception. This
>> fixes a bug with singlestepping in gdbstub.
>>
>> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
>> ---
>> This is a v2 of 'target-arm: Fix arm_debug_excp_handler() for singlestep
>> enabled.'
>>
>> Changes in v2:
>>  * Commit subject and body changed
>>  * Instead of checking for singlestep enabled, CPU breakpoint match checked
> Isn't this fixing singlestep, not non-CPU breakpoints?
> (GDB breakpoints were already being checked for and early-returned.)

Sorry for the confusing commit subject, actually, I meant "fix handling
of EXCP_DEBUG in case of no CPU breakpoint fired" :)

>
> I'll tweak the commit subject line and apply to target-arm.next.
>
> Incidentally, this only affects gdbstub singlestep for aarch64
> in practice, because gdb for 32-bit ARM uses set-breakpoint-and-continue
> rather than the singlestep gdbstub protocol command.
>
>>  target-arm/op_helper.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
>> index b5db345..6cd54c8 100644
>> --- a/target-arm/op_helper.c
>> +++ b/target-arm/op_helper.c
>> @@ -917,7 +917,13 @@ void arm_debug_excp_handler(CPUState *cs)
>>          uint64_t pc = is_a64(env) ? env->pc : env->regs[15];
>>          bool same_el = (arm_debug_target_el(env) == arm_current_el(env));
>>
>> -        if (cpu_breakpoint_test(cs, pc, BP_GDB)) {
>> +        /* (1) GDB breakpoints should be handled first.
>> +         * (2) Do not raise a CPU exception if no CPU breakpoint has fired,
>> +         * since singlestep is also done by generating a debug internal
>> +         * exception.
>> +         */
>> +        if (cpu_breakpoint_test(cs, pc, BP_GDB)
>> +            || !cpu_breakpoint_test(cs, pc, BP_CPU)) {
>>              return;
>>          }
>>
>> --
>> 1.9.1
>>
>

Thanks,
Sergey