diff mbox

fix misparsing in remote_parse_stop_reply

Message ID 55D15E73.7040306@codesourcery.com
State New, archived
Headers

Commit Message

Sandra Loosemore Aug. 17, 2015, 4:09 a.m. UTC 
  While trying to do some testing on one of our ARM boards via a debug 
probe and stub that speaks RSP, I was mystified by getting this error:

(gdb) target remote ...
(gdb) break main
(gdb) load
(gdb) c
Cannot detach breakpoints of inferior_ptid

Eventually I realized that this wasn't just a badly-worded message; GDB 
was somehow mis-interpreting the stop reply 'T05f:f0021000;' as 
TARGET_WAITKIND_FORKED, and wandering off into code that it shouldn't 
have been executing in the first place.  (On ARM, register 0xf is the 
PC, BTW, and this particular stub works fine with a GDB from last year.)

Tracking it down further to remote_parse_stop_reply, the trouble is that 
the test

else if (strncmp (p, "fork", p1 - p) == 0)

is matching here; p1 points to the colon so it is matching exactly one 
character, the initial 'f'.  While "fork" is new-ish, all the stop 
reason keywords use similar matching logic.

I don't see anything in the RSP documentation to indicate that the 
special stop reason keywords in a 'T' reply can be abbreviated, so I 
suspect this is just a think-o in the code, and it should be testing for 
an exact match of the keyword instead.  Is there something else going on 
here that I'm missing?

The attached patch is sufficient to unblock testing.  Does it seem 
plausible?  This particular debug probe is quite slow so I haven't had 
time to run the full GDB testsuite yet, or test on any other target.

