From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29943 invoked by alias); 14 May 2013 15:27:38 -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 29934 invoked by uid 89); 14 May 2013 15:27:37 -0000 X-Spam-SWARE-Status: No, score=-8.0 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_PASS,TW_XM autolearn=ham version=3.3.1 Received: from mga03.intel.com (HELO mga03.intel.com) (143.182.124.21) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 14 May 2013 15:27:36 +0000 Received: from azsmga001.ch.intel.com ([10.2.17.19]) by azsmga101.ch.intel.com with ESMTP; 14 May 2013 08:27:06 -0700 X-ExtLoop1: 1 Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by azsmga001.ch.intel.com with ESMTP; 14 May 2013 08:27:05 -0700 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.193]) by IRSMSX101.ger.corp.intel.com ([169.254.1.127]) with mapi id 14.03.0123.003; Tue, 14 May 2013 16:26:50 +0100 From: "Metzger, Markus T" To: Jan Kratochvil CC: "gdb-patches@sourceware.org" , "Himpel, Christian" Subject: RE: [PATCH 02/15] btrace: change branch trace data structure Date: Tue, 14 May 2013 15:27:00 -0000 Message-ID: References: <1367496216-21217-1-git-send-email-markus.t.metzger@intel.com> <1367496216-21217-3-git-send-email-markus.t.metzger@intel.com> <20130513152450.GB29683@host2.jankratochvil.net> In-Reply-To: <20130513152450.GB29683@host2.jankratochvil.net> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2013-05/txt/msg00472.txt.bz2 > -----Original Message----- > From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com] > Sent: Monday, May 13, 2013 5:25 PM > To: Metzger, Markus T > Cc: gdb-patches@sourceware.org; Himpel, Christian > Subject: Re: [PATCH 02/15] btrace: change branch trace data structure >=20 > Hi Markus, >=20 > as mailed off-list you have an updated version so sending only a partial > review as I have done so far. Thanks for your review. [...] > > -/* Compute the function trace from the instruction trace. */ > > +/* Allocate and initialize a new branch trace function segment. */ >=20 > Something like: >=20 > +/* If both FUN and MFUN are NULL it is marker of the end of trace. */ I do not understand this comment. Actually, it turns out that we can have functions without even minimal symb= ol Information. See testsuite/gdb.btrace/unknown_functions.exp in a later pat= ch. [...] > > + bfun->insn_offset =3D prev->insn_offset > > + + VEC_length (btrace_insn_s, prev->insn); >=20 > GNU Coding Standards require multi-line expressions to use parentheses, > therefore: > bfun->insn_offset =3D (prev->insn_offset > + VEC_length (btrace_insn_s, prev->insn)); Thanks. I believe there are more instances of this around. [...] > > + if (VEC_empty (btrace_insn_s, bfun->insn)) > > + continue; >=20 > Shouldn't here be rather "return NULL" instead of continue? I do not > understand in which cases BFUN->INSN can be empty. Empty INSN case could= be > also described in btrace_function->insn. In the sense of this function, continue is the right thing to do. Neverthe= less, we currently do not generate such function segments and their handling is not consistent. They were intended to allow adding artificial frames later on = to represent for example inlined functions. I changed this into gdb_assert and added a comment to the declaration of struct btrace_fucntion. [...] > > + /* If we didn't have a function before, we create one. */ > > + if (bfun =3D=3D NULL) > > + return ftrace_new_function (bfun, mfun, fun); >=20 > mfun =3D=3D NULL && fun =3D=3D NULL is not allowed according to the comme= nts in struct > btrace_function. Already suggested gdb_assert for ftrace_new_function bu= t it > seems it may happen here. I updated the comment. See above for an example. [...] > > + /* Check the last instruction, if we have one. > > + We do this check first, since it allows us to fill in the call st= ack > > + links in addition to the normal flow links. */ > > + last =3D NULL; > > + if (!VEC_empty (btrace_insn_s, bfun->insn)) > > + last =3D VEC_last (btrace_insn_s, bfun->insn); > > > > - /* Let's see if we have source correlation, as well. */ > > - sal =3D find_pc_line (pc, 0); > > - if (sal.symtab =3D=3D NULL || sal.line =3D=3D 0) > > + if (last !=3D NULL) >=20 > This conditional is excessive, it is true iff !VEC_empty is true above. I still need to check either one of them again, here. Last is used also in the if statement below.... [...] > > + { > > + } > > + > > + /* Check if we're switching functions for some other reason. */ > > + if (ftrace_function_switched (bfun, mfun, fun)) > > + { [...] > > + if (last !=3D NULL) ...here. > > { > > - DEBUG_FTRACE ("ignoring file at %u, pc=3D%s, file=3D%s", idx, > > - core_addr_to_string_nz (pc), filename); > > - continue; > > + CORE_ADDR start, lpc; > > + > > + /* If we have symbol information for our current location, use > > + it to check that we jump to the start of a function. */ > > + if (fun !=3D NULL || mfun !=3D NULL) > > + start =3D get_pc_function_start (pc); > > + else > > + start =3D pc; >=20 > This goes into implementation detail of get_pc_function_start. Rather al= ways > call get_pc_function_start but one should check if it failed anyway > - get_pc_function_start returns 0 if it has failed. >=20 > Or was the 'fun !=3D NULL || mfun !=3D NULL' check there for performance = reasons? Without symbol information, we assume that we do jump to the beginning of a function. If we called get_pc_function_start unconditionally, we wouldn'= t be able to tell from a zero return whether we knew that we did not jump to the= start of a function or whether we just don't know. We already know that we switched functions (i.e. that the symbol information changed). I put in this PC check as another safeguard for detecting tail c= alls. [...] > > + /* Jumps indicate optimized tail calls. */ > > + if (start =3D=3D pc > > + && gdbarch_insn_jump_p_p (gdbarch) > > + && gdbarch_insn_jump_p (gdbarch, lpc)) > > + return ftrace_new_tailcall (bfun, mfun, fun); >=20 > Cannot a plain intra-function jump be confused into a tailcall due to > a tripped binary? FUN and MFUN are NULL for stripped binary, > therefore "start =3D pc;" gets assigned above, which will call > ftrace_new_tailcall. I did not try to reproduce it, though. Yes, but unlikely. We already had a change in symbol information, so we're jumping from a section with symbol information to one without (or vice versa or within a section with symbol information but for those get_pc_function_s= tart should return the proper address). [...] > > btrace =3D target_read_btrace (btinfo->target, btrace_read_new); > > - if (VEC_empty (btrace_block_s, btrace)) > > - return; > > - > > - btrace_clear (tp); > > + cleanup =3D make_cleanup (VEC_cleanup (btrace_block_s), &btrace); > > > > - btinfo->btrace =3D btrace; > > - btinfo->itrace =3D compute_itrace (btinfo->btrace); > > - btinfo->ftrace =3D compute_ftrace (btinfo->itrace); > > + if (!VEC_empty (btrace_block_s, btrace)) > > + { > > + btrace_clear (tp); > > + btrace_compute_ftrace (btinfo, btrace); > > + } >=20 > If BTRACE is empty shouldn't TP's btrace be still cleared? I'm calling target_read_btrace with flag btrace_read_new. It returns an empty block trace vector if the trace has not changed. In this case, the o= ld trace is still correct. [...] > > +enum btrace_function_flag > > +{ > > + /* The 'up' link interpretation. > > + If set, it points to the function segment we returned to. > > + If clear, it points to the function segment we called from. */ > > + bfun_up_links_to_ret =3D (1 << 0), > > + > > + /* The 'up' link points to a tail call. This obviously only makes s= ense > > + if bfun_up_links_to_ret is clear. */ > > + bfun_up_links_to_tailcall =3D (1 << 1) >=20 > enum values are uppercased in GDB. It appears to me that most enumeration constants are lowercase. If you insist, I'll make mine UPPERCASE, but I find lowercase more readable. > But I do not see a reason to have the flags field and this enum. You can= just > use something like > unsigned bfun_up_links_to_ret : 1; > unsigned bfun_up_links_to_tailcall : 1; >=20 > instead of the flags field in struct btrace_function. Sometimes one want= s to > pass FLAGS as a function parameter but that is not the case here. We don't know what other flags we might need. A flags bit-vector seems more flexible. [...] > > +struct btrace_function > > { > > /* The full and minimal symbol for the function. One of them may be= NULL. */ >=20 > IIUC from the code rather: >=20 > +/* Iff both FUN and MFUN are NULL it is marker of the end of trace. */ That changed, meanwhile. I removed the empty function as end marker and I = found exmples where both are NULL. [...] > > +/* Return the instruction number for a branch trace iterator. Returns= zero >=20 > s/the instruction number/index of the instruction relative to start of the > function/ I use the term "instruction number" in other places, as well. [...] > > struct gdbarch *gdbarch; > > - struct btrace_inst *inst; > > - unsigned int idx; > > + struct btrace_insn *inst; >=20 > Dead variable 'inst'. Thanks. Couldn't we make gcc warn about this? Regards, Markus. Intel GmbH Dornacher Strasse 1 85622 Feldkirchen/Muenchen, Deutschland Sitz der Gesellschaft: Feldkirchen bei Muenchen Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk Registergericht: Muenchen HRB 47456 Ust.-IdNr./VAT Registration No.: DE129385895 Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052