Re: [patch+7.6] Fix 7.5 regression crashing GDB if gdbserver dies

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Fri, 22 Mar 2013 13:39:11 +0100, Pedro Alves wrote:
> The 22 in "E22" is not really a "trace api error" (whatever that
> meant for the original non-public tracepoint target in GDB years and
> years ago; the errors handled in trace_error are not really specified
> in the RSP docs).  It just happened that kgdb leaked
> EINVAL (=22)

In such case the patch should be limited specifically for E22, done.


> I had closed PR 12146 as invalid back in 2010, because people that
> use kgdb are people hacking on the kernel, so if they stumble upon
> this kgdb bug, they should pull/merge the relevant kernel fix into
> their trees instead of gdb working around the kgdb bug.
[...]
> A couple of years more have passed, which makes the old kgdbs even
> less relevant, so IMO, we should just go ahead and revert the
> initial workaround.

Red Hat is actively supporting RHEL-5 linux-2.6.18 from 2006.  It does not yet
had kgdb but it illustrates other production systems with kernels from
2008..2010 may use kgdb.  On production system one cannot feasibly change the
kernel just because of a bugfix for debugging purposes.

Therefore your patch will be a new regression against FSF GDB 7.5.

People still face for example the bug
	new gdbserver is incompat. with old gdb - ;core: suffix
	http://sourceware.org/bugzilla/show_bug.cgi?id=15230
which is "fixed" in GDB (also) since 2010:
	commit 3d972c80ab566c07f5e55d356821fb883c9ade88
	Date:   Tue Jan 12 21:40:23 2010 +0000

As I said I leave gdbserver up to you but I do not much understand why to
break compatibility when the fix is simple and done.


> If we went this path, I believe it would be the "connection
> closed" that should get special treatment as being a hard
> error that usually you'd want to propagate to some higher
> level and react by canceling all ongoing commands and cleaning
> up.  I've occasionally thought of adding it on other occasions
> before.

OK, done.


> We should just eliminate trace_error -- it's magical error
> handling is meaningless today.

Done.


> Hmm.  Moved my ssh and rsh binaries from the path (well, renamed
> them) and did s/host/target/ below, and the test still worked.
> I wonder what's different in our environments?

I was using old (and apparently incomplete) native-gdbserver.exp from the
times it was still not a part of the FSF GDB tree.


> While at it, I tweaked the test to make it also work with the
> extended-remote-gdbserver.exp board like other test files handle it.

Thanks.


Jan


gdb/
2013-03-15  Jan Kratochvil  <jan.kratochvil@redhat.com>

        * exceptions.h (enum errors): New entry TARGET_CLOSE_ERROR.
        * remote.c (trace_error): Remove the special handling of '2'.
	(readchar) <SERIAL_EOF>
	(readchar) <SERIAL_ERROR>
	(getpkt_or_notif_sane_1): Use TARGET_CLOSE_ERROR for them.
	(remote_get_trace_status): Call throw_exception if EX is
	TARGET_CLOSE_ERROR.
	* utils.c (perror_with_name): Rename to ...
	(throw_perror_with_name): ... here.  New parameter errcode, describe it
	in the function comment.
	(perror_with_name): New function wrapper.
	* utils.h (enum errors): New stub declaration.
	(throw_perror_with_name): New declaration.

gdb/testsuite/
2013-03-22  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Pedro Alves  <palves@redhat.com>

        * gdb.server/server-kill.c: New file.
        * gdb.server/server-kill.exp: New file.

diff --git a/gdb/exceptions.h b/gdb/exceptions.h
index 0d67719..630eb2e 100644
--- a/gdb/exceptions.h
+++ b/gdb/exceptions.h
@@ -86,6 +86,10 @@ enum errors {
   /* DW_OP_GNU_entry_value resolving failed.  */
   NO_ENTRY_VALUE_ERROR,
 
+  /* Target throwing an error has been closed.  Current command should be
+     aborted as the inferior state is no longer valid.  */
+  TARGET_CLOSE_ERROR,
+
   /* Add more errors here.  */
   NR_ERRORS
 };
diff --git a/gdb/remote.c b/gdb/remote.c
index 21d86f7..c5bdc5d 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -430,8 +430,6 @@ trace_error (char *buf)
       else
 	error (_("remote.c: error in outgoing packet at field #%ld."),
 	       strtol (buf, NULL, 16));
-    case '2':
-      error (_("trace API error 0x%s."), ++buf);
     default:
       error (_("Target returns error code '%s'."), buf);
     }
