Re: [PATCH RFC RESEND] Perf: lookup dwarf unwind stack info in debug file pointed by .gnu_debuglink

[Matija Glavinic Pecotic]

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