Include putpkt in TRY_CATCH. PR gdb/51275

Include putpkt in TRY_CATCH. PR gdb/51275

[Gareth McMullin]

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.

Including the call to putpkt in the TRY_CATCH in remote_get_trace_status
allows the exception to be caught and reported, but still allows GDB to exit
on the quit command.

This fixes bug 15275 and probably 11221 which I believe is the same problem.

gdb:

2013-03-14  Gareth McMullin  <gareth@blacksphere.co.nz>

        PR gdb/15275
        * remote.c (remote_get_trace_status): Include putpkt in TRY_CATCH.

---
diff --git a/gdb/remote.c b/gdb/remote.c
index 21d86f7..29c64c0 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10695,10 +10695,9 @@ remote_get_trace_status (struct trace_status *ts)

   trace_regblock_size = get_remote_arch_state ()->sizeof_g_packet;

-  putpkt ("qTStatus");
-
   TRY_CATCH (ex, RETURN_MASK_ERROR)
     {
+      putpkt ("qTStatus");
       p = remote_get_noisy_reply (&target_buf, &target_buf_size);
     }
   if (ex.reason < 0)

Re: Include putpkt in TRY_CATCH. PR gdb/51275

[Jan Kratochvil]

On Thu, 14 Mar 2013 03:02:29 +0100, Gareth McMullin wrote:
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -10695,10 +10695,9 @@ remote_get_trace_status (struct trace_status *ts)
> 
>    trace_regblock_size = get_remote_arch_state ()->sizeof_g_packet;
> 
> -  putpkt ("qTStatus");
> -
>    TRY_CATCH (ex, RETURN_MASK_ERROR)
>      {
> +      putpkt ("qTStatus");
>        p = remote_get_noisy_reply (&target_buf, &target_buf_size);
>      }
>    if (ex.reason < 0)

Could you test this patch instead?
	Fix 7.5 regression crashing GDB if gdbserver dies
	http://sourceware.org/ml/gdb-patches/2013-03/msg00691.html

I guess it should also fix it but I probably cannot not replicate your serial
stub conditions here.


Thanks,
Jan

Re: Include putpkt in TRY_CATCH. PR gdb/51275

[Gareth McMullin]

On Thu, Mar 21, 2013 at 6:08 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> On Thu, 14 Mar 2013 03:02:29 +0100, Gareth McMullin wrote:
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -10695,10 +10695,9 @@ remote_get_trace_status (struct trace_status *ts)
>>
>>    trace_regblock_size = get_remote_arch_state ()->sizeof_g_packet;
>>
>> -  putpkt ("qTStatus");
>> -
>>    TRY_CATCH (ex, RETURN_MASK_ERROR)
>>      {
>> +      putpkt ("qTStatus");
>>        p = remote_get_noisy_reply (&target_buf, &target_buf_size);
>>      }
>>    if (ex.reason < 0)
>
> Could you test this patch instead?
>         Fix 7.5 regression crashing GDB if gdbserver dies
>         http://sourceware.org/ml/gdb-patches/2013-03/msg00691.html

That patch doesn't solve the problem, as the exception is raised in putpkt when
the serial_write returns an error.  The problem is not an error reported by the
remote side, but an error reported by the host system because it is unable to
send.  Applying that patch and my patch together also does not solve
the problem,  as the exception raised is GENERAL_ERROR which is
pushed up by your patch.

> I guess it should also fix it but I probably cannot not replicate your serial
> stub conditions here.

I tried to reproduce with GDB server, because I expected similar behaviour on
a broken socket, but this actually works, on Linux, at least:

(gdb) tar ext localhost:2000
Remote debugging using localhost:2000
(gdb) set debug remote 1
... gdbserver killed from another terminal ...
(gdb) quit
Sending packet: $qTStatus#49...Remote connection closed
(gdb) quit

The send(2) ultimately called by putpkt actually succeeds on the broken socket,
while the write(2) used for Unix serial ports fails with EIO.  Changing
ser_unix_write_prim to fake success on EIO doesn't solve the problem as the
serial read doesn't distinguish between EOF and TIMEOUT.  I'll need to examine
ser-unix.c in more detail to see exactly what's going on there.

