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

[Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com>]

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