Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Orjan Friberg]

Hi all,

I'm working on a gdb port for a remote target that has hardware 
watchpoint support.  I have defined HAVE_NONSTEPPABLE_WATCHPOINT, to 
make gdb disable the watchpoint and single-step over it when it's hit. 
The basic stuff works fine; the problem I have is the displaying of "Old 
value/New value" when a read or access watchpoint is hit.  (Normal write 
watchpoints are displayed correctly.)

The problem seems to be that target_stopped_data_address () is called in 
bp_stop_status () *after* the single-step over the disabled breakpoint 
has been carried out, at which point the target is no longer stopped due 
to a hardware watchpoint (but due to the single-step).  Consequently, 
target_stopped_data_address () returns 0, and gdb thinks that the 
watchpoint didn't hit.  I can't see that any other targets keep the last 
"stopped data address" until the next hardware watchpoint is hit (which 
would make the address still available after the single-step).

Any suggestions as to what I might have missed, or any insight in how 
it's supposed to work would be appreciated.

Thanks,
Orjan

-- 
Orjan Friberg
Axis Communications

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Wed, 08 Oct 2003 10:50:25 +0200
> From: Orjan Friberg <orjan.friberg@axis.com>
> 
> The problem seems to be that target_stopped_data_address () is called in 
> bp_stop_status () *after* the single-step over the disabled breakpoint 
> has been carried out, at which point the target is no longer stopped due 
> to a hardware watchpoint (but due to the single-step).  Consequently, 
> target_stopped_data_address () returns 0, and gdb thinks that the 
> watchpoint didn't hit.  I can't see that any other targets keep the last 
> "stopped data address" until the next hardware watchpoint is hit (which 
> would make the address still available after the single-step).

Perhaps none of the targets that support hardware read and access
watchpoints define HAVE_NONSTEPPABLE_WATCHPOINT?

Anyway, from your description, it is quite clear that if a target
defines HAVE_NONSTEPPABLE_WATCHPOINT, GDB must call
target_stopped_data_address before it disables the watchpoint and
steps over it, or else the target end should store the necessary info
somewhere and deliver it when target_stopped_data_address is called.

In other words, it sounds like a bug.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Orjan Friberg]

Eli Zaretskii wrote:
> 
> Perhaps none of the targets that support hardware read and access
> watchpoints define HAVE_NONSTEPPABLE_WATCHPOINT?

Yes, that seems to be the case.

> Anyway, from your description, it is quite clear that if a target
> defines HAVE_NONSTEPPABLE_WATCHPOINT, GDB must call
> target_stopped_data_address before it disables the watchpoint and
> steps over it, or else the target end should store the necessary info
> somewhere and deliver it when target_stopped_data_address is called.

Right.  I was thinking I could make watchpoints "steppable" by disabling 
them in the remote stub when a watchpoint hits, and then enabling them 
again when a continue is issued, but it feels like that might create 
more problems than it solves.  (For example, watchpoints would never hit 
when single-stepping.)

I'll see if I can understand enough of handle_inferior_event to whip up 
a patch for either of your suggestions.  Thanks for your answer.

-- 
Orjan Friberg
Axis Communications

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Paul Koning]

>>>>> "Orjan" == Orjan Friberg <orjan.friberg@axis.com> writes:

 Orjan> Eli Zaretskii wrote:
 >>  Perhaps none of the targets that support hardware read and access
 >> watchpoints define HAVE_NONSTEPPABLE_WATCHPOINT?

 Orjan> Yes, that seems to be the case.

No; I found that the SB-1 (a MIPS) needs to have this defined.

 >> Anyway, from your description, it is quite clear that if a target
 >> defines HAVE_NONSTEPPABLE_WATCHPOINT, GDB must call
 >> target_stopped_data_address before it disables the watchpoint and
 >> steps over it, or else the target end should store the necessary
 >> info somewhere and deliver it when target_stopped_data_address is
 >> called.

 Orjan> Right.  I was thinking I could make watchpoints "steppable" by
 Orjan> disabling them in the remote stub when a watchpoint hits, and
 Orjan> then enabling them again when a continue is issued, but it
 Orjan> feels like that might create more problems than it solves.
 Orjan> (For example, watchpoints would never hit when
 Orjan> single-stepping.)

I agree.  I ended up defining HAVE_NONSTEPPABLE_WATCHPOINT, and I had
to make a patch to deal with the target_stopped_data_address issue you
mentioned.

(In case someone wonders: this was on a private variant of a 5.3
snapshot; I haven't had enough time to try to submit it as mainstream
patches, and parts of what I did appear to be controversial -- such as
fixes for the broken shared library handling in that version at least
with MIPS NetBSD.)

	paul

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Orjan Friberg]

Eli Zaretskii wrote:
> 
> Anyway, from your description, it is quite clear that if a target
> defines HAVE_NONSTEPPABLE_WATCHPOINT, GDB must call
> target_stopped_data_address before it disables the watchpoint and
> steps over it, or else the target end should store the necessary info
> somewhere and deliver it when target_stopped_data_address is called.
> 
> In other words, it sounds like a bug.

I had missed a previous thread on the subject, initiated by Paul Koning 
(starting at 
http://sources.redhat.com/ml/gdb-patches/2003-05/msg00506.html).  Paul 
sent me a patch for 5.3 which no longer applies cleanly, and with his 
blessings I'm trying to update his patch to the current CVS.

Doing that, I encountered a problem with read watchpoints (rwatch 
command): in insert_breakpoints, before calling insert_bp_location, the 
current value of the watched expression is read.  Then in 
bpstat_stop_status, when the watchpoint has hit, there's the following 
code snippet:

case WP_VALUE_CHANGED:
   if (b->type == bp_read_watchpoint)
     {
       /* Don't stop: read watchpoints shouldn't fire if
          the value has changed.  This is for targets
          which cannot set read-only watchpoints.  */
       bs->print_it = print_it_noop;
       bs->stop = 0;
       continue;
     }
   ++(b->hit_count);
   break;

So, if we're "read watching" a variable that indeed has changed since we 
read it before inserting it we decide not to stop.  Here's another 
example (foo is rwatched):

   volatile int foo = 0;
   int a;

   a = foo;   /* foo read; stops.  */
   foo = 1;
   a = foo;   /* foo read; doesn't stop because value changed.  */


The "Don't stop" comment is consistent with the behaviour, so it seems 
like this is intended.  What am I missing here?

-- 
Orjan Friberg
Axis Communications

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Daniel Jacobowitz]

