Re: Include putpkt in TRY_CATCH. PR gdb/15275

[Pedro Alves <palves at redhat dot com>]

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  <gareth@blacksphere.co.nz>
> 
> 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  <palves@redhat.com>

	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;