From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@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, 01 Feb 2017 09:08:00 -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: Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2017-02/txt/msg00007.txt.bz2 Hi Pedro, > > > I was also thinking of changing the error messages to refer to the ar= gument > > > 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 al= ways. > > > > > > Maybe this is not an issue in practice and the "selected thread" erro= rs are > > > understandable enough. What do you think? > > > > I think we may be able to sidestep deciding this. :-) >=20 > 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 fai= l" 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 =20=20=20=20 When recording is started for a running thread, GDB was able to start t= racing but then failed to read registers to insert the initial entry for the c= urrent PC. We don't really need that initial entry if we don't know where exa= ctly we started recording. Skip that step to allow recording to be started whi= le threads are running. =20=20=20=20 If we do run into errors, we need to undo the tracing enable to not lea= k this thread. The operation did not complete so our caller won't clean up th= is thread. =20=20=20=20 For the BTRACE_FORMAT_PT btrace format, we don't need that initial entr= y since it will be recorded in the trace. We can omit the call to btrace_add_p= c. =20=20=20=20 CC: Simon Marchi =20=20=20=20 Signed-off-by: Markus Metzger =20=20=20=20 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. =20=20=20=20 testsuite/ * gdb.btrace/enable-running.c: New. * gdb.btrace/enable-running.exp: New. 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) =20 tp->btrace.target =3D target_enable_btrace (tp->ptid, conf); =20 - /* Add an entry for the current PC so we start tracing from where we - enabled it. */ - if (tp->btrace.target !=3D NULL) - btrace_add_pc (tp); + /* We're done if we failed to enable tracing. */ + if (tp->btrace.target =3D=3D 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 !=3D 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 } =20 /* See btrace.h. */ @@ -1709,6 +1732,9 @@ btrace_fetch (struct thread_info *tp) if (btinfo->replay !=3D NULL) return; =20 + /* We should not be called on running or exited threads. */ + gdb_assert (can_access_registers_ptid (tp->ptid)); + btrace_data_init (&btrace); cleanup =3D make_cleanup_btrace_data (&btrace); =20 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 =3D=3D NULL) error (_("No thread.")); =20 + validate_registers_access (); + btrace_fetch (tp); =20 if (btrace_is_empty (tp)) @@ -417,6 +419,8 @@ record_btrace_info (struct target_ops *self) if (tp =3D=3D NULL) error (_("No thread.")); =20 + validate_registers_access (); + btinfo =3D &tp->btrace; =20 conf =3D 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 th= at + everything is working as expected. */ + sleep (10); + + return arg; +} + +int +main (void) +{ + pthread_t th[NTHREADS]; + int i; + + for (i =3D 0; i < NTHREADS; ++i) + pthread_create (&th[i], NULL, test, NULL); + + test (NULL); /* bp.1 */ + + for (i =3D 0; i < NTHREADS; ++i) + pthread_join (th[i], NULL); + + return 0; +} diff --git a/gdb/testsuite/gdb.btrace/enable-running.exp b/gdb/testsuite/gd= b.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}] !=3D "" } { + 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 runnin= g\." + + # 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\]*" 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