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

[Pedro Alves <palves at redhat dot com>]

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