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

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Tuesday, March 05, 2013 9:13 PM


> > +  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

OK. I'm printing nothing, in this case, now.


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

OK.  I'll prepare a separate patch for that while I'm waiting for a decision
on the gdbserver changes.  I'll file a PR if the gdbserver approval is faster.


> > +      /* Print the function index.  */
> > +      ui_out_field_uint (uiout, "index", idx);
> > +      ui_out_text (uiout, "\t ");
> 
> Why here

So the other functions only print one of the fields and leave the
formatting to this function.


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

OK.

> > +	}
> > +
> > +      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.

I read this as OK.  Nevertheless, since I'm resending the series with the merged
gdbserver changes, you may object if I got this wrong.

Thanks,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052