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

Pedro Alves
2017-01-25T14:32:46Z

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