Is reworking ser-unix.c to behave more like ser-tcp.c a better fix than catching
the exception raised by putpkt?

Regards,
Gareth

Re: Include putpkt in TRY_CATCH. PR gdb/51275

[Gareth McMullin]

On Thu, Mar 21, 2013 at 1:41 PM, Gareth McMullin
<gareth@blacksphere.co.nz> wrote:
> On Thu, Mar 21, 2013 at 6:08 AM, Jan Kratochvil
> <jan.kratochvil@redhat.com> wrote:
>> I guess it should also fix it but I probably cannot not replicate your serial
>> stub conditions here.
>
> I tried to reproduce with GDB server, because I expected similar behaviour on
> a broken socket, but this actually works, on Linux, at least:
>
> (gdb) tar ext localhost:2000
> Remote debugging using localhost:2000
> (gdb) set debug remote 1
> ... gdbserver killed from another terminal ...
> (gdb) quit
> Sending packet: $qTStatus#49...Remote connection closed
> (gdb) quit
>
> The send(2) ultimately called by putpkt actually succeeds on the broken socket,
> while the write(2) used for Unix serial ports fails with EIO.  Changing
> ser_unix_write_prim to fake success on EIO doesn't solve the problem as the
> serial read doesn't distinguish between EOF and TIMEOUT.  I'll need to examine
> ser-unix.c in more detail to see exactly what's going on there.

Changing ser-unix.c to use ser_base_readchar and faking success on EIO in
ser_unix_write_prim fixes this for me.  There are warnings in the comments
about using user_base_readchar in ser-unix.c because of read returning no
data on timeout, but if I read it correctly, read won't be called unless select
found it ready to read.  The timeout is handled by select which works for me on
Linux/termios and seems like a better solution than using the termios VTIME
parameter.  It also appears that this is exactly what it would be doing if using
the SGTTY serial code too.  Is there a reason we would prefer to use VTIME
for the timeout rather than select when using the termio/termios interface?

Regards,
Gareth

Re: Include putpkt in TRY_CATCH. PR gdb/15275

[Jan Kratochvil]

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>
> 
>         PR gdb/15275
>         * remote.c (remote_get_trace_status): Include putpkt in TRY_CATCH.

I am OK with the patch.

Ccing also Pedro, the TARGET_CLOSE_ERROR was probably meant to handle it.

I will check it in.

Used this patch to simulate the problem with TCP gdbserver:
	(gdb) q
	A debugging session is active.

		Inferior 1 [process 30674] will be killed.

	Quit anyway? (y or n) y
	Sending packet: $qTStatus#49...putpkt: write failed: Input/output error.
	(gdb) q
->
	(gdb) q
	A debugging session is active.

		Inferior 1 [process 31864] will be killed.

	Quit anyway? (y or n) y
	Sending packet: $qTStatus#49...qTStatus: putpkt: write failed: Input/output error.
	Sending packet: $k#6b...putpkt: write failed: Input/output error.
	$ _

#0  net_write_prim (scb=0x41f4b90, buf=0x7fff617af320, count=12) at ser-tcp.c:352
#1  in ser_base_write (scb=0x41f4b90, str=0x7fff617af320 "$qTStatus#49", len=12) at ser-base.c:449
#2  in serial_write (scb=0x41f4b90, str=0x7fff617af320 "$qTStatus#49", len=12) at serial.c:427
#3  in putpkt_binary (buf=0xf617b7 "qTStatus", cnt=8) at remote.c:7183
#4  in putpkt (buf=0xf617b7 "qTStatus") at remote.c:7114
#5  in remote_get_trace_status (ts=0x1ebd860 <trace_status>) at remote.c:10695
#6  in disconnect_tracing (from_tty=1) at tracepoint.c:2224
#7  in quit_command (args=0x0, from_tty=1) at ./cli/cli-cmds.c:325


