From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 88958 invoked by alias); 10 May 2017 11:46:45 -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 88674 invoked by uid 89); 10 May 2017 11:46:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.0 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: mga01.intel.com Received: from mga01.intel.com (HELO mga01.intel.com) (192.55.52.88) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 10 May 2017 11:46:39 +0000 Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 May 2017 04:46:42 -0700 X-ExtLoop1: 1 Received: from irsmsx104.ger.corp.intel.com ([163.33.3.159]) by orsmga005.jf.intel.com with ESMTP; 10 May 2017 04:46:41 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.163]) by IRSMSX104.ger.corp.intel.com ([163.33.3.159]) with mapi id 14.03.0319.002; Wed, 10 May 2017 12:46:40 +0100 From: "Wiederhake, Tim" To: Simon Marchi CC: "gdb-patches@sourceware.org" , "Metzger, Markus T" Subject: RE: [PATCH v3 12/12] btrace: Store function segments as objects. Date: Wed, 10 May 2017 11:46:00 -0000 Message-ID: <9676A094AF46E14E8265E7A3F4CCE9AF3C14CFF2@irsmsx105.ger.corp.intel.com> References: <1494312929-22749-1-git-send-email-tim.wiederhake@intel.com> <1494312929-22749-13-git-send-email-tim.wiederhake@intel.com> <03bcd302ae8b132f5d2209c3e40cf494@polymtl.ca> In-Reply-To: <03bcd302ae8b132f5d2209c3e40cf494@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/msg00251.txt.bz2 Hi Simon, Thanks for reviewing! > -----Original Message----- > From: Simon Marchi [mailto:simon.marchi@polymtl.ca] > Sent: Wednesday, May 10, 2017 7:09 AM > To: Wiederhake, Tim > Cc: gdb-patches@sourceware.org; Metzger, Markus T > > Subject: Re: [PATCH v3 12/12] btrace: Store function segments as objects. >=20 > On 2017-05-09 02:55, Tim Wiederhake wrote: > > 2017-05-09 Tim Wiederhake > > > > gdb/ChangeLog: > > * btrace.c: > > * btrace.h: > > * record-btrace.c: >=20 > IWBN if you could be more explicit :). Whoops. > > > > --- > > gdb/btrace.c | 94 > > ++++++++++++++++++++++++++--------------------------- > > gdb/btrace.h | 7 ++-- > > gdb/record-btrace.c | 10 +++--- > > 3 files changed, 56 insertions(+), 55 deletions(-) > > > > diff --git a/gdb/btrace.c b/gdb/btrace.c > > index 7b82000..4c7020d 100644 > > --- a/gdb/btrace.c > > +++ b/gdb/btrace.c > > @@ -155,14 +155,27 @@ ftrace_call_num_insn (const struct > > btrace_function* bfun) > > /* Return the function segment with the given NUMBER or NULL if no > > such segment > > exists. BTINFO is the branch trace information for the current > > thread. */ > > > > -static struct btrace_function * > > +const static struct btrace_function * >=20 > It would make more sense to put the "static" first. Done. > > ftrace_find_call_by_number (const struct btrace_thread_info *btinfo, > > unsigned int number) > > { > > if (number =3D=3D 0 || number > btinfo->functions.size ()) > > return NULL; > > > > - return btinfo->functions[number - 1]; > > + return &btinfo->functions[number - 1]; > > +} > > + > > +/* Return the function segment with the given NUMBER or NULL if no > > such segment > > + exists. BTINFO is the branch trace information for the current > > thread. */ >=20 > It took me a surprisingly high amount of seconds to understand that this > was a const version of the function below. To avoid reapeating the > comment and to make it clear it's the same thing, you can replace the > comment of the const version to something like: >=20 > /* A const version of the function above. */ Done. > > + > > +static struct btrace_function * > > +ftrace_find_call_by_number (struct btrace_thread_info *btinfo, > > + unsigned int number) > > +{ > > + if (number =3D=3D 0 || number > btinfo->functions.size ()) > > + return NULL; > > + > > + return &btinfo->functions[number - 1]; > > } > > > > /* Return non-zero if BFUN does not match MFUN and FUN, > > @@ -214,37 +227,33 @@ ftrace_function_switched (const struct > > btrace_function *bfun, > > /* 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. > > - MFUN and FUN are the symbol information we have for this function. > > */ > > + MFUN and FUN are the symbol information we have for this function. > > + This invalidates all struct btrace_function pointer currently held. > > */ > > > > static struct btrace_function * > > ftrace_new_function (struct btrace_thread_info *btinfo, > > struct minimal_symbol *mfun, > > struct symbol *fun) > > { > > - struct btrace_function *bfun; > > - > > - bfun =3D XCNEW (struct btrace_function); > > - > > - bfun->msym =3D mfun; > > - bfun->sym =3D fun; > > + struct btrace_function bfun {mfun, fun, 0, 0, 0, NULL, 0, 0, 0, 0, > > 0}; >=20 > I think it would be much better to add a simple constructor to > btrace_function. For the fields that should simply be zero'ed, you can > initialize fields directly, like we do in many other places (e.g. class > inferior). Having a proper constructor would definitely be beneficial here. Nevertheless, I would do such a change in a separate patch set. > > > > if (btinfo->functions.empty ()) > > { > > /* Start counting at one. */ > > - bfun->number =3D 1; > > - bfun->insn_offset =3D 1; > > + bfun.number =3D 1; > > + bfun.insn_offset =3D 1; > > } > > else > > { > > - struct btrace_function *prev =3D btinfo->functions.back (); > > + struct btrace_function *prev =3D &btinfo->functions.back (); > > > > - bfun->number =3D prev->number + 1; > > - bfun->insn_offset =3D prev->insn_offset + ftrace_call_num_insn > > (prev); > > - bfun->level =3D prev->level; > > + bfun.number =3D prev->number + 1; > > + bfun.insn_offset =3D prev->insn_offset + ftrace_call_num_insn > > (prev); > > + bfun.level =3D prev->level; > > } > > > > - btinfo->functions.push_back (bfun); > > - return bfun; > > + btinfo->functions.push_back (std::move (bfun)); > > + return &btinfo->functions.back (); >=20 > I could be mistaken, but I don't think the move is very useful here, > since all fields of btrace_function are trivial (?). You could use > emplace_back instead: >=20 > btinfo->functions.emplace_back (mfun, fun); > btrace_function &bfun =3D btinfo->functions.back (); >=20 > ... >=20 > return &bfun; >=20 > or >=20 > unsigned int number, insn_offset; > unsigned int insn_offset =3D prev->insn_offset + ftrace_call_num_insn > (prev); > int level =3D prev->level; >=20 > if (btinfo->functions.empty ()) > { > /* Start counting at one. */ > number =3D 1; > insn_offset =3D 1; >=20 > level =3D 0; > } > else > { > struct btrace_function *prev =3D &btinfo->functions.back (); >=20 > number =3D prev->number + 1; > insn_offset =3D prev->insn_offset + ftrace_call_num_insn (prev); > level =3D prev->level; > } >=20 > btinfo->functions.emplace_back (mfun, fun, number, insn_offset, > level); >=20 > return &btinfo->functions.back (); You are right, the std::move is quiete pointless. Sadly, we can't use emplace_back() yet until btrace_function gets a constructor. Removed the move. > 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