proposed PATCH: make watchpoints work correctly

proposed PATCH: make watchpoints work correctly

[Paul Koning]

The following patch fixes a couple of problems.  

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).

By the way, I first tried clearing "stop" in the status entry for a
watchpoint if the watchpoint wasn't the stop reason.  That doesn't
work -- if you do that, then the program doesn't stop if a watchpoint
is active but the stop reason is something else.

I have copyright assignment papers on file with FSF for gcc and
binutils -- does that cover this or should I add paperwork for gdb
specifically?   Also, I have no write access to anything.  Lastly, I'm
not sure how many things have to be tested for a patch to be
considered acceptable.  

     paul

diff -u gdb-5.3/gdb/breakpoint.c gdb-patches/gdb/breakpoint.c
--- gdb-5.3/gdb/breakpoint.c	Fri Aug 30 03:14:19 2002
+++ gdb-patches/gdb/breakpoint.c	Wed May 28 11:29:08 2003
@@ -704,6 +704,22 @@
 }
 \f
 
+/* Helper routine: free the value chain for a breakpoint (watchpoint) */
+static void free_valchain (struct breakpoint *b)
+{
+  struct value *v;
+  struct value *n;
+
+  /* Free the saved value chain.  We will construct a new one
+     the next time the watchpoint is inserted.  */
+  for (v = b->val_chain; v; v = n)
+    {
+      n = v->next;
+      value_free (v);
+    }
+  b->val_chain = NULL;
+}
+
 /* insert_breakpoints is used when starting or continuing the program.
    remove_breakpoints is used when the program stops.
    Both return zero if successful,
@@ -961,6 +977,8 @@
 
 	if (within_current_scope)
 	  {
+	    free_valchain (b);
+
 	    /* Evaluate the expression and cut the chain of values
 	       produced off from the value chain.
 
@@ -1461,15 +1479,6 @@
       if ((is == mark_uninserted) && (b->inserted))
 	warning ("Could not remove hardware watchpoint %d.",
 		 b->number);
-
-      /* Free the saved value chain.  We will construct a new one
-         the next time the watchpoint is inserted.  */
-      for (v = b->val_chain; v; v = n)
-	{
-	  n = v->next;
-	  value_free (v);
-	}
-      b->val_chain = NULL;
     }
   else if ((b->type == bp_catch_fork ||
 	    b->type == bp_catch_vfork ||
@@ -2503,10 +2512,16 @@
 	|| b->enable_state == bp_call_disabled)
       continue;
 
-    if (b->type != bp_watchpoint
-	&& b->type != bp_hardware_watchpoint
-	&& b->type != bp_read_watchpoint
-	&& b->type != bp_access_watchpoint
+    /* Watchpoints are treated as non-existent if the reason we stopped
+       wasn't a watchpoint (we didn't stop on some data address).  
+       Otherwise gdb won't stop on a break instruction in the code
+       (not from a breakpoint) when a watchpoint has been defined.
+    */
+    if (!((b->type == bp_watchpoint ||
+           b->type == bp_hardware_watchpoint ||
+           b->type == bp_read_watchpoint ||
+           b->type == bp_access_watchpoint)
+          && target_stopped_data_address () != 0)
 	&& b->type != bp_hardware_breakpoint
 	&& b->type != bp_catch_fork
 	&& b->type != bp_catch_vfork
@@ -6679,6 +6694,8 @@
   if (bpt->inserted)
     remove_breakpoint (bpt, mark_inserted);
 
+  free_valchain (bpt);
+  
   if (breakpoint_chain == bpt)
     breakpoint_chain = bpt->next;
 
diff -u gdb-5.3/gdb/infrun.c gdb-patches/gdb/infrun.c
--- gdb-5.3/gdb/infrun.c	Wed Sep 18 18:44:33 2002
+++ gdb-patches/gdb/infrun.c	Wed May 28 11:29:02 2003
@@ -1389,11 +1389,12 @@
    by an event from the inferior, figure out what it means and take
    appropriate action.  */
 
+int stepped_after_stopped_by_watchpoint;
+
 void
 handle_inferior_event (struct execution_control_state *ecs)
 {
   CORE_ADDR tmp;
-  int stepped_after_stopped_by_watchpoint;
   int sw_single_step_trap_p = 0;
 
   /* Cache the last pid/waitstatus. */
diff -u gdb-5.3/gdb/remote.c gdb-patches/gdb/remote.c
--- gdb-5.3/gdb/remote.c	Sun Aug 18 19:17:57 2002
+++ gdb-patches/gdb/remote.c	Wed May 28 11:30:45 2003
@@ -4869,10 +4869,13 @@
     return remote_stopped_by_watchpoint_p;
 }
 
+extern int stepped_after_stopped_by_watchpoint;
+
 CORE_ADDR
 remote_stopped_data_address (void)
 {
-  if (remote_stopped_by_watchpoint ())
+  if (remote_stopped_by_watchpoint () ||
+      stepped_after_stopped_by_watchpoint)
     return remote_watch_data_address;
   return (CORE_ADDR)0;
 }

Re: proposed PATCH: make watchpoints work correctly

[Paul Koning]

Missed the changelog entry for this patch:

2003-05-28  Paul Koning  <pkoning@equallogic.com>

	* breakpoint.c: (free_valchain) New function.
	(insert_breakpoints, delete_breakpoint) Use free_valchain.
	(remove_breakpoint) Do not remove the valchain.
	(bpstat_stop_status) If not stopped by watchpoint, skip
	watchpoints when generating stop status list.
	* infrun.c: (handle_inferior_event) Make
	stepped_after_stopped_by_watchpoint a global variable.
	* remote.c: (remote_stopped_data_address) Return watch data
	address rather than zero if stepped_after_stopped_by_watchpoint is
	set.


  paul

Re: proposed PATCH: make watchpoints work correctly

[Daniel Jacobowitz]

On Wed, May 28, 2003 at 12:01:25PM -0400, Paul Koning wrote:
> I have copyright assignment papers on file with FSF for gcc and
> binutils -- does that cover this or should I add paperwork for gdb
> specifically?   Also, I have no write access to anything.  Lastly, I'm
> not sure how many things have to be tested for a patch to be
> considered acceptable.  

You'll need separate copyright papers for GDB; sorry, I was under the
impression you already had them.

As for the patch itself, I just skimmed it, but using a global in
handle_inferior_event and in remote.c is not kosher.  We're trying to
cut back on the mess in infrun, not increase it.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: proposed PATCH: make watchpoints work correctly

[Eli Zaretskii]

> 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).

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

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

Re: proposed PATCH: make watchpoints work correctly

[Paul Koning]

>>>>> "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

Re: proposed PATCH: make watchpoints work correctly

[Eli Zaretskii]

> Date: Wed, 28 May 2003 16:27:33 -0400
> From: Paul Koning <pkoning@equallogic.com>
> 
>  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.

That's strange: I thought hardware-assisted watchpoints were supported
for all native x86 platforms.  Mark, could you please help us out
here? is NetBSD an exception?

I don't have time right now to read the parts of breakpoint.c that you
describe, but I promise to do that later today.  Thanks for taking
time to explain your reasoning.

> 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...

Certainly, I understand that.  I just was surprised that your
description of the problem was so different from my recollection of
how watchpoints work.

Re: proposed PATCH: make watchpoints work correctly

[Nathan J. Williams]

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

> That's strange: I thought hardware-assisted watchpoints were supported
> for all native x86 platforms.  Mark, could you please help us out
> here? is NetBSD an exception?

NetBSD/i386 doesn't provide a way for software to use the x86-series
hardware debug registers, so gdb doesn't use them there. 
(I had a look at this once, and didn't get very far; it's just as well
for me, because my problem turned out to be garbage core dumps rather
than garbage meory).

        - Nathan

Re: proposed PATCH: make watchpoints work correctly

[Eli Zaretskii]

> From: "Nathan J. Williams" <nathanw@wasabisystems.com>
> Date: 29 May 2003 00:35:48 -0400
> 
> NetBSD/i386 doesn't provide a way for software to use the x86-series
> hardware debug registers, so gdb doesn't use them there. 

Too bad.

Thanks for clarifying this.

Re: proposed PATCH: make watchpoints work correctly

[Paul Koning]

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

 >> Date: Wed, 28 May 2003 16:27:33 -0400 From: Paul Koning
 >> <pkoning@equallogic.com>
 >> 
 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.

 Eli> That's strange: I thought hardware-assisted watchpoints were
 Eli> supported for all native x86 platforms.  Mark, could you please
 Eli> help us out here? is NetBSD an exception?

I built 5.3 for Linux and did the experiment there.  Hardware
watchpoints do work there.

 Eli> I don't have time right now to read the parts of breakpoint.c
 Eli> that you describe, but I promise to do that later today.  Thanks
 Eli> for taking time to explain your reasoning.

 >> 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...

 Eli> Certainly, I understand that.  I just was surprised that your
 Eli> description of the problem was so different from my recollection
 Eli> of how watchpoints work.

I just ran a small test case on the x86 Linux native build of gdb 5.3,
and the problem (step works as if it were stepi, falsely reported as a
watchpoint hit) occurs there as well -- just as expected.

     paul

Re: proposed PATCH: make watchpoints work correctly

[Daniel Jacobowitz]

On Thu, May 29, 2003 at 11:17:54AM -0400, Paul Koning wrote:
> >>>>> "Eli" == Eli Zaretskii <eliz@elta.co.il> writes:
> 
>  >> Date: Wed, 28 May 2003 16:27:33 -0400 From: Paul Koning
>  >> <pkoning@equallogic.com>
>  >> 
>  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.
> 
>  Eli> That's strange: I thought hardware-assisted watchpoints were
>  Eli> supported for all native x86 platforms.  Mark, could you please
>  Eli> help us out here? is NetBSD an exception?
> 
> I built 5.3 for Linux and did the experiment there.  Hardware
> watchpoints do work there.
> 
>  Eli> I don't have time right now to read the parts of breakpoint.c
>  Eli> that you describe, but I promise to do that later today.  Thanks
>  Eli> for taking time to explain your reasoning.
> 
>  >> 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...
> 
>  Eli> Certainly, I understand that.  I just was surprised that your
>  Eli> description of the problem was so different from my recollection
>  Eli> of how watchpoints work.
> 
> I just ran a small test case on the x86 Linux native build of gdb 5.3,
> and the problem (step works as if it were stepi, falsely reported as a
> watchpoint hit) occurs there as well -- just as expected.

I don't know how facile you are with expect, but could you either write
a full testcase or at least give me a small sample code and session
transcript to reduce the problem, so that this can go into the
testsuite?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: proposed PATCH: make watchpoints work correctly

[Paul Koning]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 632 bytes --]

>>>>> "Daniel" == Daniel Jacobowitz <drow@mvista.com> writes:

 Daniel> I don't know how facile you are with expect, but could you
 Daniel> either write a full testcase or at least give me a small
 Daniel> sample code and session transcript to reduce the problem, so
 Daniel> that this can go into the testsuite?

I know next to nothing about expect, and from a quick look at some
testsuite files it would take me some time to construct a proper
testcase file.

Attached are a test program and a before and after log of a session
that shows the issue with awatch.  The test run was on RedHat 8.0
linux, gdb 5.3 native.

       paul

[-- Attachment #2: awatch.c --]
[-- Type: text/plain, Size: 113 bytes --]

volatile int i, j;

int main (int argc, char **argv)
{
    i = 1;
    j = 1;
    j = 2;
    j = 3;
    i = 2;
}


[-- Attachment #3: before.log --]
[-- Type: text/plain, Size: 1274 bytes --]

[pkoning@pkoning gdb-5.3]$ gdb/gdb a.out
GNU gdb 5.3
Copyright 2002 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i686-pc-linux-gnu"...
(gdb) b main
Breakpoint 1 at 0x8048318: file awatch.c, line 5.
(gdb) run
Starting program: /usr/src/gdb-5.3/a.out

Breakpoint 1, main (argc=1, argv=0xbffffae4) at awatch.c:5
5           i = 1;
(gdb) awatch i
Hardware access (read/write) watchpoint 2: i
(gdb) s
6           j = 1;
(gdb)
Hardware access (read/write) watchpoint 2: i

Value = 0
main (argc=1, argv=0xbffffae4) at awatch.c:7
7           j = 2;
(gdb)
Hardware access (read/write) watchpoint 2: i

Value = 0
main (argc=1, argv=0xbffffae4) at awatch.c:8
8           j = 3;
(gdb)
Hardware access (read/write) watchpoint 2: i

Value = 0
main (argc=1, argv=0xbffffae4) at awatch.c:9
9           i = 2;
(gdb)
Hardware access (read/write) watchpoint 2: i

Old value = 0
New value = 2
main (argc=1, argv=0xbffffae4) at awatch.c:10
10      }
(gdb) q
The program is running.  Exit anyway? (y or n) y

[-- Attachment #4: after.log --]
[-- Type: text/plain, Size: 971 bytes --]

[pkoning@pkoning gdb-5.3]$ gdb/gdb a.out
GNU gdb 5.3
Copyright 2002 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i686-pc-linux-gnu"...
(gdb) b main
Breakpoint 1 at 0x8048318: file awatch.c, line 5.
(gdb) run
Starting program: /usr/src/gdb-5.3/a.out

Breakpoint 1, main (argc=1, argv=0xbffffae4) at awatch.c:5
5           i = 1;
(gdb) awatch i
Hardware access (read/write) watchpoint 2: i
(gdb) s
6           j = 1;
(gdb)
7           j = 2;
(gdb)
8           j = 3;
(gdb)
9           i = 2;
(gdb)
Hardware access (read/write) watchpoint 2: i

Old value = 0
New value = 2
main (argc=1, argv=0xbffffae4) at awatch.c:10
10      }
(gdb) q
The program is running.  Exit anyway? (y or n) y

Re: proposed PATCH: make watchpoints work correctly

[Eli Zaretskii]

> Date: Thu, 29 May 2003 11:17:54 -0400
> From: Paul Koning <pkoning@equallogic.com>
> 
>  Eli> Certainly, I understand that.  I just was surprised that your
>  Eli> description of the problem was so different from my recollection
>  Eli> of how watchpoints work.
> 
> I just ran a small test case on the x86 Linux native build of gdb 5.3,
> and the problem (step works as if it were stepi, falsely reported as a
> watchpoint hit) occurs there as well -- just as expected.

Thanks, I now see the problem.

I think your solution is correct, but I'd like to minimize the number
of calls to target_stopped_data_address (they might be expensive).
Since the code already does call that function that elsewhere, could
we just reuse the result of that call, or rearrange your patch so that
a single call would do?

Otherwise, I think your change should go in.  Thanks.

Re: proposed PATCH: make watchpoints work correctly

[Paul Koning]

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

 >> Date: Thu, 29 May 2003 11:17:54 -0400 From: Paul Koning
 >> <pkoning@equallogic.com>
 >> 
 Eli> Certainly, I understand that.  I just was surprised that your
 Eli> description of the problem was so different from my recollection
 Eli> of how watchpoints work.
 >> I just ran a small test case on the x86 Linux native build of gdb
 >> 5.3, and the problem (step works as if it were stepi, falsely
 >> reported as a watchpoint hit) occurs there as well -- just as
 >> expected.

 Eli> Thanks, I now see the problem.

 Eli> I think your solution is correct, but I'd like to minimize the
 Eli> number of calls to target_stopped_data_address (they might be
 Eli> expensive).  Since the code already does call that function that
 Eli> elsewhere, could we just reuse the result of that call, or
 Eli> rearrange your patch so that a single call would do?

 Eli> Otherwise, I think your change should go in.  Thanks.

Thanks.  I'll look at the rearranging you suggested.

	 paul

Re: proposed PATCH: make watchpoints work correctly

[Daniel Jacobowitz]

On Wed, Jun 04, 2003 at 10:50:59AM -0400, Paul Koning wrote:
> >>>>> "Eli" == Eli Zaretskii <eliz@elta.co.il> writes:
> 
>  >> Date: Thu, 29 May 2003 11:17:54 -0400 From: Paul Koning
>  >> <pkoning@equallogic.com>
>  >> 
>  Eli> Certainly, I understand that.  I just was surprised that your
>  Eli> description of the problem was so different from my recollection
>  Eli> of how watchpoints work.
>  >> I just ran a small test case on the x86 Linux native build of gdb
>  >> 5.3, and the problem (step works as if it were stepi, falsely
>  >> reported as a watchpoint hit) occurs there as well -- just as
>  >> expected.
> 
>  Eli> Thanks, I now see the problem.
> 
>  Eli> I think your solution is correct, but I'd like to minimize the
>  Eli> number of calls to target_stopped_data_address (they might be
>  Eli> expensive).  Since the code already does call that function that
>  Eli> elsewhere, could we just reuse the result of that call, or
>  Eli> rearrange your patch so that a single call would do?
> 
>  Eli> Otherwise, I think your change should go in.  Thanks.
> 
> Thanks.  I'll look at the rearranging you suggested.

Sorting through my TODO list I found your test code for this issue; did
you ever finish the patch?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer