remote.c: Ensure that inferior_ptid is on the thread list

  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.

The errant line of code (after some analysis) seems to be this line in
remote_start_remote.  (I've provided the preceding comment as
context.)

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

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.  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.

I'm guessing 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;
	}

This change simply extends the test so that the "Odd..." case will
be used when inferior_ptid is not in the current set of threads.

gdb/ChangeLog:

	* remote.c (remote_start_remote): Ensure that inferior_ptid is
	set to a ptid from the thread list.
---
 gdb/remote.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

On 07/10/2015 09:50 PM, Kevin Buettner wrote:

> 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}.
> 

Urgh.

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

> 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.

> I'm guessing 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.

Yeah, that seems fine.

> 
> 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?

> 	  /* 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;
> 	}
> 
> This change simply extends the test so that the "Odd..." case will
> be used when inferior_ptid is not in the current set of threads.
> 
> gdb/ChangeLog:
> 
> 	* remote.c (remote_start_remote): Ensure that inferior_ptid is
> 	set to a ptid from the thread list.


Thanks,
Pedro Alves