[binutils-gdb] remote.c: Make read_ptid return a null value when no thread id is found.

[Kevin Buettner <kevinb at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=c9f35b348e586c0f48592918324b6e15c815a702

commit c9f35b348e586c0f48592918324b6e15c815a702
Author: Kevin Buettner <kevinb@redhat.com>
Date:   Fri Jul 10 10:25:29 2015 -0700

    remote.c: Make read_ptid return a null value when no thread id is found.
    
    When using GDB to debug an RX target using the GDB remote protocol,
    using a Renesas supplied debug agent, I encountered the following
    assertion error:
    
    thread.c:85: internal-error: inferior_thread: Assertion `tp' failed.
    A problem internal to GDB has been detected,
    further debugging may prove unreliable.
    Create a core file of GDB? (y or n) n
    Command aborted.
    
    This assertion error occurs due to the fact that the value associated
    with inferior_ptid is not on the thread list.
    
    The remote debug output (obtained with "set debug remote 1") is fairly
    short, so I will include it up to the point where things go wrong -
    which is somewhat before the assertion failure:
    
        (gdb) target remote coyote.lan:61234
        Remote debugging using coyote.lan:61234
        Sending packet: $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+#c9...Ack
        Packet received: PacketSize=c00;qXfer:memory-map:read-;qXfer:features:read-;QStartNoAckMode+;multiprocess+;QNonStop+
        Packet qSupported (supported-packets) is supported
        Sending packet: $QStartNoAckMode#b0...Ack
        Packet received: OK
        Sending packet: $Hgp0.0#ad...Packet received: OK
        Sending packet: $QNonStop:0#8c...Packet received: OK
        Sending packet: $qTStatus#49...Packet received:
        Packet qTStatus (trace-status) is NOT supported
        Sending packet: $?#3f...Packet received: S02
        Sending packet: $qfThreadInfo#bb...Packet received: m1
        Sending packet: $qsThreadInfo#c8...Packet received: l
        Sending packet: $qAttached:a410#bf...Packet received: 0
        Packet qAttached (query-attached) is supported
        Sending packet: $Hc-1#09...Packet received: OK
        Sending packet: $qC#b4...Packet received: QC not supported
    
    Above is the trace starting from the invocation of "target remote"
    through the call of get_current_thread() in remote_start_remote().
    Below, I've pasted this line of code along with additional lines of
    context.  The test following the call is especially important to
    understanding both the problem and my patch.
    
              /* We have thread information; select the thread the target
                 says should be current.  If we're reconnecting to a
                 multi-threaded program, this will ideally be the thread
                 that last reported an event before GDB disconnected.  */
              inferior_ptid = get_current_thread (wait_status);
              if (ptid_equal (inferior_ptid, null_ptid))
                {
                  /* Odd... The target was able to list threads, but not
                     tell us which thread was current (no "thread"
                     register in T stop reply?).  Just pick the first
                     thread in the thread list then.  */
                  inferior_ptid = thread_list->ptid;
                }
            }
    
    Prior to getting to the code pasted above, remote_start_remote()
    made a call to target_update_thread_list().  This corresponds to the
    following lines from the above trace:
    
        Sending packet: $qfThreadInfo#bb...Packet received: m1
        Sending packet: $qsThreadInfo#c8...Packet received: l
        Sending packet: $qAttached:a410#bf...Packet received: 0
        Packet qAttached (query-attached) is supported
    
    Once target_update_thread_list has completed, the thread list
    contains a single entry: {pid = 42000, lwp = 1, tid = 0}.
    
    remote_start_remote() then makes a call to set_continue_thread(),
    accounting for this line of the trace:
    
        Sending packet: $Hc-1#09...Packet received: OK
    
    Finally, the call to get_current_thread() is responsible for the last
    line of the trace that I provided above:
    
        Sending packet: $qC#b4...Packet received: QC not supported
    
    get_current_thread() calls stop_reply_extract_thread() with the wait
    status. This returns null_ptid.
    
    get_current_thread() then calls remote_current_thread with a null
    inferior_ptid.  After the calls to putpkt() and getpkt(), rs->buf[0]
    is 'Q', so read_ptid() is called and its result is returned.
    
    The buffer passed to read_ptid() is " not supported".  read_ptid ultimately
    returns a ptid of {pid = 4200, lwp = 0, tid = 0}.
    
    However, this thread is not on the thread list.  As noted earlier, the
    call to target_update_thread_list() had placed {pid = 42000, lwp = 1,
    tid = 0} on the list.  This is the only thread in the list.
    
    When these calls ultimately return to remote_start_remote(),
    inferior_ptid gets set to {pid = 4200, lwp = 0, tid = 0}, which
    (again) is not on the thread list.
    
    It appears to me that the string " not supported" is coming from the
    debug agent.  If so, it should be fixed, but I don't see a reason to
    not consult the thread list in order to place a valid thread id in
    inferior_ptid.
    
    This (consultation of the thread list) is what is done when
    inferior_ptid is null_ptid:
    
    	  if (ptid_equal (inferior_ptid, null_ptid))
    	    {
    	      /* Odd... The target was able to list threads, but not
    		 tell us which thread was current (no "thread"
    		 register in T stop reply?).  Just pick the first
    		 thread in the thread list then.  */
    	      inferior_ptid = thread_list->ptid;
    	    }
    
    My patch causes a null inferior_ptid to be returned by read_ptid when
    no thread id is found in the response from the debug agent.  This
    return value ends up being returned by remote_current_thread() and
    then by get_current_thread.  The assignment then places this null
    value into inferior_ptid.  That, in turn, allows the ptid_equal test
    (noted above) to fetch a valid thread from the thread list.  I no
    longer see the assertion failure due a good value (which is on the
    thread list) being placed in inferior_ptid.
    
    This patch also adds two log warnings that may be output when "set
    debug remote 1" is used.  When running against the Renesas debug agent
    mentioned earlier, this is the relevant portion of the log output:
    
    Sending packet: $qC#b4...Packet received: QC not supported
    warning: garbage in qC reply
    warning: couldn't determine remote current thread; picking first in list.
    
    gdb/ChangeLog:
    
    	* remote.c (read_ptid): Return null_ptid when no thread id
    	is found.
    	(remote_current_thread): Add log warning for malformed
    	qC reply.
    	(remote_start_remote): Add log warning when current thread
    	not found.

Diff:
---
 gdb/ChangeLog |  9 +++++++++
 gdb/remote.c  | 26 +++++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0061bff..7608ac1 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,12 @@
+2015-07-25  Kevin Buettner  <kevinb@redhat.com>
+
+	* remote.c (read_ptid): Return null_ptid when no thread id
+	is found.
+	(remote_current_thread): Add log warning for malformed
+	qC reply.
+	(remote_start_remote): Add log warning when current thread
+	not found.
+
 2015-07-24  Pedro Alves  <palves@redhat.com>
 
 	* s390-linux-nat.c (fetch_regs, store_regs, fetch_fpregs)
diff --git a/gdb/remote.c b/gdb/remote.c
index 94899bd..69da508 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2158,6 +2158,14 @@ read_ptid (char *buf, char **obuf)
   /* No multi-process.  Just a tid.  */
   pp = unpack_varlen_hex (p, &tid);
 
+  /* Return null_ptid when no thread id is found.  */
+  if (p == pp)
+    {
+      if (obuf)
+	*obuf = pp;
+      return null_ptid;
+    }
+
   /* Since the stub is not sending a process id, then default to
      what's in inferior_ptid, unless it's null at this point.  If so,
      then since there's no way to know the pid of the reported
@@ -2750,7 +2758,17 @@ remote_current_thread (ptid_t oldpid)
   putpkt ("qC");
   getpkt (&rs->buf, &rs->buf_size, 0);
   if (rs->buf[0] == 'Q' && rs->buf[1] == 'C')
-    return read_ptid (&rs->buf[2], NULL);
+    {
+      char *obuf;
+      ptid_t result;
+
+      result = read_ptid (&rs->buf[2], &obuf);
+      if (*obuf != '\0' && remote_debug)
+        fprintf_unfiltered (gdb_stdlog,
+	                    "warning: garbage in qC reply\n");
+
+      return result;
+    }
   else
     return oldpid;
 }
@@ -3701,6 +3719,12 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
 		 tell us which thread was current (no "thread"
 		 register in T stop reply?).  Just pick the first
 		 thread in the thread list then.  */
+	      
+	      if (remote_debug)
+		fprintf_unfiltered (gdb_stdlog,
+		                    "warning: couldn't determine remote "
+				    "current thread; picking first in list.\n");
+
 	      inferior_ptid = thread_list->ptid;
 	    }
 	}