This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch v9 01/23] thread, btrace: add generic branch trace support
On Mon, 04 Mar 2013 18:05:48 +0100, Markus Metzger wrote:
> Add branch trace information to struct thread_info to hold the branch trace
> information for that thread.
>
> Add functions to enable, disable, clear, and fetch a thread's branch trace.
>
> 2013-03-04 Markus Metzger <markus.t.metzger@intel.com>
>
> * target.h: Include btrace.h.
> (struct target_ops): Add btrace ops.
> * target.c (update_current_target): Initialize btrace ops.
> (target_supports_btrace): New function.
> (target_enable_btrace): New function.
> (target_disable_btrace): New function.
> (target_read_btrace): New function.
> (target_btrace_has_changed): New function.
> * btrace.h: New file.
> * btrace.c: New file.
> * Makefile.in: Add btrace.c.
> * gdbthread.h: Include btrace.h.
> (struct thread_info): Add btrace field.
> * thread.c: Include btrace.h.
> (clear_thread_inferior_resources): Call btrace_disable.
> * infcmd.c: Include btrace.h.
> (detach_command): Call btrace_disconnect.
> * common/btrace-common.h: New file.
[...]
> --- /dev/null
> +++ b/gdb/btrace.c
> @@ -0,0 +1,467 @@
> +/* Branch trace support for GDB, the GNU debugger.
> +
> + Copyright (C) 2012 Free Software Foundation, Inc.
Please grep all files so that only 2013 is present in new files.
[...]
> +/* Return the file name of a recorded function segment for printing.
> + This function never returns NULL. */
> +
> +static const char *
> +ftrace_print_filename (struct btrace_func *bfun)
> +{
> + struct minimal_symbol *msym;
> + struct symbol *sym;
> + const char *filename;
> +
> + msym = bfun->msym;
> + sym = bfun->sym;
> +
> + if (sym != NULL)
> + filename = symtab_to_filename_for_display (sym->symtab);
> + else if (msym != NULL)
> + {
> + filename = msym->filename;
> + if (filename == NULL || *filename == 0)
> + filename = "<unknown>";
Please never use minimal_symbol->filename (as I already asked for). It is
there only for some STABS compatibility, it works only for static (not global)
ELF symbols, during a small test it reports for main.c file 'crtstuff.c'
(where it really is not from).
GDB does not currently try to print any filenames for minimal symbols:
Temporary breakpoint 1, 0x00000000004004fa in main ()
vs.
Temporary breakpoint 1, main () at msym.c:3
If you have some reason to start printing filenames for minimal symbols it
needs to be a GDB global effort and it is a completely separate patchset.
And last but not least in general minimal_symbol->filename is never useful.
It is present only for file static symbols, not for global symbols. Therefore
the binary must not be stripped to have such file static ELF symbol at all.
But if it is not stripped there will be (in most cases) full DWARF.
If you want to make an excuse it is used only for DEBUG_FTRACE then this
function needs to be called as a debug function, like
ftrace_print_filename_for_debug. But even inthis case I do not
minimal_symbol->filename printing acceptable for reasons above.
> + }
> + else
> + filename = "<unknown>";
> +
> + return filename;
> +}
> +
> +/* Print an ftrace debug status message. */
> +
> +static void
> +ftrace_debug (struct btrace_func *bfun, const char *prefix)
> +{
> + DEBUG_FTRACE ("%s: fun = %s, file = %s, lines = [%d; %d], insn = [%u; %u]",
> + prefix, ftrace_print_function_name (bfun),
> + ftrace_print_filename (bfun), bfun->lbegin, bfun->lend,
> + bfun->ibegin, bfun->iend);
> +}
> +
> +/* Initialize a recorded function segment. */
> +
> +static void
> +ftrace_init_func (struct btrace_func *bfun, struct minimal_symbol *mfun,
> + struct symbol *fun, unsigned int idx)
> +{
> + bfun->msym = mfun;
> + bfun->sym = fun;
> + bfun->lbegin = INT_MAX;
> + bfun->lend = 0;
> + bfun->ibegin = idx;
> + bfun->iend = idx;
> +}
> +
> +/* Check whether the function has changed. */
> +
> +static int
> +ftrace_function_switched (struct btrace_func *bfun,
> + struct minimal_symbol *mfun, struct symbol *fun)
> +{
> + struct minimal_symbol *msym;
> + struct symbol *sym;
> +
> + /* The function changed if we did not have one before. */
> + if (bfun == NULL)
> + return 1;
> +
> + msym = bfun->msym;
> + sym = bfun->sym;
> +
> + /* If the minimal symbol changed, we certainly switched functions. */
> + if (mfun != NULL && msym != NULL)
> + {
> + const char *bfname, *fname;
> +
> + /* Check the function name. */
> + if (strcmp_iw (SYMBOL_PRINT_NAME (mfun),
> + SYMBOL_PRINT_NAME (msym)) != 0)
In fact I suggested it wrongly before, not realizing/seeing it whole.
Here could be rather SYMBOL_LINKAGE_NAME. For example various ctor/dtors
kinds like gnu_v3_base_object_ctor are not visible in SYMBOL_PRINT_NAME (nor
SYMBOL_SEARCH_NAME) but they are visible in SYMBOL_LINKAGE_NAME. But for
SYMBOL_LINKAGE_NAME one should use strcmp (not strcmp_iw).
If you would like to compare demangled names then strcmp_iw would be OK but
then rather SYMBOL_SEARCH_NAME.
But still rather strcmp and SYMBOL_LINKAGE_NAME.
> + return 1;
> +
> + /* Check the location of those functions, as well. */
> + bfname = msym->filename;
> + fname = mfun->filename;
> + if (fname != NULL && bfname != NULL
> + && filename_cmp (fname, bfname) != 0)
> + return 1;
This block should be deleted, minimal_symbol->filename is unsupported.
> + }
> +
> + /* If the symbol changed, we certainly switched functions. */
> + if (fun != NULL && sym != NULL)
> + {
> + const char *bfname, *fname;
> +
> + /* Check the function name. */
> + if (strcmp_iw (SYMBOL_PRINT_NAME (fun),
> + SYMBOL_PRINT_NAME (sym)) != 0)
Change like above.
> + return 1;
> +
> + /* Check the location of those functions, as well. */
> + bfname = symtab_to_fullname (sym->symtab);
> + fname = symtab_to_fullname (fun->symtab);
> + if (filename_cmp (fname, bfname) != 0)
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +/* Check if we should skip this file when generating the function call
> + history. We would want to do that if, say, a macro that is defined
> + in another file is expanded in this function. */
> +
> +static int
> +ftrace_skip_file (struct btrace_func *bfun, const char *filename)
> +{
> + struct minimal_symbol *msym;
> + struct symbol *sym;
> + const char *bfile;
> +
> + msym = bfun->msym;
> + sym = bfun->sym;
> +
> + if (sym != NULL)
> + bfile = symtab_to_fullname (sym->symtab);
> + else if (msym != NULL && msym->filename != NULL)
> + bfile = msym->filename;
This block should be removed, minimal_symbol->filename is unsupported, present
AFAIK only for STABS compatibility.
[...]
> diff --git a/gdb/target.c b/gdb/target.c
> index ecb1325..9dfbe08 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -4147,6 +4147,75 @@ target_ranged_break_num_registers (void)
> return -1;
> }
>
> +/* See target.h. */
BTW empty line here and in all the function definition cases below, see:
http://sourceware.org/gdb/wiki/JoelsCodingStyleCheatSheet#Empty_line_between_subprogram_description_and_the_subprogram_implementation
> +int
> +target_supports_btrace (void)
> +{
> + struct target_ops *t;
> +
> + for (t = current_target.beneath; t != NULL; t = t->beneath)
> + if (t->to_supports_btrace != NULL)
> + return t->to_supports_btrace ();
> +
> + return 0;
> +}
[...]
OK to check it in with the requested changes, those are straight to implement.
Thanks,
Jan