* [PATCH] remote.c: Ensure that inferior_ptid is on the thread list @ 2015-07-10 20:50 Kevin Buettner 2015-07-14 10:18 ` Pedro Alves 0 siblings, 1 reply; 5+ messages in thread From: Kevin Buettner @ 2015-07-10 20:50 UTC (permalink / raw) To: gdb-patches 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(-) diff --git a/gdb/remote.c b/gdb/remote.c index 9d97f6b..2f2bb28 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -3695,7 +3695,8 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p) 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)) + if (ptid_equal (inferior_ptid, null_ptid) + || find_thread_ptid (inferior_ptid) == NULL) { /* Odd... The target was able to list threads, but not tell us which thread was current (no "thread" ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] remote.c: Ensure that inferior_ptid is on the thread list 2015-07-10 20:50 [PATCH] remote.c: Ensure that inferior_ptid is on the thread list Kevin Buettner @ 2015-07-14 10:18 ` Pedro Alves 2015-07-18 20:27 ` Kevin Buettner 0 siblings, 1 reply; 5+ messages in thread From: Pedro Alves @ 2015-07-14 10:18 UTC (permalink / raw) To: Kevin Buettner, gdb-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] remote.c: Ensure that inferior_ptid is on the thread list 2015-07-14 10:18 ` Pedro Alves @ 2015-07-18 20:27 ` Kevin Buettner 2015-07-23 21:24 ` Pedro Alves 0 siblings, 1 reply; 5+ messages in thread From: Kevin Buettner @ 2015-07-18 20:27 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] remote.c: Ensure that inferior_ptid is on the thread list 2015-07-18 20:27 ` Kevin Buettner @ 2015-07-23 21:24 ` Pedro Alves 2015-07-26 5:10 ` Kevin Buettner 0 siblings, 1 reply; 5+ messages in thread From: Pedro Alves @ 2015-07-23 21:24 UTC (permalink / raw) To: Kevin Buettner, gdb-patches On 07/18/2015 09:27 PM, Kevin Buettner wrote: > 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. We could add a debug log, so that someone working on some random stub sees the issue early. My thinking is that with the fix in place, it's easy to miss the stub bug, as the first thread will usually be the thread that the remote side says is current too. static ptid_t remote_current_thread (ptid_t oldpid) { struct remote_state *rs = get_remote_state (); putpkt ("qC"); getpkt (&rs->buf, &rs->buf_size, 0); if (rs->buf[0] == 'Q' && rs->buf[1] == 'C') { char *obuf; ptid_t res; res = read_ptid (&rs->buf[2], &obuf); if (*obuf != '\0' && remote_debug) { fprintf_unfiltered (gdb_stdlog, "warning: garbage after thread id " "in qC reply\n") } return res; } else return oldpid; } > >>> 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. > Oh, I just meant adding something like: if (remote_debug) fprintf_unfiltered (gdb_stdlog, "couldn't determine remote current thread " "picking first in list."); here. > Here's my updated change: Looks good to me. Thanks! -- Pedro Alves ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] remote.c: Ensure that inferior_ptid is on the thread list 2015-07-23 21:24 ` Pedro Alves @ 2015-07-26 5:10 ` Kevin Buettner 0 siblings, 0 replies; 5+ messages in thread From: Kevin Buettner @ 2015-07-26 5:10 UTC (permalink / raw) To: gdb-patches On Thu, 23 Jul 2015 22:24:48 +0100 Pedro Alves <palves@redhat.com> wrote: > Looks good to me. Thanks! Hi Pedro, Thanks for the review and the suggestions regarding log output. I've updated my patch to include the "debug remote" logging statements that you recommended. This is what I've pushed: 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. --- 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; } } ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-07-26 5:10 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-10 20:50 [PATCH] remote.c: Ensure that inferior_ptid is on the thread list Kevin Buettner 2015-07-14 10:18 ` Pedro Alves 2015-07-18 20:27 ` Kevin Buettner 2015-07-23 21:24 ` Pedro Alves 2015-07-26 5:10 ` Kevin Buettner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).