On Tue, Apr 06, 2004 at 12:14:12PM +0200, Orjan Friberg wrote:
> Eli Zaretskii wrote:
> >
> >Anyway, from your description, it is quite clear that if a target
> >defines HAVE_NONSTEPPABLE_WATCHPOINT, GDB must call
> >target_stopped_data_address before it disables the watchpoint and
> >steps over it, or else the target end should store the necessary info
> >somewhere and deliver it when target_stopped_data_address is called.
> >
> >In other words, it sounds like a bug.
> 
> I had missed a previous thread on the subject, initiated by Paul Koning 
> (starting at 
> http://sources.redhat.com/ml/gdb-patches/2003-05/msg00506.html).  Paul 
> sent me a patch for 5.3 which no longer applies cleanly, and with his 
> blessings I'm trying to update his patch to the current CVS.
> 
> Doing that, I encountered a problem with read watchpoints (rwatch 
> command): in insert_breakpoints, before calling insert_bp_location, the 
> current value of the watched expression is read.  Then in 
> bpstat_stop_status, when the watchpoint has hit, there's the following 
> code snippet:
> 
> case WP_VALUE_CHANGED:
>   if (b->type == bp_read_watchpoint)
>     {
>       /* Don't stop: read watchpoints shouldn't fire if
>          the value has changed.  This is for targets
>          which cannot set read-only watchpoints.  */
>       bs->print_it = print_it_noop;
>       bs->stop = 0;
>       continue;
>     }
>   ++(b->hit_count);
>   break;
> 
> So, if we're "read watching" a variable that indeed has changed since we 
> read it before inserting it we decide not to stop.  Here's another 
> example (foo is rwatched):
> 
>   volatile int foo = 0;
>   int a;
> 
>   a = foo;   /* foo read; stops.  */
>   foo = 1;
>   a = foo;   /* foo read; doesn't stop because value changed.  */
> 
> 
> The "Don't stop" comment is consistent with the behaviour, so it seems 
> like this is intended.  What am I missing here?

Presumably, the relevant part of the comment is the second half: "this
is for targets which cannot set read-only watchpoints".  On the other
hand, it does not sound like that code will work for targets which
_can_ set read-only watchpoints.  The patch was:

2000-03-21  Eli Zaretskii  <eliz@is.elta.co.il>

        * breakpoint.c (bpstat_stop_status): Don't stop if a read
        watchpoint appears to break, but the watched value changed.

And since Eli demonstrably knows more than I do about how watchpoints
work, I'm going to defer further comments at this point :)

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Orjan Friberg]

Daniel Jacobowitz wrote:
> 
> Presumably, the relevant part of the comment is the second half: "this
> is for targets which cannot set read-only watchpoints".  On the other
> hand, it does not sound like that code will work for targets which
> _can_ set read-only watchpoints.  The patch was:

FWIW, the other changes from Paul's patch (returning the remote stopped 
data address even after single-stepping past a nonsteppable watchpoint, 
and (perhaps more relevant for the issue at hand) freeing the value 
chain for the breakpoint when inserting/deleting instead of when 
removing it) doesn't seem to affect this behaviour.

-- 
Orjan Friberg
Axis Communications

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

Sorry for a late response: I was travelling beyond the reach of my
email.

> Date: Tue, 6 Apr 2004 10:22:28 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> Presumably, the relevant part of the comment is the second half: "this
> is for targets which cannot set read-only watchpoints".

Yes.  The x86 processors belong to that family, so we didn't just
change the mainline code for dome isoteric CPU.

> On the other hand, it does not sound like that code will work for
> targets which _can_ set read-only watchpoints.

Patches to make that work for those platforms are welcome.

However, it seems to me that the right thing to do is to fix the cited
code in bpstat_stop_status (and perhaps elsewhere in breakpoint.c) to
DTRT for the test program posted by Orjan, since it clearly should
work with i386 as well, and since this specific failure of GDB has
nothing to do with read-only vs read-write watchpoints.

> The patch was:
> 
> 2000-03-21  Eli Zaretskii  <eliz@is.elta.co.il>
> 
>         * breakpoint.c (bpstat_stop_status): Don't stop if a read
>         watchpoint appears to break, but the watched value changed.

See the test program that reveals the original problem I wanted to
fix, and the ensuing discussion, here:

  http://sources.redhat.com/ml/gdb-patches/2000-03/msg00174.html

Any suggested patch should not break what was fixed back then.

AFAIR, without the value-changed test, the rwatch watchpoints were
totally unusable on x86, they would simply fire every time the watched
variable is set.  So simply removing the test is not the way to solve
this, IMHO.

Btw, a workaround for the case in point is to use awatch, but of
course if rwatch can be fixed to avoid failure here, it's a better
solution.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Orjan Friberg]

Eli Zaretskii wrote:
> 
> However, it seems to me that the right thing to do is to fix the cited
> code in bpstat_stop_status (and perhaps elsewhere in breakpoint.c) to
> DTRT for the test program posted by Orjan, since it clearly should
> work with i386 as well, and since this specific failure of GDB has
> nothing to do with read-only vs read-write watchpoints.

The test program (repeated below) *does* work for i386 (though I didn't 
say that), because it also stops when foo is written (thus updating the 
value of foo when watchpoint_check is called), so by the time it stops 
when the second read happens the value hasn't changed since the last 
time and GDB decides it's a valid hit.

   volatile int foo = 0;
   int a;

   a = foo;
   foo = 1;
   a = foo;

Question is how to fix this (comments are most welcome):

