This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch 1/4] Fix hw watchpoints: [x86*] repeated rwatch output
> > I don't understand why you clear the bit when removing the watchpoint,
> > however. Can you explain?
>
> This is a more general question about sanity checking the DR registers
> in general. The registers apparently can be also changed by the
> inferior itself.
The thing that I don't get is why this is useful at this point in time.
Since you're going to reset the DR register anyway when you re-insert...
Oh, but wait: You may not re-insert the watchpoint later (eg: If you
delete or disable the watchpoint after you hit it). Would we have
a reproducible bug if this code wasn't there? But even that shouldn't
a problem, since I assume that we've disabled the watchpoint anyway.
i386_stopped_data_address does verify that the associated DR is active
before reporting a hit:
if (I386_DR_WATCH_HIT (i)
/* This second condition makes sure DRi is set up for a data
watchpoint, not a hardware breakpoint. The reason is
that GDB doesn't call the target_stopped_data_address
method except for data watchpoints. In other words, I'm
being paranoiac. */
&& I386_DR_GET_RW_LEN (i) != 0
/* This third condition makes sure DRi is not vacant, this
avoids false positives in windows-nat.c. */
&& !I386_DR_VACANT (i))
> gdb/
> 2009-10-03 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> Fix repeated rwatch output.
> * amd64-linux-nat.c (amd64_linux_dr_set, amd64_linux_dr_set_control)
> (amd64_linux_dr_set_addr, amd64_linux_dr_reset_addr)
> (amd64_linux_dr_get_status): New comments.
> (amd64_linux_dr_unset_status): New function.
> (_initialize_amd64_linux_nat): Install it.
> * i386-linux-nat.c (i386_linux_dr_get, i386_linux_dr_set)
> (i386_linux_dr_set_control, i386_linux_dr_set_addr)
> (i386_linux_dr_reset_addr, i386_linux_dr_get_status): New comments.
> (i386_linux_dr_unset_status): New function.
> (_initialize_i386_linux_nat): Install it.
> * i386-nat.c (I386_DR_WATCH_MASK): New macro.
> (I386_DR_WATCH_HIT): Use I386_DR_WATCH_MASK.
> (i386_insert_aligned_watchpoint, i386_remove_aligned_watchpoint): Call
> i386_dr_low.unset_status.
> * i386-nat.h (struct i386_dr_low_type): Extend comments for
> set_control, set_addr, reset_addr and get_status. New unset_status.
> * breakpoint.c (update_watchpoint): Comment resetting watchpoints.
Looks mostly OK. I just still do not understand what would happen if
we removed the hunk in i386_remove_aligned_watchpoint.
> gdb/testsuite/
> 2009-10-03 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> * gdb.base/watchpoint-hw.c: New variable dummy, assign dummies to it.
> * gdb.base/watchpoint-hw.exp: Test rwatch does not repeat. Replace
> `start' by ${test}.exp. Replace gdb_compile by prepare_for_testing.
> Remove variable `binfile'. Use clean_restart and runto_main for the
> new testcase part.
> + Even with `set breakpoint always-inserted on' the watchpoints are
> + removed + inserted on each stop here. Normal breakpoinst (bp_breakpoint;
^^
> + not processed here) must never be removed to not to be missed in non-stop
> + mode by running threads.
Suggest re-phrasing to avoid the "to not to". For instance:
Normal breakpoints must never be removed because they might be
missed by a running thread when debugging in non-stop mode.
> Hardware watchpoints (is_hardware_watchpoina;
^^^^
> + processed here) are specific for each LWP (as they are stored to tasks's
are specific to each LWP
> + hardware debug registers) and therefore such LWP must be stopped first to be
> + able to modify its hardware watchpoints.
Suggest anothre re-phrasing again to avoid the "as they are stored to
tasks's [...]":
On the other hand, hardware watchpoints (is_hardware_watchpoint;
processed here) are specific to each LWP since they are stored in
each LWP's hardware debug registers. Therefore, such LWP must be
stopped first in order to be able to modify its hardware watchpoints.
> + Hardware watchpoints (bp_read_watchpoint and bp_access_watchpoint; false hit
> + by bp_hardware_watchpoint is not user-visible - its hit is suppressed if the
> + memory content has not changed) must be reset exactly once after being
> + presented to the user, neither sooner nor later.
The digressions make it hard to see where you're trying to go. I'd
suggest moving it to later. For instance;
Hardware watchpoints must be reset exactly once after being presented
to the user, neither sooner nor later. Note that [...].
I'd even get rid of the digression. It seems to me that this is only mindly
related to the discussion, and that piece of information would be better
located near the actual code where it matters.
I would be a little more specific as to what "reset" means in this case
(removed/re-inserted?), and explain why it cannot be done sooner, nor later.
For instance:
Hardware watchpoints must be reset exactly once after being presented
to the user. It cannot be done sooner, because it would reset the
data used to present the watchpoint hit to the user. And it must
not be done later because (?).
> + The reset point must comply to these rules:
The following constraints influence the location where we can reset
hardware watchpoints:
> + * target_stopped_by_watchpoint and target_stopped_data_address are called
> + many times in GDB during one stop.
several times when GDB stops.
> + * Multiple hardware watchpoints can be hit on the first stop while GDB
> + presents only one reason on each stop event to the user. The other hits
> + will get present to the user without resuming inferior (and thus removing
I am no longer familiar with that part of GDB, so the following is only
a rewrite based on what you wrote. I am a little surprised, now that
I think of it, as I thought all breakpoints (including watchpoints were
removed at normal_stop:
if (!breakpoints_always_inserted_mode () && target_has_execution)
{
if (remove_breakpoints ())
Multiple hardware watchpoints can be hit at the same time, causing
GDB to stop. GDB only presents one hardware watchpoint hit at a
time as the reason for stopping, and all the other hits are presented
later, one after the other, each time the user requests the execution
to be resumed. Since watchpoints events are pending, the execution
is not resumed until all events are presented to the user, which
means that the corresponding watchpoint is not removed until then.
> + /* Status must be already queried for each LWP. Otherwise it will
> + be lost in all-stop mode + breakpoint always-inserted off. */
> + if (i386_dr_low.unset_status)
> + i386_dr_low.unset_status (I386_DR_WATCH_MASK (i));
If we end up keeping the comment in breakpoint.c, perhaps the following
comment has become superfluous (and it's a little cryptic). You're trying
to explain why it's safe to clear the watchpoint status bit, right?
> +# Here should be no repeated notification of the read watchpoint.
Here, there should
(or: "Verify that there is no repeated notification of the read watchpoint
when we continue")
> +gdb_test "continue" \
> + "Continuing\\.\[ \r\n\]+Breakpoint \[0-9\]+, .*break-at-exit.*" \
> + "continue to break-at-exit"
--
Joel