This is the mail archive of the gdb-patches@sourceware.org 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: [PATCH] remote.c: Ensure that inferior_ptid is on the thread list


On Tue, 14 Jul 2015 11:18:23 +0100
Pedro Alves <palves@redhat.com> wrote:

> Showing a snippet of the "set debug remote 1" logs in question
> here would make this explanation clearer I think.

That's a good suggestion; I've updated my description with the log.

> > However, this thread is not on the thread list.  An earlier 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.
> > 
> 
> Seems like read_ptid should return null_ptid if it parsed nothing
> instead of that.  And/or remote_current_thread should return null_ptid
> if there's more text after the read ptid string.

Yes, that's reasonable too.  I have new patch, appended below, which
does this.  It (only) checks for the "parsed nothing" case, which is
what I need for the bug that I'm addressing.

I'm wary about adding an additional check to make sure that nothing
follows the thread id.  It could be that there's something innocuous
there.  That said, if you really want it in there, I'll revise my
patch again.

> > This (consultation of the thread list) is what is done when
> > inferior_ptid is null_ptid:
> > 
> >       if (ptid_equal (inferior_ptid, null_ptid))
> > 	{
> 
> Would you mind adding a remote_debug log here?

As noted at the outset, I've updated my description.  I haven't
changed this comment in the actual code.  Hopefully, that's what
you want.

> > 	  /* 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;
> > 	}

Here's my updated change:

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.

gdb/ChangeLog:
    
    	* remote.c (read_ptid): Return null_ptid when no thread id
    	is found.
---
 gdb/remote.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/remote.c b/gdb/remote.c
index 9d97f6b..24c9628 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2158,6 +2158,10 @@ 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)
+    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


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