#--- a/gdb/ser-tcp.c
#+++ b/gdb/ser-tcp.c
#@@ -341,9 +341,19 @@ net_read_prim (struct serial *scb, size_t count)
#   return recv (scb->fd, scb->buf, count, 0);
# }
# 
#+static volatile int hacked;
#+static void hackusr2(int signo) {
#+  hacked=1;
#+}
#+
# int
# net_write_prim (struct serial *scb, const void *buf, size_t count)
# {
#+signal(SIGUSR2,hackusr2);
#+if (hacked) {
#+  errno=EIO;
#+  return -1;
#+}
#   return send (scb->fd, buf, count, 0);
# }
# 


Thanks,
Jan

> ---
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 21d86f7..29c64c0 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -10695,10 +10695,9 @@ remote_get_trace_status (struct trace_status *ts)
> 
>    trace_regblock_size = get_remote_arch_state ()->sizeof_g_packet;
> 
> -  putpkt ("qTStatus");
> -
>    TRY_CATCH (ex, RETURN_MASK_ERROR)
>      {
> +      putpkt ("qTStatus");
>        p = remote_get_noisy_reply (&target_buf, &target_buf_size);
>      }
>    if (ex.reason < 0)

[commit+7.6] Re: Include putpkt in TRY_CATCH. PR gdb/15275

[Jan Kratochvil]

On Mon, 25 Mar 2013 20:58:32 +0100, Jan Kratochvil wrote:
> On Thu, 14 Mar 2013 03:02:29 +0100, Gareth McMullin wrote:
> > 2013-03-14  Gareth McMullin  <gareth@blacksphere.co.nz>
> > 
> >         PR gdb/15275
> >         * remote.c (remote_get_trace_status): Include putpkt in TRY_CATCH.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2013-03/msg00259.html
and for 7.6:
	http://sourceware.org/ml/gdb-cvs/2013-03/msg00260.html


Thanks,
Jan

Re: Include putpkt in TRY_CATCH. PR gdb/15275

[Pedro Alves]

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;

[PATCH] make -gdb-exit call disconnect_tracing too, and don't lose history if the target errors on "quit" (was: Re: Include putpkt in TRY_CATCH. PR gdb/15275)

[Pedro Alves]

Gareth mentions in PR gdb/15275:

 "The MI '-gdb-exit' command mi_cmd_gdb_exit() never calls disconnect_tracing()
 and therefore exits correctly."

It should, so to get out of tfind mode, as quit may detach instead of
kill, and we don't want to confuse the memory/register accesses etc. of
the detach process.  So we should push down the disconnect tracing bits
to quit_force.  But we can't as is, as that would swallow the
error thrown by answering no to:

  Trace is running but will stop on detach; detach anyway? (y or n) 

So to address that, we split disconnect_tracing in two.  One part
the query, another part the rest, and we make quit_force call the
latter.

Looking at quit_force, it does several things, some of which are
a bit independent of the others.  It first kills/detaches, and
then writes history, and then runs the final cleanups.  It seems
better to me to do each of these things even if the previous
thing throws.  E.g., as is, if something throws while detaching,
then we skip writing history.

Tested on x86_64 Fedora 17.

gdb/
2013-03-28  Pedro Alves  <palves@redhat.com>

	* cli/cli-cmds.c (quit_command): Call query_if_trace_running
	instead of disconnect_tracing.
	* infcmd.c (detach_command, disconnect_command): Call
	query_if_trace_running.  Adjust.
	* top.c: Include "tracepoint.h".
	(quit_target): Delete.  Contents moved ...
	(quit_force): ... here.  Wrap each stage of tear down in
	TRY_CATCH.  Call disconnect_tracing before detaching.