1. Add a check if the target cannot set "pure" read watchpoints to the 
b->type == bp_read_watchpoint check at WP_VALUE_CHANGED (my 
interpretation of Eli's suggestion).

or

2. Somehow don't update the value in watchpoint_check when it's a false 
hit.  (Then the b->type == bp_read_watchpoint check at WP_VALUE_CHANGED 
isn't needed.)

or

3. Add some distinction between "wanted watchpoint type" and "actual 
watchpoint type".

-- 
Orjan Friberg
Axis Communications

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Thu, 15 Apr 2004 15:23:59 +0200
> From: Orjan Friberg <orjan.friberg@axis.com>
> 
> The test program (repeated below) *does* work for i386 (though I didn't 
> say that), because it also stops when foo is written (thus updating the 
> value of foo when watchpoint_check is called), so by the time it stops 
> when the second read happens the value hasn't changed since the last 
> time and GDB decides it's a valid hit.

Right; sorry I forgot about that.  It's been a long time since I
hacked that part of GDB.

> 1. Add a check if the target cannot set "pure" read watchpoints to the 
> b->type == bp_read_watchpoint check at WP_VALUE_CHANGED (my 
> interpretation of Eli's suggestion).

On balance, this is probably the best solution, although it's not
quite clean.  The ability to set read watchpoints would be part of the
architecture vector, right?

> 2. Somehow don't update the value in watchpoint_check when it's a false 
> hit.  (Then the b->type == bp_read_watchpoint check at WP_VALUE_CHANGED 
> isn't needed.)

Wouldn't this reintroduce the bug that I was trying to solve back
then?  That is, will GDB still DTRT when both rwatch and watch are set
at the same variable?

> 3. Add some distinction between "wanted watchpoint type" and "actual 
> watchpoint type".

Any specific ideas how to do that?  For that matter, what is ``wanted
watchpoint type'', and how can we decide that?

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Orjan Friberg]

Eli Zaretskii wrote:
>>Date: Thu, 15 Apr 2004 15:23:59 +0200
>>From: Orjan Friberg <orjan.friberg@axis.com>
>>
>>The test program (repeated below) *does* work for i386 (though I didn't 
>>say that), because it also stops when foo is written (thus updating the 
>>value of foo when watchpoint_check is called), so by the time it stops 
>>when the second read happens the value hasn't changed since the last 
>>time and GDB decides it's a valid hit.
> 
> 
> Right; sorry I forgot about that.  It's been a long time since I
> hacked that part of GDB.

Apologies; I didn't mean to imply that that was obvious.  (I certainly 
didn't know that was why it worked on the i386 when I posted the example 
in the first place.)

>>1. Add a check if the target cannot set "pure" read watchpoints to the 
>>b->type == bp_read_watchpoint check at WP_VALUE_CHANGED (my 
>>interpretation of Eli's suggestion).
> 
> 
> On balance, this is probably the best solution, although it's not
> quite clean.  The ability to set read watchpoints would be part of the
> architecture vector, right?

Yes, I think so.  Also, this seems like the least intrusive change. 
(Nevertheless, since you were kind enough to comment on the other 
suggestions, I'll continue the discussion of them.)

>>2. Somehow don't update the value in watchpoint_check when it's a false 
>>hit.  (Then the b->type == bp_read_watchpoint check at WP_VALUE_CHANGED 
>>isn't needed.)
> 
> 
> Wouldn't this reintroduce the bug that I was trying to solve back
> then?  That is, will GDB still DTRT when both rwatch and watch are set
> at the same variable?

You're right; this won't work.  I failed to account for the fact that 
the code *relies* on the i386 stopping (and updating the value) when 
writing to an rwatched variable.

In addition, I'm not sure how we'd detect a false hit for the i386 (the 
"somehow" part).  For my upcoming target, I have bits telling me if it 
was a read or write that caused the exception.

>>3. Add some distinction between "wanted watchpoint type" and "actual 
>>watchpoint type".
> 
> 
> Any specific ideas how to do that?  For that matter, what is ``wanted
> watchpoint type'', and how can we decide that?

The idea was that when inserting a watchpoint, target_insert_watchpoint 
would return the type of watchpoint that was actually inserted.  So, for 
the i386 an insertion of a read watchpoint would return an access 
watchpoint (if I've understood correctly).  The breakpoint struct would 
then have an additional field storing this returned value.

In bpstat_stop_status, the code would then be something like

   case WP_VALUE_CHANGED:
     if (b->type == bp_read_watchpoint
         && b->actual_type == bp_access_watchpoint)
       /* Don't stop.  */
       ...


I'll have a look at implementing the first suggestion.  Thanks.

-- 
Orjan Friberg
Axis Communications

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Orjan Friberg]

Orjan Friberg wrote:
> 
> 1. Add a check if the target cannot set "pure" read watchpoints to the 
> b->type == bp_read_watchpoint check at WP_VALUE_CHANGED (my 
> interpretation of Eli's suggestion).

On second thought, this actually is no good for a target which *can* set 
pure read watchpoints.  If there's both a watch and an rwatch on a 
variable that is written, we'll get a false read watchpoint hit.

For the sake of argument, say that the implementation is called 
TARGET_CANNOT_SET_PURE_READ_WATCHPOINTS (the i386 would return 1, my 
target would return 0).  The check for a read watchpoint would then be

   case WP_VALUE_CHANGED:
     if (b->type == bp_read_watchpoint
         && TARGET_CANNOT_SET_PURE_READ_WATCHPOINTS)
     /* Don't stop.  */

For i386, nothing would change.  For my target, the following would 
happen (foo is watched and rwatched, original value != 0):

   a = foo;   /* foo read; read watchpoint hits.  */
   foo = 1;   /* foo written; write watchpoint hits, read watchpoint
                 hits incorrectly.  */

-- 
Orjan Friberg
Axis Communications

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Fri, 16 Apr 2004 13:42:18 +0200
> From: Orjan Friberg <orjan.friberg@axis.com>
> 
> Orjan Friberg wrote:
> > 
> > 1. Add a check if the target cannot set "pure" read watchpoints to the 
> > b->type == bp_read_watchpoint check at WP_VALUE_CHANGED (my 
> > interpretation of Eli's suggestion).
> 
> On second thought, this actually is no good for a target which *can* set 
> pure read watchpoints.  If there's both a watch and an rwatch on a 
> variable that is written, we'll get a false read watchpoint hit.

How about if such targets would set a read/write (i.e. access)
watchpoint, and then use the same logic as i386: if the value didn't
change, don't announce the rwatch hit?  In effect, you would be
emulating i386 behavior.

That's a band-aid, I know.  But to get rid of this mess altogether,
the GDB's watchpoint processing will need to be thoroughly revamped.
In particular, instead of walking all the watchpoints armed only with
the address that caused the inferior to stop, we would need to have
the target to tell us exactly what watchpoint triggered, and why.
That would mean pushing most of the related logic in
bpstat_stop_status into the target/architecture vectors, leaving the
GDB's application level with only the higher-level API that
communicates with the lower-level layers via watchpoint handles of
some kind.

By contrast, the current GDB code for hardware-assisted watchpoints
grew up from the software watchpoints, whereby GDB would single-step
the program and after each step check whether the value of any of the
watched expressions changed.  This legacy clearly shows for quite some
time.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Orjan Friberg]

Eli Zaretskii wrote:
> 
> How about if such targets would set a read/write (i.e. access)
> watchpoint, and then use the same logic as i386: if the value didn't
> change, don't announce the rwatch hit?  In effect, you would be
> emulating i386 behavior.

Ah, good suggestion.  Excellent in fact - both rwatch and watch/rwatch 
on the same variable work fine now.  Thanks a lot!

I was a bit worried, this being a remote target, that I'd have to 
remember the rwatch -> awatch conversion made internally in the stub, 
and report it as an rwatch when stopping.  But the watch/rwatch/awatch 
information in the stop packet isn't passed on from remote.c - it's only 
used to parse out the remote watched data address.

> That's a band-aid, I know.  But to get rid of this mess altogether,
> the GDB's watchpoint processing will need to be thoroughly revamped.
> In particular, instead of walking all the watchpoints armed only with
> the address that caused the inferior to stop, we would need to have
> the target to tell us exactly what watchpoint triggered, and why.
> That would mean pushing most of the related logic in
> bpstat_stop_status into the target/architecture vectors, leaving the
> GDB's application level with only the higher-level API that
> communicates with the lower-level layers via watchpoint handles of
> some kind.

Agreed.  I am quite happy to live with your suggested solution, at least 
for now.  I certainly don't have the audacity to suggest such changes 
should be made to accomodate a target that isn't even submitted yet ;) .

-- 
Orjan Friberg
Axis Communications

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Orjan Friberg]

Orjan Friberg wrote:
> 
> Agreed.  I am quite happy to live with your suggested solution, at least 
> for now.  I certainly don't have the audacity to suggest such changes 
> should be made to accomodate a target that isn't even submitted yet ;) .

... which brings me back to the reason for re-opening this thread: getting Paul 
Koning's patch to make read/access watchpoints work when 
HAVE_NONSTEPPABLE_WATCHPOINT is defined accepted.

I did change one thing in Paul's patch, which should be highlighted: the change 
in bpstat_stop_status previously applied to bp_watchpoint types also (in 
addition to bp_hardware_watchpoint, bp_read_watchpoint, and 
bp_access_watchpoint), but as far as I can tell target_stopped_data_address 
applies only to hardware-assisted watchpoints, not software watchpoints.  Maybe 
that needs to be made more clear in the comment.

Comments?


2004-04-22  Orjan Friberg <orjanf@axis.com>

	From Paul Koning <pkoning@equallogic.com>:
	* breakpoint.c (free_valchain): New function.
	(insert_bp_location, 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.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.167
diff -u -p -r1.167 breakpoint.c
--- breakpoint.c        21 Apr 2004 23:52:19 -0000      1.167
+++ breakpoint.c        22 Apr 2004 14:56:02 -0000
@@ -746,6 +746,23 @@ insert_catchpoint (struct ui_out *uo, vo
    return 0;
  }

+/* Helper routine: free the value chain for a breakpoint (watchpoint).  */
+
+static void free_valchain (struct bp_location *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->owner->val_chain; v; v = n)
+    {
+      n = v->next;
+      value_free (v);
+    }
+  b->owner->val_chain = NULL;
+}
+
  /* Insert a low-level "breakpoint" of some type.  BPT is the breakpoint.
     Any error messages are printed to TMP_ERROR_STREAM; and DISABLED_BREAKS,
     PROCESS_WARNING, and HW_BREAKPOINT_ERROR are used to report problems.
@@ -920,6 +937,8 @@ insert_bp_location (struct bp_location *

        if (within_current_scope)
         {
+         free_valchain (bpt);
+
           /* Evaluate the expression and cut the chain of values
              produced off from the value chain.

@@ -1505,15 +1524,6 @@ remove_breakpoint (struct bp_location *b
        if ((is == mark_uninserted) && (b->inserted))
         warning ("Could not remove hardware watchpoint %d.",
                  b->owner->number);
-
-      /* Free the saved value chain.  We will construct a new one
-         the next time the watchpoint is inserted.  */
-      for (v = b->owner->val_chain; v; v = n)
-       {
-         n = v->next;
-         value_free (v);
-       }
-      b->owner->val_chain = NULL;
      }
    else if ((b->owner->type == bp_catch_fork ||
             b->owner->type == bp_catch_vfork ||
@@ -2616,10 +2626,15 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
      if (!breakpoint_enabled (b) && b->enable_state != bp_permanent)
        continue;

+    /* Watchpoints are treated as non-existent if the reason we stopped
+       wasn't a hardware 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
+       && !((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
@@ -6880,6 +6895,8 @@ delete_breakpoint (struct breakpoint *bp

    if (bpt->loc->inserted)
      remove_breakpoint (bpt->loc, mark_inserted);
+
+  free_valchain (bpt->loc);

    if (breakpoint_chain == bpt)
      breakpoint_chain = bpt->next;
Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.133
diff -u -p -r1.133 remote.c
--- remote.c    21 Apr 2004 23:52:20 -0000      1.133
+++ remote.c    22 Apr 2004 14:56:05 -0000
@@ -4640,10 +4640,13 @@ remote_stopped_by_watchpoint (void)
      return remote_stopped_by_watchpoint_p;
  }

+extern int stepped_after_stopped_by_watchpoint;
+
  static 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;
  }
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.147
diff -u -p -r1.147 infrun.c
--- infrun.c    21 Apr 2004 23:52:20 -0000      1.147
+++ infrun.c    22 Apr 2004 14:56:07 -0000
@@ -1356,6 +1356,8 @@ adjust_pc_after_break (struct execution_
     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)
  {
@@ -1364,7 +1366,6 @@ handle_inferior_event (struct execution_
       isn't used, then you're wrong!  The macro STOPPED_BY_WATCHPOINT,
       defined in the file "config/pa/nm-hppah.h", accesses the variable
       indirectly.  Mutter something rude about the HP merge.  */
-  int stepped_after_stopped_by_watchpoint;
    int sw_single_step_trap_p = 0;

    /* Cache the last pid/waitstatus. */

-- 
Orjan Friberg
Axis Communications

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Paul Koning]

>>>>> "Orjan" == Orjan Friberg <orjan.friberg@axis.com> writes:

 Orjan> Orjan Friberg wrote:
 >>  Agreed.  I am quite happy to live with your suggested solution,
 >> at least for now.  I certainly don't have the audacity to suggest
 >> such changes should be made to accomodate a target that isn't even
 >> submitted yet ;) .

 Orjan> ... which brings me back to the reason for re-opening this
 Orjan> thread: getting Paul Koning's patch to make read/access
 Orjan> watchpoints work when HAVE_NONSTEPPABLE_WATCHPOINT is defined
 Orjan> accepted.

 Orjan> I did change one thing in Paul's patch, which should be
 Orjan> highlighted: the change in bpstat_stop_status previously
 Orjan> applied to bp_watchpoint types also (in addition to
 Orjan> bp_hardware_watchpoint, bp_read_watchpoint, and
 Orjan> bp_access_watchpoint), but as far as I can tell
 Orjan> target_stopped_data_address applies only to hardware-assisted
 Orjan> watchpoints, not software watchpoints.  Maybe that needs to be
 Orjan> made more clear in the comment.

Thanks.  I developed the original patch on a platform with hardware
watchpoints (MIPS SB-1) so I don't know if it needs to be restricted
only to hardware watchpoints or should apply to both types.

     paul

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Thu, 22 Apr 2004 17:07:34 +0200
> From: Orjan Friberg <orjan.friberg@axis.com>
> 
> ... which brings me back to the reason for re-opening this thread: getting Paul 
> Koning's patch to make read/access watchpoints work when 
> HAVE_NONSTEPPABLE_WATCHPOINT is defined accepted.

I want to approve this, but first I'd like to refresh my memory about
the original patch.  Could you please point me at the message where
Paul posted his patch and its reasons?

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Paul Koning]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

 >> Date: Thu, 22 Apr 2004 17:07:34 +0200 From: Orjan Friberg
 >> <orjan.friberg@axis.com>
 >> 
 >> ... which brings me back to the reason for re-opening this thread:
 >> getting Paul Koning's patch to make read/access watchpoints work
 >> when HAVE_NONSTEPPABLE_WATCHPOINT is defined accepted.

 Eli> I want to approve this, but first I'd like to refresh my memory
 Eli> about the original patch.  Could you please point me at the
 Eli> message where Paul posted his patch and its reasons?

Original message:
http://sources.redhat.com/ml/gdb-patches/2003-05/msg00506.html

Further explanation:
http://sources.redhat.com/ml/gdb-patches/2003-05/msg00516.html
http://sources.redhat.com/ml/gdb-patches/2003-05/msg00540.html

Discussion about a testcase:
http://sources.redhat.com/ml/gdb-patches/2003-05/msg00546.html

Do I need gdb copyright assignment for this patch to go in?  I don't
remember if I have it at this point; if it's easy to check with FSF
that may be best, otherwise I can dig through piles of paper...

     paul

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Paul Koning]

>>>>> "Paul" == Paul Koning <pkoning@equallogic.com> writes:

 Paul> Do I need gdb copyright assignment for this patch to go in?  I
 Paul> don't remember if I have it at this point; ...

Oops, I should have looked just a bit longer.  I found the record; yes
I have copyright assignment in place for GDB (#40153, 6 aug 2003).

    paul

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Thu, 22 Apr 2004 15:07:16 -0400
> From: Paul Koning <pkoning@equallogic.com>
> 
>  Eli> I want to approve this, but first I'd like to refresh my memory
>  Eli> about the original patch.  Could you please point me at the
>  Eli> message where Paul posted his patch and its reasons?
> 
> Original message:
> http://sources.redhat.com/ml/gdb-patches/2003-05/msg00506.html
> 
> Further explanation:
> http://sources.redhat.com/ml/gdb-patches/2003-05/msg00516.html
> http://sources.redhat.com/ml/gdb-patches/2003-05/msg00540.html
> 
> Discussion about a testcase:
> http://sources.redhat.com/ml/gdb-patches/2003-05/msg00546.html

Thanks; I recollect that discussion now.

Your patch is approved, please commit it.

Thanks.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Thu, 22 Apr 2004 17:07:34 +0200
> From: Orjan Friberg <orjan.friberg@axis.com>
> 
> as far as I can tell target_stopped_data_address applies only to
> hardware-assisted watchpoints, not software watchpoints.

That's true: software watchpoints don't use target_stopped_data_address
because they single-step the inferior and evaluate each watched
expression by themselves.

> Maybe that needs to be made more clear in the comment.

A good idea, IMHO.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Orjan Friberg]

Eli Zaretskii wrote:
> 
>>Maybe that needs to be made more clear in the comment.
> 
> 
> A good idea, IMHO.

Comment tweaked to say "hardware watchpoint" instead of just "watchpoint. 
Thanks so much your help and for reviewing so quickly.

Committed.

-- 
Orjan Friberg
Axis Communications

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Mon, 26 Apr 2004 11:03:04 +0200
> From: Orjan Friberg <orjan.friberg@axis.com>
> 
> Thanks so much your help and for reviewing so quickly.

Rather thank _you_ and Paul for finding and fixing a bad bug.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Mark Kettenis]

   Date: Thu, 22 Apr 2004 17:07:34 +0200
   From: Orjan Friberg <orjan.friberg@axis.com>

   Orjan Friberg wrote:
   > 
   > Agreed.  I am quite happy to live with your suggested solution, at least 
   > for now.  I certainly don't have the audacity to suggest such changes 
   > should be made to accomodate a target that isn't even submitted yet ;) .

   ... which brings me back to the reason for re-opening this thread:
   getting Paul Koning's patch to make read/access watchpoints work
   when HAVE_NONSTEPPABLE_WATCHPOINT is defined accepted.

   I did change one thing in Paul's patch, which should be
   highlighted: the change in bpstat_stop_status previously applied to
   bp_watchpoint types also (in addition to bp_hardware_watchpoint,
   bp_read_watchpoint, and bp_access_watchpoint), but as far as I can
   tell target_stopped_data_address applies only to hardware-assisted
   watchpoints, not software watchpoints.  Maybe that needs to be made
   more clear in the comment.

   Comments?

This patch breaks hardware watchpoints in SVR4-derived systems.  Those
systems don't provide target_stopped_data_address().  The default
target_stopped_data_address() will always return zero, which means
that triggered watchpoints aren't properly caught.  This results in
spurious SIGTRAPS.

Providing target_stopped_data_address() for the SVR4 proc(4) should be
possible, but its a risky bussiness, since there are several slight
variations of that interface.

   2004-04-22  Orjan Friberg <orjanf@axis.com>

	   From Paul Koning <pkoning@equallogic.com>:
	   * breakpoint.c (free_valchain): New function.
	   (insert_bp_location, 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.

Anyway.  The problem is clearly that the whole target-specific
interface for hardware watchpoints is a mess.  We should really try to
*design* a proper interface instead of continuing to tweak the
existing interfaces.  Any people interested in makeing a proposal?

Mark

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Sat, 1 May 2004 23:17:35 +0200 (CEST)
> From: Mark Kettenis <kettenis@chello.nl>
> 
> This patch breaks hardware watchpoints in SVR4-derived systems.  Those
> systems don't provide target_stopped_data_address().  The default
> target_stopped_data_address() will always return zero

Then how do hardware watchpoints work on those systems?  IIRC, without
a functional target_stopped_data_address, hardware watchpoints would
not really work, except by chance and only in simple cases.  For
example, multiple watchpoints at the same address will not DTRT.

> Anyway.  The problem is clearly that the whole target-specific
> interface for hardware watchpoints is a mess.  We should really try to
> *design* a proper interface instead of continuing to tweak the
> existing interfaces.

Agreed.

> Any people interested in makeing a proposal?

I could try, but I need help: I need to know how hardware watchpoints
work on supported platforms, including remote ones.  If global and
area maintainers could describe that for systems they know, I will try
to come up with a proposal.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Orjan Friberg]

Eli Zaretskii wrote:
> 
> I could try, but I need help: I need to know how hardware watchpoints
> work on supported platforms, including remote ones.  If global and
> area maintainers could describe that for systems they know, I will try
> to come up with a proposal.

This for a not-yet-submitted remote target, called CRISv32.

It has 6 hardware data watchpoints, configurable as read/write or both 
(i.e. access).  There are no alignment or length restriction on the 
memory regions that are watched.

When a watchpoint hits, an exception register tells which watchpoints 
that triggered, and whether they trigged on read or write.

I have defined HAVE_NONSTEPPABLE_WATCHPOINT, to make GDB disable the 
watchpoint and step over it to evaluate it.

(In addition there is also one hardware instruction breakpoint, but I 
guess that doesn't matter in this context.)

-- 
Orjan Friberg
Axis Communications

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Andrew Cagney]

> Eli Zaretskii wrote:
> 
>>
>> I could try, but I need help: I need to know how hardware watchpoints
>> work on supported platforms, including remote ones.  If global and
>> area maintainers could describe that for systems they know, I will try
>> to come up with a proposal.
> 
> 
> This for a not-yet-submitted remote target, called CRISv32.
> 
> It has 6 hardware data watchpoints, configurable as read/write or both (i.e. access).  There are no alignment or length restriction on the memory regions that are watched.
> 
> When a watchpoint hits, an exception register tells which watchpoints that triggered, and whether they trigged on read or write.
> 
> I have defined HAVE_NONSTEPPABLE_WATCHPOINT, to make GDB disable the watchpoint and step over it to evaluate it.
> 
> (In addition there is also one hardware instruction breakpoint, but I guess that doesn't matter in this context.)

Check these bug/threads threads for background info.

http://sources.redhat.com/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gdb&pr=967
http://sources.redhat.com/ml/gdb-patches/2002-09/msg00739.html

Andrew

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Mon, 03 May 2004 13:24:32 +0200
> From: Orjan Friberg <orjan.friberg@axis.com>
> 
> This for a not-yet-submitted remote target, called CRISv32.

Thanks!

> It has 6 hardware data watchpoints, configurable as read/write or both 
> (i.e. access).  There are no alignment or length restriction on the 
> memory regions that are watched.
> 
> When a watchpoint hits, an exception register tells which watchpoints 
> that triggered, and whether they trigged on read or write.

So this target could relatively easily support an API where the target
itself tells GDB which of watchpoints triggered and why, provided that
we invent a way for GDB to map between its watchpoint numbers and the
target-size hardware data watchpoints.  Is that true?

> (In addition there is also one hardware instruction breakpoint, but I 
> guess that doesn't matter in this context.)

Used for hardware-assisted breakpoints, yes?

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Mon, 03 May 2004 11:05:18 -0400
> From: Andrew Cagney <cagney@gnu.org>
> 
> Check these bug/threads threads for background info.
> 
> http://sources.redhat.com/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gdb&pr=967
> http://sources.redhat.com/ml/gdb-patches/2002-09/msg00739.html

Thanks for the pointers.

Having read them, I wonder: what targets can benefit from this change?
As similar discussuions in the past indicated, targets such as the
i386, which have a small number of debug registers, can nevertheless
support an infinite number of watchpoints if they watch the same
memory region.  So, in general, for such a machinery to be useful on
x86 at least, we need to pass more info about the watchpoints to get
meaningful results.

In any case, I think this particular issue if whether a target could
accomodate all the watchpoints that GDB wants to insert is not a very
important one.  Currently, GDB simply tries to insert them all, and if
that fails, it barfs.  If the procedure to find out whether they could
be all accomodated is going to be as complex as actually inserting
them, nothing is gained from making TARGET_CAN_USE_HARDWARE_WATCHPOINT
more accurate than it is now.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Andrew Cagney]

>>Date: Mon, 03 May 2004 11:05:18 -0400
>>> From: Andrew Cagney <cagney@gnu.org>
>>> 
>>> Check these bug/threads threads for background info.
>>> 
>>> http://sources.redhat.com/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gdb&pr=967
>>> http://sources.redhat.com/ml/gdb-patches/2002-09/msg00739.html
> 
> 
> Thanks for the pointers.

Also:
http://sources.redhat.com/ml/gdb-patches/2002-09/msg00738.html

> Having read them, I wonder: what targets can benefit from this change?
> As similar discussuions in the past indicated, targets such as the
> i386, which have a small number of debug registers, can nevertheless
> support an infinite number of watchpoints if they watch the same
> memory region.  So, in general, for such a machinery to be useful on
> x86 at least, we need to pass more info about the watchpoints to get
> meaningful results.

I believe that was the conclusion last time.

> In any case, I think this particular issue if whether a target could
> accomodate all the watchpoints that GDB wants to insert is not a very
> important one.  Currently, GDB simply tries to insert them all, and if
> that fails, it barfs.  If the procedure to find out whether they could
> be all accomodated is going to be as complex as actually inserting
> them, nothing is gained from making TARGET_CAN_USE_HARDWARE_WATCHPOINT
> more accurate than it is now.

Andrew

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Orjan Friberg]

Eli Zaretskii wrote:
> 
> So this target could relatively easily support an API where the target
> itself tells GDB which of watchpoints triggered and why, provided that
> we invent a way for GDB to map between its watchpoint numbers and the
> target-size hardware data watchpoints.  Is that true?

Yes (provided the register contents haven't changed since the watchpoint 
hit, of course - I assume the mapping procedure you're describing should 
be done before single-stepping over the watchpoint for targets with 
nonsteppable watchpoints).

Another thing, which may or may not be relevant in this discussion, is 
if the remote protocol should be altered.  Right now, the 
watch/rwatch/awatch part of the stop reply packet when hitting a 
watchpoint is not used (except for parsing out the stopped data 
address).  In addition, it doesn't play nice with multiple watchpoint 
hits, since only one of watch/rwatch/awatch is possible.  I don't have 
any practical suggestion on how to make it more useful though :( .

>>(In addition there is also one hardware instruction breakpoint, but I 
>>guess that doesn't matter in this context.)
> 
> 
> Used for hardware-assisted breakpoints, yes?

(Ugh, I always botch the terminology.)  Yes, invoked by the "hbreak" 
command.

-- 
Orjan Friberg
Axis Communications

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Daniel Jacobowitz]

On Tue, May 04, 2004 at 09:31:11AM +0200, Orjan Friberg wrote:
> Another thing, which may or may not be relevant in this discussion, is 
> if the remote protocol should be altered.  Right now, the 
> watch/rwatch/awatch part of the stop reply packet when hitting a 
> watchpoint is not used (except for parsing out the stopped data 
> address).  In addition, it doesn't play nice with multiple watchpoint 
> hits, since only one of watch/rwatch/awatch is possible.  I don't have 
> any practical suggestion on how to make it more useful though :( .

I think this is a separate discussion.  It's a general problem of both
GDB (breakpoint.c in particular) and also of the remote protocol.

I'm avoiding that discussion until I finish a dozen other projects :)

-- 
Daniel Jacobowitz

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> From: Ulrich Weigand <weigand@i1.informatik.uni-erlangen.de>
> Date: Thu, 6 May 2004 23:34:10 +0200 (CEST)
> 
> Well, the current behaviour of GDB is correct on targets like s390
> that cannot implement target_stopped_data_address, while your suggested
> change would break such targets.   May I suggest that your change be
> implemented conditionally on whether the target supports it?
> 
> > So right now, target_stopped_data_address is almost an alias for
> > STOPPED_BY_WATCHPOINT, but it is IMHO wrong to continue this illusion
> > into the future.  Therefore, I like your patch better than the
> > alternative which would modify target_stopped_data_address.
> 
> Actually I didn't suggest modifying target_stopped_data_address.

Someone, perhaps not you, posted a patch that uses
STOPPED_BY_WATCHPOINT.  That was the patch I liked better; I
understand that it would work for s390 as well.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Thu, 6 May 2004 17:36:34 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> This means that write watchpoints which use the same value as the
> previous value wouldn't trigger, on platforms where we could support
> that; which is kind of a shame.

Exactly.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Thu, 6 May 2004 14:04:50 -0400
> From: Daniel Jacobowitz <drow@false.org>
> 
> I'm saying that I don't think exposing the internals of the execution
> state machine to the separate module responsible for breakpoints, even
> to a function only used by the execution state machine, is a good idea.
> If two interfaces of STOPPED_BY_WATCHPOINT need access to something
> from ecs, then pass it independently, or come up with a better-defined
> interface.

If the issue is whether to pass the entire structure or some of its
members, then we I don't mind either way and thus we don't really
disagree; modifying the arguments passed to STOPPED_BY_WATCHPOINT does
not count as redesigning the interface in my book.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> From: Ulrich Weigand <weigand@i1.informatik.uni-erlangen.de>
> Date: Thu, 6 May 2004 23:38:02 +0200 (CEST)
> 
> Because a write watchpoints can be handled without hardware support to
> provide the address

They can be handled to some extent, but once there are complications,
like the one that started this thread, the current scheme pretty much
collapses.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Paul Koning]

>>>>> "Ulrich" == Ulrich Weigand <weigand@i1.informatik.uni-erlangen.de> writes:

 >> My experience was that the old code worked if you had only a
 >> watchpoint active, but it would produce the wrong results if you
 >> had both watchpoints and breakpoints active.  The reason was that
 >> the scan for matching break/watch points would conclude that the
 >> target break had happened due to a non-matching watchpoint and
 >> would proceed, rather than break.

 Ulrich> But this error scenario should only apply to read/access
 Ulrich> watchpoints, never write watchpoints.  A write watchpoint
 Ulrich> should never be misdetected ...

Sorry, I wasn't clear in the explanation, faulty memory.

The actual explanation: a "hard breakpoint" ("break" instruction
assembled-in, rather than inserted by gdb) is detected by the fact
that nothing matches in the scan of the break/watch point list.

The original code would work correctly if only breakpoints were set.
If watchpoints were also set, a hard breakpoint would be treated as a
mismatched watchpoint, and gdb would just continue rather than
stopping.  

 >> That doesn't seem like a good idea.  Why would it be reasonable to
 >> treat the two differently?

 Ulrich> Because a write watchpoints can be handled without hardware
 Ulrich> support to provide the address, while read/access watchpoints
 Ulrich> fundamentally cannot be.

Except for the issue that Daniel just mentioned (foo = 1; foo = 1;)

       paul

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Ulrich Weigand]

> My experience was that the old code worked if you had only a
> watchpoint active, but it would produce the wrong results if you had
> both watchpoints and breakpoints active.  The reason was that the scan
> for matching break/watch points would conclude that the target break
> had happened due to a non-matching watchpoint and would proceed,
> rather than break.

But this error scenario should only apply to read/access watchpoints,
never write watchpoints.  A write watchpoint should never be misdetected ...

> That doesn't seem like a good idea.  Why would it be reasonable to
> treat the two differently?

Because a write watchpoints can be handled without hardware support to
provide the address, while read/access watchpoints fundamentally
cannot be.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Daniel Jacobowitz]

