From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 67189 invoked by alias); 31 Jan 2017 16:06:55 -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 67176 invoked by uid 89); 31 Jan 2017 16:06:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.3 required=5.0 tests=AWL,BAYES_20,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=ptid_t, ptid_equal, null_ptid, do_cleanups 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; Tue, 31 Jan 2017 16:06:44 +0000 Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP; 31 Jan 2017 08:06:43 -0800 X-ExtLoop1: 1 Received: from irsmsx153.ger.corp.intel.com ([163.33.192.75]) by fmsmga004.fm.intel.com with ESMTP; 31 Jan 2017 08:06:41 -0800 Received: from irsmsx112.ger.corp.intel.com (10.108.20.5) by IRSMSX153.ger.corp.intel.com (163.33.192.75) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 31 Jan 2017 16:06:41 +0000 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.142]) by irsmsx112.ger.corp.intel.com ([169.254.1.175]) with mapi id 14.03.0248.002; Tue, 31 Jan 2017 16:06:41 +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: Tue, 31 Jan 2017 16:06: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> 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-01/txt/msg00671.txt.bz2 Hi Pedro, Thanks for your review. > > +static void > > +validate_registers_access_ptid (ptid_t ptid) > > +{ > > + struct cleanup *cleanup =3D save_inferior_ptid (); > > + > > + inferior_ptid =3D ptid; > > + validate_registers_access (); > > + do_cleanups (cleanup); > > +} > > + >=20 > Please let's refactor things a bit to avoid the need to frob > inferior_ptid and restore with a cleanup. Change the existing > validate_registers_access like: >=20 > static void > -validate_registers_access () > +validate_registers_access_ptid (ptid_t ptid) > { > /* No selected thread, no registers. */ > -if (ptid_equal (inferior_ptid, null_ptid)) > +if (ptid_equal (ptid, null_ptid)) > error (_("No thread selected.")); > [etc.] >=20 > and then reimplement it back on top of validate_registers_access_ptid: >=20 > validate_registers_access () > { > return validate_registers_access_ptid (inferior_ptid); > } I had that and then chose to rewrite it again because the errors that validate_registers_access throws refer to "selected thread". In the btrace case, the argument thread is the selected thread (I think this is true for all flows that call btrace_fetch) but this is not necessar= ily the case in general. Do you think that might be confusing to the user? 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? Thanks, Markus. 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