Re: [PATCH v2] gdbserver: Fix crash when QTinit is handled with no inferior process attached

[Pedro Alves <palves at redhat dot com>]

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 <http://www.gnu.org/licenses/>.  */
> +
> +/* This program is intended to be a simple dummy program for gdb to read */

Period, double-space.

> +
> +#include <unistd.h>
> +
> +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 <http://www.gnu.org/licenses/>.
> +
> +# 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