From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5722 invoked by alias); 10 Sep 2013 09:11:22 -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 5710 invoked by uid 89); 10 Sep 2013 09:11:21 -0000 Received: from mga11.intel.com (HELO mga11.intel.com) (192.55.52.93) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 10 Sep 2013 09:11:21 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_40,KHOP_THREADED,RDNS_NONE,SPF_SOFTFAIL autolearn=no version=3.3.2 X-HELO: mga11.intel.com Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 10 Sep 2013 02:11:18 -0700 X-ExtLoop1: 1 Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by fmsmga002.fm.intel.com with ESMTP; 10 Sep 2013 02:10:35 -0700 Received: from irsmsx153.ger.corp.intel.com (163.33.192.75) by IRSMSX101.ger.corp.intel.com (163.33.3.153) with Microsoft SMTP Server (TLS) id 14.3.123.3; Tue, 10 Sep 2013 10:10:34 +0100 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.69]) by IRSMSX153.ger.corp.intel.com ([169.254.9.47]) with mapi id 14.03.0123.003; Tue, 10 Sep 2013 10:10:34 +0100 From: "Metzger, Markus T" To: Jan Kratochvil CC: "gdb-patches@sourceware.org" , "Himpel, Christian" Subject: RE: [patch v4 03/24] btrace: change branch trace data structure Date: Tue, 10 Sep 2013 09:11:00 -0000 Message-ID: References: <1372842874-28951-1-git-send-email-markus.t.metzger@intel.com> <1372842874-28951-4-git-send-email-markus.t.metzger@intel.com> <20130818190426.GC24153@host2.jankratochvil.net> In-Reply-To: <20130818190426.GC24153@host2.jankratochvil.net> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2013-09/txt/msg00324.txt.bz2 > -----Original Message----- > From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com] > Sent: Sunday, August 18, 2013 9:04 PM Thanks for your review. > > @@ -248,89 +185,477 @@ ftrace_skip_file (struct btrace_func *bfun, const > char *filename) > > else > > bfile =3D ""; > > > > - if (filename =3D=3D NULL) > > - filename =3D ""; > > + if (fullname =3D=3D NULL) > > + fullname =3D ""; >=20 > The code should not assume FULLNAME cannot be "", "" is theoretically a > valid > source file filename. >=20 > Second reason is that currently no caller of ftrace_skip_file will pass N= ULL > as the second parameter. >=20 > So the function can be just: >=20 > if (sym =3D=3D NULL) > return 1; >=20 > bfile =3D symtab_to_fullname (sym->symtab); >=20 > return filename_cmp (bfile, fullname) !=3D 0; Sounds good. Changed it. > And the function has only one caller so it would be IMO easier to read to= get > it inlined. I'd rather keep it separate for documentation purposes. > And not sure if it matters much but doing two symtab_to_fullname for > comparison of two symtabs equality is needlessly expensive > - symtab_to_fullname is very expensive. There are several places in GDB > doing first: > /* Before we invoke realpath, which can get expensive when many > files are involved, do a quick comparison of the basenames. = */ > if (!basenames_may_differ > && filename_cmp (lbasename (symtab1->filename), > lbasename (symtab2->filename)) !=3D 0) > continue; I would expect it does matter. I noticed a slowdown when the trace gets in the order of 1M instructions. I have not done any profiling, yet, but I= will get back to this once I look into performance. > > +static void > > +ftrace_update_caller (struct btrace_function *bfun, > > + struct btrace_function *caller, > > + unsigned int flags) >=20 > FLAGS should be enum btrace_function_flag (it is ORed bitmask but GDB > displays > enum ORed bitmasks appropriately). Changed it. This will burn us when we want to switch to C++ someday. > > +/* Fix up the caller for a function segment. */ >=20 > IIUC it should be: >=20 > /* Fix up the caller for all segments of a function call. */ Thanks. Yes, that's how it should be. > > + /* We maintain levels for a series of returns for which we have > > + not seen the calls, but we restart at level 0, otherwise. */ > > + bfun->level =3D min (0, prev->level) - 1; >=20 > Why is there the 'min (0, ' part? When we return from some tail call chain, for example, and we have not traced the actual function call that started this chain. I added a reference to tail calls in the comment. > > - bfun =3D VEC_safe_push (btrace_func_s, ftrace, NULL); > > + /* There is a call in PREV's back trace to which we should have > > + returned. Let's remain at this level. */ > > + bfun->level =3D prev->level; >=20 > Shouldn't here be rather: > bfun->level =3D caller->level; We did not return to this caller - otherwise, we would have found it before. This is handling a case that should not normally occur. No matter what we do, the indentation will likely be off in one or the other case. > > + /* 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 > always > call get_pc_function_start but one should check if it failed in all cases > (you do not check if get_pc_function_start failed). get_pc_function_start > returns 0 if it has failed. The check is implicit since pc can't be zero. > Or was the 'fun !=3D NULL || mfun !=3D NULL' check there for performance > reasons? That's for performance reasons. No need to call the function if we know it won't help us. > > +static void > > +btrace_compute_ftrace (struct btrace_thread_info *btinfo, > > + VEC (btrace_block_s) *btrace) >=20 > When doing any non-trivial trace on buggy Nehalem (enabling btrace by a > GDB > patch) GDB locks up on "info record". I found it is looping in this func= tion > with too big btrace range: > (gdb) p *block > $5 =3D {begin =3D 4777824, end =3D 9153192} >=20 > But one can break it easily with CTRL-C and hopefully on btrace-correct C= PUs > such things do not happen. We should not normally get such trace. I could add a simple heuristic that blocks can't be bigger than x bytes but I don't think that's necessary. > > +const struct btrace_insn * > > +btrace_insn_get (const struct btrace_insn_iterator *it) > > +{ > > + const struct btrace_function *bfun; > > + unsigned int index, end; > > + > > + if (it =3D=3D NULL) > > + return NULL; >=20 > I do not see this style in GDB and IMO it can delay bug report from where= it > occured. Either gdb_assert (it !=3D NULL); or just to leave it crashing = below. OK. It's just a habit. > > + index =3D it->index; > > + bfun =3D it->function; > > + if (bfun =3D=3D NULL) > > + return NULL; >=20 > btrace_insn_iterator::function does not state if NULL is allowed and its > meaning in such case. btrace_call_get description states "NULL if the > interator points past the end of the branch trace." but I do not see it c= ould > be set to NULL in any current code (expecting it was so in older code). > btrace_insn_next returns the last instruction not the last+1 pointer. >=20 > IMO it should be stated btrace_insn_iterator::function can never be NULL > and > here should be either gdb_assert (bfun !=3D NULL); or just nothing, like = above. Done. That's indeed a leftover. Thanks for pointing it out. > > + > > + btinfo =3D it->btinfo; > > + if (btinfo =3D=3D NULL) > > + return 0; >=20 > Similiarly btrace_call_iterator::btinfo does not state if it can be NULL = and > consequently here to code should rather gdb_assert it (or ignore it all). OK. I ignored it. > > + bfun =3D it->function; > > + if (bfun !=3D NULL) > > + return bfun->number; >=20 > Similiarly btrace_call_iterator::function does not state if it can be NUL= L and > consequently here to code should rather gdb_assert it (or ignore it all). For the call iterator, function can be NULL (see e.g. btrace_call_end). It is documented in btrace.h (maybe I added this afterwards). > > + > > + /* The branch trace function segment. > > + This will be NULL for the iterator pointing to the end of the tra= ce. */ >=20 > btrace_call_next can return NULL in function while btrace_insn_next retur= ns > rather the very last of all instructions. Is there a reason for this > difference? The end iterator points one past the last element. For instructions, this = is one past the last instruction index in the last (non-empty) function segmen= t. For calls, this is a NULL function segment. > > uiout =3D current_uiout; > > uiout_cleanup =3D make_cleanup_ui_out_tuple_begin_end (uiout, > > "insn history"); > > - btinfo =3D require_btrace (); > > - last =3D VEC_length (btrace_inst_s, btinfo->itrace); > > + low =3D (unsigned int) from; > > + high =3D (unsigned int) to; >=20 > I do not see a reason for this cast, it is even not signed vs. unsigned. =46rom and to are ULONGEST which may be 64bit whereas low and high are 32bit. > > - if (end <=3D begin) > > + if (high <=3D low) > > error (_("Bad range.")); >=20 > Function description says: > /* Disassemble a section of the recorded execution trace from instruc= tion > BEGIN (inclusive) to instruction END (exclusive). */ >=20 > But it beahves as if END was inclusive. Or I do not understand something? > (gdb) record instruction-history 1925,1926 > 1925 0x00007ffff62f6afc : ja 0x7ffff62f6b30 > > 1926 0x00007ffff62f6afe : cmp $0x10,%rdx >=20 > If it should be inclusive then LOW =3D=3D HIGH should be allowed: > (gdb) record instruction-history 1925,1925 > Bad range. >=20 > Not in this patch (in some later one) but there is also: > /* We want both begin and end to be inclusive. */ > btrace_insn_next (&end, 1); >=20 > which contradicts the description of to_insn_history_range. Initially, I had it exclusive, and this should be the behaviour if you just= apply this patch. Later on, I changed it to be inclusive, instead, to better mat= ch existing commands like list. I fixed this behaviour in the respective patch, added a test, and updated the comment in target.h. > Unrelated to this patch but the function record_btrace_insn_history_from > does > not need to be virtualized. It does not access any internals of > record-btrace.c, it could be fully implemented in the superclass record.c= and > to_insn_history_from could be deleted. >=20 > The same applies for record_btrace_call_history_from and > to_call_history_from. Both depend on the numbering scheme, which is an implementation detail. They both assume that counting starts at 0 (at 1 in a later patch). This does not hold for record-full, where the lowest instruction may be bigger than zero. > > - if (end <=3D begin) > > + if (high <=3D low) > > error (_("Bad range.")); >=20 > Similar inclusive/exclusive question as in record_btrace_insn_history_ran= ge > and > to_insn_history_range. >=20 > /* We want both begin and end to be inclusive. */ > btrace_call_next (&end, 1); >=20 > (gdb) record function-call-history 700,701 > 700 _dl_lookup_symbol_x > 701 _dl_fixup > (gdb) record function-call-history 700,700 > Bad range. I fixed this behaviour in the respective patch, added a test, and updated the comment in target.h. 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