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

Doug Evans
2014-03-16T03:41:59Z

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.