On Tue, Aug 23, 2016 at 01:22:04PM +0200, Jiri Olsa wrote:
> On Tue, Aug 23, 2016 at 07:09:18AM +0200, Matija Glavinic Pecotic wrote:
> > On 22/08/16 17:19, Jiri Olsa wrote:
> > > should you also set following?
> > >
> > > *offset = ofs
> > > dso->debug_frame_offset = ofs;
> > >
> > > I guess if we found the debuglink section with file having .debug_frame
> > > section, we want to use it for this dso from now on..
> >
> > I omitted this at first as I was thinking whether it is correct to do so.
> > For
> > our case, stripped dso, symbols in debug file, we are marking offset in
> > other
> > file, and not *this* dso. But I agree to you now, I have checked, and debug
> > frame offset is not used anywhere, so it is a future problem. Someone might
> > be surprised though that offset is marked, but for the other file.
> >
> > > I'd think let's have read_unwind_spec_debug_frame to find .debug_frame
> > > and provide that info under dso object for subsequent reads
> >
> > Yes, that sounds.
> >
> > > so in case we find debuglink-ed file, it has precedence over the file
> > > we found symtab in? assuming thats what dso->symsrc_filename is..
> >
> > Thanks for pointing that one out, I haven't seen before we could use it.
> > It was introduced with 0058aef65eda9c9dde8253af702d542ba7eef697 and it is
> > aimed to keep name of the file with debug symbols, similar as needed here.
> >
> > I would then propose something like this below. This significantly changes
> > dso__read_binary_type_filename, but I would say it wasn't proper before and
> > it is not in sync with the rest of the cases in it. Idea of this function is
> > to provide path to dso, and DSO_BINARY_TYPE__DEBUGLINK implies one location,
> > which is not entirely correct.
> >
> > gdb and objcopy docs give points what debug link might be, and where debug
> > file might reside. Debug link might be absolute path, or just name, in which
> > case debug file should be looked up in several places. Here is what gdb
> > does:
> >
> > So, for example, suppose you ask gdb to debug /usr/bin/ls, which has a debug
> > link that specifies the file ls.debug, and a build ID whose value in hex is
> > abcdef1234. If the list of the global debug directories includes
> > /usr/lib/debug, then gdb will look for the following debug information
> > files,
> > in the indicated order:
> >
> > - /usr/lib/debug/.build-id/ab/cdef1234.debug
> > - /usr/bin/ls.debug
> > - /usr/bin/.debug/ls.debug
> > - /usr/lib/debug/usr/bin/ls.debug.
> >
> > https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html
> > https://sourceware.org/binutils/docs/binutils/objcopy.html
> >
> > Could you please tell me what are your thoughts on this kind of approach?
> >
> > ---
> > tools/perf/util/dso.c | 40
> > ++++++++++++++++++++++++++++++++
> > tools/perf/util/unwind-libunwind-local.c | 28 +++++++++++++++++++++-
> > 2 files changed, 67 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > index 774f6ec..ecc859e 100644
> > --- a/tools/perf/util/dso.c
> > +++ b/tools/perf/util/dso.c
> > @@ -46,11 +46,14 @@ int dso__read_binary_type_filename(const struct dso
> > *dso,
> > switch (type) {
> > case DSO_BINARY_TYPE__DEBUGLINK: {
> > char *debuglink;
> > + char *dir;
> > + char symfile[PATH_MAX];
> >
> > len = __symbol__join_symfs(filename, size, dso->long_name);
> > debuglink = filename + len;
> > while (debuglink != filename && *debuglink != '/')
> > debuglink--;
> > + dir = debuglink;
> > if (*debuglink == '/')
> > debuglink++;
> >
> > @@ -60,8 +63,45 @@ int dso__read_binary_type_filename(const struct dso *dso,
> >
> > ret = filename__read_debuglink(filename, debuglink,
> > size - (debuglink - filename));
> > + if (ret)
> > + break;
> > +
> > + /* Check predefined locations where debug file might reside:
> > + * - if debuglink is absolute path, check only that one
> > + * If debuglink provides just name:
> > + * - in the same directory as dso
> > + * - in the .debug subdirectory of dso directory
> > + * - in the /usr/lib/debug/[path to dso directory]
> > + * */
> > + if (*debuglink == '/') {
> > + ret = is_regular_file(debuglink);
> > + break;
> > + }
> > +
> > + snprintf(symfile, PATH_MAX, "%s/%s", dir, debuglink);
> > + ret = is_regular_file(symfile);
> > + if(!ret) {
> > + strncpy(debuglink, symfile, size);
> > + break;
> > + }
> > +
> > + snprintf(symfile, PATH_MAX, "%s/.debug/%s", dir, debuglink);
> > + ret = is_regular_file(symfile);
> > + if(!ret) {
> > + strncpy(debuglink, symfile, size);
> > + break;
> > + }
> > +
> > + snprintf(symfile, PATH_MAX, "/usr/bin/debug/%s/%s", dir,
> > debuglink);
> > + ret = is_regular_file(symfile);
> > + if(!ret) {
> > + strncpy(debuglink, symfile, size);
> > + break;
> > + }
> > +
> > }
> > break;
> > +
> > case DSO_BINARY_TYPE__BUILD_ID_CACHE:
> > if (dso__build_id_filename(dso, filename, size) == NULL)
> > ret = -1;
> > diff --git a/tools/perf/util/unwind-libunwind-local.c
> > b/tools/perf/util/unwind-libunwind-local.c
> > index 97c0f8f..a1d3c93 100644
> > --- a/tools/perf/util/unwind-libunwind-local.c
> > +++ b/tools/perf/util/unwind-libunwind-local.c
> > @@ -35,6 +35,7 @@
> > #include "util.h"
> > #include "debug.h"
> > #include "asm/bug.h"
> > +#include "dso.h"
> >
> > extern int
> > UNW_OBJ(dwarf_search_unwind_table) (unw_addr_space_t as,
> > @@ -296,6 +297,8 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
> > {
> > int fd;
> > u64 ofs = dso->data.debug_frame_offset;
> > + char *debuglink = malloc(PATH_MAX);
> > + int ret = 0;
> >
> > if (ofs == 0) {
> > fd = dso__data_get_fd(dso, machine);
> > @@ -304,8 +307,31 @@ static int read_unwind_spec_debug_frame(struct dso
> > *dso,
> >
> > /* Check the .debug_frame section for unwinding info */
> > ofs = elf_section_offset(fd, ".debug_frame");
> > - dso->data.debug_frame_offset = ofs;
> > dso__data_put_fd(dso);
> > +
> > + if (!ofs) {
> > + /* If not found, try to lookup in debuglink */
> > + ret = dso__read_binary_type_filename(
> > + dso, DSO_BINARY_TYPE__DEBUGLINK,
> > + machine->root_dir, debuglink, PATH_MAX);
> > + if (!ret) {
> > + fd = open(debuglink, O_RDONLY);
> > + if (fd < 0)
> > + return -EINVAL;
> > +
> > + ofs = elf_section_offset(fd, ".debug_frame");
> > + close(fd);
> > +
> > + if (ofs) {
> > + dso->symsrc_filename = debuglink;
>
> symsrc_filename is initialized with file that has symtab,
> which I'm not sure is guaranteed in here as well..
I'm also not sure it's guaranteed that having debuginfo implies having
symtab. But I didn't see any ELF binary which has debugginfo but no
symtab. Maybe we can add the check for sure.
Anyway, it needs to free the debuglink if not used.
Thanks,
Namhyung
>
> > + }
> > + }
> > + }
> > +
> > + pr_debug("%s: dso: %s, ret: %d, debuglink: <%s>\n",
> > + __func__, dso->short_name, ret, debuglink);
> > +
> > + dso->data.debug_frame_offset = ofs;
> > }
> >
> > *offset = ofs;
> > --
> > 2.1.4
> >
> >
