[PATCH 2/2] btrace: allow recording to be started for running threads

[Luis Machado]

On 12/06/2016 09:54 AM, Markus Metzger wrote:
> When recording is started for a running thread, GDB was able to start tracing
> but then failed to read registers to insert the initial entry for the current
> PC.  We don't really need that initial entry if we don't know where exactly we
> started recording.  Silently ignore such errors to allow recording to be started
> while threads are running.
>
> For the BTRACE_FORMAT_PT btrace format, we don't need that initial entry since
> it will be recorded in the trace.  We can omit the call to btrace_add_pc.
>
> 2016-12-06  Markus Metzger  <markus.t.metzger@intel.com>
>
> gdb/
> 	* btrace.c (btrace_enable): Do not call btrace_add_pc for
> 	BTRACE_FORMAT_PT.  Silently ignore errors from btrace_add_pc.
>
> testsuite/
> 	* gdb.btrace/enable-running.c: New.
> 	* gdb.btrace/enable-running.exp: New.
> ---
>  gdb/btrace.c                                | 24 +++++++++-
>  gdb/testsuite/gdb.btrace/enable-running.c   | 47 +++++++++++++++++++
>  gdb/testsuite/gdb.btrace/enable-running.exp | 72 +++++++++++++++++++++++++++++
>  3 files changed, 141 insertions(+), 2 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.btrace/enable-running.c
>  create mode 100644 gdb/testsuite/gdb.btrace/enable-running.exp
>
> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index 39d537c..920b9ab 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -1474,8 +1474,28 @@ btrace_enable (struct thread_info *tp, const struct btrace_config *conf)
>
>    /* Add an entry for the current PC so we start tracing from where we
>       enabled it.  */
> -  if (tp->btrace.target != NULL)
> -    btrace_add_pc (tp);
> +  TRY
> +    {
> +      /* This is not relevant for BTRACE_FORMAT_PT since the trace will already
> +	 start at the enable location.  */

enabled location?


> +      if ((tp->btrace.target != NULL) && (conf->format != BTRACE_FORMAT_PT))
> +	btrace_add_pc (tp);
> +    }
> +  CATCH (exception, RETURN_MASK_ALL)
> +    {
> +      /* We may fail to add the initial entry, for example if TP is currently
> +	 running.
> +
> +	 We won't be able to stitch the initial trace chunk to this initial
> +	 entry so tracing will start at the next branch target instead of at the
> +	 current PC.  Since TP is currently running, this shouldn't make a
> +	 difference.
> +
> +	 If TP were waiting most of the time and made only a little bit of
> +	 progress before it was stopped, we'd lose the instructions until the
> +	 first branch.  */
> +    }
> +  END_CATCH
>  }
>
>  /* See btrace.h.  */
> diff --git a/gdb/testsuite/gdb.btrace/enable-running.c b/gdb/testsuite/gdb.btrace/enable-running.c
> new file mode 100644
> index 0000000..6380c29
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/enable-running.c
> @@ -0,0 +1,47 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2016 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/>.  */
> +
> +#include <pthread.h>
> +
> +static int global;
> +
> +static void *
> +test (void *arg)
> +{
> +  for (;;)
> +    global += 1;
> +
> +  return arg;
> +}
> +
> +int
> +main (void)
> +{
> +  int i;
> +
> +  for (i = 0; i < 3; ++i)
> +    {
> +      pthread_t th;
> +
> +      pthread_create (&th, NULL, test, NULL);
> +      pthread_detach (th);
> +    }
> +
> +  test (NULL); /* bp.1 */
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.btrace/enable-running.exp b/gdb/testsuite/gdb.btrace/enable-running.exp
> new file mode 100644
> index 0000000..ead86af
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/enable-running.exp
> @@ -0,0 +1,72 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2016 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/>.
> +
> +if { [skip_btrace_tests] } { return -1 }

unsupported "btrace not supported on this target"?

> +
> +standard_testfile
> +if {[gdb_compile_pthreads "$srcdir/$subdir/$srcfile" "$binfile" executable {debug}] != "" } {

untested "failed to compile"

> +    return -1
> +}
> +clean_restart $testfile
> +
> +# We need to enable non-stop mode for the remote case.
> +gdb_test_no_output "set non-stop on"
> +
> +if ![runto_main] {

untested "couldn't run to main"

> +    return -1
> +}
> +
> +set bp_1 [gdb_get_line_number "bp.1" $srcfile]
> +
> +gdb_breakpoint $bp_1
> +gdb_continue_to_breakpoint "cont to $bp_1" ".*$bp_1\r\n.*"
> +gdb_test "cont&" "Continuing\."
> +
> +# All threads are running.  Let's start recording.
> +gdb_test_no_output "record btrace"
> +
> +proc check_tracing_enabled { thread } {
> +    global gdb_prompt
> +
> +    gdb_test "thread $thread" "(running).*"

Maybe add a more meaningful test name here? "Check if thread $thread is 
running"?

> +    with_test_prefix "thread $thread" {
> +        # Stop the thread before reading the trace.
> +        gdb_test_multiple "interrupt" "interrupt" {
> +            -re "interrupt\r\n$gdb_prompt " {
> +                pass "interrupt"
> +            }

Any reason why we couldn't use gdb_test here? Also, we a more meaningful 
test name and make the name unique (by mentioning the thread number, 
possibly)

> +        }
> +        # Wait until the thread actually stopped.
> +        gdb_test_multiple "" "stopped" {
> +            -re "Thread $thread.*stopped\." {
> +                pass "stopped"
> +            }
> +        }

Same here. Couldn't we use gdb_test? Also a more meaningful and unique 
test name.

> +        # We will consume the thread's current location as part of the
> +        # "info record" output.
> +        gdb_test "info record" [multi_line \
> +            "Active record target: record-btrace" \
> +            "Recording format: .*" \
> +            "Recorded \[0-9\]+ instructions \[^\\\r\\\n\]*" \
> +        ]
> +    }
> +}
> +
> +# Check that recording was started on each thread.
> +foreach thread {1 2 3 4} {
> +    check_tracing_enabled $thread
> +}
>