| Message ID | 1446726361-18328-1-git-send-email-serge.fdrv@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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; }
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(-)