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

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

[Matija Glavinic Pecotic]

(Resend due to previous mail marked as 'Suspected SPAM')

Using perf with call graph method dwarf fails to provide backtrace support with
stripped binary even though .gnu_debuglink points to *.dbg flavor with properly
populated debug symbols.

Problem is reproduced on ARM (v7), kernels 3.14.y, 4.4.y and 4.8.0-rc2. Perf is
configured with libunwind and unwind dwarf support [1]. Test code (stress_bt.c)
can be found on [2].

Running (explicitly disable other unwinding methods):

$ gcc -g -o stress_bt -fomit-frame-pointer -fno-unwind-tables -fno-asynchronous-unwind-tables stress_bt.c
$ perf record -N --call-graph dwarf ./stress_bt
$ perf report

results in properly generated call graph. Stripping the binary and rerunning it
results with missing callgraph. Expected result is to have callgraph:

$ gcc -g -o stress_bt -fomit-frame-pointer -fno-unwind-tables -fno-asynchronous-unwind-tables stress_bt.c
$ objcopy --only-keep-debug stress_bt stress_bt.dbg
$ objcopy --strip-debug stress_bt
$ objcopy --add-gnu-debuglink=stress_bt.dbg stress_bt
$ perf record -N --call-graph dwarf ./stress_bt
$ perf report

The patch itself is for the sake of discussion and it is not proposed solution.
I understood the problem as perf not even trying to lookup into debug version,
but only in the original dso. Patch tries to read symbols from file in the same
directory where binary or library resides. What is  missing is lookup in other
standard locations, and proper integration, I'm sure debug package should be
treated like the rest of dsos. With applied patch, .debug_frame is read from
the debug file, and callgraph is properly displayed.

Interesting to note, I could not reproduce on x86_64 with above given compiler
flags, call graph was not generated. I had to define asynchronous-unwind-tables

$ gcc -g -o stress_bt -fomit-frame-pointer -fno-unwind-tables -fasynchronous-unwind-tables stress_bt.c
$ perf record -N --call-graph dwarf ./stress_bt
$ perf report

which makes me wonder about dwarf on x86. Since our target is arm, I have not
delved into this, but problem should not be arch specific.

[1] https://wiki.linaro.org/LEG/Engineering/TOOLS/perf-callstack-unwinding
[2] https://wiki.linaro.org/LEG/Engineering/TOOLS/perf-callstack-unwinding#Backtrace_stress_application

Signed-off-by: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com>
---
 tools/perf/util/unwind-libunwind-local.c | 36 ++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/unwind-libunwind-local.c b/tools/perf/util/unwind-libunwind-local.c
index 97c0f8f..5d40acd 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,
@@ -292,10 +293,13 @@ static int read_unwind_spec_eh_frame(struct dso *dso, struct machine *machine,
 
 #ifndef NO_LIBUNWIND_DEBUG_FRAME
 static int read_unwind_spec_debug_frame(struct dso *dso,
-					struct machine *machine, u64 *offset)
+					struct machine *machine, u64 *offset,
+					char **symfile)
 {
 	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);
@@ -312,6 +316,26 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
 	if (*offset)
 		return 0;
 