On Thu, May 06, 2004 at 11:34:10PM +0200, Ulrich Weigand wrote:
> This simply ensures that target_stopped_data_address is only ever used
> for read or access watchpoints (where it is in fact required).  Targets
> that don't support it would still be able to support hardware write
> watchpoints.

This means that write watchpoints which use the same value as the
previous value wouldn't trigger, on platforms where we could support
that; which is kind of a shame.

-- 
Daniel Jacobowitz

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Ulrich Weigand]

> So the s390(x) has a means of telling that some watchpoint fired, but
> there's no way to know which one, is that true?
Exactly.

> If the hardware supports only write watchpoints, and since you don't
> know what address triggered a watchpoint, this would seem to imply
> that there's no way of getting awatch and rwatch commands to give
> useful results, is that right?  
Yes.

> So what does GDB on the s390 do when the user tries to invoke 
> awatch or rwatch?
It should probably fail; I'll make sure of this ...

> > Alternatively, we could also make the target_stopped_data_address
> > check only for read and access watch points, *not* for write
> > watchpoints ...
> 
> GDB should use the result of target_stopped_data_address to find out
> which watchpoints are candidates for being a possible reason for
> causing SIGTRAP.  GDB doesn't do so right now, but that's because the
> hardware watchpoints handling is an incremental modification of the
> original code that handled only software watchpoints.

