From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2023 invoked by alias); 28 Mar 2013 17:07:26 -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 1770 invoked by uid 89); 28 Mar 2013 17:07:18 -0000 X-Spam-SWARE-Status: No, score=-8.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 28 Mar 2013 17:07:15 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2SH7Dt4006446 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 28 Mar 2013 13:07:13 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r2SH7ACF001926; Thu, 28 Mar 2013 13:07:11 -0400 Message-ID: <515478BE.3030801@redhat.com> Date: Thu, 28 Mar 2013 20:14:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: Jan Kratochvil CC: Gareth McMullin , gdb-patches@sourceware.org Subject: Re: Include putpkt in TRY_CATCH. PR gdb/15275 References: <20130325195832.GA15218@host2.jankratochvil.net> In-Reply-To: <20130325195832.GA15218@host2.jankratochvil.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-03/txt/msg01066.txt.bz2 On 03/25/2013 07:58 PM, Jan Kratochvil wrote: > On Thu, 14 Mar 2013 03:02:29 +0100, Gareth McMullin wrote: >> If debugging with a remote connection over a serial link and the link is >> disconnected, say by disconnecting USB serial port, the GDB quit command no >> longer works. > [...] >> 2013-03-14 Gareth McMullin > > I am OK with the patch. > > Ccing also Pedro, the TARGET_CLOSE_ERROR was probably meant to handle it. Hmm, yes, but ... > > PR gdb/15275 > * remote.c (remote_get_trace_status): Include putpkt in TRY_CATCH. ... not this way. We just so happen to always send qTStatus, on and on, today, even if the target doesn't support tracing and keeps replying "" (meaning, I don't know that packet) to that packet. That's a bit silly, we should be using packet_ok, and disabling the packet altogether if the target ever reports it doesn't recognize it. I have a TODO to fix that, and once that's fixed, we'll expose this problem again. We'll fail in the whatever putpkt that happens to be tried next. 'k' happens to already have a hack: static void remote_kill (struct target_ops *ops) { /* Use catch_errors so the user can quit from gdb even when we aren't on speaking terms with the remote system. */ catch_errors (putpkt_for_catch_errors, "k", "", RETURN_MASK_ERROR); But e.g., vKill or "D" for detach doesn't. And detaching should delete breakpoints from the target before sending "D" (although detach command does, a detach caused by "quit" doesn't (if you "gdbserver --attach", "quit" will offer to detach instead of kill), and that's a bug...)), so it would fail earlier, on a memory/breakpoint access, or maybe something else, like trying to pause the target first, in non-stop mode. So, per Gareth's analysis, with tcp, where this is not reproducible, we don't error out on sends, but instead detect the error on the subsequent readchar. When that read fails, we throw the target away, and throw TARGET_CLOSE_ERROR. He was suggesting making ser-unix.c fake success too on failed writes, so we'd get to readchar detecting the error on serial ports too. But, why not let the error propagate out of serial_write, and catch it at the remote level instead of delaying the inevitable? IOW, throw away the target if writing fails too. That's what this patch does. It also fixes the original bug. ( There's a latent bug in the patch that that throw_perror_with_name call assumes error is still set to the right error, although remote_unpush_target is called in between. I'm copying this from preexisting code, and we can fix both at once.) if (serial_write (remote_desc, str, len)) { remote_unpush_target (); throw_perror_with_name (TARGET_CLOSE_ERROR, ) gdb/ 2013-03-28 Pedro Alves PR gdb/15275 * remote.c (send_interrupt_sequence): Use remote_serial_write. (remote_serial_write): New function. (putpkt_binary, getpkt_or_notif_sane_1): Use remote_serial_write. --- gdb/remote.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/gdb/remote.c b/gdb/remote.c index 8659953..a421ee5 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -122,6 +122,8 @@ static void remote_send (char **buf, long *sizeof_buf_p); static int readchar (int timeout); +static void remote_serial_write (const char *str, int len); + static void remote_kill (struct target_ops *ops); static int tohex (int nib); @@ -3209,13 +3211,13 @@ static void send_interrupt_sequence (void) { if (interrupt_sequence_mode == interrupt_sequence_control_c) - serial_write (remote_desc, "\x03", 1); + remote_serial_write ("\x03", 1); else if (interrupt_sequence_mode == interrupt_sequence_break) serial_send_break (remote_desc); else if (interrupt_sequence_mode == interrupt_sequence_break_g) { serial_send_break (remote_desc); - serial_write (remote_desc, "g", 1); + remote_serial_write ("g", 1); } else internal_error (__FILE__, __LINE__, @@ -7060,6 +7062,20 @@ readchar (int timeout) return ch; } +/* Wrapper for serial_write that closes the target on write error. */ + +static void +remote_serial_write (const char *str, int len) +{ + if (serial_write (remote_desc, str, len)) + { + remote_unpush_target (); + throw_perror_with_name (TARGET_CLOSE_ERROR, + _("Remote communication error. " + "Target disconnected.")); + } +} + /* Send the command in *BUF to the remote machine, and read the reply into *BUF. Report an error if we get an error reply. Resize *BUF using xrealloc if necessary to hold the result, and update @@ -7180,8 +7196,7 @@ putpkt_binary (char *buf, int cnt) gdb_flush (gdb_stdlog); do_cleanups (old_chain); } - if (serial_write (remote_desc, buf2, p - buf2)) - perror_with_name (_("putpkt: write failed")); + remote_serial_write (buf2, p - buf2); /* If this is a no acks version of the remote protocol, send the packet and move on. */ @@ -7236,7 +7251,7 @@ putpkt_binary (char *buf, int cnt) doesn't get retransmitted when we resend this packet. */ skip_frame (); - serial_write (remote_desc, "+", 1); + remote_serial_write ("+", 1); continue; /* Now, go look for +. */ } @@ -7591,7 +7606,7 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever, break; } - serial_write (remote_desc, "-", 1); + remote_serial_write ("-", 1); } if (tries > MAX_TRIES) @@ -7602,7 +7617,7 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever, /* Skip the ack char if we're in no-ack mode. */ if (!rs->noack_mode) - serial_write (remote_desc, "+", 1); + remote_serial_write ("+", 1); return -1; } @@ -7622,7 +7637,7 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever, /* Skip the ack char if we're in no-ack mode. */ if (!rs->noack_mode) - serial_write (remote_desc, "+", 1); + remote_serial_write ("+", 1); if (is_notif != NULL) *is_notif = 0; return val;