From patchwork Wed Feb 1 09:08:09 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Metzger, Markus T" X-Patchwork-Id: 19077 Received: (qmail 30864 invoked by alias); 1 Feb 2017 09:08:25 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 30850 invoked by uid 89); 1 Feb 2017 09:08:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=elected, 4176, Tel, tel X-HELO: mga05.intel.com Received: from mga05.intel.com (HELO mga05.intel.com) (192.55.52.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 01 Feb 2017 09:08:14 +0000 Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga105.fm.intel.com with ESMTP; 01 Feb 2017 01:08:12 -0800 X-ExtLoop1: 1 Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by orsmga005.jf.intel.com with ESMTP; 01 Feb 2017 01:08:11 -0800 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.142]) by IRSMSX151.ger.corp.intel.com ([169.254.4.20]) with mapi id 14.03.0248.002; Wed, 1 Feb 2017 09:08:10 +0000 From: "Metzger, Markus T" To: Pedro Alves , "gdb-patches@sourceware.org" CC: Simon Marchi Subject: RE: [PATCH v3 2/5] btrace: allow recording to be started (and stopped) for running threads Date: Wed, 1 Feb 2017 09:08:09 +0000 Message-ID: References: <1485770743-6603-1-git-send-email-markus.t.metzger@intel.com> <1485770743-6603-3-git-send-email-markus.t.metzger@intel.com> <1b369026-46a2-199c-ec6d-8302a462c10f@redhat.com> In-Reply-To: MIME-Version: 1.0 X-IsSubscribed: yes Hi Pedro, > > > I was also thinking of changing the error messages to refer to the argument > > > thread but that would make them less clear when PTID really refers to the > > > selected thread - which should be the case most of the time if not always. > > > > > > Maybe this is not an issue in practice and the "selected thread" errors are > > > understandable enough. What do you think? > > > > I think we may be able to sidestep deciding this. :-) > > That seems to work. I added a few tests to cover other record commands > while threads are running. Here's the resulting patch. You commented on "[PATCH v3 5/5] btrace, testsuite: fix extended-remote fail" but as far as I can tell this has been resolved and we don't need an update. commit eccb3848eadfc5d7a89e53d21d975e80f050a98a Author: Markus Metzger Date: Wed Nov 30 11:05:38 2016 +0100 btrace: allow recording to be started (and stopped) for running threads 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 Signed-off-by: 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. (btrace_fetch): Assert can_access_registers_ptid. * record-btrace.c (require_btrace_thread, record_btrace_info): Call validate_registers_access. testsuite/ * gdb.btrace/enable-running.c: New. * gdb.btrace/enable-running.exp: New. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 diff --git a/gdb/btrace.c b/gdb/btrace.c index d266af7..6d621e4 100644 --- a/gdb/btrace.c +++ b/gdb/btrace.c @@ -1472,10 +1472,33 @@ 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 + && can_access_registers_ptid (tp->ptid)) + btrace_add_pc (tp); + } + CATCH (exception, RETURN_MASK_ALL) + { + btrace_disable (tp); + + throw_exception (exception); + } + END_CATCH } /* See btrace.h. */ @@ -1709,6 +1732,9 @@ btrace_fetch (struct thread_info *tp) if (btinfo->replay != NULL) return; + /* We should not be called on running or exited threads. */ + gdb_assert (can_access_registers_ptid (tp->ptid)); + btrace_data_init (&btrace); cleanup = make_cleanup_btrace_data (&btrace); diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c index 8896241..ee0d22c 100644 --- a/gdb/record-btrace.c +++ b/gdb/record-btrace.c @@ -117,6 +117,8 @@ require_btrace_thread (void) if (tp == NULL) error (_("No thread.")); + validate_registers_access (); + btrace_fetch (tp); if (btrace_is_empty (tp)) @@ -417,6 +419,8 @@ record_btrace_info (struct target_ops *self) if (tp == NULL) error (_("No thread.")); + validate_registers_access (); + btinfo = &tp->btrace; conf = btrace_conf (btinfo); diff --git a/gdb/testsuite/gdb.btrace/enable-running.c b/gdb/testsuite/gdb.btrace/enable-running.c new file mode 100644 index 0000000..c2edf1e --- /dev/null +++ b/gdb/testsuite/gdb.btrace/enable-running.c @@ -0,0 +1,48 @@ +/* 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 +#include + +#define NTHREADS 3 + +static void * +test (void *arg) +{ + /* Let's hope this is long enough for GDB to enable tracing and check that + everything is working as expected. */ + sleep (10); + + return arg; +} + +int +main (void) +{ + pthread_t th[NTHREADS]; + int i; + + for (i = 0; i < NTHREADS; ++i) + pthread_create (&th[i], NULL, test, NULL); + + test (NULL); /* bp.1 */ + + for (i = 0; i < NTHREADS; ++i) + pthread_join (th[i], NULL); + + 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..d549a40 --- /dev/null +++ b/gdb/testsuite/gdb.btrace/enable-running.exp @@ -0,0 +1,95 @@ +# 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 +} + +# We need to enable non-stop mode for the remote case. +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" + + # We can't read the trace while the thread is running. + gdb_test "info record" "Selected thread is running\." \ + "info record while running" + + # Try various commands that try to read trace. + gdb_test "record instruction-history" "Selected thread is running\." + gdb_test "record function-call-history" "Selected thread is running\." + + # Including reverse-stepping commands. + gdb_test "reverse-continue" "\[Ss\]elected thread is running\." + gdb_test "reverse-step" "\[Ss\]elected thread is running\." + gdb_test "reverse-next" "\[Ss\]elected thread is running\." + gdb_test "reverse-finish" "\[Ss\]elected thread 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\]*" \ + ] + + # Continue the thread again. + gdb_test "cont&" "Continuing\." + } +} + +# Check that recording was started on each thread. +foreach thread {1 2 3 4} { + check_tracing_enabled $thread +} + +# Stop recording while all threads are running. +gdb_test "record stop" "Process record is stopped \[^\\\r\\\n\]*"