@@ -7052,12 +7050,13 @@ readchar (int timeout)
     {
     case SERIAL_EOF:
       pop_target ();
-      error (_("Remote connection closed"));
+      throw_error (TARGET_CLOSE_ERROR, _("Remote connection closed"));
       /* no return */
     case SERIAL_ERROR:
       pop_target ();
-      perror_with_name (_("Remote communication error.  "
-			  "Target disconnected."));
+      throw_perror_with_name (TARGET_CLOSE_ERROR,
+			      _("Remote communication error.  "
+				"Target disconnected."));
       /* no return */
     case SERIAL_TIMEOUT:
       break;
@@ -7580,7 +7579,9 @@ getpkt_or_notif_sane_1 (char **buf, long *sizeof_buf, int forever,
 		{
 		  QUIT;
 		  pop_target ();
-		  error (_("Watchdog timeout has expired.  Target detached."));
+		  throw_error (TARGET_CLOSE_ERROR,
+			       _("Watchdog timeout has expired.  "
+				 "Target detached."));
 		}
 	      if (remote_debug)
 		fputs_filtered ("Timed out.\n", gdb_stdlog);
@@ -10703,8 +10704,12 @@ remote_get_trace_status (struct trace_status *ts)
     }
   if (ex.reason < 0)
     {
-      exception_fprintf (gdb_stderr, ex, "qTStatus: ");
-      return -1;
+      if (ex.error != TARGET_CLOSE_ERROR)
+	{
+	  exception_fprintf (gdb_stderr, ex, "qTStatus: ");
+	  return -1;
+	}
+      throw_exception (ex);
     }
 
   /* If the remote target doesn't do tracing, flag it.  */
diff --git a/gdb/utils.c b/gdb/utils.c
index 4c2f08c..1fdc877 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1077,11 +1077,11 @@ add_internal_problem_command (struct internal_problem *problem)
 }
 
 /* Print the system error message for errno, and also mention STRING
-   as the file name for which the error was encountered.
-   Then return to command level.  */
+   as the file name for which the error was encountered.  Use ERRCODE
+   for the thrown exception.  Then return to command level.  */
 
 void
-perror_with_name (const char *string)
+throw_perror_with_name (enum errors errcode, const char *string)
 {
   char *err;
   char *combined;
@@ -1098,7 +1098,15 @@ perror_with_name (const char *string)
   bfd_set_error (bfd_error_no_error);
   errno = 0;
 
-  error (_("%s."), combined);
+  throw_error (errcode, _("%s."), combined);
+}
+
+/* See throw_perror_with_name, ERRCODE defaults here to GENERIC_ERROR.  */
+
+void
+perror_with_name (const char *string)
+{
+  throw_perror_with_name (GENERIC_ERROR, string);
 }
 
 /* Print the system error message for ERRCODE, and also mention STRING
diff --git a/gdb/utils.h b/gdb/utils.h
index 52bcaff..d6d49e6 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -279,6 +279,9 @@ extern char *hex_string_custom (LONGEST, int);
 extern void fprintf_symbol_filtered (struct ui_file *, const char *,
 				     enum language, int);
 
+enum errors;
+extern void throw_perror_with_name (enum errors errcode, const char *string)
+  ATTRIBUTE_NORETURN;
 extern void perror_with_name (const char *) ATTRIBUTE_NORETURN;
 
 extern void print_sys_errmsg (const char *, int);
diff --git a/gdb/testsuite/gdb.server/server-kill.c b/gdb/testsuite/gdb.server/server-kill.c
new file mode 100644
index 0000000..1949d64
--- /dev/null
+++ b/gdb/testsuite/gdb.server/server-kill.c
@@ -0,0 +1,24 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main (void)
+{
+  int i = 0;
+
+  return i;
+}
diff --git a/gdb/testsuite/gdb.server/server-kill.exp b/gdb/testsuite/gdb.server/server-kill.exp
new file mode 100644
index 0000000..45a2a89
--- /dev/null
+++ b/gdb/testsuite/gdb.server/server-kill.exp
@@ -0,0 +1,43 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2013 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib gdbserver-support.exp
+
+standard_testfile
+
+if {[skip_gdbserver_tests]} {
+    return 0
+}
+
+if { [prepare_for_testing ${testfile}.exp ${testfile}] } {
+    return -1
+}
+
+# Make sure we're disconnected, in case we're testing with an
+# extended-remote board, therefore already connected.
+gdb_test "disconnect" ".*"
+
+gdbserver_run ""
+
+# Otherwise the breakpoint at 'main' would not cause insert
+# breakpoints during first step.
+delete_breakpoints
+
+set server_pid [exp_pid -i [board_info target fileid]]
+remote_exec target "kill -9 $server_pid"
+
+gdb_test "step" "Remote connection closed"