Date:   Tue, 6 Dec 2016 17:02:59 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Jin Yao <yao.jin@...ux.intel.com>
Cc:     jolsa@...nel.org, Linux-kernel@...r.kernel.org, ak@...ux.intel.com,
        kan.liang@...el.com
Subject: Re: [PATCH v1 1/4] perf report: Find the inline stack for a given
 address

Em Tue, Nov 29, 2016 at 10:55:41PM +0800, Jin Yao escreveu:
> It would be useful for perf to support a mode to query the
> inline stack for a given callgraph address. This would simplify
> finding the right code in code that does a lot of inlining.
> 
> The srcline.c has contained the code which supports to translate
> the address to filename:line_nr. This patch just extends the
> function to let it support getting the inline stacks.
> 
> The results (filename:line_nr) would be saved in a list and
> returned to the caller.

You're doing multiple things in this changeset, which makes reviewing
harder than it could be, so please consider breaking it in at least:

1.  introduce dso_name_get() out of existing code and use it, renaming
it to dso__name(), as "_get()" is usually reserved for refcount
operations (see dso__get(), thread__get(), etc, for instance), just like
with the kernel sources.

2. introduce the inline_list thing and name the functions handling it
using that prefix, and as well separate the method name from the struct
name using __, as done elsewhere in tools/perf/, i.e. that function:

  ilist_apend()

should be renamed to:

  inline_list__append()

Better, follow the list_head model and instead call it:

  inline_list__add_tail()

But as it also allocates space for the node, maybe inline_list__append()
is appropriate :-\

- Arnaldo
 
