This is the mail archive of the gdb-patches@sources.redhat.com mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [rfa] hardware breakpoints -- remote targets


I revised the patch to include Mark Salter's earlier patch, as well as
his documentation changes.
Lets see, Mark's doco patch was previously approved vis:
http://sources.redhat.com/ml/gdb-patches/2000-11/msg00020.html
(Someone (me) has even gone through the table and has a patch to fix the formatting pending :-)

Hold back on the commit, though, until we've got the below ok.



-char *unpack_varlen_hex (char *buff, int *result);
+char *unpack_varlen_hex (char *buff, ULONGEST *result);
I didn't see a changelog entry for this. Anyway, this change (and the corresponding int->ULONGEST tweaks) should be separated out committed independantly. This part is approved.



+/* This is set to the data address of the access causing the target
+ * to stop for a watchpoint. */
(NB: no leading ``*'' on the second comment line.)

+static CORE_ADDR remote_watch_data_address;
+
+/* This is non-zero if taregt stopped for a watchpoint. */
+static int remote_stopped_by_watchpoint_p;
+
+
For the above can you please add a ``FIXME: graces/YYYY-MM-DD:'' style comment noteing that, like gdbarch_tdep() for ``struct gdbarch'', these static variables should be bound to an instance of the target object. Only there isn't such a thing :-(



@@ -3025,6 +3035,8 @@
if (target_wait_loop_hook)
(*target_wait_loop_hook) ();
+ remote_stopped_by_watchpoint_p = 0;
+
switch (buf[0])
{
case 'E': /* Error of some sort */
@@ -3048,10 +3060,19 @@
unsigned char *p1;
char *p_temp;
int fieldsize;
+ LONGEST pnum = 0;
- /* Read the ``P'' register number. */
- LONGEST pnum = strtol ((const char *) p, &p_temp, 16);
- p1 = (unsigned char *) p_temp;
+ /* If this packet is an awatch packet, don't parse the 'a'
+ as a register number. */
Add a comment mentioning that ``p1'' is left pointing to ``p'' if there wasn't a register number.



+		if (!strstr ((const char *) p, "awatch"))
Use ``strncmp (p, "awatch", strlen ("awatch")) != 0''.

This is because the string compare needs to be anchored on the first character (otherwize it could miss-parse ``00=a123;awatch''. BTW, it should be possible to safely remove most of those ``(const char*)'' and ``(char*)'' casts.


+ {
+ /* Read the ``P'' register number. */
+ pnum = strtol ((const char *) p, &p_temp, 16);
+ p1 = (unsigned char *) p_temp;
+ }
+ else + p1=p;
Don't forget the spaces:
	p1 = p;



 		if (p1 == p)	/* No register number present here */
 		  {
@@ -3066,6 +3087,21 @@
 			record_currthread (thread_num);
 			p = (unsigned char *) p_temp;
 		      }
+		    else if ((strncmp ((const char *) p, "watch", p1 - p) == 0)
+			     || (strncmp ((const char *) p, "rwatch", p1 - p) == 0)
+			     || (strncmp ((const char *) p, "awatch", p1 - p) == 0))
+		      {
+			remote_stopped_by_watchpoint_p = 1;
+			p = unpack_varlen_hex (++p1, &addr);
+			remote_watch_data_address = (CORE_ADDR)addr;
+		      }
+		    else
+ 		      {
+ 			/* silently skip unknown optional info */
+ 			p_temp = (unsigned char *) strchr ((const char *) p1+1, ';');
Spaces, ``p1 + 1'', no cast. The comment should be a sentence (hi MarkK ;-) with period and two spaces after it :-)


@@ -3268,10 +3307,19 @@
unsigned char *p1;
char *p_temp;
int fieldsize;
+ long pnum = 0;
- /* Read the register number */
- long pnum = strtol ((const char *) p, &p_temp, 16);
- p1 = (unsigned char *) p_temp;
+ /* If this packet is an awatch packet, don't parse the 'a'
see the comments above ...



+static void
+set_hardware_watchpoint_limit (char *args, int from_tty)
+{
+  if (args)
+    {
+      char *arg_end;
+      int val = strtoul (args, &arg_end, 10);
+      if (*args && *arg_end == '\0')
+        {
+          remote_hw_watchpoint_limit = val;
+	  return;
+	}
+    }
+  error ("Illegal argument for \"set remote hardware-watchpoint-limit\" command");
+}
Hmm, can you please split out the new commands from the other parts of the patch so that they can be considered separatly. I'm wondering if a better way of doing this is to have GDB automatically detect if another watchpoint is available.

Anyway new commands would need doco.

Andrew

(And thanks for finally creating a patch that adds watchpoint support to the target vector where it belongs!)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]