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

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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"
}

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).





> > +{
> > +  /* Update the filename in case we didn't get it from the function symbol.  */
> > +  if (bfun->filename == NULL)
> > +    {
> > +      DEBUG_CALL ("file: '%s'", filename);
> > +
> > +      bfun->filename = filename;
> > +      return 0;
> > +    }
> > +
> > +  /* 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'.
> 
> 
> > +
> > +  /* We switched files, but not functions.  Skip this file.  */
> > +  DEBUG_CALL ("ignoring file '%s' for %s in %s.", filename,
> > +	      bfun->function, bfun->filename);
> > +  return 1;
> > +}


[...]
> > +  /* Let's see if we have source correlation, as well.  */
> > +  line = find_pc_line (ip, 0);
> 
> Such variable is commonly called 'sal' (source-and-line) in GDB.
> 
> 
> > +  if (line.symtab != NULL)
> > +    {
> > +      const char *filename;
> > +
> > +      /* Without a filename, we ignore this instruction.  */
> > +      filename = symtab_to_filename_for_display (line.symtab);
> > +      if (filename == NULL)
> > +	return;
> 
> As you have non-NULL symtab here you always have non-NULL filename.
> 
> You should use symtab_to_fullname here as discussed elsewhere.
> 
> 
> > +
> > +      /* 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.
> 
> 
> > +	return;
> > +
> > +      if (line.line == 0)
> > +	return;
> > +
> > +      btrace_function_update_lines (bfun, line.line, ip);
> > +    }
> > +}


Jan