Well, the current behaviour of GDB is correct on targets like s390
that cannot implement target_stopped_data_address, while your suggested
change would break such targets.   May I suggest that your change be
implemented conditionally on whether the target supports it?

> So right now, target_stopped_data_address is almost an alias for
> STOPPED_BY_WATCHPOINT, but it is IMHO wrong to continue this illusion
> into the future.  Therefore, I like your patch better than the
> alternative which would modify target_stopped_data_address.

Actually I didn't suggest modifying target_stopped_data_address.
I mean something like:

Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.170
diff -c -p -r1.170 breakpoint.c
*** gdb/breakpoint.c    2 May 2004 00:21:41 -0000       1.170
--- gdb/breakpoint.c    6 May 2004 21:31:30 -0000
*************** bpstat_stop_status (CORE_ADDR bp_addr, p
*** 2628,2635 ****
         address).  Otherwise gdb won't stop on a break instruction in the code
         (not from a breakpoint) when a hardware 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
--- 2628,2635 ----
         address).  Otherwise gdb won't stop on a break instruction in the code
         (not from a breakpoint) when a hardware 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


This simply ensures that target_stopped_data_address is only ever used
for read or access watchpoints (where it is in fact required).  Targets
that don't support it would still be able to support hardware write
watchpoints.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Daniel Jacobowitz]

