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

[Pedro Alves <palves at redhat dot com>]

On 01/20/2017 02:38 PM, 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.  Skip that step to allow recording to be started while
> threads are running.
> 
> If we do run into errors, we need to undo the tracing enable to not leak this
> thread.  The operation did not complete so our caller won't clean up this
> thread.
> 
> 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.
> 
> CC:  Simon Marchi  <simon.marchi@ericsson.com>
> 
> 2017-01-20  Markus Metzger  <markus.t.metzger@intel.com>
> 
> gdb/
> 	* btrace.c (btrace_enable): Do not call btrace_add_pc for
> 	BTRACE_FORMAT_PT or if can_access_registers_ptid returns false.
> 	(validate_registers_access_ptid): New.
> 	(btrace_add_pc): Call validate_registers_access_ptid.
> 
> testsuite/
> 	* gdb.btrace/enable-running.c: New.
> 	* gdb.btrace/enable-running.exp: New.
> ---
>  gdb/btrace.c                                | 44 ++++++++++++++++--
>  gdb/testsuite/gdb.btrace/enable-running.c   | 47 +++++++++++++++++++
>  gdb/testsuite/gdb.btrace/enable-running.exp | 72 +++++++++++++++++++++++++++++
>  3 files changed, 159 insertions(+), 4 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 d266af7..4380e6d 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -1422,6 +1422,18 @@ btrace_compute_ftrace (struct thread_info *tp, struct btrace_data *btrace)
>    do_cleanups (old_chain);
>  }
>  
> +/* Validate that we can read PTID's registers.  */
> +
> +static void
> +validate_registers_access_ptid (ptid_t ptid)
> +{
> +  struct cleanup *cleanup = save_inferior_ptid ();
> +
> +  inferior_ptid = ptid;
> +  validate_registers_access ();
> +  do_cleanups (cleanup);
> +}

Do we need this, we we have the new function added
by patch #1 ?

> +
>  /* Add an entry for the current PC.  */
>  
>  static void
> @@ -1433,6 +1445,9 @@ btrace_add_pc (struct thread_info *tp)
>    struct cleanup *cleanup;
>    CORE_ADDR pc;
>  
> +  /* Make sure we can read TP's registers.  */
> +  validate_registers_access_ptid (tp->ptid);
> +
>    regcache = get_thread_regcache (tp->ptid);
>    pc = regcache_read_pc (regcache);
>  
> @@ -1472,10 +1487,31 @@ btrace_enable (struct thread_info *tp, const struct btrace_config *conf)
>  
>    tp->btrace.target = target_enable_btrace (tp->ptid, 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);
> +  /* We're done if we failed to enable tracing.  */
> +  if (tp->btrace.target == NULL)
> +    return;
> +
> +  /* We need to undo the enable in case of errors.  */
> +  TRY
> +    {
> +      /* Add an entry for the current PC so we start tracing from where we
> +	 enabled it.
> +
> +	 If we can't access TP's registers, TP is most likely running.  In this
> +	 case, we can't really say where tracing was enabled so it should be
> +	 safe to simply skip this step.
> +
> +	 This is not relevant for BTRACE_FORMAT_PT since the trace will already
> +	 start at the PC at which tracing was enabled.  */
> +      if ((conf->format != BTRACE_FORMAT_PT)

Unnecessary parens.

> +	  && can_access_registers_ptid (tp->ptid))

If you have this check, why do you need to the TRY/CATCH ?

Or even, given the validate_registers_access_ptid check
above, why is this check necessary?

> +	btrace_add_pc (tp);
> +    }
> +  CATCH (exception, RETURN_MASK_ALL)

Adding a RETURN_MASK_ALL always raises alarm bells,
because this swallows a user-typed ctrl-c, which
is probably wrong.

> +    {
> +      btrace_disable (tp);
> +    }
> +  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..9fd9aca
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/enable-running.c
> @@ -0,0 +1,47 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2017 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 (;;)

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Don.27t_write_tests_that_run_forever

> +    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..577c319
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/enable-running.exp
> @@ -0,0 +1,72 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2017 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 }
> +
> +standard_testfile
> +if {[gdb_compile_pthreads "$srcdir/$subdir/$srcfile" "$binfile" executable {debug}] != "" } {
> +    return -1
> +}
> +clean_restart $testfile
> +
> +# We need to enable non-stop mode for the remote case.
> +gdb_test_no_output "set non-stop on"

This is too late with --target_board=native-extended-gdbserver.
Use instead:

save_vars { GDBFLAGS } {
    append GDBFLAGS " -ex \"set non-stop on\""
    clean_restart $testfile
}



> +
> +if ![runto_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
> +
> +    with_test_prefix "thread $thread" {
> +        gdb_test "thread $thread" "(running).*" "is running"
> +        # Stop the thread before reading the trace.
> +        gdb_test_multiple "interrupt" "interrupt" {
> +            -re "interrupt\r\n$gdb_prompt " {
> +                pass "interrupt"
> +            }
> +        }
> +        # Wait until the thread actually stopped.
> +        gdb_test_multiple "" "stopped" {
> +            -re "Thread $thread.*stopped\." {
> +                pass "stopped"
> +            }
> +        }
> +        # 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

Looks like this is adding duplicate test messages?  See:

https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

Thanks,
Pedro Alves