Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [patch+7.6] Fix 7.5 regression crashing GDB if gdbserver dies
@ 2013-03-15 19:55 Jan Kratochvil
  2013-03-22 13:04 ` Pedro Alves
  0 siblings, 1 reply; 15+ messages in thread
From: Jan Kratochvil @ 2013-03-15 19:55 UTC (permalink / raw)
  To: gdb-patches; +Cc: Hui Zhu

Hi,

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

I did not try to find that kgdb but I made a reproducer with:

#--- a/gdb/gdbserver/tracepoint.c
#+++ b/gdb/gdbserver/tracepoint.c
#@@ -4203,7 +4203,8 @@ handle_tracepoint_query (char *packet)
# {
#   if (strcmp ("qTStatus", packet) == 0)
#     {
#-      cmd_qtstatus (packet);
#+      strcpy (packet, "E22");
#+      if (0) cmd_qtstatus (packet);
#       return 1;
#     }
#   else if (strncmp ("qTP:", packet, strlen ("qTP:")) == 0)

which does not work with Hui's original patch so probably kgdb behaves better
than this reproducer.  This I believe if GDB survives this reproducer such GDB
should survive even kgdb.

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.

No regressions on {x86_64,x86_64-m32,i686}-fedora19pre-linux-gnu and with
gdbserver.


Thanks,
Jan


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

	* exceptions.h (enum errors): New entry TRACE_ERROR.
	* remote.c (trace_error) <2>: Use throw_error for TRACE_ERROR.
	(remote_get_trace_status): Call throw_exception if EX is not
	TRACE_ERROR.

gdb/testsuite/
2013-03-15  Jan Kratochvil  <jan.kratochvil@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..b11af87 100644
--- a/gdb/exceptions.h
+++ b/gdb/exceptions.h
@@ -86,6 +86,9 @@ enum errors {
   /* DW_OP_GNU_entry_value resolving failed.  */
   NO_ENTRY_VALUE_ERROR,
 
+  /* remote.c got error from remote end on trace packet.  */
+  TRACE_ERROR,
+
   /* Add more errors here.  */
   NR_ERRORS
 };
diff --git a/gdb/remote.c b/gdb/remote.c
index 21d86f7..207180d 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -431,7 +431,7 @@ trace_error (char *buf)
 	error (_("remote.c: error in outgoing packet at field #%ld."),
 	       strtol (buf, NULL, 16));
     case '2':
-      error (_("trace API error 0x%s."), ++buf);
+      throw_error (TRACE_ERROR, _("trace API error 0x%s."), ++buf);
     default:
       error (_("Target returns error code '%s'."), buf);
     }
@@ -10703,8 +10699,12 @@ remote_get_trace_status (struct trace_status *ts)
     }
   if (ex.reason < 0)
     {
-      exception_fprintf (gdb_stderr, ex, "qTStatus: ");
-      return -1;
+      if (ex.error == TRACE_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/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..3bf5207
--- /dev/null
+++ b/gdb/testsuite/gdb.server/server-kill.exp
@@ -0,0 +1,55 @@
+# 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 ![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
+}
+
+if { [prepare_for_testing ${testfile}.exp ${testfile}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+# Otherwise the breakpoint at 'main' would not cause insert breakpoints during
+# first step.
+delete_breakpoints
+
+# It should be "remote_exec target" but it does not work, see above.
+set server_pid [exp_pid -i [board_info target fileid]]
+remote_exec host "kill -9 $server_pid"
+
+gdb_test "step" "Remote connection closed"


^ permalink raw reply	[flat|nested] 15+ messages in thread
end of thread, other threads:[~2013-04-09 15:28 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-15 19:55 [patch+7.6] Fix 7.5 regression crashing GDB if gdbserver dies Jan Kratochvil
2013-03-22 13:04 ` Pedro Alves
2013-03-22 20:29   ` Jan Kratochvil
2013-03-22 20:45     ` Pedro Alves
2013-03-22 20:55       ` Tom Tromey
2013-03-23 10:51         ` Pedro Alves
2013-03-23 16:25           ` Tom Tromey
2013-03-23 18:31       ` [commit+7.6] " Jan Kratochvil
2013-03-23 22:49         ` Pedro Alves
2013-04-02 13:41         ` Yao Qi
2013-04-02 15:20           ` Jan Kratochvil
2013-04-04 11:09             ` Yao Qi
2013-04-08  7:17     ` [testsuite patch] race: server-kill.exp: Connection reset by peer [Re: [patch+7.6] Fix 7.5 regression crashing GDB if gdbserver dies] Jan Kratochvil
2013-04-09 15:44       ` Pedro Alves
2013-04-09 16:00         ` [commit+7.6] " Jan Kratochvil

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox