From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28451 invoked by alias); 16 Oct 2003 20:31:59 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 28398 invoked from network); 16 Oct 2003 20:31:56 -0000 Received: from unknown (HELO nevyn.them.org) (66.93.172.17) by sources.redhat.com with SMTP; 16 Oct 2003 20:31:56 -0000 Received: from drow by nevyn.them.org with local (Exim 4.24 #1 (Debian)) id 1AAEmq-0007ZH-Dd for ; Thu, 16 Oct 2003 16:31:56 -0400 Date: Thu, 16 Oct 2003 20:31:00 -0000 From: Daniel Jacobowitz To: gdb-patches@sources.redhat.com Subject: Re: RFA/RFC: vCont for the remote protocol [client] Message-ID: <20031016203156.GA24204@nevyn.them.org> Mail-Followup-To: gdb-patches@sources.redhat.com References: <20030929152831.GA23286@nevyn.them.org> <20030930211717.GB19869@nevyn.them.org> <3F8C917C.1080708@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3F8C917C.1080708@gnu.org> User-Agent: Mutt/1.5.1i X-SW-Source: 2003-10/txt/msg00561.txt.bz2 On Tue, Oct 14, 2003 at 08:14:52PM -0400, Andrew Cagney wrote: > [I think my GNU e-mail is down] [I'm also having trouble reading this > unified diff, so watch out :-)] > > +/* Check for the availability of vCont. This function should also check > + the response. */ > > see below ... > > > static void > >-remote_resume (ptid_t ptid, int step, enum target_signal siggnal) > >+remote_vcont_probe (struct remote_state *rs, char *buf) > > { > >- struct remote_state *rs = get_remote_state (); > >- char *buf = alloca (rs->remote_packet_size); > >- int pid = PIDGET (ptid); > >- char *p; > >- > >- if (pid == -1) > >- set_thread (0, 0); /* run any thread */ > >- else > >- set_thread (pid, 0); /* run this thread */ > >- > >- last_sent_signal = siggnal; > >- last_sent_step = step; > >+ strcpy (buf, "vCont?"); > >+ putpkt (buf); > >+ getpkt (buf, rs->remote_packet_size, 0); > >+ packet_ok (buf, &remote_protocol_vcont); > >+} > > packet_ok will go through the path: > > /* The packet may or may not be OK. Just assume it is */ > return PACKET_OK; > > Shouldn't remote_vcont_probe apply additional sanity checks. For > instance that the response is well formed, and that all the letters > [SCsc] appeared? If they are not all present, just mark the packet is > not supported for now. (But also comment this). The "should" there was supposed to imply "but we don't". But hey, it was easy, so fixed. > +static int > +remote_vcont_resume (ptid_t ptid, int step, enum target_signal siggnal) > > What's the return value? >From the comment a few lines up. This function returns non-zero iff it resumes the inferior. > +{ > + struct remote_state *rs = get_remote_state (); > + int pid = PIDGET (ptid); > + char *buf = alloca (rs->remote_packet_size); > > Rather than ALLOCA here, can you ... > > + /* If we could generate a wider range of packets, we'd have to worry > + about overflowing BUF. Should there be a generic > + "multi-part-packet" packet? */ > + > + if (PIDGET (inferior_ptid) == MAGIC_NULL_PID) > + { > + /* MAGIC_NULL_PTID means that we don't have any active threads, so we > + don't have any PID numbers the inferior will understand. Make sure > + to only send forms that do not specify a PID. */ > + if (step && siggnal != TARGET_SIGNAL_0) > + sprintf (buf, "vCont;S%02x", siggnal); > > ... use XASPRINTF here (along with some sort of cleanup). Is GDB trying to move away from alloca? The internals manual says: GDB can use the non-portable function `alloca' for the allocation of small temporary values (such as strings). So I use it to avoid cleanups. OTOH, it occurs to me that rs->remote_packet_size is a bit large; OTOOH, remote.c uses this idiom all over the place already. I've used xmalloc instead, since the buf is used for getpkt and thus must be remote_packet_size large. Here's what I am about to check in. -- Daniel Jacobowitz MontaVista Software Debian GNU/Linux Developer 2003-10-16 Daniel Jacobowitz * remote.c (remote_protocol_vcont): New variable. (set_remote_protocol_vcont_packet_cmd): New function. (show_remote_protocol_vcont_packet_cmd): New function. (init_all_packet_configs): Handle remote_protocol_vcont. (remote_vcont_probe): New function. (remote_vcont_resume): New function. (remote_resume): Use it. (remote_async_resume): Call remote_resume. (_initialize_remote): Add verbose-resume packet commands. Index: gdb/remote.c =================================================================== RCS file: /big/fsf/rsync/src-cvs/src/gdb/remote.c,v retrieving revision 1.117 diff -u -p -r1.117 remote.c --- gdb/remote.c 15 Oct 2003 21:10:55 -0000 1.117 +++ gdb/remote.c 16 Oct 2003 20:09:26 -0000 @@ -749,6 +749,23 @@ packet_ok (const char *buf, struct packe } } +/* Should we try the 'vCont' (descriptive resume) request? */ +static struct packet_config remote_protocol_vcont; + +static void +set_remote_protocol_vcont_packet_cmd (char *args, int from_tty, + struct cmd_list_element *c) +{ + update_packet_config (&remote_protocol_vcont); +} + +static void +show_remote_protocol_vcont_packet_cmd (char *args, int from_tty, + struct cmd_list_element *c) +{ + show_packet_config_cmd (&remote_protocol_vcont); +} + /* Should we try the 'qSymbol' (target symbol lookup service) request? */ static struct packet_config remote_protocol_qSymbol; @@ -2190,6 +2207,7 @@ init_all_packet_configs (void) update_packet_config (&remote_protocol_E); update_packet_config (&remote_protocol_P); update_packet_config (&remote_protocol_qSymbol); + update_packet_config (&remote_protocol_vcont); for (i = 0; i < NR_Z_PACKET_TYPES; i++) update_packet_config (&remote_protocol_Z[i]); /* Force remote_write_bytes to check whether target supports binary @@ -2503,115 +2521,144 @@ bin2hex (const char *bin, char *hex, int return i; } -/* Tell the remote machine to resume. */ - -static enum target_signal last_sent_signal = TARGET_SIGNAL_0; - -static int last_sent_step; +/* Check for the availability of vCont. This function should also check + the response. */ static void -remote_resume (ptid_t ptid, int step, enum target_signal siggnal) +remote_vcont_probe (struct remote_state *rs, char *buf) { - struct remote_state *rs = get_remote_state (); - char *buf = alloca (rs->remote_packet_size); - int pid = PIDGET (ptid); - char *p; + strcpy (buf, "vCont?"); + putpkt (buf); + getpkt (buf, rs->remote_packet_size, 0); - if (pid == -1) - set_thread (0, 0); /* run any thread */ - else - set_thread (pid, 0); /* run this thread */ + /* Make sure that the features we assume are supported. */ + if (strncmp (buf, "vCont", 5) == 0) + { + char *p = &buf[5]; + int support_s, support_S, support_c, support_C; - last_sent_signal = siggnal; - last_sent_step = step; + support_s = 0; + support_S = 0; + support_c = 0; + support_C = 0; + while (p && *p == ';') + { + p++; + if (*p == 's' && (*(p + 1) == ';' || *(p + 1) == 0)) + support_s = 1; + else if (*p == 'S' && (*(p + 1) == ';' || *(p + 1) == 0)) + support_S = 1; + else if (*p == 'c' && (*(p + 1) == ';' || *(p + 1) == 0)) + support_c = 1; + else if (*p == 'C' && (*(p + 1) == ';' || *(p + 1) == 0)) + support_C = 1; - /* A hook for when we need to do something at the last moment before - resumption. */ - if (target_resume_hook) - (*target_resume_hook) (); + p = strchr (p, ';'); + } + /* If s, S, c, and C are not all supported, we can't use vCont. Clearing + BUF will make packet_ok disable the packet. */ + if (!support_s || !support_S || !support_c || !support_C) + buf[0] = 0; + } - /* The s/S/c/C packets do not return status. So if the target does - not support the S or C packets, the debug agent returns an empty - string which is detected in remote_wait(). This protocol defect - is fixed in the e/E packets. */ + packet_ok (buf, &remote_protocol_vcont); +} - if (step && step_range_end) - { - /* If the target does not support the 'E' packet, we try the 'S' - packet. Ideally we would fall back to the 'e' packet if that - too is not supported. But that would require another copy of - the code to issue the 'e' packet (and fall back to 's' if not - supported) in remote_wait(). */ - - if (siggnal != TARGET_SIGNAL_0) - { - if (remote_protocol_E.support != PACKET_DISABLE) - { - p = buf; - *p++ = 'E'; - *p++ = tohex (((int) siggnal >> 4) & 0xf); - *p++ = tohex (((int) siggnal) & 0xf); - *p++ = ','; - p += hexnumstr (p, (ULONGEST) step_range_start); - *p++ = ','; - p += hexnumstr (p, (ULONGEST) step_range_end); - *p++ = 0; +/* 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's + PID is -1, then all threads are resumed; the thread to be stepped and/or + signalled is given in the global INFERIOR_PTID. This function returns + non-zero iff it resumes the inferior. - putpkt (buf); - getpkt (buf, (rs->remote_packet_size), 0); + This function issues a strict subset of all possible vCont commands at the + moment. */ - if (packet_ok (buf, &remote_protocol_E) == PACKET_OK) - return; - } - } - else - { - if (remote_protocol_e.support != PACKET_DISABLE) - { - p = buf; - *p++ = 'e'; - p += hexnumstr (p, (ULONGEST) step_range_start); - *p++ = ','; - p += hexnumstr (p, (ULONGEST) step_range_end); - *p++ = 0; +static int +remote_vcont_resume (ptid_t ptid, int step, enum target_signal siggnal) +{ + struct remote_state *rs = get_remote_state (); + int pid = PIDGET (ptid); + char *buf = NULL; + struct cleanup *old_cleanup; - putpkt (buf); - getpkt (buf, (rs->remote_packet_size), 0); + buf = xmalloc (rs->remote_packet_size); + old_cleanup = make_cleanup (xfree, buf); - if (packet_ok (buf, &remote_protocol_e) == PACKET_OK) - return; - } - } + if (remote_protocol_vcont.support == PACKET_SUPPORT_UNKNOWN) + remote_vcont_probe (rs, buf); + + if (remote_protocol_vcont.support == PACKET_DISABLE) + { + do_cleanups (old_cleanup); + return 0; } - if (siggnal != TARGET_SIGNAL_0) + /* If we could generate a wider range of packets, we'd have to worry + about overflowing BUF. Should there be a generic + "multi-part-packet" packet? */ + + if (PIDGET (inferior_ptid) == MAGIC_NULL_PID) + { + /* MAGIC_NULL_PTID means that we don't have any active threads, so we + don't have any PID numbers the inferior will understand. Make sure + to only send forms that do not specify a PID. */ + if (step && siggnal != TARGET_SIGNAL_0) + sprintf (buf, "vCont;S%02x", siggnal); + else if (step) + sprintf (buf, "vCont;s"); + else if (siggnal != TARGET_SIGNAL_0) + sprintf (buf, "vCont;C%02x", siggnal); + else + sprintf (buf, "vCont;c"); + } + else if (pid == -1) { - buf[0] = step ? 'S' : 'C'; - buf[1] = tohex (((int) siggnal >> 4) & 0xf); - buf[2] = tohex (((int) siggnal) & 0xf); - buf[3] = '\0'; + /* Resume all threads, with preference for INFERIOR_PTID. */ + if (step && siggnal != TARGET_SIGNAL_0) + sprintf (buf, "vCont;S%02x:%x;c", siggnal, PIDGET (inferior_ptid)); + else if (step) + sprintf (buf, "vCont;s:%x;c", PIDGET (inferior_ptid)); + else if (siggnal != TARGET_SIGNAL_0) + sprintf (buf, "vCont;C%02x:%x;c", siggnal, PIDGET (inferior_ptid)); + else + sprintf (buf, "vCont;c"); } else - strcpy (buf, step ? "s" : "c"); + { + /* Scheduler locking; resume only PTID. */ + if (step && siggnal != TARGET_SIGNAL_0) + sprintf (buf, "vCont;S%02x:%x", siggnal, pid); + else if (step) + sprintf (buf, "vCont;s:%x", pid); + else if (siggnal != TARGET_SIGNAL_0) + sprintf (buf, "vCont;C%02x:%x", siggnal, pid); + else + sprintf (buf, "vCont;c:%x", pid); + } putpkt (buf); + + do_cleanups (old_cleanup); + + return 1; } -/* Same as remote_resume, but with async support. */ +/* Tell the remote machine to resume. */ + +static enum target_signal last_sent_signal = TARGET_SIGNAL_0; + +static int last_sent_step; + static void -remote_async_resume (ptid_t ptid, int step, enum target_signal siggnal) +remote_resume (ptid_t ptid, int step, enum target_signal siggnal) { struct remote_state *rs = get_remote_state (); char *buf = alloca (rs->remote_packet_size); int pid = PIDGET (ptid); char *p; - if (pid == -1) - set_thread (0, 0); /* run any thread */ - else - set_thread (pid, 0); /* run this thread */ - last_sent_signal = siggnal; last_sent_step = step; @@ -2620,6 +2667,16 @@ remote_async_resume (ptid_t ptid, int st if (target_resume_hook) (*target_resume_hook) (); + /* The vCont packet doesn't need to specify threads via Hc. */ + if (remote_vcont_resume (ptid, step, siggnal)) + return; + + /* All other supported resume packets do use Hc, so call set_thread. */ + if (pid == -1) + set_thread (0, 0); /* run any thread */ + else + set_thread (pid, 0); /* run this thread */ + /* The s/S/c/C packets do not return status. So if the target does not support the S or C packets, the debug agent returns an empty string which is detected in remote_wait(). This protocol defect @@ -2651,7 +2708,7 @@ remote_async_resume (ptid_t ptid, int st getpkt (buf, (rs->remote_packet_size), 0); if (packet_ok (buf, &remote_protocol_E) == PACKET_OK) - goto register_event_loop; + return; } } else @@ -2669,7 +2726,7 @@ remote_async_resume (ptid_t ptid, int st getpkt (buf, (rs->remote_packet_size), 0); if (packet_ok (buf, &remote_protocol_e) == PACKET_OK) - goto register_event_loop; + return; } } } @@ -2678,15 +2735,21 @@ remote_async_resume (ptid_t ptid, int st { buf[0] = step ? 'S' : 'C'; buf[1] = tohex (((int) siggnal >> 4) & 0xf); - buf[2] = tohex ((int) siggnal & 0xf); + buf[2] = tohex (((int) siggnal) & 0xf); buf[3] = '\0'; } else strcpy (buf, step ? "s" : "c"); - + putpkt (buf); +} + +/* Same as remote_resume, but with async support. */ +static void +remote_async_resume (ptid_t ptid, int step, enum target_signal siggnal) +{ + remote_resume (ptid, step, siggnal); -register_event_loop: /* 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 @@ -5952,6 +6015,7 @@ show_remote_cmd (char *args, int from_tt show_remote_protocol_E_packet_cmd (args, from_tty, NULL); show_remote_protocol_P_packet_cmd (args, from_tty, NULL); show_remote_protocol_qSymbol_packet_cmd (args, from_tty, NULL); + show_remote_protocol_vcont_packet_cmd (args, from_tty, NULL); show_remote_protocol_binary_download_cmd (args, from_tty, NULL); } @@ -6124,6 +6188,13 @@ in a memory packet.\n", add_info ("remote-process", remote_info_process, "Query the remote system for process info."); + + add_packet_config_cmd (&remote_protocol_vcont, + "vCont", "verbose-resume", + set_remote_protocol_vcont_packet_cmd, + show_remote_protocol_vcont_packet_cmd, + &remote_set_cmdlist, &remote_show_cmdlist, + 0); add_packet_config_cmd (&remote_protocol_qSymbol, "qSymbol", "symbol-lookup",