---

 gdb/cli/cli-cmds.c |    2 +-
 gdb/infcmd.c       |    7 ++++--
 gdb/top.c          |   66 ++++++++++++++++++++++++++++++++--------------------
 gdb/tracepoint.c   |   21 +++++++++++++----
 gdb/tracepoint.h   |    3 ++
 5 files changed, 65 insertions(+), 34 deletions(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index c05f77f..c4721bd 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -322,7 +322,7 @@ quit_command (char *args, int from_tty)
   if (!quit_confirm ())
     error (_("Not confirmed."));
 
-  disconnect_tracing (from_tty);
+  query_if_trace_running (from_tty);
 
   quit_force (args, from_tty);
 }
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 0b40f22..ee90f95 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2721,7 +2721,9 @@ detach_command (char *args, int from_tty)
   if (ptid_equal (inferior_ptid, null_ptid))
     error (_("The program is not being run."));
 
-  disconnect_tracing (from_tty);
+  query_if_trace_running (from_tty);
+
+  disconnect_tracing ();
 
   target_detach (args, from_tty);
 
@@ -2751,7 +2753,8 @@ static void
 disconnect_command (char *args, int from_tty)
 {
   dont_repeat ();		/* Not for the faint of heart.  */
-  disconnect_tracing (from_tty);
+  query_if_trace_running (from_tty);
+  disconnect_tracing ();
   target_disconnect (args, from_tty);
   no_shared_libraries (NULL, from_tty);
   init_thread_list ();
diff --git a/gdb/top.c b/gdb/top.c
index e2c4c61..bc61d3b 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -64,6 +64,7 @@
 #include <ctype.h>
 #include "ui-out.h"
 #include "cli-out.h"
+#include "tracepoint.h"
 
 extern void initialize_all_files (void);
 
@@ -1282,29 +1283,6 @@ quit_confirm (void)
   return qr;
 }
 
-/* Helper routine for quit_force that requires error handling.  */
-
-static int
-quit_target (void *arg)
-{
-  struct qt_args *qt = (struct qt_args *)arg;
-
-  /* Kill or detach all inferiors.  */
-  iterate_over_inferiors (kill_or_detach, qt);
-
-  /* Give all pushed targets a chance to do minimal cleanup, and pop
-     them all out.  */
-  pop_all_targets ();
-
-  /* Save the history information if it is appropriate to do so.  */
-  if (write_history_p && history_filename)
-    write_history (history_filename);
-
-  do_final_cleanups (all_cleanups ());    /* Do any final cleanups before
-					     exiting.  */
-  return 0;
-}
-
 /* Quit without asking for confirmation.  */
 
 void
@@ -1312,6 +1290,7 @@ quit_force (char *args, int from_tty)
 {
   int exit_code = 0;
   struct qt_args qt;
+  volatile struct gdb_exception ex;
 
   /* An optional expression may be used to cause gdb to terminate with the 
      value of that expression.  */
@@ -1327,9 +1306,46 @@ quit_force (char *args, int from_tty)
   qt.args = args;
   qt.from_tty = from_tty;
 
+  /* Wrappers to make the code below a bit more readable.  */
+#define DO_TRY \
+  TRY_CATCH (ex, RETURN_MASK_ALL)
+
+#define DO_PRINT_EX \
+  if (ex.reason < 0) \
+    exception_print (gdb_stderr, ex)
+
   /* We want to handle any quit errors and exit regardless.  */
-  catch_errors (quit_target, &qt,
-	        "Quitting: ", RETURN_MASK_ALL);
+
+  /* Get out of tfind mode, and kill or detach all inferiors.  */
+  DO_TRY
+    {
+      disconnect_tracing ();
+      iterate_over_inferiors (kill_or_detach, &qt);
+    }
+  DO_PRINT_EX;
+
+  /* Give all pushed targets a chance to do minimal cleanup, and pop
+     them all out.  */
+  DO_TRY
+    {
+      pop_all_targets ();
+    }
+  DO_PRINT_EX;
+
+  /* Save the history information if it is appropriate to do so.  */
+  DO_TRY
+    {
+      if (write_history_p && history_filename)
+	write_history (history_filename);
+    }
+  DO_PRINT_EX;
+
+  /* Do any final cleanups before exiting.  */
+  DO_TRY
+    {
+      do_final_cleanups (all_cleanups ());
+    }
+  DO_PRINT_EX;
 
   exit (exit_code);
 }
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 9a2425b..04125fb 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -2211,12 +2211,15 @@ trace_status_mi (int on_stop)
   }
 }
 
