From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 116792 invoked by alias); 25 Jan 2017 14:32:46 -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 116780 invoked by uid 89); 25 Jan 2017 14:32:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.1 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=n.*, n$gdb_prompt, clean_restart, ngdb_prompt 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 ESMTP; Wed, 25 Jan 2017 14:32:35 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6DE0161E4A; Wed, 25 Jan 2017 14:32:35 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.phx2.redhat.com [10.5.9.4]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v0PEWSI5005938; Wed, 25 Jan 2017 09:32:34 -0500 Subject: Re: [PATCH v2 2/2] btrace: allow recording to be started for running threads To: Markus Metzger , gdb-patches@sourceware.org References: <1484923135-31270-1-git-send-email-markus.t.metzger@intel.com> <1484923135-31270-3-git-send-email-markus.t.metzger@intel.com> Cc: Simon Marchi From: Pedro Alves Message-ID: Date: Wed, 25 Jan 2017 14:32:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1484923135-31270-3-git-send-email-markus.t.metzger@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-01/txt/msg00526.txt.bz2 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 > > 2017-01-20 Markus Metzger > > 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 . */ > + > +#include > + > +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 . > + > +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