From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 87213 invoked by alias); 10 May 2017 11:46:31 -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 87083 invoked by uid 89); 10 May 2017 11:46:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mga02.intel.com Received: from mga02.intel.com (HELO mga02.intel.com) (134.134.136.20) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 10 May 2017 11:46:28 +0000 Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 May 2017 04:46:29 -0700 X-ExtLoop1: 1 Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by orsmga005.jf.intel.com with ESMTP; 10 May 2017 04:46:28 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.163]) by IRSMSX151.ger.corp.intel.com ([169.254.4.123]) with mapi id 14.03.0319.002; Wed, 10 May 2017 12:46:28 +0100 From: "Wiederhake, Tim" To: Simon Marchi CC: "gdb-patches@sourceware.org" , "Metzger, Markus T" Subject: RE: [PATCH v3 06/12] btrace: Remove constant arguments. Date: Wed, 10 May 2017 11:46:00 -0000 Message-ID: <9676A094AF46E14E8265E7A3F4CCE9AF3C14CFC5@irsmsx105.ger.corp.intel.com> References: <1494312929-22749-1-git-send-email-tim.wiederhake@intel.com> <1494312929-22749-7-git-send-email-tim.wiederhake@intel.com> <05da714930334ac9ae083f28dcfa2fe0@polymtl.ca> In-Reply-To: <05da714930334ac9ae083f28dcfa2fe0@polymtl.ca> dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg00246.txt.bz2 Hi Simon, Thanks for reviewing! > -----Original Message----- > From: Simon Marchi [mailto:simon.marchi@polymtl.ca] > Sent: Wednesday, May 10, 2017 4:46 AM > To: Wiederhake, Tim > Cc: gdb-patches@sourceware.org; Metzger, Markus T > > Subject: Re: [PATCH v3 06/12] btrace: Remove constant arguments. >=20 > On 2017-05-09 02:55, Tim Wiederhake wrote: > > 2017-05-09 Tim Wiederhake > > > > gdb/ChangeLog: > > > > * btrace.c (ftrace_new_function, ftrace_new_call, > ftrace_new_tailcall, > > ftrace_new_return, ftrace_new_switch, ftrace_new_gap, > > ftrace_update_function): Remove arguments that implicitly were > always > > BTINFO->END. > > (btrace_compute_ftrace_bts, ftrace_add_pt, > btrace_compute_ftrace_pt): > > Don't pass BTINFO->END. >=20 > Looks good, just a few comments below. >=20 > > diff --git a/gdb/btrace.c b/gdb/btrace.c > > index cb30dcf..1bd11f0 100644 > > --- a/gdb/btrace.c > > +++ b/gdb/btrace.c > > @@ -202,19 +202,19 @@ ftrace_function_switched (const struct > > btrace_function *bfun, > > return 0; > > } > > > > -/* Allocate and initialize a new branch trace function segment. > > +/* Allocate and initialize a new branch trace function segment at the > > end of > > + the trace. > > BTINFO is the branch trace information for the current thread. > > - PREV is the chronologically preceding function segment. > > MFUN and FUN are the symbol information we have for this function. > > */ > > > > static struct btrace_function * > > ftrace_new_function (struct btrace_thread_info *btinfo, > > - struct btrace_function *prev, > > struct minimal_symbol *mfun, > > struct symbol *fun) > > { > > - struct btrace_function *bfun; > > + struct btrace_function *bfun, *prev; > > > > + prev =3D btinfo->end; >=20 > Note that we can now declare variables at the point we use it, and drop > the struct keyword, like: >=20 > btrace_function *prev =3D btinfo->end; >=20 > It's up to you, you can continue with your current style if you wish. I do prefer declaring (and if at all possible, initialising) variables at t= he point of first usage, too. For now I'll stick with the current style to = leave this kind of change to a separate C++-ification patch set. > > bfun =3D XCNEW (struct btrace_function); > > > > bfun->msym =3D mfun; > > @@ -238,7 +238,7 @@ ftrace_new_function (struct btrace_thread_info > > *btinfo, > > } > > > > btinfo->functions.push_back (bfun); > > - return bfun; > > + return btinfo->end =3D bfun; >=20 > Err I'm really not a fan of assignment as a side effect. This line is removed by the very next patch in the series. Anyway, I change= d it. >=20 > > @@ -515,13 +510,13 @@ ftrace_new_gap (struct btrace_thread_info > > *btinfo, > > Return the chronologically latest function segment, never NULL. */ > > > > static struct btrace_function * > > -ftrace_update_function (struct btrace_thread_info *btinfo, > > - struct btrace_function *bfun, CORE_ADDR pc) > > +ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR > > pc) >=20 > The comment of this function would need to be updated as well. Fixed. > Thanks, >=20 > Simon Regards, Tim 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