From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8294 invoked by alias); 6 Feb 2015 15:50:33 -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 8267 invoked by uid 89); 6 Feb 2015 15:50:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 06 Feb 2015 15:50:31 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t16FoQVa028413 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 6 Feb 2015 10:50:26 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t16FoO9R009953; Fri, 6 Feb 2015 10:50:25 -0500 Message-ID: <54D4E2C0.8070407@redhat.com> Date: Fri, 06 Feb 2015 15:50:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Antoine Tremblay , gdb-patches@sourceware.org Subject: Re: [PATCH v2] gdbserver: Fix crash when QTinit is handled with no inferior process attached References: <54D3A9FB.2050305@redhat.com> <1423166768-24921-1-git-send-email-antoine.tremblay@ericsson.com> In-Reply-To: <1423166768-24921-1-git-send-email-antoine.tremblay@ericsson.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-02/txt/msg00155.txt.bz2 On 02/05/2015 08:06 PM, Antoine Tremblay wrote: > When gdbserver is called with --multi and attach has not been called yet > and tstart is called on the gdb client, gdbserver would crash. > This patch fixes gdbserver so that it returns E01 to the gdb client. > > Also this patch adds a testcase to verify this bug named no-attach-trace.exp Thanks! Pretty good. Just a few minor nits. Just the usual formatting details that are easy to miss the first time around. > gdb/ChangeLog: > PR gdb/15956 > * gdb/gdbserver/tracepoint.c (cmd_qtinit): Added check for > current_thread. gdbserver has it's own ChangeLog file, gdb/gdbserver/ChangeLog. Then adjust the entry to read "* gdbserver/tracepoint.c", as entries are relative to the ChangeLog file's path. Also, s/Added/Add/. > > gdb/testsuite/ChangeLog: > * gdb.trace/no-attach-trace.c: New file. > * gdb.trace/no-attach-trace.exp: New file. > --- > gdb/gdbserver/tracepoint.c | 7 +++++ > gdb/testsuite/gdb.trace/Makefile.in | 4 +-- > gdb/testsuite/gdb.trace/no-attach-trace.c | 25 ++++++++++++++++ > gdb/testsuite/gdb.trace/no-attach-trace.exp | 43 +++++++++++++++++++++++++++ > 4 files changed, 77 insertions(+), 2 deletions(-) > create mode 100644 gdb/testsuite/gdb.trace/no-attach-trace.c > create mode 100644 gdb/testsuite/gdb.trace/no-attach-trace.exp > > diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c > index 82d6ce5..0518530 100644 > --- a/gdb/gdbserver/tracepoint.c > +++ b/gdb/gdbserver/tracepoint.c > @@ -2377,6 +2377,13 @@ cmd_qtinit (char *packet) > { > struct trace_state_variable *tsv, *prev, *next; > > + /* Can't do this command without a pid attached */ Period and double-space at end of all sentences: /* Can't do this command without a pid attached. */ > + if (current_thread == NULL) > + { > + write_enn(packet); Space before parens: write_enn (packet); > + return; > + } > + > /* Make sure we don't try to read from a trace frame. */ > current_traceframe = -1; > > diff --git a/gdb/testsuite/gdb.trace/Makefile.in b/gdb/testsuite/gdb.trace/Makefile.in > index 2e23223..514beab 100644 > --- a/gdb/testsuite/gdb.trace/Makefile.in > +++ b/gdb/testsuite/gdb.trace/Makefile.in > @@ -4,8 +4,8 @@ srcdir = @srcdir@ > .PHONY: all clean mostlyclean distclean realclean > > PROGS = actions-changed ax backtrace deltrace disconnected-tracing \ > - infotrace packetlen passc-dyn passcount report save-trace tfile \ > - tfind tracecmd tsv unavailable while-dyn while-stepping > + infotrace no-attach-trace packetlen passc-dyn passcount report \ > + save-trace tfile tfind tracecmd tsv unavailable while-dyn while-stepping > > all info install-info dvi install uninstall installcheck check: > @echo "Nothing to be done for $@..." > diff --git a/gdb/testsuite/gdb.trace/no-attach-trace.c b/gdb/testsuite/gdb.trace/no-attach-trace.c > new file mode 100644 > index 0000000..383e6d0 > --- /dev/null > +++ b/gdb/testsuite/gdb.trace/no-attach-trace.c > @@ -0,0 +1,25 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2015 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 . */ > + > +/* This program is intended to be a simple dummy program for gdb to read */ Period, double-space. > + > +#include > + > +int main () Break before function name, and "(void)": int main (void) > +{ > + return 0; > +} > diff --git a/gdb/testsuite/gdb.trace/no-attach-trace.exp b/gdb/testsuite/gdb.trace/no-attach-trace.exp > new file mode 100644 > index 0000000..2470347 > --- /dev/null > +++ b/gdb/testsuite/gdb.trace/no-attach-trace.exp > @@ -0,0 +1,43 @@ > +# This testcase is part of GDB, the GNU debugger. > + > +# Copyright 2015 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 . > + > +# Test that trying to trace without a program attached fails properly. > + > +load_lib trace-support.exp > + > +standard_testfile > + > +if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} { > + return -1 > +} > + > +if ![runto_main] { > + fail "Can't run to main to check for trace support" > + return -1 > +} > + > +if { ![gdb_target_supports_trace] } then { > + unsupported "Current target does not support trace" > + return 1 > + > +} > + > +#clean test state so that we have no process attached Space after #, Uppercase, period at end: # Clean test state so that we have no process attached. > +clean_restart $testfile > + > +gdb_test "trace main" "Tracepoint.*" Let's expand this a bit more, like in other tests: gdb_test "trace main" \ "Tracepoint \[0-9\] at.* file .*$srcfile, line.*" \ "set tracepoint on main" Basically, reasonably avoid false positives. > +gdb_test "tstart" "Target returns error code '01'\." Targets can send other error codes, so I think it'd be prudent to .* the '01' part, like: gdb_test "tstart" "Target returns error code.*\." Can you also after this run to main? That'd make sure that the target didn't crash or get wedged. If you decide to use runto_main, wrap it in with_test_prefix "after tstart" { ... } to make its potential FAIL test message distinguishable from the runto_main at the top. Thanks, Pedro Alves