| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <69a799bb-1c54-928e-f58b-fdc8bb3fac0c@nokia.com>
Date: Tue, 23 Aug 2016 14:33:49 +0200
From: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@...ia.com>
To: Jiri Olsa <jolsa@...hat.com>
CC: <acme@...hat.com>, <linux-kernel@...r.kernel.org>,
Namhyung Kim <namhyung@...nel.org>,
Masami Hiramatsu <mhiramat@...nel.org>
Subject: Re: [PATCH RFC RESEND] Perf: lookup dwarf unwind stack info in debug
file pointed by .gnu_debuglink
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.
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
>>
>>
Powered by blists - more mailing lists