Re: [PATCH RFC RESEND] Perf: lookup dwarf unwind stack info in debug file pointed by .gnu_debuglink
From: Matija Glavinic Pecotic
Date: Tue Aug 23 2016 - 12:19:42 EST
On 23/08/16 14:33, Matija Glavinic Pecotic wrote:
> On 23/08/16 13:22, 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..
>>
>> Namhyung, Massami?
>
> Yes, I see it. This file could also be debuglink, but that is not guaranteed.
>
> I would propose to look in symsrc_filename for .debug_frame. If its not there,
> we could still take a look into debuglink as a last resort.
Something like this. This also expands dso__read_binary_type_filename to return
actual path to debuglink, and not just name provided there, which is imho how
it should behave:
---
tools/perf/util/dso.c | 60 ++++++++++++++++++++++++++------
tools/perf/util/unwind-libunwind-local.c | 47 +++++++++++++++++++++++--
2 files changed, 94 insertions(+), 13 deletions(-)
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 774f6ec..486470f 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -44,24 +44,62 @@ int dso__read_binary_type_filename(const struct dso *dso,
size_t len;
switch (type) {
- case DSO_BINARY_TYPE__DEBUGLINK: {
- char *debuglink;
+ case DSO_BINARY_TYPE__DEBUGLINK:
+ {
+ const char *last_slash;
+ char dso_dir[PATH_MAX];
+ char symfile[PATH_MAX];
len = __symbol__join_symfs(filename, size, dso->long_name);
- debuglink = filename + len;
- while (debuglink != filename && *debuglink != '/')
- debuglink--;
- if (*debuglink == '/')
- debuglink++;
+ last_slash = filename + len;
+ while (last_slash != filename && *last_slash != '/')
+ last_slash--;
- ret = -1;
- if (!is_regular_file(filename))
+ strncpy(dso_dir, filename, last_slash - filename);
+ dso_dir[last_slash-filename] = '\0';
+
+ if (!is_regular_file(filename)) {
+ ret = -1;
break;
+ }
- ret = filename__read_debuglink(filename, debuglink,
- size - (debuglink - filename));
+ ret = filename__read_debuglink(filename, symfile, PATH_MAX);
+ if (ret)
+ break;
+
+ /* Check predefined locations where debug file might reside:
+ * - if debuglink is absolute path, check only that one
+ * If debuglink provides name w/o path, look for debug file:
+ * - in the same directory as dso
+ * - in the .debug subdirectory of dso directory
+ * - in the /usr/lib/debug/[dso directory]
+ * */
+ ret = 0;
+ if (symfile[0] == '/') {
+ if (!is_regular_file(symfile))
+ ret = -1;
+ else
+ strncpy(filename, symfile, size);
+ break;
}
+
+ snprintf(filename, size, "%s/%s", dso_dir, symfile);
+ if(is_regular_file(filename))
+ break;
+
+ snprintf(filename, size, "%s/.debug/%s", dso_dir, symfile);
+ if(is_regular_file(filename))
+ break;
+
+ snprintf(filename, size, "/usr/lib/debug/%s/%s",
+ dso_dir, symfile);
+ if(is_regular_file(filename))
+ break;
+
+ ret = -1;
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..345541b 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,
@@ -297,15 +298,57 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
int fd;
u64 ofs = dso->data.debug_frame_offset;
+ /* debug_frame can reside in:
+ * - dso
+ * - debug pointed by symsrc_filename
+ * - gnu_debuglink, which doesnt necessary
+ * has to be pointed by symsrc_filename
+ * */
if (ofs == 0) {
fd = dso__data_get_fd(dso, machine);
if (fd < 0)
return -EINVAL;
- /* 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) {
+ fd = open(dso->symsrc_filename, O_RDONLY);
+ if (fd >= 0) {
+ ofs = elf_section_offset(fd, ".debug_frame");
+ close(fd);
+ }
+ }
+
+ if (!ofs) {
+ char *debuglink = malloc(PATH_MAX);
+ int ret = 0;
+
+ 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) {
+ ofs = elf_section_offset(fd,
+ ".debug_frame");
+ close(fd);
+ }
+ }
+ if (ofs) {
+ if (dso->symsrc_filename != NULL) {
+ pr_warning(
+ "%s:overwrite symsrc(%s,%s)\n",
+ __func__,
+ dso->symsrc_filename,
+ debuglink);
+ free(dso->symsrc_filename);
+ }
+ dso->symsrc_filename = debuglink;
+ }
+ }
+
+ dso->data.debug_frame_offset = ofs;
}
*offset = ofs;
--
2.1.4
> What I wonder, would it be fine in such a case to store debuglink to symsrc, if
> its already set? Though, this case is somewhat unlikely, but not entirely impossible
>
> Thanks,
>
> Matija
>
>> thanks,
>> jirka
>>
>>> + }
>>> + }
>>> + }
>>> +
>>> + 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
>>>
>>>