-/* This function handles the details of what to do about an ongoing
-   tracing run if the user has asked to detach or otherwise disconnect
-   from the target.  */
+/* Check if a trace run is ongoing.  If so, and FROM_TTY, query the
+   user if she really wants to detach.  */
+
 void
-disconnect_tracing (int from_tty)
+query_if_trace_running (int from_tty)
 {
+  if (!from_tty)
+    return;
+
   /* It can happen that the target that was tracing went away on its
      own, and we didn't notice.  Get a status update, and if the
      current target doesn't even do tracing, then assume it's not
@@ -2229,7 +2232,7 @@ disconnect_tracing (int from_tty)
      just going to disconnect and let the target deal with it,
      according to how it's been instructed previously via
      disconnected-tracing.  */
-  if (current_trace_status ()->running && from_tty)
+  if (current_trace_status ()->running)
     {
       process_tracepoint_on_disconnect ();
 
@@ -2246,7 +2249,15 @@ disconnect_tracing (int from_tty)
 	    error (_("Not confirmed."));
 	}
     }
+}
 
+/* This function handles the details of what to do about an ongoing
+   tracing run if the user has asked to detach or otherwise disconnect
+   from the target.  */
+
+void
+disconnect_tracing (void)
+{
   /* Also we want to be out of tfind mode, otherwise things can get
      confusing upon reconnection.  Just use these calls instead of
      full tfind_1 behavior because we're in the middle of detaching,
diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h
index c7eef7b..f603c75 100644
--- a/gdb/tracepoint.h
+++ b/gdb/tracepoint.h
@@ -372,7 +372,8 @@ extern struct tracepoint *create_tracepoint_from_upload (struct uploaded_tp *utp
 extern void merge_uploaded_tracepoints (struct uploaded_tp **utpp);
 extern void merge_uploaded_trace_state_variables (struct uploaded_tsv **utsvp);
 
-extern void disconnect_tracing (int from_tty);
+extern void query_if_trace_running (int from_tty);
+extern void disconnect_tracing (void);
 
 extern void start_tracing (char *notes);
 extern void stop_tracing (char *notes);

Re: Include putpkt in TRY_CATCH. PR gdb/15275

[Gareth McMullin]

On Thu, Mar 28, 2013 at 10:07 AM, Pedro Alves <palves@redhat.com> wrote:
> 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.

I like this approach, it is a better solution.  Thank you.

Gareth

Re: Include putpkt in TRY_CATCH. PR gdb/15275

[Jan Kratochvil]

On Thu, 28 Mar 2013 18:07:10 +0100, Pedro Alves wrote:
> 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",

I see it working:

#0  remote_remove_breakpoint at remote.c:8113
#1  in target_remove_breakpoint at target.c:2462
#2  in bkpt_remove_location at breakpoint.c:12983
#3  in remove_breakpoint_1 at breakpoint.c:3590
#4  in remove_breakpoint at breakpoint.c:3696
#5  in remove_breakpoints_pid at breakpoint.c:2955
#6  in target_detach at target.c:2595
#7  in kill_or_detach at top.c:1212
#8  in iterate_over_inferiors at inferior.c:395
#9  in quit_target at top.c:1293
#10 in catch_errors at exceptions.c:546
#11 in quit_force at top.c:1331
#12 in quit_command at ./cli/cli-cmds.c:327


> "quit" will offer to detach instead of kill), and that's a bug...)),

It will but I do not see where is a bug:
(gdb) q
A debugging session is active.
	Inferior 1 [process 11571] will be detached.
Quit anyway? (y or n) y


> 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.

I agree this fix is better.  I will revert the previous check-in after this
check-in.


Thanks,
Jan

Re: Include putpkt in TRY_CATCH. PR gdb/15275

[Pedro Alves]

On 03/29/2013 02:02 PM, Jan Kratochvil wrote:
> On Thu, 28 Mar 2013 18:07:10 +0100, Pedro Alves wrote:
>> 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",
> 
> I see it working:

Whoops.  I remembered incorrectly where we're removing breakpoints.

>> 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.
> 
> I agree this fix is better.  I will revert the previous check-in after this
> check-in.

Now pushed.

Thanks,
-- 
Pedro Alves

Re: Include putpkt in TRY_CATCH. PR gdb/15275

[Jan Kratochvil]

On Tue, 02 Apr 2013 16:03:25 +0200, Pedro Alves wrote:
> On 03/29/2013 02:02 PM, Jan Kratochvil wrote:
> > I agree this fix is better.  I will revert the previous check-in after this
> > check-in.
> 
> Now pushed.

Checked in the revert:
	http://sourceware.org/ml/gdb-cvs/2013-04/msg00016.html
and for 7.6:
	http://sourceware.org/ml/gdb-cvs/2013-04/msg00017.html


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2013-04/msg00016.html

--- src/gdb/ChangeLog	2013/04/02 13:51:06	1.15355
+++ src/gdb/ChangeLog	2013/04/02 14:09:08	1.15356
@@ -1,3 +1,9 @@
+2013-04-02  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	Revert this patch:
+	PR gdb/15275
+	* remote.c (remote_get_trace_status): Include putpkt in TRY_CATCH.
+
 2013-04-02  Pedro Alves  <palves@redhat.com>
 
 	PR gdb/15275
--- src/gdb/remote.c	2013/04/02 13:51:07	1.536
+++ src/gdb/remote.c	2013/04/02 14:09:08	1.537
@@ -10713,9 +10713,10 @@
 
   trace_regblock_size = get_remote_arch_state ()->sizeof_g_packet;
 
+  putpkt ("qTStatus");
+
   TRY_CATCH (ex, RETURN_MASK_ERROR)
     {
-      putpkt ("qTStatus");
       p = remote_get_noisy_reply (&target_buf, &target_buf_size);
     }
   if (ex.reason < 0)

[PATCH] Avoid potencially-stale errno usage (was: Re: Include putpkt in TRY_CATCH. PR gdb/15275)

[Pedro Alves]

On 03/28/2013 05:07 PM, Pedro Alves wrote:
> (
> 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,
> )

Fixing this before it ends up forgotten and bites us at some
point.

Comments?

------------
Avoid potencially-stale errno usage.

The current throw_perror_with_name/TARGET_CLOSE_ERROR calls assume
errno is still set to the right error, although remote_unpush_target
is called in between, which may well change errno.

Tested on x86_64 Fedora 17 w/ gdbserver.

gdb/
2013-04-08  Pedro Alves  <palves@redhat.com>

	* remote.c (unpush_and_perror): New function.
	(readchar, remote_serial_write): Use it.

---

 gdb/remote.c |   32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 740324b..e60ff78 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7033,6 +7033,23 @@ remote_files_info (struct target_ops *ignore)
 /* Stuff for dealing with the packets which are part of this protocol.
    See comment at top of file for details.  */
 
+/* Close/unpush the remote target, and throw a TARGET_CLOSE_ERROR
+   error to higher layers.  Called when a serial error is detected.
+   The exception message is STRING, followed by a colon and a blank,
+   then the system error message for errno at function entry.  */
+
+static void
+unpush_and_perror (const char *string)
+{
+  char *errstr;
+
+  errstr = xstrprintf ("%s: %s", string, safe_strerror (errno));
+  make_cleanup (xfree, errstr);
+
+  remote_unpush_target ();
+  throw_error (TARGET_CLOSE_ERROR, "%s", errstr);
+}
+
 /* Read a single character from the remote end.  */
 
 static int
@@ -7048,14 +7065,11 @@ readchar (int timeout)
   switch ((enum serial_rc) ch)
     {
     case SERIAL_EOF:
-      remote_unpush_target ();
-      throw_error (TARGET_CLOSE_ERROR, _("Remote connection closed"));
+      unpush_and_perror (_("Remote connection closed"));
       /* no return */
     case SERIAL_ERROR:
-      remote_unpush_target ();
-      throw_perror_with_name (TARGET_CLOSE_ERROR,
-			      _("Remote communication error.  "
-				"Target disconnected."));
+      unpush_and_perror (_("Remote communication error.  "
+			   "Target disconnected."));
       /* no return */
     case SERIAL_TIMEOUT:
       break;
@@ -7071,10 +7085,8 @@ 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."));
+      unpush_and_perror (_("Remote communication error.  "
+			   "Target disconnected."));
     }
 }
 

