From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24690 invoked by alias); 17 Feb 2016 11:45:52 -0000 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 Received: (qmail 24668 invoked by uid 89); 17 Feb 2016 11:45:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 17 Feb 2016 11:45:45 +0000 Received: from svr-orw-fem-06.mgc.mentorg.com ([147.34.97.120]) by relay1.mentorg.com with esmtp id 1aW0Y9-0004em-MZ from Luis_Gustavo@mentor.com ; Wed, 17 Feb 2016 03:45:41 -0800 Received: from [172.30.1.77] (147.34.91.1) by SVR-ORW-FEM-06.mgc.mentorg.com (147.34.97.120) with Microsoft SMTP Server id 14.3.224.2; Wed, 17 Feb 2016 03:45:41 -0800 Subject: Re: [PATCH 1/5] gdb: Clean up remote.c:remote_resume References: <1455677091-13683-1-git-send-email-palves@redhat.com> <1455677091-13683-2-git-send-email-palves@redhat.com> To: Pedro Alves , Reply-To: Luis Machado From: Luis Machado Message-ID: <56C45D60.5060303@codesourcery.com> Date: Wed, 17 Feb 2016 11:45:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <1455677091-13683-2-git-send-email-palves@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-02/txt/msg00503.txt.bz2 Just nits. On 02/17/2016 12:44 AM, Pedro Alves wrote: > Just some refactoring / TLC. Mainly split the old c/s/C/S packet > handling to a separate function. > > gdb/ChangeLog: > 2016-02-09 Pedro Alves > > * remote.c (remote_resume_with_hc): New function, factored out > from ... > (remote_resume): ... this. Always try vCont first. > (remote_vcont_resume): Rename to ... > (remote_resume_with_vcont): ... this. Bail out if execution > direction is reverse. > --- > gdb/remote.c | 113 ++++++++++++++++++++++++++++++++--------------------------- > 1 file changed, 62 insertions(+), 51 deletions(-) > > diff --git a/gdb/remote.c b/gdb/remote.c > index fa97e1e..60e2dda 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -5460,6 +5460,58 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid) > return p; > } > > +/* Set the target running, using the packets that use Hc > + (c/s/C/S). */ > + > +static void > +remote_resume_with_hc (struct target_ops *ops, > + ptid_t ptid, int step, enum gdb_signal siggnal) > +{ > + struct remote_state *rs = get_remote_state (); > + struct thread_info *thread; > + char *buf; > + > + rs->last_sent_signal = siggnal; > + rs->last_sent_step = step; > + > + /* The c/s/C/S resume packets use Hc, so set the continue > + thread. */ > + if (ptid_equal (ptid, minus_one_ptid)) > + set_continue_thread (any_thread_ptid); > + else > + set_continue_thread (ptid); > + > + ALL_NON_EXITED_THREADS (thread) > + resume_clear_thread_private_info (thread); > + > + buf = rs->buf; > + if (execution_direction == EXEC_REVERSE) > + { > + /* We don't pass signals to the target in reverse exec mode. */ > + if (info_verbose && siggnal != GDB_SIGNAL_0) > + warning (_(" - Can't pass signal %d to target in reverse: ignored."), > + siggnal); > + Even though it is existing code, this reads a bit odd. Should we update it to "... in reverse execution: ..." maybe? > + if (step && packet_support (PACKET_bs) == PACKET_DISABLE) > + error (_("Remote reverse-step not supported.")); > + if (!step && packet_support (PACKET_bc) == PACKET_DISABLE) > + error (_("Remote reverse-continue not supported.")); > + > + strcpy (buf, step ? "bs" : "bc"); > + } > + else if (siggnal != GDB_SIGNAL_0) > + { > + buf[0] = step ? 'S' : 'C'; > + buf[1] = tohex (((int) siggnal >> 4) & 0xf); > + buf[2] = tohex (((int) siggnal) & 0xf); > + buf[3] = '\0'; > + } > + else > + strcpy (buf, step ? "s" : "c"); > + > + putpkt (buf); > +} > + > /* Resume the remote inferior by using a "vCont" packet. The thread > to be resumed is PTID; STEP and SIGGNAL indicate whether the > resumed thread should be single-stepped and/or signalled. If PTID > @@ -5467,16 +5519,20 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid) > be stepped and/or signalled is given in the global INFERIOR_PTID. > This function returns non-zero iff it resumes the inferior. > > - This function issues a strict subset of all possible vCont commands at the > - moment. */ > + This function issues a strict subset of all possible vCont commands > + at the moment. */ > > static int > -remote_vcont_resume (ptid_t ptid, int step, enum gdb_signal siggnal) > +remote_resume_with_vcont (ptid_t ptid, int step, enum gdb_signal siggnal) > { > struct remote_state *rs = get_remote_state (); > char *p; > char *endp; > > + /* No reverse support (yet) for vCont. */ > + if (execution_direction == EXEC_REVERSE) > + return 0; > + Same case as above. Also, do we need "(yet)"? > if (packet_support (PACKET_vCont) == PACKET_SUPPORT_UNKNOWN) > remote_vcont_probe (rs); > > @@ -5548,8 +5604,6 @@ remote_resume (struct target_ops *ops, > ptid_t ptid, int step, enum gdb_signal siggnal) > { > struct remote_state *rs = get_remote_state (); > - char *buf; > - struct thread_info *thread; > > /* In all-stop, we can't mark REMOTE_ASYNC_GET_PENDING_EVENTS_TOKEN > (explained in remote-notif.c:handle_notification) so > @@ -5560,53 +5614,10 @@ remote_resume (struct target_ops *ops, > if (!target_is_non_stop_p ()) > remote_notif_process (rs->notif_state, ¬if_client_stop); > > - rs->last_sent_signal = siggnal; > - rs->last_sent_step = step; > - > - /* The vCont packet doesn't need to specify threads via Hc. */ > - /* No reverse support (yet) for vCont. */ > - if (execution_direction != EXEC_REVERSE) > - if (remote_vcont_resume (ptid, step, siggnal)) > - goto done; > - > - /* All other supported resume packets do use Hc, so set the continue > - thread. */ > - if (ptid_equal (ptid, minus_one_ptid)) > - set_continue_thread (any_thread_ptid); > - else > - set_continue_thread (ptid); > - > - ALL_NON_EXITED_THREADS (thread) > - resume_clear_thread_private_info (thread); > - > - buf = rs->buf; > - if (execution_direction == EXEC_REVERSE) > - { > - /* We don't pass signals to the target in reverse exec mode. */ > - if (info_verbose && siggnal != GDB_SIGNAL_0) > - warning (_(" - Can't pass signal %d to target in reverse: ignored."), > - siggnal); > - > - if (step && packet_support (PACKET_bs) == PACKET_DISABLE) > - error (_("Remote reverse-step not supported.")); > - if (!step && packet_support (PACKET_bc) == PACKET_DISABLE) > - error (_("Remote reverse-continue not supported.")); > - > - strcpy (buf, step ? "bs" : "bc"); > - } > - else if (siggnal != GDB_SIGNAL_0) > - { > - buf[0] = step ? 'S' : 'C'; > - buf[1] = tohex (((int) siggnal >> 4) & 0xf); > - buf[2] = tohex (((int) siggnal) & 0xf); > - buf[3] = '\0'; > - } > - else > - strcpy (buf, step ? "s" : "c"); > - > - putpkt (buf); > + /* Prefer vCont, and fallback to s/c/S/C, which use Hc. */ > + if (!remote_resume_with_vcont (ptid, step, siggnal)) > + remote_resume_with_hc (ops, ptid, step, siggnal); > > - done: > /* We are about to start executing the inferior, let's register it > with the event loop. NOTE: this is the one place where all the > execution commands end up. We could alternatively do this in each >