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

[Pedro Alves <palves at redhat dot com>]

On 03/15/2013 07:53 PM, Jan Kratochvil wrote:

> there was a patch:
> 	[RFA/tracepoint] Make GDB can work with some old GDB server
> 	http://sourceware.org/ml/gdb-patches/2011-06/msg00368.html
> 	http://sourceware.org/ml/gdb-patches/2011-07/msg00231.html
> 	Message-ID: <BANLkTimuz12OVGoF9tKoV2ikcj9LKFax_g@mail.gmail.com>
> 	872b7668686ffae68f1f3397de9d68512679e8e7

Thanks.

A bit of further background:

 http://sourceware.org/bugzilla/show_bug.cgi?id=12146
 http://kerneltrap.org/mailarchive/linux-kernel/2010/7/22/4596723

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) to GDB whenever it got a packet that started with 
either "qf" or "qT" no matter if it was a tracing packet or not.
The RSP-defined way to reply to unknown packets is to send back
an empty reply, which GDB understands as "not supported".

The kgdb bug was fixed back in 2010/07, with:

commit fb82c0ff27b2c40c6f7a3d1a94cafb154591fa80
Author: Jason Wessel <jason.wessel@windriver.com>
Date:   Wed Jul 21 19:27:05 2010 -0500

    repair gdbstub to match the gdbserial protocol specification
    
    The gdbserial protocol handler should return an empty packet instead
    of an error string when ever it responds to a command it does not
    implement.
    
    The problem cases come from a debugger client sending
    qTBuffer, qTStatus, qSearch, qSupported.
    
    The incorrect response from the gdbstub leads the debugger clients to
    not function correctly.  Recent versions of gdb will not detach correctly as a result of this behavior.
    
    Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
    Signed-off-by: Dongdong Deng <dongdong.deng@windriver.com>


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.  The PR
shows an example of a reporter that was happy with that.

I'd have the same opinion of Hui's patch in 2011 -- "fix your
kernel instead".  :-)

Hui said back in 2011:

  "This is a bug of kgdb, but I think make gdb just output a
  warning is not affect anything else.  So I make this patch."

Clearly, it's showing now that it does affect some things.

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.  I checked that also fixes your new test.

> By my change to the Hui's patch I caused a regression if gdbserver dies.
> In such case GDB crashes as the "Remote connection closed" error gets filtered
> out but the callers no longer have valid inferior and at least the proceed
> function then crashes on stale local variable pointer.  The original goal of
> the "Remote connection closed" error was to completely abort the current
> command.
> 
> #0  error (string=0xf5d5e0 "Remote connection closed") at utils.c:716
> #1  in readchar (timeout=2) at remote.c:7051
> #2  in getpkt_or_notif_sane_1 (buf=0x1eb8c60 <target_buf>, sizeof_buf=0x1eb8c68 <target_buf_size>, forever=0, expecting_notif=0, is_notif=0x0) at remote.c:7566
> #3  in getpkt_sane (buf=0x1eb8c60 <target_buf>, sizeof_buf=0x1eb8c68 <target_buf_size>, forever=0) at remote.c:7664
> #4  in getpkt (buf=0x1eb8c60 <target_buf>, sizeof_buf=0x1eb8c68 <target_buf_size>, forever=0) at remote.c:7506
> #5  in remote_get_noisy_reply (buf_p=0x1eb8c60 <target_buf>, sizeof_buf=0x1eb8c68 <target_buf_size>) at remote.c:450
> #6  in remote_get_trace_status (ts=0x1eba8a0 <trace_status>) at remote.c:10698
>         10696     TRY_CATCH (ex, RETURN_MASK_ERROR)
>         10698         p = remote_get_noisy_reply (&target_buf, &target_buf_size);
> #7  in remote_can_download_tracepoint () at remote.c:10553
> #8  in download_tracepoint_locations () at breakpoint.c:12174
> #9  in update_global_location_list (should_insert=1) at breakpoint.c:12661
> #10 in insert_breakpoints () at breakpoint.c:2732
> #11 in proceed (addr=18446744073709551615, siggnal=GDB_SIGNAL_DEFAULT, step=1) at infrun.c:2243
> #12 in step_once (skip_subroutines=0, single_inst=1, count=1, thread=-1) at infcmd.c:1092
> #13 in step_1 (skip_subroutines=0, single_inst=1, count_string=0x0) at infcmd.c:927
> #14 in stepi_command (count_string=0x0, from_tty=1) at infcmd.c:863
> 
> Therefore I limited the TRY_CATCH to 'E2...' errors which are reported as
> "trace API error 0x%s." so it should hopefully fix the kgdb compatibility
> problem without causing anything else.

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.

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

> +load_lib gdbserver-support.exp
> +
> +standard_testfile
> +
> +if {[skip_gdbserver_tests]} {
> +    return 0
> +}
> +
> +if ![is_remote target] {
> +    return 0
> +}
> +
> +if { [target_info exists sockethost]
> +     && [target_info sockethost] != "localhost:"
> +     && [target_info sockethost] != "stdio" } {
> +    # "remote_exec target" unsuccessfully tries "rsh" even with localhost.
> +    # Therefore use "remote_exec host" instead limiting this testcase for
> +    # testing on localhost only.
> +    return 0

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'm testing with
native-gdbserver.exp board in the 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.
See patch below.

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

	* remote.c (remote_get_trace_status): Don't wrap
	remote_get_noisy_reply in TRY_CATCH.

gdb/testsuite/
2013-03-15  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.
---

 gdb/testsuite/gdb.server/server-kill.c   |   24 +++++++++++++++++
 gdb/testsuite/gdb.server/server-kill.exp |   43 ++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)
 create mode 100644 gdb/testsuite/gdb.server/server-kill.c
 create mode 100644 gdb/testsuite/gdb.server/server-kill.exp

diff --git a/gdb/remote.c b/gdb/remote.c
index 21d86f7..c682602 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10696,16 +10696,7 @@ 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)
-    {
-      p = remote_get_noisy_reply (&target_buf, &target_buf_size);
-    }
-  if (ex.reason < 0)
-    {
-      exception_fprintf (gdb_stderr, ex, "qTStatus: ");
-      return -1;
-    }
+  p = remote_get_noisy_reply (&target_buf, &target_buf_size);
 
   /* If the remote target doesn't do tracing, flag it.  */
   if (*p == '\0')
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"