It would also be good if somebody could translate that puzzling error 
message into something more user-friendly and less 
implementor-speaky....  but I still don't know what it means, so I can't 
suggest anything.  :-(

-Sandra

Comments

Pedro Alves Aug. 17, 2015, 2:46 p.m. UTC | #1 
On 08/17/2015 05:09 AM, Sandra Loosemore wrote:
> While trying to do some testing on one of our ARM boards via a debug 
> probe and stub that speaks RSP, I was mystified by getting this error:
> 
> (gdb) target remote ...
> (gdb) break main
> (gdb) load
> (gdb) c
> Cannot detach breakpoints of inferior_ptid
> 
> Eventually I realized that this wasn't just a badly-worded message; GDB 
> was somehow mis-interpreting the stop reply 'T05f:f0021000;' as 
> TARGET_WAITKIND_FORKED, and wandering off into code that it shouldn't 
> have been executing in the first place.  (On ARM, register 0xf is the 
> PC, BTW, and this particular stub works fine with a GDB from last year.)
> 
> Tracking it down further to remote_parse_stop_reply, the trouble is that 
> the test
> 
> else if (strncmp (p, "fork", p1 - p) == 0)
> 
> is matching here; p1 points to the colon so it is matching exactly one 
> character, the initial 'f'.  While "fork" is new-ish, all the stop 
> reason keywords use similar matching logic.
> 
> I don't see anything in the RSP documentation to indicate that the 
> special stop reason keywords in a 'T' reply can be abbreviated, so I 
> suspect this is just a think-o in the code, and it should be testing for 
> an exact match of the keyword instead.  

Indeed.

> Is there something else going on here that I'm missing?

Nope.

> 
> The attached patch is sufficient to unblock testing.  Does it seem 
> plausible?  This particular debug probe is quite slow so I haven't had 
> time to run the full GDB testsuite yet, or test on any other target.
> 
> It would also be good if somebody could translate that puzzling error 
> message into something more user-friendly and less 
> implementor-speaky....  but I still don't know what it means, so I can't 
> suggest anything.  :-(

This is really an internal error.  Users shouldn't be seeing it.
After a fork, because the child is a copy of the parent, it has
all the parent's breakpoints inserted in memory as well.
detach_breakpoints's job is to replace them with the original
instructions in the child, while leaving the parent's breakpoints marked
inserted in gdb's structures.  The parent is inferior_ptid, the child is
the passed in ptid.  So if ptid is inferior_ptid, we'd be removing
breakpoints from the parent, which is not what is wanted here.

> 2015-08-16  Sandra Loosemore  <sandra@codesourcery.com>
> 
> 	gdb/
> 	* remote.c (strprefix): New.
> 	(remote_parse_stop_reply): Use strprefix instead of strncmp
> 	to ensure exact match of keyword.
> 

This is OK (master and release branches that need it).

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index ca1f0df..4e483fd 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5835,6 +5835,18 @@  skip_to_semicolon (char *p)
   return p;
 }
 
+/* Helper for remote_parse_stop_reply.  Return nonzero if the substring
+   starting with P and ending with PEND matches PREFIX.  */
+
+static int
+strprefix (const char *p, const char *pend, const char *prefix)
+{
+  for ( ; p < pend; p++, prefix++)
+    if (*p != *prefix)
+      return 0;
+  return *prefix == '\0';
+}
+
 /* Parse the stop reply in BUF.  Either the function succeeds, and the
    result is stored in EVENT, or throws an error.  */
 
@@ -5886,17 +5898,17 @@  Packet: '%s'\n"),
 	     the server only sends such a packet if it knows the
 	     client understands it.  */
 
-	  if (strncmp (p, "thread", p1 - p) == 0)
+	  if (strprefix (p, p1, "thread"))
 	    event->ptid = read_ptid (++p1, &p);
-	  else if ((strncmp (p, "watch", p1 - p) == 0)
-		   || (strncmp (p, "rwatch", p1 - p) == 0)
-		   || (strncmp (p, "awatch", p1 - p) == 0))
+	  else if (strprefix (p, p1, "watch")
+		   || strprefix (p, p1, "rwatch")
+		   || strprefix (p, p1, "awatch"))
 	    {
 	      event->stop_reason = TARGET_STOPPED_BY_WATCHPOINT;
 	      p = unpack_varlen_hex (++p1, &addr);
 	      event->watch_data_address = (CORE_ADDR) addr;
 	    }
-	  else if (strncmp (p, "swbreak", p1 - p) == 0)
+	  else if (strprefix (p, p1, "swbreak"))
 	    {
 	      event->stop_reason = TARGET_STOPPED_BY_SW_BREAKPOINT;
 
@@ -5910,7 +5922,7 @@  Packet: '%s'\n"),
 		 use of it in a backward compatible way.  */
 	      p = skip_to_semicolon (p1 + 1);
 	    }
-	  else if (strncmp (p, "hwbreak", p1 - p) == 0)
+	  else if (strprefix (p, p1, "hwbreak"))
 	    {
 	      event->stop_reason = TARGET_STOPPED_BY_HW_BREAKPOINT;
 
@@ -5922,36 +5934,36 @@  Packet: '%s'\n"),
 	      /* See above.  */
 	      p = skip_to_semicolon (p1 + 1);
 	    }
-	  else if (strncmp (p, "library", p1 - p) == 0)
+	  else if (strprefix (p, p1, "library"))
 	    {
 	      event->ws.kind = TARGET_WAITKIND_LOADED;
 	      p = skip_to_semicolon (p1 + 1);
 	    }
-	  else if (strncmp (p, "replaylog", p1 - p) == 0)
+	  else if (strprefix (p, p1, "replaylog"))
 	    {
 	      event->ws.kind = TARGET_WAITKIND_NO_HISTORY;
 	      /* p1 will indicate "begin" or "end", but it makes
 		 no difference for now, so ignore it.  */
 	      p = skip_to_semicolon (p1 + 1);
 	    }
-	  else if (strncmp (p, "core", p1 - p) == 0)
+	  else if (strprefix (p, p1, "core"))
 	    {
 	      ULONGEST c;
 
 	      p = unpack_varlen_hex (++p1, &c);
 	      event->core = c;
 	    }
-	  else if (strncmp (p, "fork", p1 - p) == 0)
+	  else if (strprefix (p, p1, "fork"))
 	    {
 	      event->ws.value.related_pid = read_ptid (++p1, &p);
 	      event->ws.kind = TARGET_WAITKIND_FORKED;
 	    }
-	  else if (strncmp (p, "vfork", p1 - p) == 0)
+	  else if (strprefix (p, p1, "vfork"))
 	    {
 	      event->ws.value.related_pid = read_ptid (++p1, &p);
 	      event->ws.kind = TARGET_WAITKIND_VFORKED;
 	    }
-	  else if (strncmp (p, "vforkdone", p1 - p) == 0)
+	  else if (strprefix (p, p1, "vforkdone"))
 	    {
 	      event->ws.kind = TARGET_WAITKIND_VFORK_DONE;
 	      p = skip_to_semicolon (p1 + 1);