Re: proposed PATCH: make watchpoints work correctly

[Paul Koning <pkoning at equallogic dot com>]

>>>>> "Eli" == Eli Zaretskii <eliz@elta.co.il> writes:

 >> Date: Wed, 28 May 2003 12:01:25 -0400 From: Paul Koning
 >> <pkoning@equallogic.com>
 >> 
 >> If HAVE_NONSTEPPABLE_WATCHPOINT is defined, watchpoints would not
 >> stop for several reasons.  First of all, remote.c would return 0
 >> rather than the watch address after the single step past the
 >> instruction where the watch trap occurred.  (Changes to infrun.c,
 >> remote.c) Second, even after that is fixed, bpstat_stop_status
 >> wouldn't match the watchpoint because removing the watchpoint (to
 >> single step) deleted the valchain which is used to do the matching
 >> (Change to breakpoint.c)
 >> 
 >> If a watchpoint is defined, but the program stops for some other
 >> reason -- either a breakpoint, or a break instruction hardcoded in
 >> the target code -- bpstat_stop_status would encounter the
 >> watchpoint in its scan for possible reasons.  It would take no
 >> action on it but leave its "stop" and "print" bits set so you
 >> would see the stop reported as if it were a watchpoint hit.  Also,
 >> a "next" or "step" command would act as "stepi", i.e., stop after
 >> every instruction.  (Changes to breakpoint.c).

 Eli> (It's been a while since I hacked on watchpoint-related code, so
 Eli> I apologize in advance for asking possibly dumb questions.)

 Eli> The above description made me nervous: it almost sounds like the
 Eli> current watchpoint support is pretty much dysfunctional, as most
 Eli> of the changes you suggest are not specific neither to remote.c
 Eli> nor to HAVE_NONSTEPPABLE_WATCHPOINT.  So could you please
 Eli> explain how, given those deficiencies, watchpoints do work for
 Eli> native targets such as x86, but did not work for your target?

I'm not sure. I just built a gdb for x86 on NetBSD, and all I get is
software write watchpoints, no hardware watch support seems to be
present.

I arrived at the patch I sent in two steps.  The description I gave
combines what I found in those two steps, and I may have made errors
in some details.

In fact, looking back, there definitely is an error.

The original problem I saw with stepping turning into stepi happened
*only* for rwatch and awatch, NOT for write watchpoints.  The reason
is that the existinc code looks like this:

    /* Come here if it's a watchpoint, or if the break address matches */

    bs = bpstat_alloc (b, bs);	/* Alloc a bpstat to explain stop */

    /* Watchpoints may change this, if not found to have triggered. */
    bs->stop = 1;
    bs->print = 1;

    sprintf (message, message1, b->number);
    if (b->type == bp_watchpoint ||
	b->type == bp_hardware_watchpoint)
      {
	...
      }
    else if (b->type == bp_read_watchpoint || 
	     b->type == bp_access_watchpoint)
      {
	CORE_ADDR addr;
	struct value *v;
	int found = 0;

	addr = target_stopped_data_address ();
	if (addr == 0)
	  continue;

The comment says "if it's a watchpoint" -- what that means is: if the
entry in the breakpoint list we're currently looking at is a
watchpoint.  It does not mean "if we stopped because of a watchpoint".

The code then allocates a bpstat entry, and sets it to stop and
print.  Then if it turns out to be a rwatch or awatch, but there is no
data address (the stop wasn't a watchpoint stop) the code just leaves
the bpstat entry in the bpstat list marked for stop and print...

My first fix was to set bs->stop = bs->print = 0 when the "continue"
is done.  That fixed the stopping after every instruction, but now the
stop == 0 status meant that permanent breakpoints would no longer
stop.  (Regular ones would, but for permanent breakpoints you end up
with a single bpstat entry that says stop==0, rather than a null
list.)  So my new fix is to skip the watchpoint entries in the
breakpoint list entirely if the stop isn't a watchpoint stop.  That
fixes all the problems I've run into.

Does that make sense?

Clearly I'm not a gdb expert.  I've been trying to learn selected
pieces in order to fix problems we run into in our product
development, and when those look like they are more generally useful I
try to pass them along.  The purpose of this patch submission is to
get input from experts -- not necessarily to claim that the fix I
submitted is the best way to solve the problem...

	  paul