Re: [PATCH] Avoid potencially-stale errno usage (was: Re: Include putpkt in TRY_CATCH. PR gdb/15275)

[Jan Kratochvil]

On Mon, 08 Apr 2013 19:57:22 +0200, Pedro Alves wrote:
> @@ -7048,14 +7065,11 @@ readchar (int timeout)
>    switch ((enum serial_rc) ch)
>      {
>      case SERIAL_EOF:
> -      remote_unpush_target ();
> -      throw_error (TARGET_CLOSE_ERROR, _("Remote connection closed"));
> +      unpush_and_perror (_("Remote connection closed"));

I do not see why you have changed this part.  I have checked SERIAL_EOF is
returned in cases where errno is not set (errno is unchanged).


Otherwise it looks OK to me.


Thanks,
Jan

[commit] Re: [PATCH] Avoid potencially-stale errno usage

[Pedro Alves]

On 04/08/2013 07:34 PM, Jan Kratochvil wrote:
> On Mon, 08 Apr 2013 19:57:22 +0200, Pedro Alves wrote:
>> @@ -7048,14 +7065,11 @@ readchar (int timeout)
>>    switch ((enum serial_rc) ch)
>>      {
>>      case SERIAL_EOF:
>> -      remote_unpush_target ();
>> -      throw_error (TARGET_CLOSE_ERROR, _("Remote connection closed"));
>> +      unpush_and_perror (_("Remote connection closed"));
> 
> I do not see why you have changed this part.  I have checked SERIAL_EOF is
> returned in cases where errno is not set (errno is unchanged).

Gah.  Is "patch sent in a hurry; wife and kid waiting in car" a good excuse?  :-P

> Otherwise it looks OK to me.

Thanks.  Committed.

---------------
Avoid potencially-stale errno usage.

The current throw_perror_with_name/TARGET_CLOSE_ERROR calls assume
errno is still set to the right error, although remote_unpush_target
is called in between, which may well change errno.

Tested on x86_64 Fedora 17 w/ gdbserver.

gdb/
2013-04-09  Pedro Alves  <palves@redhat.com>

	* remote.c (unpush_and_perror): New function.
	(readchar, remote_serial_write): Use it.
---
 gdb/remote.c |   29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 740324b..de075c8 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7033,6 +7033,23 @@ remote_files_info (struct target_ops *ignore)
 /* Stuff for dealing with the packets which are part of this protocol.
    See comment at top of file for details.  */
 
+/* Close/unpush the remote target, and throw a TARGET_CLOSE_ERROR
+   error to higher layers.  Called when a serial error is detected.
+   The exception message is STRING, followed by a colon and a blank,
+   then the system error message for errno at function entry.  */
+
+static void
+unpush_and_perror (const char *string)
+{
+  char *errstr;
+
+  errstr = xstrprintf ("%s: %s", string, safe_strerror (errno));
+  make_cleanup (xfree, errstr);
+
+  remote_unpush_target ();
+  throw_error (TARGET_CLOSE_ERROR, "%s", errstr);
+}
+
 /* Read a single character from the remote end.  */
 
 static int
@@ -7052,10 +7069,8 @@ readchar (int timeout)
       throw_error (TARGET_CLOSE_ERROR, _("Remote connection closed"));
       /* no return */
     case SERIAL_ERROR:
-      remote_unpush_target ();
-      throw_perror_with_name (TARGET_CLOSE_ERROR,
-			      _("Remote communication error.  "
-				"Target disconnected."));
+      unpush_and_perror (_("Remote communication error.  "
+			   "Target disconnected."));
       /* no return */
     case SERIAL_TIMEOUT:
       break;
@@ -7071,10 +7086,8 @@ 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."));
+      unpush_and_perror (_("Remote communication error.  "
+			   "Target disconnected."));
     }
 }
 

