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

Metzger, Markus T
2013-03-06T09:57:29Z

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