Re: [PATCH 02/10] Fix instruction skipping when using software single step in GDBServer

Pedro Alves
2015-11-04T18:08:00Z

On 10/29/2015 05:09 PM, Antoine Tremblay wrote:

> ---
> gdb/gdbserver/linux-low.c | 23 +++++++++++++++--------
> gdb/gdbserver/mem-break.c | 17 +++++++++++++++++
> gdb/gdbserver/mem-break.h | 4 ++++
> 3 files changed, 36 insertions(+), 8 deletions(-)
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 3b6c131..853a289 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -2993,14 +2993,21 @@ linux_wait_1 (ptid_t ptid,
> return ptid_of (current_thread);
> }
>
> - /* If step-over executes a breakpoint instruction, it means a
> - gdb/gdbserver breakpoint had been planted on top of a permanent
> - breakpoint. The PC has been adjusted by
> - check_stopped_by_breakpoint to point at the breakpoint address.
> - Advance the PC manually past the breakpoint, otherwise the
> - program would keep trapping the permanent breakpoint forever. */
> - if (!ptid_equal (step_over_bkpt, null_ptid)
> - && event_child->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
> + /* If step-over executes a breakpoint instruction, in the case of a
> + hardware single step it means a gdb/gdbserver breakpoint had been
> + planted on top of a permanent breakpoint, in the case of a software
> + single step it may just mean that gdbserver hit the reinsert breakpoint.
> + The PC has been adjusted by check_stopped_by_breakpoint to point at
> + the breakpoint address.
> + So in the case of the hardware single step advance the PC manually
> + past the breakpoint and in the case of software single step advance only
> + if it's not the reinsert_breakpoint we are hitting.
> + This avoids that a program would keep trapping a permanent breakpoint
> + forever. */
> + if ((!ptid_equal (step_over_bkpt, null_ptid)
> + && event_child->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT) &&
> + (event_child->stepping ||
> + !reinsert_breakpoint_inserted_here (event_child->stop_pc)))

Formatting isn't right. && and || go at the beginning of the next line.
Unnecessary parens. Like:

if (!ptid_equal (step_over_bkpt, null_ptid)
&& event_child->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT
&& (event_child->stepping
|| !reinsert_breakpoint_inserted_here (event_child->stop_pc)))

Looks good to me with that change.

Thanks,
Pedro Alves