Re: [PATCH] make -gdb-exit call disconnect_tracing too, and don't lose history if the target errors on "quit"

[Pedro Alves]

I've now committed this.

-- 
Pedro Alves

[patch] Re: [commit] Re: [PATCH] Avoid potencially-stale errno usage

[Jan Kratochvil]

On Tue, 09 Apr 2013 14:19:42 +0200, Pedro Alves wrote:
> +static void
> +unpush_and_perror (const char *string)
> +{
> +  char *errstr;
> +
> +  errstr = xstrprintf ("%s: %s", string, safe_strerror (errno));
> +  make_cleanup (xfree, errstr);
> +
> +  remote_unpush_target ();
> +  throw_error (TARGET_CLOSE_ERROR, "%s", errstr);
> +}

It has changed the output message:

20130409Build-gdbcvs-dwarf41-gcchead-f19/fedora-19-x86_64/out/gdb-m32.log:Remote communication error.  Target disconnected.: Connection reset by peer.
20130410Build-gdbcvs-dwarf41-gcchead-f19/fedora-19-x86_64/out/gdb-m64.log:Remote communication error.  Target disconnected.: Connection reset by peer

Formerly:

throw_perror_with_name (enum errors errcode, const char *string)
{
[...]
  throw_error (errcode, _("%s."), combined);
}

