RE: [PATCH 1/3] record, btrace: add record-btrace target

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.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.
> 
> 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;
> 
> 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;
> 
> 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;
> 
> 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 = target_read_btrace (btinfo->target);
> 
> In btrace.c/update_btrace from archer-mmetzger-btrace:
> 	btp->btrace = 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. 

 
> And gdb/ has only two calls of target_btrace_has_changed and
> target_read_btrace and both are called this similar way.
> 
> Couldn't just be always called just target_read_btrace and the target would
> 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.  */
> 
> 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 = find_thread_ptid (inferior_ptid);
> 
> 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 = target_gdbarch ();
> > +
> > +  for (idx = begin; idx < end; ++idx)
> 
> 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 vector.


> > +  /* Initialize file and function name based on the information we have.  */
> > +  if (fun != NULL)
> > +    {
> > +      bfun->filename = symtab_to_filename_for_display (fun->symtab);
> 
> 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".  Moreover
> the result is _for_display - so not for any comparisons.
> 
> 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_objfile 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 to store
the symtab pointer separately.

I changed it to obtain the filename via a call to symtab_to_fullname. I store 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 = SYMBOL_PRINT_NAME (fun);
> 
> 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 case.


> > +    }
> > +  else if (mfun != NULL)
> > +    {
> > +      bfun->filename = mfun->filename;
> 
> 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 alternative
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 call
> > > +   history.
> > > +   Update the recorded function segment.  */
> >
> > I do not see a reason for this functionality.  There really may be a valid 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)
> 
> 
> Probably if one does:
> 
> 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.)
> 
> 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 sense.
> 
> So my comments are valid, just use filename_cmp and compare symtab_to_fullname
> 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 filename, right?

So I would store the fullname for the above comparison and compute the filename 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;
> 
> Use filename_cmp for full pathnames.  Therefore always use symtab_to_fullname.
> 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);
> 
> Just do not print anything (no "filename:" prefix) for minimal symbols; also
> 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 = VEC_last (btr_fun_s, *bt);
> > +  else
> > +    bfun = NULL;
> > +
> > +  /* If we're switching functions, we start over.  */
> > +  if (bfun == NULL || (fun != bfun->fun && mfun != bfun->mfun))
> 
> One should compare them using strcmp_iw for SYMBOL_PRINT_NAME strings of the
> functions.
> 
> 'struct symbol *' will differ for example for an inlined function in two
> different compilation units (=two different symtabs).
> 
> 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 print
> 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))
> 
> 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