From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26076 invoked by alias); 25 Jan 2013 17:30:25 -0000 Received: (qmail 26059 invoked by uid 22791); 25 Jan 2013 17:30:22 -0000 X-SWARE-Spam-Status: No, hits=-7.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 25 Jan 2013 17:29:47 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r0PHTiwP016606 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 25 Jan 2013 12:29:44 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r0PHTghb016838; Fri, 25 Jan 2013 12:29:43 -0500 Message-ID: <5102C106.5060703@redhat.com> Date: Fri, 25 Jan 2013 17:30:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: Yao Qi CC: gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] Fix error when GDB connects to GDBserver with qC disabled References: <1358930116-29038-1-git-send-email-yao@codesourcery.com> <510169A9.7090002@redhat.com> <510261A5.1080200@codesourcery.com> In-Reply-To: <510261A5.1080200@codesourcery.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2013-01/txt/msg00635.txt.bz2 On 01/25/2013 10:42 AM, Yao Qi wrote: > So the rationale here is to prefer to use expedite registers in the stop reply, and postpone or even avoid fetching the whole registers by 'g' packet, right? Right. >> The patch still tries qC first, but I'm thinking we can flip that >> around, and only try qC if the stop reply didn't include a thread. > > Why don't we do in this way? I've made it do that now. >> + >> + 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. */ > > redundant "in"? Sounds right to me as is. Read as, the stop reply is "passed in". But where?, "in WAITSTATUS". I'll change it to something else if a native speaker prefers it. > The patch looks right to me. Thanks. Okay great, below's what I checked in. -- Pedro Alves 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/remote.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 71 insertions(+), 9 deletions(-) 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 ();