This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set
- From: Pedro Alves <palves at redhat dot com>
- To: Doug Evans <xdje42 at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 07 Mar 2014 01:21:59 +0000
- Subject: Re: [PATCH v2 2/5] PR breakpoints/7143 - Watchpoint does not trigger when first set
- Authentication-results: sourceware.org; auth=none
- References: <1394037536-7526-1-git-send-email-palves at redhat dot com> <1394037536-7526-3-git-send-email-palves at redhat dot com> <wrb8usogpn3 dot fsf at sspiff dot org>
On 03/06/2014 12:02 AM, Doug Evans wrote:
> Pedro Alves <palves@redhat.com> writes:
> Hi.
> I took a look at this patch and have some comments.
Thanks!
> First, some nits to get them out of the way:
>
> 1) This can be deleted, remove_status is no longer used:
>
> handle_signal_stop:
> int remove_status = 0;
>
Thanks. I missed this, as the whole block is removed in the
next patch.
> 2) ChangeLog issues:
> - two ChangeLog entries for proceed, one for keep_going is missing
> - ChangeLog entry for process_event_stop_test doesn't match change
> - ChangeLog entry for handle_signal_stop doesn't match change
Fixed.
>
> 3) This looks unusual for long line handling:
>
>> +extern int
>> + breakpoint_address_match (struct address_space *aspace1, CORE_ADDR addr1,
>> + struct address_space *aspace2, CORE_ADDR addr2);
Fixed.
>
> My more meatier comment is that it's hard to reason about the passing
> of parameters from the caller of insert_breakpoints to
> should_be_inserted via global variables (which is essentially what
> you're doing (*1)) because each of them have multiple callers, and some of
> the callers don't appear (AFAICT) immediately related to the task at hand.
>
> (*1) It's not clear from my reading of the patch that there isn't more to
> it than that though.
> I see you clearing step_over_{aspace,address} in handle_inferior_event
> so I'm left wondering if I'm missing something. E.g., is there a reason
> to not clear them immediately after calling insert_breakpoints?
Yes. Say you're in async + bps always-inserted mode, and the
user sets a breakpoint just while GDB is stepping over
a breakpoint. We'd end up inserting all breakpoints, even
the one being stepped over. That's actually a bug at present.
So we should make sure that we don't insert a breakpoint at the
location we're trying to step over, until the step over is
done (or aborted). That's usually until the next event, but
thinking again, it might not be -- say we get a signal while
stepping over the breakpoint. So I simplified "too much"
in v2...
The code that handles stepping to complete non-continuable
watchpoints:
/* If necessary, step over this watchpoint. We'll be back to display
it in a moment. */
if (stopped_by_watchpoint
&& (target_have_steppable_watchpoint
|| gdbarch_have_nonsteppable_watchpoint (gdbarch)))
{
/* At this point, we are stopped at an instruction which has
attempted to write to a piece of memory under control of
a watchpoint. The instruction hasn't actually executed
yet. If we were to evaluate the watchpoint expression
now, we would get the old value, and therefore no change
would seem to have occurred.
In order to make watchpoints work `right', we really need
to complete the memory write, and then evaluate the
watchpoint expression. We do this by single-stepping the
target.
It may not be necessary to disable the watchpoint to stop over
it. For example, the PA can (with some kernel cooperation)
single step over a watchpoint without disabling the watchpoint.
It is far more common to need to disable a watchpoint to step
the inferior over it. If we have non-steppable watchpoints,
we must disable the current watchpoint; it's simplest to
disable all watchpoints and breakpoints. */
int hw_step = 1;
if (!target_have_steppable_watchpoint)
{
remove_breakpoints ();
/* See comment in resume why we need to stop bypassing signals
while breakpoints have been removed. */
target_pass_signals (0, NULL);
}
/* Single step */
hw_step = maybe_software_singlestep (gdbarch, stop_pc);
target_resume (ecs->ptid, hw_step, GDB_SIGNAL_0);
waiton_ptid = ecs->ptid;
if (target_have_steppable_watchpoint)
infwait_state = infwait_step_watch_state;
else
infwait_state = infwait_nonstep_watch_state;
prepare_to_wait (ecs);
return;
}
could also be adjusted to use this scheme, btw. But I
didn't try that, at least yet. We might end up generalizing
a bit further stepping_over_breakpoint_at (stepping_past_instruction_at
in v3) into a "can I insert this breakpoint location" predicate,
but I'd rather not add abstraction when it's not necessary yet.
> If so that needs to be clearly documented.
I've polished the patch further, and extended the comments, trying
to capture the rationale for what we end up with. I think
the result is better and clearer now -- many thanks for the poke.
Let me know if something's still not clear. Here's what the
main comment says:
~~~~~~~~~~~~~~~~~~~~
/* 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.
Given threads that can't run code in the same address space as the
breakpoint's can't really miss the breakpoint, GDB could be taught
to step-over at most one breakpoint per address space (so this info
could move to the address space object if/when GDB is extended).
The set of breakpoints being stepped over will normally be much
smaller than the set of all breakpoints, so a flag in the
breakpoint location structure would be wasteful. A separate list
also saves complexity and run-time, as otherwise we'd have to go
through all breakpoint locations clearing their flag whenever we
start a new sequence. Similar considerations weigh against storing
this info in the thread object. Plus, not all step overs actually
have breakpoint locations -- e.g., stepping past a single-step
breakpoint, or stepping to complete a non-continuable
watchpoint. */
static struct step_over_info step_over_info;
~~~~~~~~~~~~~~~~~~~~
>
> AFAICT (and if I'm wrong hopefully this will at least lead to more
> comments in the code to help clear things up), ideally (in the abstract
> since we don't have lambdas) what we want is a variant of insert_breakpoints
> that takes a predicate as an argument that says whether each bkpt should be
> inserted. Since we can't easily have that, how about this?
>
> void
> insert_breakpoints_except_at (struct address_space *except_aspace,
> CORE_ADDR except_address;
> {
> step_over_aspace = except_aspace;
> step_over_address = except_address
>
> insert_breakpoints ();
>
> step_over_aspace = NULL;
> step_over_address = 0;
> }
>
> ... and not reset step_over_{aspace,address} anywhere else.
Yeah, I agree it'd look better, though it doesn't really work
for the reasons mentioned above. My first attempt actually
went in that direction (it's similar to how gdbserver handles
this), and passed down the "except at" address down to
insert_breakpoints all the way to update_global_location_list.
Now that was getting ugly, to the point I considered switching
away from the current breakpoint location duplication scheme
to instead refcount locations/bp_target_info's. :-P
> Doing it that way would go a long way to helping me, the reader,
> to understand what's going on (but see caveats above).
I hope v3 is clearer now:
https://sourceware.org/ml/gdb-patches/2014-03/msg00166.html
--
Pedro Alves