Re: [pushed] remote.c: simplify parsing stop reasons in T stop replies

[Pedro Alves <palves at redhat dot com>]

On 02/23/2015 04:47 PM, Pedro Alves wrote:
> We need to be careful with parsing optional stop reasons that start
> with an hex character ("awatch", "core"), as GDBs that aren't aware of
> them parse them as real numbers.  That's silly of course, given that
> there should be a colon after those magic "numbers".  So if strtol on
> "abbz:" doesn't return "first invalid char" pointing to the colon, we
> know that "abbz" isn't really a register number.  It must be optional
> stop info we don't know about.  This adjusts GDB to work that way,
> removing the need for the special casing done upfront:
> 
> 	  /* If this packet is an awatch packet, don't parse the 'a'
> 	     as a register number.  */
> 	  if (strncmp (p, "awatch", strlen("awatch")) != 0
> 	      && strncmp (p, "core", strlen ("core") != 0))
> 
> For as long as we care about compatibility with GDB 7.9, we'll need to
> continue to be careful about this, so I added a comment.
> 
> Tested on x86_64 Fedora 20, native gdbserver.

For the record, here's the -w version:

diff --git c/gdb/ChangeLog w/gdb/ChangeLog
index faccd5f..b8bdf58 100644
--- c/gdb/ChangeLog
+++ w/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2015-02-23  Pedro Alves  <palves@redhat.com>
+
+	* remote.c (skip_to_semicolon): New function.
+	(remote_parse_stop_reply) <T stop reply>: Use it.  Don't
+	special case the stop reasons that look like hex numbers
+	upfront.  Instead handle real register numbers after matching
+	all the known stop reasons.
+
 2015-02-21  Doug Evans  <dje@google.com>
 
 	PR c++/17976, symtab/17821
diff --git c/gdb/remote.c w/gdb/remote.c
index dbfc10b..3479140 100644
--- c/gdb/remote.c
+++ w/gdb/remote.c
@@ -5489,6 +5489,16 @@ peek_stop_reply (ptid_t ptid)
 			 stop_reply_match_ptid_and_ws, &ptid);
 }
 
+/* Skip PACKET until the next semi-colon (or end of string).  */
+
+static char *
+skip_to_semicolon (char *p)
+{
+  while (*p != '\0' && *p != ';')
+    p++;
+  return p;
+}
+
 /* Parse the stop reply in BUF.  Either the function succeeds, and the
    result is stored in EVENT, or throws an error.  */
 
@@ -5521,34 +5531,25 @@ remote_parse_stop_reply (char *buf, struct stop_reply *event)
       while (*p)
 	{
 	  char *p1;
-	  char *p_temp;
 	  int fieldsize;
-	  LONGEST pnum = 0;
-
-	  /* If the packet contains a register number, save it in
-	     pnum and set p1 to point to the character following it.
-	     Otherwise p1 points to p.  */
 
-	  /* If this packet is an awatch packet, don't parse the 'a'
-	     as a register number.  */
-
-	  if (strncmp (p, "awatch", strlen("awatch")) != 0
-	      && strncmp (p, "core", strlen ("core") != 0))
-	    {
-	      /* Read the ``P'' register number.  */
-	      pnum = strtol (p, &p_temp, 16);
-	      p1 = p_temp;
-	    }
-	  else
-	    p1 = p;
-
-	  if (p1 == p)	/* No register number present here.  */
-	    {
 	  p1 = strchr (p, ':');
 	  if (p1 == NULL)
 	    error (_("Malformed packet(a) (missing colon): %s\n\
 Packet: '%s'\n"),
 		   p, buf);
+	  if (p == p1)
+	    error (_("Malformed packet(a) (missing register number): %s\n\
+Packet: '%s'\n"),
+		   p, buf);
+
+	  /* Some "registers" are actually extended stop information.
+	     Note if you're adding a new entry here: GDB 7.9 and
+	     earlier assume that all register "numbers" that start
+	     with an hex digit are real register numbers.  Make sure
+	     the server only sends such a packet if it knows the
+	     client understands it.  */
+
 	  if (strncmp (p, "thread", p1 - p) == 0)
 	    event->ptid = read_ptid (++p1, &p);
 	  else if ((strncmp (p, "watch", p1 - p) == 0)
@@ -5561,22 +5562,15 @@ Packet: '%s'\n"),
 	    }
 	  else if (strncmp (p, "library", p1 - p) == 0)
 	    {
-		  p1++;
-		  p_temp = p1;
-		  while (*p_temp && *p_temp != ';')
-		    p_temp++;
-
 	      event->ws.kind = TARGET_WAITKIND_LOADED;
-		  p = p_temp;
+	      p = skip_to_semicolon (p1 + 1);
 	    }
 	  else if (strncmp (p, "replaylog", p1 - p) == 0)
 	    {
 	      event->ws.kind = TARGET_WAITKIND_NO_HISTORY;
 	      /* p1 will indicate "begin" or "end", but it makes
 		 no difference for now, so ignore it.  */
-		  p_temp = strchr (p1 + 1, ';');
-		  if (p_temp)
-		    p = p_temp;
+	      p = skip_to_semicolon (p1 + 1);
 	    }
 	  else if (strncmp (p, "core", p1 - p) == 0)
 	    {
@@ -5587,25 +5581,19 @@ Packet: '%s'\n"),
 	    }
 	  else
 	    {
-		  /* Silently skip unknown optional info.  */
-		  p_temp = strchr (p1 + 1, ';');
-		  if (p_temp)
-		    p = p_temp;
-		}
-	    }
-	  else
+	      ULONGEST pnum;
+	      char *p_temp;
+
+	      /* Maybe a real ``P'' register number.  */
+	      p_temp = unpack_varlen_hex (p, &pnum);
+	      /* If the first invalid character is the colon, we got a
+		 register number.  Otherwise, it's an unknown stop
+		 reason.  */
+	      if (p_temp == p1)
 		{
 		  struct packet_reg *reg = packet_reg_from_pnum (rsa, pnum);
 		  cached_reg_t cached_reg;
 
-	      p = p1;
-
-	      if (*p != ':')
-		error (_("Malformed packet(b) (missing colon): %s\n\
-Packet: '%s'\n"),
-		       p, buf);
-	      ++p;
-
 		  if (reg == NULL)
 		    error (_("Remote sent bad register number %s: %s\n\
 Packet: '%s'\n"),
@@ -5613,6 +5601,7 @@ Packet: '%s'\n"),
 
 		  cached_reg.num = reg->regnum;
 
+		  p = p1 + 1;
 		  fieldsize = hex2bin (p, cached_reg.data,
 				       register_size (target_gdbarch (),
 						      reg->regnum));
@@ -5623,6 +5612,13 @@ Packet: '%s'\n"),
 
 		  VEC_safe_push (cached_reg_t, event->regcache, &cached_reg);
 		}
+	      else
+		{
+		  /* Not a number.  Silently skip unknown optional
+		     info.  */
+		  p = skip_to_semicolon (p1 + 1);
+		}
+	    }
 
 	  if (*p != ';')
 	    error (_("Remote register badly formatted: %s\nhere: %s"),