[PATCH 1/3] record, btrace: add record-btrace target
Jan Kratochvil
jan.kratochvil@redhat.com
Wed Feb 27 19:43:00 GMT 2013
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
More information about the Gdb-patches
mailing list