+	/* 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) {
+		pr_debug("%s: dso: %s, ret: %d, debuglink: <%s>\n",
+			__func__, dso->short_name, ret, debuglink);
+
+		fd = open(debuglink, O_RDONLY);
+		if (fd >= 0) {
+			ofs = elf_section_offset(fd, ".debug_frame");
+			close(fd);
+
+			if (ofs) {
+				*symfile = debuglink;
+				return 0;
+			}
+		}
+	}
+
 	return -EINVAL;
 }
 #endif
@@ -343,6 +367,7 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
 	unw_dyn_info_t di;
 	u64 table_data, segbase, fde_count;
 	int ret = -EINVAL;
+	char *symfile = NULL;
 
 	map = find_map(ip, ui);
 	if (!map || !map->dso)
@@ -368,16 +393,19 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
 #ifndef NO_LIBUNWIND_DEBUG_FRAME
 	/* Check the .debug_frame section for unwinding info */
 	if (ret < 0 &&
-	    !read_unwind_spec_debug_frame(map->dso, ui->machine, &segbase)) {
+	    !read_unwind_spec_debug_frame(
+		    map->dso, ui->machine, &segbase, &symfile)) {
 		int fd = dso__data_get_fd(map->dso, ui->machine);
 		int is_exec = elf_is_exec(fd, map->dso->name);
 		unw_word_t base = is_exec ? 0 : map->start;
-		const char *symfile;
 
 		if (fd >= 0)
 			dso__data_put_fd(map->dso);
 
-		symfile = map->dso->symsrc_filename ?: map->dso->name;
+		if (!symfile)
+			symfile = map->dso->symsrc_filename ?: map->dso->name;
+
+		pr_debug("%s: using symfile %s\n", __func__, symfile);
 
 		memset(&di, 0, sizeof(di));
 		if (dwarf_find_debug_frame(0, &di, ip, base, symfile,
-- 
2.1.4

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

[Jiri Olsa]

On Mon, Aug 22, 2016 at 02:32:04PM +0200, Matija Glavinic Pecotic wrote:

SNIP

> Signed-off-by: Matija Glavinic Pecotic <matija.glavinic-pecotic.ext@nokia.com>
> ---
>  tools/perf/util/unwind-libunwind-local.c | 36 ++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/unwind-libunwind-local.c b/tools/perf/util/unwind-libunwind-local.c
> index 97c0f8f..5d40acd 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,
> @@ -292,10 +293,13 @@ static int read_unwind_spec_eh_frame(struct dso *dso, struct machine *machine,
>  
>  #ifndef NO_LIBUNWIND_DEBUG_FRAME
>  static int read_unwind_spec_debug_frame(struct dso *dso,
> -					struct machine *machine, u64 *offset)
> +					struct machine *machine, u64 *offset,
> +					char **symfile)
>  {
>  	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);
> @@ -312,6 +316,26 @@ static int read_unwind_spec_debug_frame(struct dso *dso,
>  	if (*offset)
>  		return 0;
>  
> +	/* 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) {
> +		pr_debug("%s: dso: %s, ret: %d, debuglink: <%s>\n",
> +			__func__, dso->short_name, ret, debuglink);
> +
> +		fd = open(debuglink, O_RDONLY);
> +		if (fd >= 0) {
> +			ofs = elf_section_offset(fd, ".debug_frame");
> +			close(fd);

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'd think let's have read_unwind_spec_debug_frame to find .debug_frame
and provide that info under dso object for subsequent reads

> +
> +			if (ofs) {
> +				*symfile = debuglink;
> +				return 0;
> +			}
> +		}
> +	}
> +
>  	return -EINVAL;
>  }
>  #endif
> @@ -343,6 +367,7 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
>  	unw_dyn_info_t di;
>  	u64 table_data, segbase, fde_count;
>  	int ret = -EINVAL;
> +	char *symfile = NULL;
>  
>  	map = find_map(ip, ui);
>  	if (!map || !map->dso)
> @@ -368,16 +393,19 @@ find_proc_info(unw_addr_space_t as, unw_word_t ip, unw_proc_info_t *pi,
>  #ifndef NO_LIBUNWIND_DEBUG_FRAME
>  	/* Check the .debug_frame section for unwinding info */
>  	if (ret < 0 &&
> -	    !read_unwind_spec_debug_frame(map->dso, ui->machine, &segbase)) {
> +	    !read_unwind_spec_debug_frame(
> +		    map->dso, ui->machine, &segbase, &symfile)) {
>  		int fd = dso__data_get_fd(map->dso, ui->machine);
>  		int is_exec = elf_is_exec(fd, map->dso->name);
>  		unw_word_t base = is_exec ? 0 : map->start;
> -		const char *symfile;
>  
>  		if (fd >= 0)
>  			dso__data_put_fd(map->dso);
>  
> -		symfile = map->dso->symsrc_filename ?: map->dso->name;
> +		if (!symfile)
> +			symfile = map->dso->symsrc_filename ?: map->dso->name;

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

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

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

[Jiri Olsa]

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?

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

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

[Matija Glavinic Pecotic]

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

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

[Matija Glavinic Pecotic]

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

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

[Jiri Olsa]

On Tue, Aug 23, 2016 at 06:18:10PM +0200, Matija Glavinic Pecotic wrote:

SNIP

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

it might be more clear to follow the same way we do for vmlinux search,
like array of possible paths and generic code to check.. search for
vmlinux_path in symbol.c

jirka

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

[Matija Glavinic Pecotic]

On 24/08/16 09:30, Jiri Olsa wrote:
> On Tue, Aug 23, 2016 at 06:18:10PM +0200, Matija Glavinic Pecotic wrote:
>> -		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;
> 
> it might be more clear to follow the same way we do for vmlinux search,
> like array of possible paths and generic code to check.. search for
> vmlinux_path in symbol.c

It indeed looks better:

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 774f6ec..3ea205cc 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -35,6 +35,13 @@ char dso__symtab_origin(const struct dso *dso)
 	return origin[dso->symtab_type];
 }
 
+static const char * const debuglink_paths[] = {
+	"%.0s%s",
+	"%s/%s",
+	"%s/.debug/%s",
+	"/usr/lib/debug%s/%s"
+};
+
 int dso__read_binary_type_filename(const struct dso *dso,
 				   enum dso_binary_type type,
 				   char *root_dir, char *filename, size_t size)
@@ -44,24 +51,44 @@ 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];
+		unsigned int i;
 
 		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, symfile, PATH_MAX);
+		if (ret)
 			break;
 
-		ret = filename__read_debuglink(filename, debuglink,
-					       size - (debuglink - filename));
+		/* Check predefined locations where debug file might reside */
+		ret = -1;
+		for (i = 0; i < ARRAY_SIZE(debuglink_paths); i++) {
+			snprintf(filename, size,
+					debuglink_paths[i], dso_dir, symfile);
+			if(is_regular_file(filename)) {
+				ret = 0;
+				break;
+			}
 		}
+
 		break;
+	}

Thanks,

Matija

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

[Namhyung Kim]

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

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

[Matija Glavinic Pecotic]

On 08/26/2016 08:12 AM, Namhyung Kim wrote:
> 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:
>>> 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.

Here is latest what I have on this:

diff --git a/tools/perf/util/unwind-libunwind-local.c b/tools/perf/util/unwind-libunwind-local.c
index 97c0f8f..5df4c74 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,58 @@ 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;
+		if (fd >= 0) {
+			ofs = elf_section_offset(fd, ".debug_frame");
+			dso__data_put_fd(dso);
+		}
+
+		if (ofs <= 0) {
+			fd = open(dso->symsrc_filename, O_RDONLY);
+			if (fd >= 0) {
+				ofs = elf_section_offset(fd, ".debug_frame");
+				close(fd);
+			}
+		}
+
+		if (ofs <= 0) {
+			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 > 0) {
+				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;
+			} else {
+				free(debuglink);
+			}
+		}
 
-		/* 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);
 	}
 
 	*offset = ofs;

Third case, when we actually try to read debuglink because .debug_frame wasn't
found in symsrc is questionable. What should not happen is that at this point
symsrc points to nothing, as symsrc is populated at dso__load, and debuglink is
one of the candidates. Unless debuglink really lacks of symtab, in which case
it will not be populated in symsrc_filename.

symsrc_filename populated, but not with debug_link will happen if multiple set
of debug files exist, e.g. debuglink and build id. That indicates missconfig
from several points of view, notably, one debug file clearly lacking debug_frame
as in that case debug link wouldn't tried to be read.

So this third case is more best effort which would result with warning thrown
and symsrc overwritten with new debuglink. Warning is there to note user that
something is wrong.

If you have some better idea to handle this situation, please feel free to suggest

Thanks,

Matija