Re: [patch v9 19/23] record, btrace: add record-btrace target

Jan Kratochvil
2013-03-05T20:13:24Z

On Mon, 04 Mar 2013 18:06:06 +0100, Markus Metzger wrote:
> Add a target for branch trace recording. This will replace the btrace
> commands added earlier in the patch series.
>
> 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.
>
> 2013-03-04 Markus Metzger <markus.t.metzger@intel.com>
>
> * Makefile.in (SFILES): Add record-btrace.c
> (COMMON_OBS): Add record-btrace.o
> * record-btrace.c: New.
> * objfiles.c: Include btrace.h.
> (free_objfile): call btrace_free_objfile.
[...]
> --- /dev/null
> +++ b/gdb/record-btrace.c
[...]
> +/* The to_open method of target record-btrace. */
> +
> +static void
> +record_btrace_open (char *args, int from_tty)
> +{
> + struct cleanup *disable_chain;
> + struct thread_info *tp;
> +
> + DEBUG ("open");
> +
> + if (RECORD_IS_USED)
> + error (_("The process is already being recorded."));
> +
> + if (!target_has_execution)
> + error (_("The program is not being run."));
> +
> + if (!target_supports_btrace ())
> + error (_("Target does not support branch tracing."));
> +
> + gdb_assert (record_btrace_thread_observer == NULL);
> +
> + disable_chain = make_cleanup (null_cleanup, NULL);
> + ALL_THREADS (tp)
> + if (args == NULL || *args == 0 || number_is_in_list (args, tp->num))
> + {
> + btrace_enable (tp);
> +
> + (void) make_cleanup (record_btrace_disable_callback, tp);

Needless cast. If it is required by your GCC warnings compatible with current
GDB codebase where it makes sense please propose them.

> + }
> +
> + record_btrace_auto_enable ();
> +
> + push_target (&record_btrace_ops);
> +
> + observer_notify_record_changed (current_inferior (), 1);
> +
> + discard_cleanups (disable_chain);
> +}
[...]
> +/* Print the source line information for a function call history line. */
> +
> +static void
> +btrace_func_history_src_line (struct ui_out *uiout, struct btrace_func *bfun)
> +{
> + struct minimal_symbol *msym;
> + struct symbol *sym;
> + const char *filename;
> +
> + msym = bfun->msym;
> + sym = bfun->sym;
> +
> + filename = NULL;
> + if (sym != NULL)
> + filename = symtab_to_filename_for_display (sym->symtab);

> + else if (msym != NULL)
> + filename = msym->filename;

No filename from minimal_symbol.

It prints for me bogus 'interp.c' there:

0 genops.c:456-486 __GI__IO_default_xsputn
1 vfprintf.c:1635-1662 _IO_vfprintf_internal
2 interp.c strchrnul
3 vfprintf.c:1662-1666 _IO_vfprintf_internal

One can always find line number information two ways.

here you use the symbol's line. But sometimes it is not available:
(gdb) p strchrnul
$3 = {<text variable, no debug info>} 0x7ffff6311480 <strchrnul>

On the other hand line number information (sal - source-and-line) may be
available even for such symbol:
(gdb) info line strchrnul
Line 26 of "/usr/src/debug/glibc-2.17-c758a686/sysdeps/x86_64/strchrnul.S" starts at address 0x7ffff6311480 <strchrnul> and ends at 0x7ffff6311484 <strchrnul+4>.

When you display ranges of lines I would find more natural to use the sal's
filename, it would also provide more rich info for the .S files.

It can be also filed as a PR so it does not block gdb-7.6; I wanted to get the
user interface final for the release.

> +
> + if (filename == NULL || *filename == 0)
> + return;
> +
> + ui_out_field_string (uiout, "file", filename);
> +
> + if (bfun->lend == 0)
> + return;
> +
> + ui_out_text (uiout, ":");
> + ui_out_field_int (uiout, "min line", bfun->lbegin);
> +
> + if (bfun->lend == bfun->lbegin)
> + return;
> +
> + ui_out_text (uiout, "-");
> + ui_out_field_int (uiout, "max line", bfun->lend);
> +}
> +
> +/* Disassemble a section of the recorded function trace. */
> +
> +static void
> +btrace_func_history (struct btrace_thread_info *btinfo, struct ui_out *uiout,
> + unsigned int begin, unsigned int end,
> + enum record_print_flag flags)
> +{
> + struct btrace_func *bfun;
> + unsigned int idx;
> +
> + DEBUG ("ftrace (0x%x): [%u; %u[", flags, begin, end);
> +
> + for (idx = begin; VEC_iterate (btrace_func_s, btinfo->ftrace, idx, bfun)
> + && idx < end; ++idx)
> + {
> + /* Print the function index. */
> + ui_out_field_uint (uiout, "index", idx);
> + ui_out_text (uiout, "\t ");

Why here

> +
> + if ((flags & record_print_insn_range) != 0)
> + {
> + btrace_func_history_insn_range (uiout, bfun);
> + ui_out_text (uiout, "\t");
> + }
> +
> + if ((flags & record_print_src_line) != 0)
> + {
> + btrace_func_history_src_line (uiout, bfun);
> + ui_out_text (uiout, "\t ");

and here is trailing space (' ')?

BTW maybe you could also align the columns by auto-sizing them, GDB does
auto-sizing (each place on its own, no utility for it) of various text tables
output.

0 ./common/common-utils.c:124 xstrprintf
1 ./cli/cli-decode.c:450-451 add_setshow_cmd_full
2 ./common/common-utils.c:116-121 xstrprintf
3 ./common/common-utils.c:128-130 xstrvprintf
4 vasprintf@plt
5 vasprintf.c:39-50 _IO_vasprintf
6 memmove@plt
7 malloc.c:2849-2875 __GI___libc_malloc
8 mcheck.c:206-223 mallochook
9 memmove@plt

This is just a future RFE.

> + }
> +
> + if (bfun->sym != NULL)
> + ui_out_field_string (uiout, "function", SYMBOL_PRINT_NAME (bfun->sym));
> + else if (bfun->msym != NULL)
> + ui_out_field_string (uiout, "function", SYMBOL_PRINT_NAME (bfun->msym));
> + ui_out_text (uiout, "\n");
> + }
> +}
[...]

Thanks; those issues could be also left as future PRs/RFEs.

Thanks,
Jan