On Thu, May 06, 2004 at 08:59:06PM +0200, Eli Zaretskii wrote:
> > Date: Thu, 6 May 2004 10:21:46 -0400
> > From: Daniel Jacobowitz <drow@false.org>
> > > 
> > > Thus, for the STOPPED_BY_WATCHPOINT trick to work, bpstat_stop_status
> > > needs to have access to the relevant parts of the inferior's struct
> > > execution_control_state variable.  It doesn't seem to me too hard to
> > > pass that as an additional argument.
> > 
> > This is the point where I want to go back to the "design" bit that Mark
> > was talking about.
> 
> Redesigning watchpoint handling doesn't mean we should abandon fixing
> bugs in the meantime, IMHO.
> 
> > execution_control_state is private to infrun.c, and
> > I would prefer to leave it that way; the more localized it is, the
> > easier it is to clean up and maintain, without having to rip tentacles
> > out of other parts of GDB.
> 
> Sorry, I don't understand: bpstat_stop_status is called by infrun, so
> passing it execution_control_state doesn't violate its being private
> to infrun.c.

I don't understand your logic.  Right now, the type "struct
execution_control_state" is private to the file "infrun.c", which is
the execution state machine.

The function "bpstat_stop_status" is in the file "breakpoint.c", which
is the bulk of GDB's breakpoint management code.

