public inbox for gdb@sourceware.org
 help / color / mirror / Atom feed
From: Daniel Jacobowitz <drow@false.org>
To: Vladimir Prus <ghost@cs.msu.su>
Cc: Eli Zaretskii <eliz@gnu.org>, gdb@sources.redhat.com
Subject: Re: read watchpoints ignored?
Date: Mon, 14 Nov 2005 14:41:00 -0000	[thread overview]
Message-ID: <20051114144118.GA22190@nevyn.them.org> (raw)
In-Reply-To: <200511141310.38302.ghost@cs.msu.su>

On Mon, Nov 14, 2005 at 01:10:37PM +0300, Vladimir Prus wrote:
> > > > This should be controlled by an architecture method indicating that
> > > > there are only access watchpoints.
> > >
> > > And then what? disable `rwatch'?  I object to such ``solution'', since
> > > the emulated read watchpoints proved to be most useful to me since the
> > > code we discuss was introduced.
> >
> > I thought this was fairly straightforward, so I didn't go into detail.
> > Certainly there's no reason to disable rwatch.  Conditionalize "skip
> > this watchpoint if the value has changed" on "this is really an access
> > watchpoint because the target does not support read watchpoints".
> >
> > It should be trivial to fix if you had a platform with read watchpoints
> > handy.  Which I don't easily.
> 
> .. I can try adding extra method to target ops, and for remote target, 
> implement it by returning the value of 
>  
>    remote_protocol_Z[Z_PACKET_READ_WP].support
> 
> Should I do this?

Hmm, hmm.  For remote targets we want to conditionalize this on the
read watchpoint packet.  For native targets I was thinking we would
need an architecture (gdbarch) method, but now that we have target
vector inheritance going, we probably don't - a target method would
work OK too.  Or we can just do it this way, even better.

Could you give this patch a try?  It will need some tweaking to build
for any native target but i386 but i386 native and remote should work. 
And I've tested that it fixes the bug for i386 native.

If it works for you I'll finish the mechanical conversion.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.760
diff -u -p -r1.760 Makefile.in
--- Makefile.in	7 Nov 2005 15:27:06 -0000	1.760
+++ Makefile.in	14 Nov 2005 14:39:18 -0000
@@ -2090,7 +2090,7 @@ i386v4-nat.o: i386v4-nat.c $(defs_h) $(v
 	$(i386_tdep_h) $(i387_tdep_h) $(gregset_h)
 i386v-nat.o: i386v-nat.c $(defs_h) $(frame_h) $(inferior_h) $(language_h) \
 	$(gdbcore_h) $(gdb_stat_h) $(floatformat_h) $(target_h) \
-	$(i386_tdep_h)
+	$(i386_tdep_h) $(breakpoint_h)
 i387-tdep.o: i387-tdep.c $(defs_h) $(doublest_h) $(floatformat_h) $(frame_h) \
 	$(gdbcore_h) $(inferior_h) $(language_h) $(regcache_h) $(value_h) \
 	$(gdb_assert_h) $(gdb_string_h) $(i386_tdep_h) $(i387_tdep_h)
Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.217
diff -u -p -r1.217 breakpoint.c
--- breakpoint.c	29 May 2005 03:13:17 -0000	1.217
+++ breakpoint.c	14 Nov 2005 14:39:19 -0000
@@ -992,7 +992,8 @@ insert_bp_location (struct bp_location *
 		      else if (bpt->owner->type == bp_access_watchpoint)
 			type = hw_access;
 
-		      val = target_insert_watchpoint (addr, len, type);
+		      val = target_insert_watchpoint (addr, len, &type);
+		      bpt->hw_type = type;
 		      if (val == -1)
 			{
 			  /* Don't exit the loop, try to insert
@@ -1511,17 +1512,12 @@ remove_breakpoint (struct bp_location *b
 		      && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
 		{
 		  CORE_ADDR addr;
-		  int len, type;
+		  int len;
 
 		  addr = VALUE_ADDRESS (v) + value_offset (v);
 		  len = TYPE_LENGTH (value_type (v));
-		  type   = hw_write;
-		  if (b->owner->type == bp_read_watchpoint)
-		    type = hw_read;
-		  else if (b->owner->type == bp_access_watchpoint)
-		    type = hw_access;
 
-		  val = target_remove_watchpoint (addr, len, type);
+		  val = target_remove_watchpoint (addr, len, b->hw_type);
 		  if (val == -1)
 		    b->inserted = 1;
 		  val = 0;
@@ -2793,7 +2789,8 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
 		/* Stop.  */
 		break;
 	      case WP_VALUE_CHANGED:
-		if (b->type == bp_read_watchpoint)
+		if (b->type == bp_read_watchpoint
+		    && b->loc->hw_type == hw_access)
 		  {
 		    /* Don't stop: read watchpoints shouldn't fire if
 		       the value has changed.  This is for targets
@@ -4029,6 +4026,7 @@ allocate_bp_location (struct breakpoint 
     case bp_read_watchpoint:
     case bp_access_watchpoint:
       loc->loc_type = bp_loc_hardware_watchpoint;
+      loc->hw_type = hw_none;
       break;
     case bp_watchpoint:
     case bp_catch_fork:
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.35
diff -u -p -r1.35 breakpoint.h
--- breakpoint.h	26 May 2005 20:48:57 -0000	1.35
+++ breakpoint.h	14 Nov 2005 14:39:19 -0000
@@ -182,7 +182,8 @@ enum target_hw_bp_type
     hw_write   = 0, 		/* Common  HW watchpoint */
     hw_read    = 1, 		/* Read    HW watchpoint */
     hw_access  = 2, 		/* Access  HW watchpoint */
-    hw_execute = 3		/* Execute HW breakpoint */
+    hw_execute = 3,		/* Execute HW breakpoint */
+    hw_none			/* Sentinel value.  */
   };
 
 /* GDB maintains two types of information about each breakpoint (or
@@ -256,6 +257,12 @@ struct bp_location
      which to place the breakpoint in order to comply with a
      processor's architectual constraints.  */
   CORE_ADDR requested_address;
+
+  /* For hardware breakpoints, the type of hardware breakpoint currently
+     used to implement it.  For instance, a read watchpoint may be
+     implemented by an access (read/write) watchpoint.  This is only
+     valid while the breakpoint is inserted.  */
+  enum target_hw_bp_type hw_type;
 };
 
 /* This structure is a collection of function pointers that, if available,
Index: i386-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-nat.c,v
retrieving revision 1.12
diff -u -p -r1.12 i386-nat.c
--- i386-nat.c	10 Sep 2005 18:11:02 -0000	1.12
+++ i386-nat.c	14 Nov 2005 14:39:19 -0000
@@ -502,22 +502,26 @@ Invalid value %d of operation in i386_ha
    of the type TYPE.  Return 0 on success, -1 on failure.  */
 
 int
-i386_insert_watchpoint (CORE_ADDR addr, int len, int type)
+i386_insert_watchpoint (CORE_ADDR addr, int len, int *type)
 {
   int retval;
 
+  if (*type == hw_read)
+    /* The i386 does not support read watchpoints.  */
+    *type = hw_access;
+
   if (((len != 1 && len !=2 && len !=4) && !(TARGET_HAS_DR_LEN_8 && len == 8))
       || addr % len != 0)
-    retval = i386_handle_nonaligned_watchpoint (WP_INSERT, addr, len, type);
+    retval = i386_handle_nonaligned_watchpoint (WP_INSERT, addr, len, *type);
   else
     {
-      unsigned len_rw = i386_length_and_rw_bits (len, type);
+      unsigned len_rw = i386_length_and_rw_bits (len, *type);
 
       retval = i386_insert_aligned_watchpoint (addr, len_rw);
     }
 
   if (maint_show_dr)
-    i386_show_dr ("insert_watchpoint", addr, len, type);
+    i386_show_dr ("insert_watchpoint", addr, len, *type);
 
   return retval;
 }
Index: i386v-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/i386v-nat.c,v
retrieving revision 1.15
diff -u -p -r1.15 i386v-nat.c
--- i386v-nat.c	7 Sep 2004 21:55:10 -0000	1.15
+++ i386v-nat.c	14 Nov 2005 14:39:19 -0000
@@ -34,6 +34,7 @@
 #include "inferior.h"
 #include "language.h"
 #include "gdbcore.h"
+#include "breakpoint.h"
 
 #include <sys/param.h>
 #include <sys/dir.h>
@@ -125,9 +126,13 @@ static int i386_insert_nonaligned_watchp
 /* Insert a watchpoint.  */
 
 int
-i386_insert_watchpoint (int pid, CORE_ADDR addr, int len, int rw)
+i386_insert_watchpoint (int pid, CORE_ADDR addr, int len, int *rw)
 {
-  return i386_insert_aligned_watchpoint (pid, addr, addr, len, rw);
+  /* The i386 does not support read watchpoints.  */
+  if (*rw == hw_read)
+    *rw = hw_access;
+
+  return i386_insert_aligned_watchpoint (pid, addr, addr, len, *rw);
 }
 
 static int
Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.195
diff -u -p -r1.195 remote.c
--- remote.c	20 Jul 2005 02:56:43 -0000	1.195
+++ remote.c	14 Nov 2005 14:39:20 -0000
@@ -4609,12 +4609,21 @@ watchpoint_to_Z_packet (int type)
 }
 
 static int
-remote_insert_watchpoint (CORE_ADDR addr, int len, int type)
+remote_insert_watchpoint (CORE_ADDR addr, int len, int *type)
 {
   struct remote_state *rs = get_remote_state ();
   char *buf = alloca (rs->remote_packet_size);
   char *p;
-  enum Z_packet_type packet = watchpoint_to_Z_packet (type);
+  enum Z_packet_type packet = watchpoint_to_Z_packet (*type);
+
+  /* If this target does not support read watchpoints, try access watchpoints
+     instead.  */
+  if (remote_protocol_Z[packet].support == PACKET_DISABLE
+      && *type == hw_read)
+    {
+      *type = hw_access;
+      packet = watchpoint_to_Z_packet (*type);
+    }
 
   if (remote_protocol_Z[packet].support == PACKET_DISABLE)
     error (_("Can't set hardware watchpoints without the '%s' (%s) packet."),
@@ -4632,8 +4641,15 @@ remote_insert_watchpoint (CORE_ADDR addr
 
   switch (packet_ok (buf, &remote_protocol_Z[packet]))
     {
-    case PACKET_ERROR:
     case PACKET_UNKNOWN:
+      if (*type == hw_read)
+	{
+	  *type = hw_access;
+	  return remote_insert_watchpoint (addr, len, type);
+	}
+      else
+	return -1;
+    case PACKET_ERROR:
       return -1;
     case PACKET_OK:
       return 0;
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.76
diff -u -p -r1.76 target.h
--- target.h	4 Sep 2005 16:18:20 -0000	1.76
+++ target.h	14 Nov 2005 14:39:20 -0000
@@ -341,7 +341,7 @@ struct target_ops
     int (*to_insert_hw_breakpoint) (CORE_ADDR, gdb_byte *);
     int (*to_remove_hw_breakpoint) (CORE_ADDR, gdb_byte *);
     int (*to_remove_watchpoint) (CORE_ADDR, int, int);
-    int (*to_insert_watchpoint) (CORE_ADDR, int, int);
+    int (*to_insert_watchpoint) (CORE_ADDR, int, int *);
     int (*to_stopped_by_watchpoint) (void);
     int to_have_continuable_watchpoint;
     int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *);
@@ -1036,7 +1036,7 @@ extern void (*deprecated_target_new_objf
 #endif
 
 
-/* Set/clear a hardware watchpoint starting at ADDR, for LEN bytes.  TYPE is 0
+/* Set/clear a hardware watchpoint starting at ADDR, for LEN bytes.  *TYPE is 0
    for write, 1 for read, and 2 for read/write accesses.  Returns 0 for
    success, non-zero for failure.  */
 
Index: config/i386/nm-i386.h
===================================================================
RCS file: /cvs/src/src/gdb/config/i386/nm-i386.h,v
retrieving revision 1.7
diff -u -p -r1.7 nm-i386.h
--- config/i386/nm-i386.h	8 Oct 2004 17:30:48 -0000	1.7
+++ config/i386/nm-i386.h	14 Nov 2005 14:39:20 -0000
@@ -31,8 +31,8 @@ extern void i386_cleanup_dregs (void);
 
 /* Insert a watchpoint to watch a memory region which starts at
    address ADDR and whose length is LEN bytes.  Watch memory accesses
-   of the type TYPE.  Return 0 on success, -1 on failure.  */
-extern int i386_insert_watchpoint (CORE_ADDR addr, int len, int type);
+   of the type *TYPE.  Return 0 on success, -1 on failure.  */
+extern int i386_insert_watchpoint (CORE_ADDR addr, int len, int *type);
 
 /* Remove a watchpoint that watched the memory region which starts at
    address ADDR, whose length is LEN bytes, and for accesses of the

  reply	other threads:[~2005-11-14 14:41 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2005-11-11 13:22 Vladimir Prus
2005-11-11 18:19 ` Eli Zaretskii
2005-11-13 16:45   ` Daniel Jacobowitz
2005-11-13 22:38     ` Eli Zaretskii
2005-11-14  2:43       ` Daniel Jacobowitz
2005-11-14  4:38         ` Eli Zaretskii
2005-11-14 14:42           ` Daniel Jacobowitz
2005-11-15  4:02             ` Eli Zaretskii
2005-11-14 10:10         ` Vladimir Prus
2005-11-14 14:41           ` Daniel Jacobowitz [this message]
2005-11-14 14:45             ` Daniel Jacobowitz
2005-11-15  4:02               ` Eli Zaretskii
2005-11-14 13:16         ` Johan Rydberg
2005-11-14 13:42           ` Vladimir Prus
2005-11-14 13:59             ` Johan Rydberg
2005-11-14 14:10               ` Vladimir Prus
2005-11-15  4:01             ` Eli Zaretskii

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20051114144118.GA22190@nevyn.them.org \
    --to=drow@false.org \
    --cc=eliz@gnu.org \
    --cc=gdb@sources.redhat.com \
    --cc=ghost@cs.msu.su \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).