From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18058 invoked by alias); 28 Feb 2013 17:04:19 -0000 Received: (qmail 18048 invoked by uid 22791); 28 Feb 2013 17:04:16 -0000 X-SWARE-Spam-Status: No, hits=-7.2 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,TW_BJ,TW_BT X-Spam-Check-By: sourceware.org Received: from mga09.intel.com (HELO mga09.intel.com) (134.134.136.24) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 28 Feb 2013 17:04:05 +0000 Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 28 Feb 2013 09:02:38 -0800 X-ExtLoop1: 1 Received: from irsmsx104.ger.corp.intel.com ([163.33.3.159]) by orsmga002.jf.intel.com with ESMTP; 28 Feb 2013 09:04:03 -0800 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.108]) by IRSMSX104.ger.corp.intel.com ([169.254.5.48]) with mapi id 14.01.0355.002; Thu, 28 Feb 2013 17:04:00 +0000 From: "Metzger, Markus T" To: Jan Kratochvil CC: "gdb-patches@sourceware.org" , "markus.t.metzger@gmail.com" Subject: RE: [PATCH 1/3] record, btrace: add record-btrace target Date: Thu, 28 Feb 2013 17:17:00 -0000 Message-ID: References: <1361808917-16934-1-git-send-email-markus.t.metzger@intel.com> <1361808917-16934-2-git-send-email-markus.t.metzger@intel.com> <20130227073731.GA1378@host2.jankratochvil.net> In-Reply-To: <20130227073731.GA1378@host2.jankratochvil.net> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes 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 X-SW-Source: 2013-02/txt/msg00748.txt.bz2 > -----Original Message----- > From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourcewa= re.org] On Behalf Of Jan Kratochvil > Sent: Wednesday, February 27, 2013 8:38 AM Thanks for your review! > On Mon, 25 Feb 2013 17:15:15 +0100, markus.t.metzger@intel.com wrote: > > The target implements the new record sub-commands > > "record instruction-history" and > > "record function-call-history". > > > > The target does not support reverse execution or navigation in the > > recorded execution log. >=20 > When you do not plan now to support either "record source-lines-history" = or > "reverse execution or navigation in the recorded execution log" it won't = be > supported in any form in gdb-7.6? Do you plan to implement it afterwards? Yes. With your help, I hope, on the reverse execution part. The "record source-lines-history" command needs some experimentation and feedback to get non-code source lines right and also to suppress the noise = of instruction scheduling. > > +/* A recorded function segment. */ > > +struct btrace_function > > +{ > > + /* The function symbol. */ > > + struct minimal_symbol *mfun; > > + struct symbol *fun; >=20 > When is one of them NULL or may be NULL? I think I should always have a minimal symbol. I may not always have a full symbol. For example, when there is no debug information. The algorithm will also work with a full but no minimal symbol, so I added a comment saying that one of them may be NULL. > > + > > + /* The name of the function. */ > > + const char *function; >=20 > Which of the forms SYMBOL_LINKAGE_NAME, SYMBOL_DEMANGLED_NAME, > SYMBOL_PRINT_NAME etc.? This field has been removed in response to other comments below. > > + > > + /* The name of the file in which the function is defined. */ > > + const char *filename; >=20 > Absolute source filename? Or relative to compilation directory? In response to other comments below, this has been changed to the full name for full symbols and to whatever a minimal symbol provides. > > + btinfo->btrace =3D target_read_btrace (btinfo->target); >=20 > In btrace.c/update_btrace from archer-mmetzger-btrace: > btp->btrace =3D target_read_btrace (btp->target); > without freeing btp->btrace there beforehand, does it leak there? Yes. I have updated the archer branch before sending this patch series, though.=20 =20 > And gdb/ has only two calls of target_btrace_has_changed and > target_read_btrace and both are called this similar way. >=20 > Couldn't just be always called just target_read_btrace and the target wou= ld > return some error if it has not changed? This also reduces one gdbserver > round-trip-time for the read. I don't think we should give an error if GDB wants to read the trace twice. This extra call to target_btrace_has_changed was meant to save the time of repeatedly sending and processing the same trace. What we could do is free the trace when the target continues (or stops) and set the trace pointers to NULL. Would that be better? If yes, is there already some notifier or hook that I could use? > > + > > + /* The first block ends at the current pc. */ >=20 > I do not understand here why the target does not know the current PC and = does > not set it up on its own. I wanted to avoid the #ifdef GDBSERVER in the common code. I added a separate patch to fill this in when reading the branch trace in common/linux-btrace.c. > > +static struct btrace_thread_info * > > +require_btrace (void) > > +{ > > + struct thread_info *tp; > > + struct btrace_thread_info *btinfo; > > + > > + DEBUG_FUN ("require trace"); > > + > > + tp =3D find_thread_ptid (inferior_ptid); >=20 > When possible/feasible we try to no longer use inferior_ptid in GDB, make= it > a ptid_t parameter instead. This function is called from command functions to get the branch trace for the current thread. At some point, I do need to get the current thread. If I made ptid_t a parameter here, I would have to get the current ptid in all callers. > > +static void > > +btrace_insn_history (struct btrace_thread_info *btinfo, struct ui_out = *uiout, > > + unsigned int begin, unsigned int end, int flags) > > +{ > > + struct gdbarch *gdbarch; > > + unsigned int idx; > > + > > + DEBUG_FUN ("itrace (0x%x): [%u; %u[", flags, begin, end); > > + > > + gdbarch =3D target_gdbarch (); > > + > > + for (idx =3D begin; idx < end; ++idx) >=20 > btrace_function->{begin,end} are inclusive but these are apparently begin > inclusive but end exclusive parameters. The [begin; end] range in struct btrace_function refers to source lines. The [begin; end[ range here refers to indices into the instruction trace ve= ctor. > > + /* Initialize file and function name based on the information we hav= e. */ > > + if (fun !=3D NULL) > > + { > > + bfun->filename =3D symtab_to_filename_for_display (fun->symtab); >=20 > Do not store the result of symtab_to_filename_for_display, it is intended= only > for immediate use - its output may change by "set filename-display". Mor= eover > the result is _for_display - so not for any comparisons. >=20 > You can store fun->symtab itself. But in such case one needs to hook > a function to free_objfile like there is now hooked breakpoint_free_objfi= le so > that there do not remain stale symtab pointers. I can compute the filename when I intend to print a line, so I don't need t= o store the symtab pointer separately. I changed it to obtain the filename via a call to symtab_to_fullname. I sto= re that while I traverse the instruction trace vector so I can compare it to other = filenames. Would I need to duplicate the string or is it safe to store it as-is for my= purpose? > > + bfun->function =3D SYMBOL_PRINT_NAME (fun); >=20 > Do not store the output of SYMBOL_PRINT_NAME. You can store FUN itself (= in > such case sure you no longer need to store fun->symtab as suggested above= ). > Again you need a hook in free_objfile to prevent stale FUN pointers. I changed it to compute the function name whenever I want to print it. I'm only storing symbol and minimal symbol pointers for computing the output of "record function-call-history". I guess I won't need the hook in that ca= se. > > + } > > + else if (mfun !=3D NULL) > > + { > > + bfun->filename =3D mfun->filename; >=20 > mfun->filename is not used in GDB, it is just a basename available only in > some cases, for minimal symbols GDB does not know their source filename. I use it only for functions for which I do not have a full symbol. The alte= rnative would be to not provide a filename in that case. Would that be preferable? An example would be library functions for which we don't have debug info. > On Wed, 27 Feb 2013 08:37:31 +0100, Jan Kratochvil wrote: > > > +/* Check if we should skip this file when generating the function ca= ll > > > + history. > > > + Update the recorded function segment. */ > > > > I do not see a reason for this functionality. There really may be a va= lid one > > but I do not see it. Could you provide an example in the comment when = it is > > useful? I cannot much review it when I do not see what is it good for. > > > > > > > + > > > +static int > > > +btrace_function_skip_file (struct btrace_function *bfun, > > > + const char *filename) >=20 >=20 > Probably if one does: >=20 > file.c: > void func(void) > { > #include "file.def" > } Suppose you're using a macro in func that has been defined in another file. I added a comment for the macro. > Then we want a single output line for file.c:func() without another output > line for file.def:func(). (It is not tested by the testsuite; OK.) >=20 > Therefore you apparently do not want to include the line numbers range of > file.def:func() into the range of lines of file.c:func(), that makes sens= e. >=20 > So my comments are valid, just use filename_cmp and compare symtab_to_ful= lname > of both symtabs (this is sub-optimal, comparing two source files for > equivalence can be made cheaper than finding out their full pathname; but= that > is an optimization for another patch). OK. I would still use symtab_to_filename_for_display when displaying the fi= lename, right? So I would store the fullname for the above comparison and compute the file= name to display when printing the line. What would I do if I only have a minimal symbol? Can I still use ->filename= for this check? See above for a related question on printing filenames. > > + /* Check if we're still in the same file. */ > > + if (compare_filenames_for_search (bfun->filename, filename)) > > + return 0; >=20 > Use filename_cmp for full pathnames. Therefore always use symtab_to_full= name. > The result of symtab_to_fullname call as 'const char *fullname'. Can I store the returned pointer? Would I need to duplicate the string? > > + ui_out_field_string (uiout, "file", filename); >=20 > Just do not print anything (no "filename:" prefix) for minimal symbols; a= lso > omit the ui_out_field_string call so that in (future) MI the field "file"= just > does not exist. That might affect a lot of library code for which we don't have debug info. > > + bfun =3D VEC_last (btr_fun_s, *bt); > > + else > > + bfun =3D NULL; > > + > > + /* If we're switching functions, we start over. */ > > + if (bfun =3D=3D NULL || (fun !=3D bfun->fun && mfun !=3D bfun->mfun)) >=20 > One should compare them using strcmp_iw for SYMBOL_PRINT_NAME strings of = the > functions. >=20 > 'struct symbol *' will differ for example for an inlined function in two > different compilation units (=3Dtwo different symtabs). >=20 > Besides SYMBOL_PRINT_NAME one should also use filename_cmp to compare > symtab_to_fullname of both locations so that file1.c:staticfunc and > file2.c:staticfunc get recognized as different functions (as you also pri= nt > the 'file1.c:' prefix in the output). OK. I assume that a full symbol will always have a symtab. Is this correct? > > + > > + /* Do this check first to make sure we get a filename even if we= don't > > + have line information. */ > > + if (btrace_function_skip_file (bfun, filename)) >=20 > But I do not see a reason for this function as written at > btrace_function_skip_file. I believe this has been clarified, meanwhile. See above. 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