I'm saying that I don't think exposing the internals of the execution
state machine to the separate module responsible for breakpoints, even
to a function only used by the execution state machine, is a good idea.
If two interfaces of STOPPED_BY_WATCHPOINT need access to something
from ecs, then pass it independently, or come up with a better-defined
interface.

-- 
Daniel Jacobowitz

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Thu, 6 May 2004 10:21:46 -0400
> From: Daniel Jacobowitz <drow@false.org>
> > 
> > Thus, for the STOPPED_BY_WATCHPOINT trick to work, bpstat_stop_status
> > needs to have access to the relevant parts of the inferior's struct
> > execution_control_state variable.  It doesn't seem to me too hard to
> > pass that as an additional argument.
> 
> This is the point where I want to go back to the "design" bit that Mark
> was talking about.

Redesigning watchpoint handling doesn't mean we should abandon fixing
bugs in the meantime, IMHO.

> execution_control_state is private to infrun.c, and
> I would prefer to leave it that way; the more localized it is, the
> easier it is to clean up and maintain, without having to rip tentacles
> out of other parts of GDB.

Sorry, I don't understand: bpstat_stop_status is called by infrun, so
passing it execution_control_state doesn't violate its being private
to infrun.c.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Daniel Jacobowitz]

On Thu, May 06, 2004 at 07:59:34AM +0200, Eli Zaretskii wrote:
> > Date: Wed, 05 May 2004 10:25:26 +0200
> > From: Orjan Friberg <orjan.friberg@axis.com>
> > 
> > Note that tm-frv.h and nm-hppah.h actually use the struct 
> > target_waitstatus that is normally passed as an argument to the 
> > STOPPED_BY_WATCHPOINT macro.
> 
> Thanks for the heads-up.
> 
> Thus, for the STOPPED_BY_WATCHPOINT trick to work, bpstat_stop_status
> needs to have access to the relevant parts of the inferior's struct
> execution_control_state variable.  It doesn't seem to me too hard to
> pass that as an additional argument.

This is the point where I want to go back to the "design" bit that Mark
was talking about.  execution_control_state is private to infrun.c, and
I would prefer to leave it that way; the more localized it is, the
easier it is to clean up and maintain, without having to rip tentacles
out of other parts of GDB.

-- 
Daniel Jacobowitz

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Paul Koning]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

 >> The trouble I ran into is that this doesn't work because (on
 >> remote target support for MIPS anyway) the watchpoint handling
 >> does an effective "stepi" after the watchpoint stop.  After that
 >> stepi, stopped_by_watchpoint() would return false because the most
 >> recent stop was due to a break (the stepi).

 Eli> But that is something the target-side code could handle, right?
 Eli> I mean, stopped_by_watchpoint has intimate knowledge about the
 Eli> target's particulars, so it could fiddle the stop reason if it
 Eli> saw the indication that a watchpoint fired _and_ that the most
 Eli> recent stop was due to a single-step, right?

 Eli> Failing that, we could have something like
 Eli> AUTO_STEPI_AFTER_WATCHPOINT in higher-level code in GDB
 Eli> (infrun.c or breakpoint.c) to handle such targets, but I
 Eli> generally think that the GDB application-level code should not
 Eli> bother itself with target-specific peculiarities if we can avoid
 Eli> that.

I'd have to do a bunch of memory refreshing to remind myself of the
various possibilities you're talking about...

History: I started from a remote stub that was very poorly
implemented, and handled things by faking a single-step locally.  This
was extremely ugly.

I observed that gdb had machinery to handle all this cleanly from the
gdb end, so I turned it on.  And sure enough, it worked for some
cases.  But not for all, for the reasons I mentioned.  I then looked
for a way to fix that with minimal perturbation to the existing
machinery.  

My conclusion was: the bug is that the internally generated stepi
causese the "stopped by watchpoint" status and associated address to
be forgotten, so the best fix is not to forget them.  That's what I
did.  The other bug was that the break/watch point list traversal
routine would mishandle some cases where no match was found, which is
why I changed that area, again trying to do minimal change to the
logic and fixing only the place where the code didn't do what the
apparent intent was.

As for gdb not doing target-specific peculiarities -- I'd say that
HAVE_NONSTEPPABLE_WATCHPOINTS is the way to pass that knowledge
around.  Having it where it lives today is clean because (as far as I
can tell) it puts the necessary special handling near where
break/watch/step live.  Trying to do it further down means you end up
replicating a lot more of the magic involved in stepping.  The mess I
started out with (stepi "implemented" at the remote stub, transparent
to gdb) convinced me of this.

	 paul

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Wed, 5 May 2004 09:44:30 -0400
> From: Paul Koning <pkoning@equallogic.com>
> 
>  Ulrich> We don't define target_stopped_data_address, and *cannot* do
>  Ulrich> so because the hardware doesn't provide this information.
> 
> Neither does MIPS, unless you disassemble the trapping instruction to
> deduce the address.  That's what I've done in our MIPS remote gdb
> stub.

Would the suggested STOPPED_BY_WATCHPOINT patch work for the MIPS
remote stub?

> The trouble I ran into is that this doesn't work because (on remote
> target support for MIPS anyway) the watchpoint handling does an
> effective "stepi" after the watchpoint stop.  After that stepi,
> stopped_by_watchpoint() would return false because the most recent
> stop was due to a break (the stepi).

But that is something the target-side code could handle, right?  I
mean, stopped_by_watchpoint has intimate knowledge about the target's
particulars, so it could fiddle the stop reason if it saw the
indication that a watchpoint fired _and_ that the most recent stop was
due to a single-step, right?

