From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8243 invoked by alias); 22 Mar 2013 12:40:15 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 7745 invoked by uid 89); 22 Mar 2013 12:39:21 -0000 X-Spam-SWARE-Status: No, score=-8.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 22 Mar 2013 12:39:17 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r2MCdEK4015263 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 22 Mar 2013 08:39:14 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r2MCdCek010245; Fri, 22 Mar 2013 08:39:12 -0400 Message-ID: <514C50EF.6030202@redhat.com> Date: Fri, 22 Mar 2013 13:04:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: Jan Kratochvil CC: gdb-patches@sourceware.org, Hui Zhu Subject: Re: [patch+7.6] Fix 7.5 regression crashing GDB if gdbserver dies References: <20130315195359.GA19841@host2.jankratochvil.net> In-Reply-To: <20130315195359.GA19841@host2.jankratochvil.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-03/txt/msg00836.txt.bz2 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: > 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 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 Signed-off-by: Dongdong Deng 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 , sizeof_buf=0x1eb8c68 , forever=0, expecting_notif=0, is_notif=0x0) at remote.c:7566 > #3 in getpkt_sane (buf=0x1eb8c60 , sizeof_buf=0x1eb8c68 , forever=0) at remote.c:7664 > #4 in getpkt (buf=0x1eb8c60 , sizeof_buf=0x1eb8c68 , forever=0) at remote.c:7506 > #5 in remote_get_noisy_reply (buf_p=0x1eb8c60 , sizeof_buf=0x1eb8c68 ) at remote.c:450 > #6 in remote_get_trace_status (ts=0x1eba8a0 ) 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 * remote.c (remote_get_trace_status): Don't wrap remote_get_noisy_reply in TRY_CATCH. gdb/testsuite/ 2013-03-15 Jan Kratochvil Pedro Alves * 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 . */ + +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 . + +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"