Re: Display of read/access watchpoints when HAVE_NONSTEPPABLE_WATCHPOINT

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

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

[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]

> 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

[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

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

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

[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]

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

[PATCH] Fix watchpoints on s390

[Ulrich Weigand]

Hello,

this is the second attempt to fix watchpoints on s390 by using
STOPPED_BY_WATCHPOINT instead of target_stopped_data_address
in bpstat_stop_status to find out whether the target stopped
due to a hardware watchpoint.

As STOPPED_BY_WATCHPOINT requires an argument not available
in breakpoint.c, and furthermore some targets define this
macro so as to access other local variables not even passed
as arguments, calling this macro from anywhere else is not
trivial.

Thus, this patch simply remembers the return value of 
STOPPED_BY_WATCHPOINT in handle_inferior_event, and passes
*that* as argument to bpstat_stop_status.  This should
work on all targets ...

Tested on s390-ibm-linux and s390x-ibm-linux, fixes all
watchpoint related regressions.

Bye,
Ulrich


ChangeLog:

	* breakpoint.c (bpstat_stop_status): Add new argument
	STOPPED_BY_WATCHPOINT.  Use it instead of testing
	target_stopped_data_address agaist 0 to check whether
	or not we stopped due to a hardware watchpoint.
	* breakpoint.h (bpstat_stop_status): Adapt prototype.
	* infrun.c (handle_inferior_event): Call bpstat_stop_status
	with new argument.

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	10 May 2004 17:13:49 -0000
*************** which its expression is valid.\n");     
*** 2590,2596 ****
  }
  
  /* Get a bpstat associated with having just stopped at address
!    BP_ADDR.  */
  
  /* Determine whether we stopped at a breakpoint, etc, or whether we
     don't understand this stop.  Result is a chain of bpstat's such that:
--- 2590,2597 ----
  }
  
  /* Get a bpstat associated with having just stopped at address
!    BP_ADDR in thread PTID.  STOPPED_BY_WATCHPOINT is true if the
!    target thinks we stopped due to a hardware watchpoint.  */
  
  /* Determine whether we stopped at a breakpoint, etc, or whether we
     don't understand this stop.  Result is a chain of bpstat's such that:
*************** which its expression is valid.\n");     
*** 2607,2613 ****
     commands, FIXME??? fields.  */
  
  bpstat
! bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid)
  {
    struct breakpoint *b, *temp;
    /* True if we've hit a breakpoint (as opposed to a watchpoint).  */
--- 2608,2614 ----
     commands, FIXME??? fields.  */
  
  bpstat
! bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid, int stopped_by_watchpoint)
  {
    struct breakpoint *b, *temp;
    /* True if we've hit a breakpoint (as opposed to a watchpoint).  */
*************** 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
--- 2632,2638 ----
  	&& !((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
Index: gdb/breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.32
diff -c -p -r1.32 breakpoint.h
*** gdb/breakpoint.h	8 Apr 2004 21:18:12 -0000	1.32
--- gdb/breakpoint.h	10 May 2004 17:13:49 -0000
*************** extern void bpstat_clear (bpstat *);
*** 414,420 ****
     is part of the bpstat is copied as well.  */
  extern bpstat bpstat_copy (bpstat);
  
! extern bpstat bpstat_stop_status (CORE_ADDR pc, ptid_t ptid);
  \f
  /* This bpstat_what stuff tells wait_for_inferior what to do with a
     breakpoint (a challenging task).  */
--- 414,421 ----
     is part of the bpstat is copied as well.  */
  extern bpstat bpstat_copy (bpstat);
  
! extern bpstat bpstat_stop_status (CORE_ADDR pc, ptid_t ptid, 
! 				  int stopped_by_watchpoint);
  \f
  /* This bpstat_what stuff tells wait_for_inferior what to do with a
     breakpoint (a challenging task).  */
Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.151
diff -c -p -r1.151 infrun.c
*** gdb/infrun.c	1 May 2004 14:15:19 -0000	1.151
--- gdb/infrun.c	10 May 2004 17:13:49 -0000
*************** handle_inferior_event (struct execution_
*** 1367,1372 ****
--- 1367,1373 ----
       defined in the file "config/pa/nm-hppah.h", accesses the variable
       indirectly.  Mutter something rude about the HP merge.  */
    int sw_single_step_trap_p = 0;
+   int stopped_by_watchpoint = 0;
  
    /* Cache the last pid/waitstatus. */
    target_last_wait_ptid = ecs->ptid;
*************** handle_inferior_event (struct execution_
*** 1556,1562 ****
  
        stop_pc = read_pc ();
  
!       stop_bpstat = bpstat_stop_status (stop_pc, ecs->ptid);
  
        ecs->random_signal = !bpstat_explains_signal (stop_bpstat);
  
--- 1557,1563 ----
  
        stop_pc = read_pc ();
  
!       stop_bpstat = bpstat_stop_status (stop_pc, ecs->ptid, 0);
  
        ecs->random_signal = !bpstat_explains_signal (stop_bpstat);
  
*************** handle_inferior_event (struct execution_
*** 1605,1611 ****
        ecs->saved_inferior_ptid = inferior_ptid;
        inferior_ptid = ecs->ptid;
  
!       stop_bpstat = bpstat_stop_status (stop_pc, ecs->ptid);
  
        ecs->random_signal = !bpstat_explains_signal (stop_bpstat);
        inferior_ptid = ecs->saved_inferior_ptid;
--- 1606,1612 ----
        ecs->saved_inferior_ptid = inferior_ptid;
        inferior_ptid = ecs->ptid;
  
!       stop_bpstat = bpstat_stop_status (stop_pc, ecs->ptid, 0);
  
        ecs->random_signal = !bpstat_explains_signal (stop_bpstat);
        inferior_ptid = ecs->saved_inferior_ptid;
*************** handle_inferior_event (struct execution_
*** 1921,1927 ****
  
    /* It may be possible to simply continue after a watchpoint.  */
    if (HAVE_CONTINUABLE_WATCHPOINT)
!     STOPPED_BY_WATCHPOINT (ecs->ws);
  
    ecs->stop_func_start = 0;
    ecs->stop_func_end = 0;
--- 1922,1928 ----
  
    /* It may be possible to simply continue after a watchpoint.  */
    if (HAVE_CONTINUABLE_WATCHPOINT)
!     stopped_by_watchpoint = STOPPED_BY_WATCHPOINT (ecs->ws);
  
    ecs->stop_func_start = 0;
    ecs->stop_func_end = 0;
*************** handle_inferior_event (struct execution_
*** 2007,2013 ****
        else
  	{
  	  /* See if there is a breakpoint at the current PC.  */
! 	  stop_bpstat = bpstat_stop_status (stop_pc, ecs->ptid);
  
  	  /* Following in case break condition called a
  	     function.  */
--- 2008,2015 ----
        else
  	{
  	  /* See if there is a breakpoint at the current PC.  */
! 	  stop_bpstat = bpstat_stop_status (stop_pc, ecs->ptid, 
! 					    stopped_by_watchpoint);
  
  	  /* Following in case break condition called a
  	     function.  */

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

Re: [PATCH] Fix watchpoints on s390

[Eli Zaretskii]

> From: Ulrich Weigand <weigand@i1.informatik.uni-erlangen.de>
> Date: Mon, 10 May 2004 19:38:29 +0200 (CEST)
> 
> this is the second attempt to fix watchpoints on s390 by using
> STOPPED_BY_WATCHPOINT instead of target_stopped_data_address
> in bpstat_stop_status to find out whether the target stopped
> due to a hardware watchpoint.
> 
> As STOPPED_BY_WATCHPOINT requires an argument not available
> in breakpoint.c, and furthermore some targets define this
> macro so as to access other local variables not even passed
> as arguments, calling this macro from anywhere else is not
> trivial.
> 
> Thus, this patch simply remembers the return value of 
> STOPPED_BY_WATCHPOINT in handle_inferior_event, and passes
> *that* as argument to bpstat_stop_status.  This should
> work on all targets ...

This patch is fine with me.

Thanks.

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

[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

[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

[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

[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

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