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

Jan Kratochvil
2013-03-22T20:43:05Z

On Fri, 22 Mar 2013 20:53:56 +0100, Pedro Alves wrote:
> Debugging a live production system with kgdb? I don't buy
> that, really. If one cares to debug the kernel with kgdb, then
> one can just as easily patch the kernel, just like one patches the
> kernel for security bugs and the like.

This is becoming offtopic but the production systems I face are isolated from
Internet and no security patches get ever applied. One of the many reasons is
to keep all the systems the same, with interchangeable hard drives.

> > 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
> >
>
> I've said before that I regret not having noticed that problem
> at patch review time or before the release. I still do. I don't
> see the parallel here though. We can't go back in time and fix older
> GDBs to cope with ;core... Users can upgrade their GDBs though.

In practice in such cases people rather stay with older application, for
whatever reason.

> Because it's a workaround that does not need to exist. It adds
> maintenance burden in the wrong place. The time we've wasted
> collectively iterating on this shows it.

Otherwise people would need to carry the workarounds downstream. So far
I think upstream GDB is supporting workarounds of broken system components.

> I like this version much better than the original. If the exception
> swallowing ever turns out problematic again, I'll propose reverting
> the original patch again. So in interest of moving forward, this one's
> fine with me.

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

Thanks,
jan

http://sourceware.org/ml/gdb-cvs/2013-03/msg00199.html

--- src/gdb/ChangeLog 2013/03/22 20:25:40 1.15305
+++ src/gdb/ChangeLog 2013/03/22 20:39:28 1.15306
@@ -1,3 +1,19 @@
+2013-03-22 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.
+
2013-03-22 Pedro Alves <palves@redhat.com>
Yao Qi <yao@codesourcery.com>
Mark Kettenis <kettenis@gnu.org>
--- src/gdb/testsuite/ChangeLog 2013/03/21 19:18:25 1.3594
+++ src/gdb/testsuite/ChangeLog 2013/03/22 20:39:28 1.3595
@@ -1,3 +1,9 @@
+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.
+
2013-03-21 Pedro Alves <palves@redhat.com>

* gdb.trace/trace-buffer-size.exp (get default buffer size):
--- src/gdb/exceptions.h 2013/01/01 06:32:42 1.38
+++ src/gdb/exceptions.h 2013/03/22 20:39:28 1.39
@@ -86,6 +86,10 @@
/* 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
};
--- src/gdb/remote.c 2013/03/22 19:07:03 1.531
+++ src/gdb/remote.c 2013/03/22 20:39:28 1.532
@@ -430,8 +430,6 @@
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);
}
@@ -7048,12 +7046,13 @@
{
case SERIAL_EOF:
remote_unpush_target ();
- error (_("Remote connection closed"));
+ throw_error (TARGET_CLOSE_ERROR, _("Remote connection closed"));
/* no return */
case SERIAL_ERROR:
remote_unpush_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;
@@ -7576,7 +7575,9 @@
{
QUIT;
remote_unpush_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);
@@ -10699,8 +10700,12 @@
}
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. */
--- src/gdb/utils.c 2013/03/21 17:37:29 1.295
+++ src/gdb/utils.c 2013/03/22 20:39:28 1.296
@@ -966,11 +966,11 @@
}

/* 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;
@@ -987,7 +987,15 @@
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
--- src/gdb/utils.h 2013/03/21 17:37:29 1.6
+++ src/gdb/utils.h 2013/03/22 20:39:28 1.7
@@ -278,6 +278,9 @@
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);
--- src/gdb/testsuite/gdb.server/server-kill.c
+++ src/gdb/testsuite/gdb.server/server-kill.c 2013-03-22 20:42:07.933259467 +0000
@@ -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;
+}
--- src/gdb/testsuite/gdb.server/server-kill.exp
+++ src/gdb/testsuite/gdb.server/server-kill.exp 2013-03-22 20:42:08.423295331 +0000
@@ -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"