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

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

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