Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1467707 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFC RESEND] Perf: lookup dwarf unwind stack info in debug  file pointed by .gnu_debuglink Jiri Olsa <jolsa@redhat.com> - 2016-08-22 17:20 +0200
    Re: [PATCH RFC RESEND] Perf: lookup dwarf unwind stack info in debug  file pointed by .gnu_debuglink Jiri Olsa <jolsa@redhat.com> - 2016-08-23 13:30 +0200
      Re: [PATCH RFC RESEND] Perf: lookup dwarf unwind stack info in debug  file pointed by .gnu_debuglink Jiri Olsa <jolsa@redhat.com> - 2016-08-24 09:40 +0200

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

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

[toc] | [next] | [standalone]

#1468488

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

[toc] | [prev] | [next] | [standalone]

#1469135

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web