Re: [PATCH v3 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set

[Doug Evans <xdje42@gmail.com>]

On Thu, Mar 6, 2014 at 5:10 PM, Pedro Alves <palves@redhat.com> wrote:
> [...]
> gdb/
> 2014-03-06  Pedro Alves  <palves@redhat.com>
>
>         PR breakpoints/7143
>         * breakpoint.c (should_be_inserted): Don't insert breakpoints that
>         are being stepped over.
>         (breakpoint_address_match): Make extern.
>         * breakpoint.h (breakpoint_address_match): New declaration.
>         * inferior.h (stepping_past_instruction_at): New declaration.
>         * infrun.c (struct step_over_info): New type.
>         (step_over_info): New global.
>         (set_step_over_info, clear_step_over_info)
>         (stepping_past_instruction_at): New functions.
>         (handle_inferior_event): Clear the step-over info when
>         trap_expected is cleared.
>         (resume): Remove now stale comment.
>         (clear_proceed_status): Clear step-over info.
>         (proceed): Adjust step-over handling to set or clear the step-over
>         info instead of removing all breakpoints.
>         (handle_signal_stop): When setting up a thread-hop, don't remove
>         breakpoints here.
>         (stop_stepping): Clear step-over info.
>         (keep_going): Adjust step-over handling to set or clear step-over
>         info and then always inserting breakpoints, instead of removing
>         all breakpoints when stepping over one.
>
> gdb/testsuite/
> 2014-03-06  Pedro Alves  <palves@redhat.com>
>
>         PR breakpoints/7143
>         * gdb.base/watchpoint.exp: Mention bugzilla bug number instead of
>         old gnats gdb/38.  Remove kfail.  Adjust to use gdb_test instead
>         of gdb_test_multiple.
>         * gdb.cp/annota2.exp: Remove kfail for gdb/38.
>         * gdb.cp/annota3.exp: Remove kfail for gdb/38.

Hi.  Ok by me with one nit.

I do like the functions that set/clear the step-over info.

> [...]
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index bd55505..b7c02d7 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -977,6 +977,76 @@ static CORE_ADDR singlestep_pc;
>  static ptid_t saved_singlestep_ptid;
>  static int stepping_past_singlestep_breakpoint;
>
> +/* Info about an instruction that is being stepped over.  Invalid if
> +   ASPACE is NULL.  */
> +
> +struct step_over_info
> +{
> +  /* The instruction's address space.  */
> +  struct address_space *aspace;
> +
> +  /* The instruction's address.  */
> +  CORE_ADDR address;
> +};
> +
> +/* The step-over info of the location that is being stepped over.
> +
> +   Note that with async/breakpoint always-inserted mode, a user might
> +   set a new breakpoint/watchpoint/etc. exactly while a breakpoint is
> +   being stepped over.  As setting a new breakpoint inserts all
> +   breakpoints, we need to make sure the breakpoint being stepped over
> +   isn't inserted then.  We do that by only clearing the step-over
> +   info when the step-over is actually finished (or aborted).
> +
> +   Presently GDB can only step over a breakpoint at any given time.

I realize this is nitpicky, but can you reword this as:

Presently GDB can only step over one breakpoint at any given time.

I realize it's saying the same thing, but when I first read it I had
to read it a couple of times to make sure I read it right.  Maybe it's
just me.