> Signed-off-by: Jin Yao <yao.jin@...ux.intel.com>
> ---
>  tools/perf/util/srcline.c | 206 +++++++++++++++++++++++++++++++++++++++-------
>  tools/perf/util/util.h    |  15 ++++
>  2 files changed, 193 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
> index b4db3f4..0145625 100644
> --- a/tools/perf/util/srcline.c
> +++ b/tools/perf/util/srcline.c
> @@ -12,6 +12,39 @@
>  
>  bool srcline_full_filename;
>  
> +static const char *dso_name_get(struct dso *dso)
> +{
> +	const char *dso_name;
> +
> +	if (dso->symsrc_filename)
> +		dso_name = dso->symsrc_filename;
> +	else
> +		dso_name = dso->long_name;
> +
> +	if (dso_name[0] == '[')
> +		return NULL;
> +
> +	if (!strncmp(dso_name, "/tmp/perf-", 10))
> +		return NULL;
> +
> +	return dso_name;
> +}
> +
> +static int ilist_apend(char *filename, int line_nr, struct inline_node *node)
> +{
> +	struct inline_list *ilist;
> +
> +	ilist = zalloc(sizeof(*ilist));
> +	if (ilist == NULL)
> +		return -1;
> +
> +	ilist->filename = filename;
> +	ilist->line_nr = line_nr;
> +	list_add_tail(&ilist->list, &node->val);
> +
> +	return 0;
> +}
> +
>  #ifdef HAVE_LIBBFD_SUPPORT
>  
>  /*
> @@ -153,7 +186,7 @@ static void addr2line_cleanup(struct a2l_data *a2l)
>  
>  static int addr2line(const char *dso_name, u64 addr,
>  		     char **file, unsigned int *line, struct dso *dso,
> -		     bool unwind_inlines)
> +		     bool unwind_inlines, struct inline_node *node)
>  {
>  	int ret = 0;
>  	struct a2l_data *a2l = dso->a2l;
> @@ -178,8 +211,14 @@ static int addr2line(const char *dso_name, u64 addr,
>  
>  		while (bfd_find_inliner_info(a2l->abfd, &a2l->filename,
>  					     &a2l->funcname, &a2l->line) &&
> -		       cnt++ < MAX_INLINE_NEST)
> -			;
> +		       cnt++ < MAX_INLINE_NEST) {
> +
> +			if (node != NULL) {
> +				if (ilist_apend(strdup(a2l->filename),
> +						a2l->line, node) != 0)
> +					return 0;
> +			}
> +		}
>  	}
>  
>  	if (a2l->found && a2l->filename) {
> @@ -205,18 +244,68 @@ void dso__free_a2l(struct dso *dso)
>  	dso->a2l = NULL;
>  }
>  
> +static struct inline_node *addr2inlines(const char *dso_name, u64 addr,
> +	struct dso *dso)
> +{
> +	char *file = NULL;
> +	unsigned int line = 0;
> +	struct inline_node *node;
> +
> +	node = zalloc(sizeof(*node));
> +	if (node == NULL) {
> +		perror("not enough memory for the inline node");
> +		return NULL;
> +	}
> +
> +	INIT_LIST_HEAD(&node->val);
> +	node->addr = addr;
> +
> +	if (!addr2line(dso_name, addr, &file, &line, dso, TRUE, node)) {
> +		free_inline_node(node);
> +		return NULL;
> +	}
> +
> +	if (list_empty(&node->val)) {
> +		free_inline_node(node);
> +		return NULL;
> +	}
> +
> +	return node;

Perhaps have:

out_free_inline_node:
	inline_node__delete(node);
	return NULL;

and jump to here in those two cases above? That after renaming
free_inline_node(node)  to inline_node__delete(node)  to follow
convention used in most parts of tools/perf/.

> +}
> +
>  #else /* HAVE_LIBBFD_SUPPORT */
>  
> +static int filename_split(const char *dso_name, char *filename,
> +			  unsigned int *line_nr)
> +{
> +	char *sep;
> +
> +	sep = strchr(filename, '\n');
> +	if (sep)
> +		*sep = '\0';
> +
> +	if (!strcmp(filename, "??:0"))
> +		return -1;
> +
> +	sep = strchr(filename, ':');
> +	if (sep) {
> +		*sep++ = '\0';
> +		*line_nr = strtoul(sep, NULL, 0);
> +	}
> +
> +	return 0;
> +}
> +
>  static int addr2line(const char *dso_name, u64 addr,
>  		     char **file, unsigned int *line_nr,
>  		     struct dso *dso __maybe_unused,
> -		     bool unwind_inlines __maybe_unused)
> +		     bool unwind_inlines __maybe_unused,
> +		     struct inline_node *node __maybe_unused)
>  {
>  	FILE *fp;
>  	char cmd[PATH_MAX];
>  	char *filename = NULL;
>  	size_t len;
> -	char *sep;
>  	int ret = 0;
>  
>  	scnprintf(cmd, sizeof(cmd), "addr2line -e %s %016"PRIx64,
> @@ -233,23 +322,14 @@ static int addr2line(const char *dso_name, u64 addr,
>  		goto out;
>  	}
>  
> -	sep = strchr(filename, '\n');
> -	if (sep)
> -		*sep = '\0';
> -
> -	if (!strcmp(filename, "??:0")) {
> -		pr_debug("no debugging info in %s\n", dso_name);
> +	if (filename_split(dso_name, filename, line_nr) != 0) {
>  		free(filename);
>  		goto out;
>  	}
>  
> -	sep = strchr(filename, ':');
> -	if (sep) {
> -		*sep++ = '\0';
> -		*file = filename;
> -		*line_nr = strtoul(sep, NULL, 0);
> -		ret = 1;
> -	}
> +	*file = filename;
> +	ret = 1;
> +
>  out:
>  	pclose(fp);
>  	return ret;
> @@ -259,6 +339,57 @@ void dso__free_a2l(struct dso *dso __maybe_unused)
>  {
>  }
>  
> +static struct inline_node *addr2inlines(const char *dso_name, u64 addr,
> +	struct dso *dso __maybe_unused)
> +{
> +	FILE *fp;
> +	char cmd[PATH_MAX];
> +	struct inline_node *node;
> +	char *filename = NULL;
> +	size_t len;
> +	unsigned int line_nr = 0;
> +
> +	scnprintf(cmd, sizeof(cmd), "addr2line -e %s -i %016"PRIx64,
> +		  dso_name, addr);
> +
> +	fp = popen(cmd, "r");
> +	if (fp == NULL) {
> +		pr_warn("popen failed for %s\n", dso_name);
> +		return NULL;
> +	}
> +
> +	node = zalloc(sizeof(*node));
> +	if (node == NULL) {
> +		perror("not enough memory for the inline node");
> +		goto out;
> +	}
> +
> +	INIT_LIST_HEAD(&node->val);
> +	node->addr = addr;
> +
> +	while (getline(&filename, &len, fp) != -1) {
> +		if (filename_split(dso_name, filename, &line_nr) != 0) {
> +			free(filename);
> +			goto out;
> +		}
> +
> +		if (ilist_apend(filename, line_nr, node) != 0)
> +			goto out;
> +
> +		filename = NULL;
> +	}
> +
> +out:
> +	pclose(fp);
> +
> +	if (list_empty(&node->val)) {
> +		free_inline_node(node);
> +		return NULL;
> +	}
> +
> +	return node;
> +}
> +
>  #endif /* HAVE_LIBBFD_SUPPORT */
>  
>  /*
> @@ -278,18 +409,11 @@ char *__get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
>  	if (!dso->has_srcline)
>  		goto out;
>  
> -	if (dso->symsrc_filename)
> -		dso_name = dso->symsrc_filename;
> -	else
> -		dso_name = dso->long_name;
> -
> -	if (dso_name[0] == '[')
> -		goto out;
> -
> -	if (!strncmp(dso_name, "/tmp/perf-", 10))
> +	dso_name = dso_name_get(dso);
> +	if (dso_name == NULL)
>  		goto out;
>  
> -	if (!addr2line(dso_name, addr, &file, &line, dso, unwind_inlines))
> +	if (!addr2line(dso_name, addr, &file, &line, dso, unwind_inlines, NULL))
>  		goto out;
>  
>  	if (asprintf(&srcline, "%s:%u",
> @@ -329,3 +453,29 @@ char *get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
>  {
>  	return __get_srcline(dso, addr, sym, show_sym, false);
>  }
> +
> +struct inline_node *get_inline_node(struct dso *dso, u64 addr)
> +{

Please rename this to dso__parse_addr_inlines().

> +	const char *dso_name;
> +
> +	dso_name = dso_name_get(dso);
> +	if (dso_name == NULL)
> +		return NULL;
> +
> +	return addr2inlines(dso_name, addr, dso);
> +}
> +
> +void free_inline_node(struct inline_node *node)

void inline_node__delete(struct inline_node *node)

> +{
> +	struct inline_list *ilist, *tmp;
> +
> +	list_for_each_entry_safe(ilist, tmp, &node->val, list) {
> +		list_del(&ilist->list);

list_del_init()

> +		if (ilist->filename != NULL)
> +			free(ilist->filename);
> +
> +		free(ilist);
> +	}
> +
> +	free(node);
> +}
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index 79662d6..febe8d7 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -78,6 +78,7 @@
>  #include <termios.h>
>  #include <linux/bitops.h>
>  #include <termios.h>
> +#include <linux/list.h>
>  #include "strlist.h"
>  
>  extern const char *graph_line;
> @@ -365,4 +366,18 @@ int is_printable_array(char *p, unsigned int len);
>  
>  int timestamp__scnprintf_usec(u64 timestamp, char *buf, size_t sz);
>  
> +struct inline_list {
> +	char			*filename;
> +	unsigned int		line_nr;
> +	struct list_head	list;
> +};
> +
> +struct inline_node {
> +	u64			addr;
> +	struct list_head	val;
> +};
> +
> +struct inline_node *get_inline_node(struct dso *dso, u64 addr);
> +void free_inline_node(struct inline_node *node);
> +
>  #endif /* GIT_COMPAT_UTIL_H */
> -- 
> 2.7.4

Powered by blists - more mailing lists