Failing that, we could have something like AUTO_STEPI_AFTER_WATCHPOINT
in higher-level code in GDB (infrun.c or breakpoint.c) to handle such
targets, but I generally think that the GDB application-level code
should not bother itself with target-specific peculiarities if we can
avoid that.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> Date: Wed, 05 May 2004 10:25:26 +0200
> From: Orjan Friberg <orjan.friberg@axis.com>
> 
> Note that tm-frv.h and nm-hppah.h actually use the struct 
> target_waitstatus that is normally passed as an argument to the 
> STOPPED_BY_WATCHPOINT macro.

Thanks for the heads-up.

Thus, for the STOPPED_BY_WATCHPOINT trick to work, bpstat_stop_status
needs to have access to the relevant parts of the inferior's struct
execution_control_state variable.  It doesn't seem to me too hard to
pass that as an additional argument.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Paul Koning]

>>>>> "Ulrich" == Ulrich Weigand <weigand@i1.informatik.uni-erlangen.de> writes:

 Ulrich> Mark Kettenis wrote:
 >> This patch breaks hardware watchpoints in SVR4-derived systems.
 >> Those systems don't provide target_stopped_data_address().  The
 >> default target_stopped_data_address() will always return zero,
 >> which means that triggered watchpoints aren't properly caught.
 >> This results in spurious SIGTRAPS.

 Ulrich> The patch also breaks s390(x), for exactly this reason.

 Ulrich> We don't define target_stopped_data_address, and *cannot* do
 Ulrich> so because the hardware doesn't provide this information.

Neither does MIPS, unless you disassemble the trapping instruction to
deduce the address.  That's what I've done in our MIPS remote gdb
stub.

 Ulrich> We do know whether a SIGTRAP was due to a (any) watchpoint or
 Ulrich> not, however, and define STOPPED_BY_WATCHPOINT to indicate
 Ulrich> this.

 Ulrich> (Since the hardware supports only write watchpoints, not read
 Ulrich> or access watchpoints, this should -and used to- be enough
 Ulrich> for gdb to find out what happened by manually checking
 Ulrich> watchpoint values.)

My experience was that the old code worked if you had only a
watchpoint active, but it would produce the wrong results if you had
both watchpoints and breakpoints active.  The reason was that the scan
for matching break/watch points would conclude that the target break
had happened due to a non-matching watchpoint and would proceed,
rather than break.

 Ulrich> The patch below uses STOPPED_BY_WATCHPOINT instead of
 Ulrich> target_stopped_data_address in bpstat_stop_status; this get
 Ulrich> s390(x) watchpoints back functional.

My memory is a little faded by now, but I think I tried that at first.

The trouble I ran into is that this doesn't work because (on remote
target support for MIPS anyway) the watchpoint handling does an
effective "stepi" after the watchpoint stop.  After that stepi,
stopped_by_watchpoint() would return false because the most recent
stop was due to a break (the stepi).  That's why
remote_stopped_data_address still returns non-zero if
stepped_after_stopped_by_watchpoint is set.

There may well be a better way to do this, but this is the reason why
I did it this way, as far as I can reconstruct it now.

 Ulrich> Alternatively, we could also make the
 Ulrich> target_stopped_data_address check only for read and access
 Ulrich> watch points, *not* for write watchpoints ...

That doesn't seem like a good idea.  Why would it be reasonable to
treat the two differently?

      paul

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Orjan Friberg]

Eli Zaretskii wrote:
> 
>>The patch below uses STOPPED_BY_WATCHPOINT instead of 
>>target_stopped_data_address in bpstat_stop_status;
>>this get s390(x) watchpoints back functional.
> 
> 
> Thanks, I think we should make this change.

Note that tm-frv.h and nm-hppah.h actually use the struct 
target_waitstatus that is normally passed as an argument to the 
STOPPED_BY_WATCHPOINT macro.

-- 
Orjan Friberg
Axis Communications

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Eli Zaretskii]

> From: Ulrich Weigand <weigand@i1.informatik.uni-erlangen.de>
> Date: Wed, 5 May 2004 00:08:29 +0200 (CEST)
> 
> We don't define target_stopped_data_address, and *cannot* do so
> because the hardware doesn't provide this information.
> 
> We do know whether a SIGTRAP was due to a (any) watchpoint or not,
> however, and define STOPPED_BY_WATCHPOINT to indicate this.

So the s390(x) has a means of telling that some watchpoint fired, but
there's no way to know which one, is that true?

> (Since the hardware supports only write watchpoints, not read
> or access watchpoints, this should -and used to- be enough for
> gdb to find out what happened by manually checking watchpoint
> values.)

If the hardware supports only write watchpoints, and since you don't
know what address triggered a watchpoint, this would seem to imply
that there's no way of getting awatch and rwatch commands to give
useful results, is that right?  So what does GDB on the s390 do when
the user tries to invoke awatch or rwatch?

> The patch below uses STOPPED_BY_WATCHPOINT instead of 
> target_stopped_data_address in bpstat_stop_status;
> this get s390(x) watchpoints back functional.

Thanks, I think we should make this change.

Mark, can the SVR4-derived systems be fixed by this patch?  If so, we
at least have the same functionality we had before Orjan's patch,
without reintroducing the problem he fixed.

> Alternatively, we could also make the target_stopped_data_address
> check only for read and access watch points, *not* for write
> watchpoints ...

GDB should use the result of target_stopped_data_address to find out
which watchpoints are candidates for being a possible reason for
causing SIGTRAP.  GDB doesn't do so right now, but that's because the
hardware watchpoints handling is an incremental modification of the
original code that handled only software watchpoints.

So right now, target_stopped_data_address is almost an alias for
STOPPED_BY_WATCHPOINT, but it is IMHO wrong to continue this illusion
into the future.  Therefore, I like your patch better than the
alternative which would modify target_stopped_data_address.

Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

[Ulrich Weigand]

Mark Kettenis wrote:

>This patch breaks hardware watchpoints in SVR4-derived systems.  Those
>systems don't provide target_stopped_data_address().  The default
>target_stopped_data_address() will always return zero, which means
>that triggered watchpoints aren't properly caught.  This results in
>spurious SIGTRAPS.

The patch also breaks s390(x), for exactly this reason.

We don't define target_stopped_data_address, and *cannot* do so
because the hardware doesn't provide this information.

We do know whether a SIGTRAP was due to a (any) watchpoint or not,
however, and define STOPPED_BY_WATCHPOINT to indicate this.

(Since the hardware supports only write watchpoints, not read
or access watchpoints, this should -and used to- be enough for
gdb to find out what happened by manually checking watchpoint
values.)

The patch below uses STOPPED_BY_WATCHPOINT instead of 
target_stopped_data_address in bpstat_stop_status;
this get s390(x) watchpoints back functional.

Alternatively, we could also make the target_stopped_data_address
check only for read and access watch points, *not* for write
watchpoints ...

What do you think?


Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.170
diff -c -p -r1.170 breakpoint.c
*** gdb/breakpoint.c	2 May 2004 00:21:41 -0000	1.170
--- gdb/breakpoint.c	4 May 2004 21:20:10 -0000
*************** bpstat_stop_status (CORE_ADDR bp_addr, p
*** 2631,2637 ****
  	&& !((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
--- 2631,2637 ----
  	&& !((b->type == bp_hardware_watchpoint
  	      || b->type == bp_read_watchpoint
  	      || b->type == bp_access_watchpoint)
! 	     && STOPPED_BY_WATCHPOINT ())
  	&& b->type != bp_hardware_breakpoint
  	&& b->type != bp_catch_fork
  	&& b->type != bp_catch_vfork

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de