From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 53086 invoked by alias); 24 Feb 2017 09:32:42 -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 53075 invoked by uid 89); 24 Feb 2017 09:32:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.5 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=Tel X-HELO: mga14.intel.com Received: from mga14.intel.com (HELO mga14.intel.com) (192.55.52.115) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 24 Feb 2017 09:32:38 +0000 Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Feb 2017 01:32:37 -0800 X-ExtLoop1: 1 Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by orsmga001.jf.intel.com with ESMTP; 24 Feb 2017 01:32:36 -0800 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.142]) by IRSMSX102.ger.corp.intel.com ([169.254.2.230]) with mapi id 14.03.0248.002; Fri, 24 Feb 2017 09:32:35 +0000 From: "Metzger, Markus T" To: "Wiederhake, Tim" , "gdb-patches@sourceware.org" Subject: RE: [PATCH 04/11] btrace: Use function segment index in call iterator. Date: Fri, 24 Feb 2017 09:32:00 -0000 Message-ID: References: <1487337989-6367-1-git-send-email-tim.wiederhake@intel.com> <1487337989-6367-5-git-send-email-tim.wiederhake@intel.com> In-Reply-To: <1487337989-6367-5-git-send-email-tim.wiederhake@intel.com> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2017-02/txt/msg00661.txt.bz2 > -----Original Message----- > From: Wiederhake, Tim > Sent: Friday, February 17, 2017 2:26 PM > To: gdb-patches@sourceware.org > Cc: Metzger, Markus T > Subject: [PATCH 04/11] btrace: Use function segment index in call iterato= r. Hello Tim, >=20 > 2017-02-17 Tim Wiederhake >=20 > gdb/ChangeLog >=20 > * btrace.c (btrace_ends_with_single_insn): New function. > (btrace_call_get, btrace_call_number, btrace_call_begin, > btrace_call_end, btrace_call_next, btrace_call_prev, > btrace_find_call_by_number): Use > index into call segment vector instead of pointer. Most, if not all, should still fit onto the previous line. > +static int > +btrace_ends_with_single_insn (const struct btrace_thread_info* btinfo) The space ' ' goes on the other side of the '*', i.e. "const struct btrace_= thread_info *btinfo". > +{ > + const btrace_function *bfun; > + > + if (VEC_empty (btrace_fun_p, btinfo->functions)) > + return 0; > + > + bfun =3D VEC_last (btrace_fun_p, btinfo->functions); > + return ftrace_call_num_insn (bfun) =3D=3D 1; Shouldn't we check for gaps? They also count as one instruction. =20 > /* See btrace.h. */ > @@ -2528,28 +2546,12 @@ btrace_call_get (const struct btrace_call_iterato= r *it) > unsigned int > btrace_call_number (const struct btrace_call_iterator *it) > { > - const struct btrace_thread_info *btinfo; > - const struct btrace_function *bfun; > - unsigned int insns; > - > - btinfo =3D it->btinfo; > - bfun =3D it->function; > - if (bfun !=3D NULL) > - return bfun->number; > - > - /* For the end iterator, i.e. bfun =3D=3D NULL, we return one more tha= n the > - number of the last function. */ > - bfun =3D btinfo->end; > - insns =3D VEC_length (btrace_insn_s, bfun->insn); > + const unsigned int length =3D VEC_length (btrace_fun_p, it->btinfo->fu= nctions); >=20 > - /* If the function contains only a single instruction (i.e. the current > - instruction), it will be skipped and its number is already the numb= er > - we seek. */ > - if (insns =3D=3D 1) > - return bfun->number; > + if ((it->call_index =3D=3D length) && btrace_ends_with_single_insn (it= ->btinfo)) > + return length; Please leave the comment or modify it to better match the new code. This is otherwise quite hard to read. > /* See btrace.h. */ > @@ -2558,14 +2560,15 @@ void > btrace_call_begin (struct btrace_call_iterator *it, > const struct btrace_thread_info *btinfo) > { > - const struct btrace_function *bfun; > - > - bfun =3D btinfo->begin; > - if (bfun =3D=3D NULL) > + if (VEC_empty (btrace_fun_p, btinfo->functions)) > error (_("No trace.")); >=20 > it->btinfo =3D btinfo; > - it->function =3D bfun; > + it->call_index =3D 0; > + > + if ((VEC_length (btrace_fun_p, it->btinfo->functions) =3D=3D 1) > + && (btrace_ends_with_single_insn (btinfo))) > + it->call_index =3D 1; We didn't have such a check before. Why is this needed? Was this a bug be= fore? > /* See btrace.h. */ > @@ -2589,35 +2589,29 @@ btrace_call_end (struct btrace_call_iterator *it, > unsigned int > btrace_call_next (struct btrace_call_iterator *it, unsigned int stride) > { > - const struct btrace_function *bfun; > - unsigned int steps; > + const unsigned int length =3D VEC_length (btrace_fun_p, it->btinfo->fu= nctions); >=20 > - bfun =3D it->function; > - steps =3D 0; > - while (bfun !=3D NULL) > + if (it->call_index + stride < length - 1) > { > - const struct btrace_function *next; > - unsigned int insns; > - > - next =3D bfun->flow.next; > - if (next =3D=3D NULL) > - { > - /* Ignore the last function if it only contains a single > - (i.e. the current) instruction. */ > - insns =3D VEC_length (btrace_insn_s, bfun->insn); > - if (insns =3D=3D 1) > - steps -=3D 1; > - } > - > - if (stride =3D=3D steps) > - break; > - > - bfun =3D next; > - steps +=3D 1; > + it->call_index +=3D stride; > + } No {} if there's a single statement left. > + else if (it->call_index + stride =3D=3D length - 1) > + { > + if (btrace_ends_with_single_insn (it->btinfo)) > + it->call_index =3D length; > + else > + it->call_index +=3D stride; > + } > + else > + { > + if (btrace_ends_with_single_insn (it->btinfo)) > + stride =3D length - it->call_index - 1; > + else > + stride =3D length - it->call_index; > + it->call_index =3D length; > } Can we merge the two last cases? Please add a comment explaining that we're ignoring that last function segm= ent if it only contains the current instruction. > /* See btrace.h. */ > @@ -2625,48 +2619,30 @@ btrace_call_next (struct btrace_call_iterator *it, > unsigned int stride) > unsigned int > btrace_call_prev (struct btrace_call_iterator *it, unsigned int stride) > { > - const struct btrace_thread_info *btinfo; > - const struct btrace_function *bfun; > - unsigned int steps; > + const unsigned int length =3D VEC_length (btrace_fun_p, it->btinfo->fu= nctions); > + int steps; > + > + if (length =3D=3D 0 || stride =3D=3D 0) > + return 0; >=20 > - bfun =3D it->function; > steps =3D 0; >=20 > - if (bfun =3D=3D NULL) > + if (it->call_index >=3D length) "> length" is really an internal error, isn't it? > { > - unsigned int insns; > - > - btinfo =3D it->btinfo; > - bfun =3D btinfo->end; > - if (bfun =3D=3D NULL) > - return 0; > - > - /* Ignore the last function if it only contains a single > - (i.e. the current) instruction. */ > - insns =3D VEC_length (btrace_insn_s, bfun->insn); > - if (insns =3D=3D 1) > - bfun =3D bfun->flow.prev; > - > - if (bfun =3D=3D NULL) > - return 0; > + if (btrace_ends_with_single_insn (it->btinfo)) > + it->call_index =3D length - 2; > + else > + it->call_index =3D length - 1; Please keep the comment or add a new one to explain why we're doing this. LENGTH - 2 may be negative (or, rather, very big) if the trace only contain= ed the current instruction. > - steps +=3D 1; > + steps =3D 1; > + stride -=3D 1; > } Please add a comment to explain this. > - while (steps < stride) > - { > - const struct btrace_function *prev; > + if (it->call_index < stride) > + stride =3D it->call_index; >=20 > - prev =3D bfun->flow.prev; > - if (prev =3D=3D NULL) > - break; > - > - bfun =3D prev; > - steps +=3D 1; > - } > - > - it->function =3D bfun; > - return steps; > + it->call_index -=3D stride; > + return steps + stride; > } I think I understand what this is doing but it wasn't obvious. Maybe the comment on the STEPS and STRIDE adjustment above suffices to explain it. > /* Branch trace iteration state for "record instruction-history". */ > diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c > index f7683f2..ba83be0 100644 > --- a/gdb/record-btrace.c > +++ b/gdb/record-btrace.c > @@ -1110,8 +1110,8 @@ record_btrace_call_history (struct target_ops *self= , int > size, int int_flags) > replay =3D btinfo->replay; > if (replay !=3D NULL) > { > - begin.function =3D replay->function; > begin.btinfo =3D btinfo; > + begin.call_index =3D replay->function->number - 1; > } No {} when there's only one statement left. 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