Instead of changing the testsuite expectation in
2a9030220efff2f7e5e7447ee523726bd9585072 I find better to retain the backward
compatibility, although it is sure a nitpick.

Also I have simplified the code a bit.


Thanks,
Jan


gdb/
2013-04-11  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* remote.c (unpush_and_perror): Add output message final dot.

diff --git a/gdb/remote.c b/gdb/remote.c
index de075c8..f0dbba6 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7036,18 +7036,17 @@ remote_files_info (struct target_ops *ignore)
 /* Close/unpush the remote target, and throw a TARGET_CLOSE_ERROR
    error to higher layers.  Called when a serial error is detected.
    The exception message is STRING, followed by a colon and a blank,
-   then the system error message for errno at function entry.  */
+   the system error message for errno at function entry and final dot
+   for output compatibility with throw_perror_with_name.  */
 
 static void
 unpush_and_perror (const char *string)
 {
-  char *errstr;
-
-  errstr = xstrprintf ("%s: %s", string, safe_strerror (errno));
-  make_cleanup (xfree, errstr);
+  int saved_errno = errno;
 
   remote_unpush_target ();
-  throw_error (TARGET_CLOSE_ERROR, "%s", errstr);
+  throw_error (TARGET_CLOSE_ERROR, "%s: %s.", string,
+	       safe_strerror (saved_errno));
 }
 
 /* Read a single character from the remote end.  */

Re: [patch] Re: [commit] Re: [PATCH] Avoid potencially-stale errno usage

[Pedro Alves]

On 04/11/2013 07:38 AM, Jan Kratochvil wrote:

> Instead of changing the testsuite expectation in
> 2a9030220efff2f7e5e7447ee523726bd9585072 I find better to retain the backward
> compatibility, although it is sure a nitpick.

That's fine with me.

> Also I have simplified the code a bit.

Thanks, that's better indeed.

> gdb/
> 2013-04-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* remote.c (unpush_and_perror): Add output message final dot.

Looks great.  Thanks, and sorry for the trouble.

-- 
Pedro Alves

[commit] [patch] Re: [commit] Re: [PATCH] Avoid potencially-stale errno usage

[Jan Kratochvil]

On Thu, 11 Apr 2013 10:35:07 +0200, Pedro Alves wrote:
> On 04/11/2013 07:38 AM, Jan Kratochvil wrote:
> > gdb/
> > 2013-04-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	* remote.c (unpush_and_perror): Add output message final dot.
> 
> Looks great.  Thanks, and sorry for the trouble.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2013-04/msg00108.html


Thanks,
Jan