From 3405876ae7971153e14fd5c2ed17cb0bb6cdf117 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Fri, 25 Jan 2013 17:25:59 +0000 Subject: [PATCH] Fix GDB internal error against targets that return a thread in T stop replies but don't support qC. Yao writes: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ GDB gets an internal error when it connects to GDBserver started with '--disable-packet=qC'. Sending packet: $QNonStop:0#8c...Packet received: OK Sending packet: $?#3f...Packet received: T0505:00000000;04:00f0ffbf;08:b0c2e44c;thread:p4255.4255;core:1; Sending packet: $Hc-1#09...Packet received: E01 Sending packet: $qC#b4...Packet received: Sending packet: $qAttached:a410#bf...Packet received: E01 Packet qAttached (query-attached) is supported warning: Remote failure reply: E01 Sending packet: $qOffsets#4b...Packet received: ../../../git/gdb/target.c:3248: internal-error: Can't determine the current address space of thread Thread 16981 When start remote, the call chain is as follows, remote_start_remote add_current_inferior_and_thread <--[1] ... start_remote wait_for_inferior remote_wait_as process_stop_reply get_thread_arch_regcache <--[2] remote_notice_new_inferior <--[3] GDB sends packet "qC" in [1] and adds the thread/inferior if the remote stubs understands "qC". In [2], GDB looks for the inferior to build a regcache, and notices a new inferior in [3]. As we can see, GDB assumes that the inferior can be found in [2]. Once the remote stub doesn't support "qC", GDB can't look for the inferior in [2], and emits an internal error. ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Right after the initial connection, we query the target for its state, with the ? packet. We store the resulting wait status / stop reply aside, and query the target for the current thread, using qC, which fails, so we fake a ptid for the target's thread. We then later, after the initial setup, end up consuming that set-aside wait status, parsing the T stop reply, which contains a "thread" "register" (which was the thread the target would have replied to qC). We get into trouble because the ptid in that stop reply doesn't match our faked up ptid in the initial setup, although the target threads are the same... So we had the T stop reply handy all along. We might as well extract the thread's ptid from it, and avoid all the resulting issues. qC is also used after vRun, in order to discover the new process'es main thread. But, vRun's reply is also a wait status, just like '?''s, which is quite convenient. This means that if we have a "Txx thread: ptid" reply, then we don't really need qC. The patch makes GDB look in the T reply first, and if not found, try with qC. The packet handling seems to have been added in gdb-4.18 (1999), and I see that in that same release, "Txx thread: ptid" didn't exist yet, which probably explains why nobody though of doing this before. Regression tested against a gdbserver with qC disabled (and then enabled), on x86_64 Fedora 17. 2013-01-25 Pedro Alves * remote.c (stop_reply_extract_thread): New. (add_current_inferior_and_thread): New parameter 'wait_status'. Handle it. (remote_start_remote): Pass wait status to add_current_inferior_and_thread. (extended_remote_run): Update comment. (extended_remote_create_inferior_1): Pass wait status to add_current_inferior_and_thread. --- gdb/ChangeLog | 11 ++++++++ gdb/remote.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 82 insertions(+), 9 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index af0d7e6..2804269 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,14 @@ +2013-01-25 Pedro Alves + + * remote.c (stop_reply_extract_thread): New. + (add_current_inferior_and_thread): New parameter 'wait_status'. + Handle it. + (remote_start_remote): Pass wait status to + add_current_inferior_and_thread. + (extended_remote_run): Update comment. + (extended_remote_create_inferior_1): Pass wait status to + add_current_inferior_and_thread. + 2013-01-25 Andrew Burgess Ulrich Weigand diff --git a/gdb/remote.c b/gdb/remote.c index 7ea9597..18fe61d 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -3224,22 +3224,77 @@ send_interrupt_sequence (void) interrupt_sequence_mode); } + +/* If STOP_REPLY is a T stop reply, look for the "thread" register, + and extract the PTID. Returns NULL_PTID if not found. */ + +static ptid_t +stop_reply_extract_thread (char *stop_reply) +{ + if (stop_reply[0] == 'T' && strlen (stop_reply) > 3) + { + char *p; + + /* Txx r:val ; r:val (...) */ + p = &stop_reply[3]; + + /* Look for "register" named "thread". */ + while (*p != '\0') + { + char *p1; + + p1 = strchr (p, ':'); + if (p1 == NULL) + return null_ptid; + + if (strncmp (p, "thread", p1 - p) == 0) + return read_ptid (++p1, &p); + + p1 = strchr (p, ';'); + if (p1 == NULL) + return null_ptid; + p1++; + + p = p1; + } + } + + return null_ptid; +} + /* Query the remote target for which is the current thread/process, add it to our tables, and update INFERIOR_PTID. The caller is responsible for setting the state such that the remote end is ready - to return the current thread. */ + to return the current thread. + + This function is called after handling the '?' or 'vRun' packets, + whose response is a stop reply from which we can also try + extracting the thread. If the target doesn't support the explicit + qC query, we infer the current thread from that stop reply, passed + in in WAIT_STATUS, which may be NULL. */ static void -add_current_inferior_and_thread (void) +add_current_inferior_and_thread (char *wait_status) { struct remote_state *rs = get_remote_state (); int fake_pid_p = 0; - ptid_t ptid; + ptid_t ptid = null_ptid; inferior_ptid = null_ptid; - /* Now, if we have thread information, update inferior_ptid. */ - ptid = remote_current_thread (inferior_ptid); + /* Now, if we have thread information, update inferior_ptid. First + if we have a stop reply handy, maybe it's a T stop reply with a + "thread" register we can extract the current thread from. If + not, ask the remote which is the current thread, with qC. The + former method avoids a roundtrip. Note we don't use + remote_parse_stop_reply as that makes use of the target + architecture, which we haven't yet fully determined at this + point. */ + if (wait_status != NULL) + ptid = stop_reply_extract_thread (wait_status); + if (ptid_equal (ptid, null_ptid)) + ptid = remote_current_thread (inferior_ptid); + if (!ptid_equal (ptid, null_ptid)) { if (!remote_multi_process_p (rs)) @@ -3400,7 +3455,7 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p) /* Let the stub know that we want it to return the thread. */ set_continue_thread (minus_one_ptid); - add_current_inferior_and_thread (); + add_current_inferior_and_thread (wait_status); /* init_wait_for_inferior should be called before get_offsets in order to manage `inserted' flag in bp loc in a correct state. @@ -7836,7 +7891,7 @@ extended_remote_run (char *args) if (packet_ok (rs->buf, &remote_protocol_packets[PACKET_vRun]) == PACKET_OK) { - /* We have a wait response; we don't need it, though. All is well. */ + /* We have a wait response. All is well. */ return 0; } else if (remote_protocol_packets[PACKET_vRun].support == PACKET_DISABLE) @@ -7863,6 +7918,10 @@ static void extended_remote_create_inferior_1 (char *exec_file, char *args, char **env, int from_tty) { + int run_worked; + char *stop_reply; + struct remote_state *rs = get_remote_state (); + /* If running asynchronously, register the target file descriptor with the event loop. */ if (target_can_async_p ()) @@ -7873,7 +7932,8 @@ extended_remote_create_inferior_1 (char *exec_file, char *args, extended_remote_disable_randomization (disable_randomization); /* Now restart the remote server. */ - if (extended_remote_run (args) == -1) + run_worked = extended_remote_run (args) != -1; + if (!run_worked) { /* vRun was not supported. Fail if we need it to do what the user requested. */ @@ -7895,7 +7955,9 @@ extended_remote_create_inferior_1 (char *exec_file, char *args, init_wait_for_inferior (); } - add_current_inferior_and_thread (); + /* vRun's success return is a stop reply. */ + stop_reply = run_worked ? rs->buf : NULL; + add_current_inferior_and_thread (stop_reply); /* Get updated offsets, if the stub uses qOffsets. */ get_offsets (); -- 2.7.4