From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 87080 invoked by alias); 10 May 2017 11:46:29 -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 86283 invoked by uid 89); 10 May 2017 11:46:28 -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:26 +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:26 -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:26 -0700 Received: from irsmsx112.ger.corp.intel.com (10.108.20.5) by IRSMSX104.ger.corp.intel.com (163.33.3.159) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 10 May 2017 12:46:25 +0100 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.163]) by irsmsx112.ger.corp.intel.com ([169.254.1.61]) with mapi id 14.03.0319.002; Wed, 10 May 2017 12:46:25 +0100 From: "Wiederhake, Tim" To: Simon Marchi CC: "gdb-patches@sourceware.org" , "Metzger, Markus T" Subject: RE: [PATCH v3 05/12] btrace: Use function segment index in insn iterator. Date: Wed, 10 May 2017 11:46:00 -0000 Message-ID: <9676A094AF46E14E8265E7A3F4CCE9AF3C14CFB5@irsmsx105.ger.corp.intel.com> References: <1494312929-22749-1-git-send-email-tim.wiederhake@intel.com> <1494312929-22749-6-git-send-email-tim.wiederhake@intel.com> <509c21522fecdf146903a231e97d531c@polymtl.ca> In-Reply-To: <509c21522fecdf146903a231e97d531c@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/msg00245.txt.bz2 Hi Simon, Thanks for reviewing! > -----Original Message----- > From: Simon Marchi [mailto:simon.marchi@polymtl.ca] > Sent: Wednesday, May 10, 2017 4:20 AM > To: Wiederhake, Tim > Cc: gdb-patches@sourceware.org; Metzger, Markus T > > Subject: Re: [PATCH v3 05/12] btrace: Use function segment index in insn > iterator. >=20 > On 2017-05-09 02:55, Tim Wiederhake wrote: > > Remove FUNCTION pointer in struct btrace_insn_iterator and use an index > > into > > the list of function segments instead. > > > > 2017-05-09 Tim Wiederhake > > > > gdb/ChangeLog: > > > > * btrace.c: (btrace_insn_get, btrace_insn_get_error, > > btrace_insn_number, > > btrace_insn_begin, btrace_insn_end, btrace_insn_next, > > btrace_insn_prev, > > btrace_find_insn_by_number): Replace function segment pointer with > > index. > > (btrace_insn_cmp): Simplify. > > * btrace.h: (struct btrace_insn_iterator) Rename index to > > insn_index. Replace function segment pointer with index into > function > > segment vector. > > * record-btrace.c (record_btrace_call_history): Replace function > > segment pointer use with index. > > (record_btrace_frame_sniffer): Retrieve function call segment > through > > vector. > > (record_btrace_set_replay): Remove defunc't safety check. >=20 > Looks good, just a few comments below. >=20 > > @@ -2468,12 +2474,21 @@ int > > btrace_insn_cmp (const struct btrace_insn_iterator *lhs, > > const struct btrace_insn_iterator *rhs) > > { > > - unsigned int lnum, rnum; > > + gdb_assert (lhs->btinfo =3D=3D rhs->btinfo); > > > > - lnum =3D btrace_insn_number (lhs); > > - rnum =3D btrace_insn_number (rhs); > > + if (lhs->call_index > rhs->call_index) > > + return 1; > > + > > + if (lhs->call_index < rhs->call_index) > > + return -1; > > + > > + if (lhs->insn_index > rhs->insn_index) > > + return 1; > > + > > + if (lhs->insn_index < rhs->insn_index) > > + return -1; > > > > - return (int) (lnum - rnum); > > + return 0; > > } >=20 > I the number of comparisons could be reduced by doing: >=20 > int > btrace_insn_cmp (const struct btrace_insn_iterator *lhs, > const struct btrace_insn_iterator *rhs) > { > gdb_assert (lhs->btinfo =3D=3D rhs->btinfo); >=20 > if (lhs->call_index !=3D rhs->call_index) > return lhs->call_index - rhs->call_index; >=20 > return lhs->insn_index - rhs->insn_index; > } You're right. Changed locally, thanks! >=20 > > > > /* See btrace.h. */ > > @@ -2522,8 +2537,8 @@ btrace_find_insn_by_number (struct > > btrace_insn_iterator *it, > > } > > > > it->btinfo =3D btinfo; > > - it->function =3D bfun; > > - it->index =3D number - bfun->insn_offset; > > + it->call_index =3D bfun->number - 1; > > + it->insn_index =3D number - bfun->insn_offset; > > return 1; > > } > > > > diff --git a/gdb/btrace.h b/gdb/btrace.h > > index ef2c781..ca79667 100644 > > --- a/gdb/btrace.h > > +++ b/gdb/btrace.h > > @@ -195,12 +195,11 @@ struct btrace_insn_iterator > > /* The branch trace information for this thread. Will never be > > NULL. */ > > const struct btrace_thread_info *btinfo; > > > > - /* The branch trace function segment containing the instruction. > > - Will never be NULL. */ > > - const struct btrace_function *function; >=20 > Just an idea, you could factor out those >=20 > it->btinfo->functions[it->call_index] >=20 > in a small helper method in btrace_insn_iterator: >=20 > btrace_function *function () > { > return this->btinfo->functions[this->call_index]; > } I'd like to postpone all further C++-ifications to a separate patch set. > > @@ -2691,7 +2691,7 @@ record_btrace_set_replay (struct thread_info *tp, > > > > btinfo =3D &tp->btrace; > > > > - if (it =3D=3D NULL || it->function =3D=3D NULL) > > + if (it =3D=3D NULL) >=20 > Not sure, but wouldn't the equivalent check be that call_index < > btinfo->functions.size () ? The comment on btrace_insn_iterator::function used to read: "The branch tra= ce function segment containing the instruction. Will never be NULL". The ch= eck for it->function =3D=3D NULL was a defensive measure but not necessary = in terms of program behavior. > 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