From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 88126 invoked by alias); 10 May 2017 11:46:39 -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 88041 invoked by uid 89); 10 May 2017 11:46:38 -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:35 +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:38 -0700 X-ExtLoop1: 1 Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by orsmga005.jf.intel.com with ESMTP; 10 May 2017 04:46:37 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.163]) by IRSMSX107.ger.corp.intel.com ([169.254.10.107]) with mapi id 14.03.0319.002; Wed, 10 May 2017 12:46:36 +0100 From: "Wiederhake, Tim" To: Simon Marchi CC: "gdb-patches@sourceware.org" , "Metzger, Markus T" Subject: RE: [PATCH v3 10/12] btrace: Replace struct btrace_thread_info::segment. Date: Wed, 10 May 2017 11:46:00 -0000 Message-ID: <9676A094AF46E14E8265E7A3F4CCE9AF3C14CFE1@irsmsx105.ger.corp.intel.com> References: <1494312929-22749-1-git-send-email-tim.wiederhake@intel.com> <1494312929-22749-11-git-send-email-tim.wiederhake@intel.com> In-Reply-To: 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/msg00247.txt.bz2 Hi Simon, thanks for reviewing! > -----Original Message----- > From: Simon Marchi [mailto:simon.marchi@polymtl.ca] > Sent: Wednesday, May 10, 2017 6:15 AM > To: Wiederhake, Tim > Cc: gdb-patches@sourceware.org; Metzger, Markus T > > Subject: Re: [PATCH v3 10/12] btrace: Replace struct > btrace_thread_info::segment. >=20 > This title too should say btrace_function. Fixed. > On 2017-05-09 02:55, Tim Wiederhake wrote: > > This used to hold a pair of pointers to the previous and next function > > segment > > that belong to this function call. Replace with a pair of indices into > > the > > vector of function segments. > > > > 2017-05-09 Tim Wiederhake > > > > gdb/ChangeLog: > > > > * btrace.c (ftrace_fixup_caller, ftrace_new_return, > > ftrace_connect_bfun, > > ftrace_bridge_gap): Replace references to > btrace_thread_info::segment > > with btrace_thread_info::next_segment and > > btrace_thread_info::prev_segment. > > * btrace.h: Remove struct btrace_func_link. > > (struct btrace_function): Replace pair of function segment pointers > > with pair of indices. > > * python/py-record-btrace.c (btpy_call_prev_sibling, > > btpy_call_next_sibling): Replace references to > > btrace_thread_info::segment with btrace_thread_info::next_segment > and > > btrace_thread_info::prev_segment. > > * record-btrace.c (record_btrace_frame_this_id): Same. > > > > --- > > gdb/btrace.c | 47 > > ++++++++++++++++++++++++++----------------- > > gdb/btrace.h | 17 ++++++---------- > > gdb/python/py-record-btrace.c | 8 ++++---- > > gdb/record-btrace.c | 4 ++-- > > 4 files changed, 40 insertions(+), 36 deletions(-) > > > > diff --git a/gdb/btrace.c b/gdb/btrace.c > > index f57bbf9..921cb64 100644 > > --- a/gdb/btrace.c > > +++ b/gdb/btrace.c > > @@ -271,20 +271,29 @@ ftrace_update_caller (struct btrace_function > > *bfun, > > /* Fix up the caller for all segments of a function. */ > > > > static void > > -ftrace_fixup_caller (struct btrace_function *bfun, > > +ftrace_fixup_caller (struct btrace_thread_info *btinfo, > > + struct btrace_function *bfun, > > struct btrace_function *caller, > > enum btrace_function_flag flags) > > { > > - struct btrace_function *prev, *next; > > + unsigned int prev, next; > > > > + prev =3D bfun->prev; > > + next =3D bfun->next; > > ftrace_update_caller (bfun, caller, flags); > > > > /* Update all function segments belonging to the same function. */ > > - for (prev =3D bfun->segment.prev; prev !=3D NULL; prev =3D > > prev->segment.prev) > > - ftrace_update_caller (prev, caller, flags); > > + for (; prev !=3D 0; prev =3D bfun->prev) > > + { > > + bfun =3D ftrace_find_call_by_number (btinfo, prev); > > + ftrace_update_caller (bfun, caller, flags); > > + } > > > > - for (next =3D bfun->segment.next; next !=3D NULL; next =3D > > next->segment.next) > > - ftrace_update_caller (next, caller, flags); > > + for (; next !=3D 0; next =3D bfun->next) > > + { > > + bfun =3D ftrace_find_call_by_number (btinfo, next); > > + ftrace_update_caller (bfun, caller, flags); > > + } >=20 > Could you define the loop variables in the for like this? >=20 > for (unsigned int prev =3D bfun->prev; prev !=3D 0; prev =3D bfun->pre= v) > { > bfun =3D ftrace_find_call_by_number (btinfo, prev); > ftrace_update_caller (bfun, caller, flags); > } >=20 > Unless is it important to capture the value of bfun->prev/next before > calling ftrace_update_caller? This way their scope is limited to where > they are used. This is what the function would look like: ftrace_update_caller (bfun, caller, flags); for (unsigned int i =3D bfun->prev; i !=3D 0;) { struct btrace_function *tmp =3D ftrace_find_call_by_number (btinfo, i= ); ftrace_update_caller (tmp, caller, flags); i =3D tmp->prev; } =20=20 for (unsigned int i =3D bfun->next; i !=3D 0;) { struct btrace_function *tmp =3D ftrace_find_call_by_number (btinfo, i= ); ftrace_update_caller (tmp, caller, flags); i =3D tmp->next; } IMO, this isn't any better. If I pull out the struct btrace_function* tmp declaration, it would look like this: struct btrace_function *tmp; =20=20 ftrace_update_caller (bfun, caller, flags); for (unsigned int i =3D bfun->prev; i !=3D 0; i =3D tmp->prev) { tmp =3D ftrace_find_call_by_number (btinfo, i); ftrace_update_caller (tmp, caller, flags); } =20=20 for (unsigned int i =3D bfun->next; i !=3D 0; i =3D tmp->next) { tmp =3D ftrace_find_call_by_number (btinfo, i); ftrace_update_caller (tmp, caller, flags); } I'd leave it as is for now and see how this code changes in a *drum roll* future C++-ification patch series. > Btw, this is another thing that could be rewritten nicely if > btrace_function had a backlink to btrace_thread_info, something like: >=20 > for (btrace_function *it =3D bfun; it !=3D NULL; it =3D it->next_segme= nt ()) > ftrace_update_caller (it, caller, flags); >=20 > Btw #2, I thing this function could be more efficient (or maybe I don't > understand as well as I think). If bfun at function entry is in the > middle of a long list of segments, it will start from there and iterate > backwards until it hits the first segment. Correct so far. > But because the same bfun > variable is reused, it will iterate forward from the start We saved PREV and NEXT beforehand and use BFUN as a temporary variable afterwards. The second "for" loop starts at NEXT, which is one past the original "middle of the long list of segments". > until the > end, whereas it only needed to iterate from the original bfun. Using a > temporary loop variable to avoid modifying bfun would correct that. > > @@ -948,7 +957,7 @@ ftrace_bridge_gap (struct btrace_thread_info > > *btinfo, > > static void > > btrace_bridge_gaps (struct thread_info *tp, VEC (bfun_s) **gaps) > > { > > - struct btrace_thread_info *btinfo; > > + struct btrace_thread_info *btinfo =3D &tp->btrace; >=20 > btinfo is now assigned twice in this function. Good catch, thank you! >=20 > 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