[PATCH 0/2] perf evsel: add support for inlined function in callchains

[PATCH 0/2] perf evsel: add support for inlined function in callchains

[Jonas Rabenstein]

Hi,
sample__fprintf_callchain currently did not use the already available
code to get the symbols of an inlined function if such information is
available in a dso. This patchset adds the required logic to add
appropriate lines.

As I am quite new to the code base of perf I am not sure how to test
that changeset in a correct way. At least the codes builds and the tools
that make use of sample__fprintf_callchain (perf-script, perf-trace and
perf-sched as far as I can see) did not fail to run - also I did not get
into the details of perf-trace and perf-sched as I have never used them
before.

Another thing I am not sure how to deal with are some warnings of
checkpatch.pl due to the 80 character line limit. Due to the long
function names in use the current implementation already exceeded that
limit in the same spots by even more characters as I have taken the
inner loop and put it into a separate function.

I hope to expand my work to perf-report but thought it might be usefull
to get already early feedback on those patches.

Thank you,
 Jonas

Jonas Rabenstein (2):
  perf evsel: split sample__fprintf_callchain in output and iteration
  perf evsel: add inline functions to sample callchain output

 tools/perf/util/evsel_fprintf.c | 157 ++++++++++++++++++++++----------
 1 file changed, 109 insertions(+), 48 deletions(-)

-- 
2.19.2

[PATCH 1/2] perf evsel: split sample__fprintf_callchain in output and iteration

[Jonas Rabenstein]

Split the iteration over the callchain and the actual output of an
link in the callchain into separate functions. This allows to reuse
the output function in a follow up patch to add inline symbols to
the output.

Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
---
 tools/perf/util/evsel_fprintf.c | 111 ++++++++++++++++++--------------
 1 file changed, 63 insertions(+), 48 deletions(-)

diff --git a/tools/perf/util/evsel_fprintf.c b/tools/perf/util/evsel_fprintf.c
index 95ea147f9e18..c710f687ddf4 100644
--- a/tools/perf/util/evsel_fprintf.c
+++ b/tools/perf/util/evsel_fprintf.c
@@ -99,12 +99,12 @@ int perf_evsel__fprintf(struct perf_evsel *evsel,
 	return ++printed;
 }
 
-int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
-			      unsigned int print_opts, struct callchain_cursor *cursor,
-			      FILE *fp)
+static int __fprintf_callchain_link(u64 ip, struct map *map, struct symbol *symbol,
+				    const char *srcline, bool first, int left_alignment,
+				    unsigned int print_opts, FILE *fp)
 {
+	u64 addr = 0;
 	int printed = 0;
-	struct callchain_cursor_node *node;
 	int print_ip = print_opts & EVSEL__PRINT_IP;
 	int print_sym = print_opts & EVSEL__PRINT_SYM;
 	int print_dso = print_opts & EVSEL__PRINT_DSO;
@@ -115,63 +115,80 @@ int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
 	int print_arrow = print_opts & EVSEL__PRINT_CALLCHAIN_ARROW;
 	int print_skip_ignored = print_opts & EVSEL__PRINT_SKIP_IGNORED;
 	char s = print_oneline ? ' ' : '\t';
-	bool first = true;
+	struct addr_location node_al;
 
-	if (sample->callchain) {
-		struct addr_location node_al;
 
-		callchain_cursor_commit(cursor);
+	if (symbol && symbol->ignore && print_skip_ignored)
+		return 0;
 
-		while (1) {
-			u64 addr = 0;
+	printed += fprintf(fp, "%-*.*s", left_alignment, left_alignment, " ");
 
-			node = callchain_cursor_current(cursor);
-			if (!node)
-				break;
+	if (print_arrow && !first)
+		printed += fprintf(fp, " <-");
 
-			if (node->sym && node->sym->ignore && print_skip_ignored)
-				goto next;
+	if (print_ip)
+		printed += fprintf(fp, "%c%16" PRIx64, s, ip);
 
-			printed += fprintf(fp, "%-*.*s", left_alignment, left_alignment, " ");
+	if (map)
+		addr = map->map_ip(map, ip);
 
-			if (print_arrow && !first)
-				printed += fprintf(fp, " <-");
+	if (print_sym) {
+		printed += fprintf(fp, " ");
+		node_al.addr = addr;
+		node_al.map  = map;
 
-			if (print_ip)
-				printed += fprintf(fp, "%c%16" PRIx64, s, node->ip);
+		if (print_symoffset) {
+			printed += __symbol__fprintf_symname_offs(symbol, &node_al,
+								  print_unknown_as_addr,
+								  true, fp);
+		} else {
+			printed += __symbol__fprintf_symname(symbol, &node_al,
+							     print_unknown_as_addr,
+							     fp);
+		}
+	}
 
-			if (node->map)
-				addr = node->map->map_ip(node->map, node->ip);
+	if (print_dso && (!symbol || !symbol->inlined)) {
+		printed += fprintf(fp, " (");
+		printed += map__fprintf_dsoname(map, fp);
+		printed += fprintf(fp, ")");
+	}
 
-			if (print_sym) {
-				printed += fprintf(fp, " ");
-				node_al.addr = addr;
-				node_al.map  = node->map;
+	if (print_srcline && srcline)
+		printed += fprintf(fp, "\n  %s", srcline);
+	else if (print_srcline)
+		printed += map__fprintf_srcline(map, addr, "\n  ", fp);
 
-				if (print_symoffset) {
-					printed += __symbol__fprintf_symname_offs(node->sym, &node_al,
-										  print_unknown_as_addr,
-										  true, fp);
-				} else {
-					printed += __symbol__fprintf_symname(node->sym, &node_al,
-									     print_unknown_as_addr, fp);
-				}
-			}
+	if (symbol && symbol->inlined)
+		printed += fprintf(fp, " (inlined)");
 
-			if (print_dso && (!node->sym || !node->sym->inlined)) {
-				printed += fprintf(fp, " (");
-				printed += map__fprintf_dsoname(node->map, fp);
-				printed += fprintf(fp, ")");
-			}
+	if (!print_oneline)
+		printed += fprintf(fp, "\n");
 
-			if (print_srcline)
-				printed += map__fprintf_srcline(node->map, addr, "\n  ", fp);
+	return printed;
+}
+
+int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
+			      unsigned int print_opts, struct callchain_cursor *cursor,
+			      FILE *fp)
+{
+	int printed = 0;
+	struct callchain_cursor_node *node;
+
+	if (sample->callchain) {
+		callchain_cursor_commit(cursor);
+
+		while (1) {
+			node = callchain_cursor_current(cursor);
+			if (!node)
+				break;
 
-			if (node->sym && node->sym->inlined)
-				printed += fprintf(fp, " (inlined)");
 
-			if (!print_oneline)
-				printed += fprintf(fp, "\n");
+			printed += __fprintf_callchain_link(node->ip, node->map,
+							    node->sym, NULL,
+							    (printed == 0),
+							    left_alignment,
+							    print_opts, fp);
 
 			/* Add srccode here too? */
 			if (symbol_conf.bt_stop_list &&
@@ -181,8 +198,6 @@ int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
 				break;
 			}
 
-			first = false;
-next:
 			callchain_cursor_advance(cursor);
 		}
 	}
-- 
2.19.2

[PATCH 2/2] perf evsel: add inline functions to sample callchain output

[Jonas Rabenstein]

Whenever a callchain shall be printed search for each address whether
inline information is available and add those symbols to the output
if symbol_conf.inline_name is enabled.

Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
---
 tools/perf/util/evsel_fprintf.c | 46 +++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/tools/perf/util/evsel_fprintf.c b/tools/perf/util/evsel_fprintf.c
index c710f687ddf4..9f9ece453dc4 100644
--- a/tools/perf/util/evsel_fprintf.c
+++ b/tools/perf/util/evsel_fprintf.c
@@ -168,6 +168,49 @@ static int __fprintf_callchain_link(u64 ip, struct map *map, struct symbol *symb
 	return printed;
 }
 
+static int __fprintf_callchain_inlines(struct callchain_cursor_node *node,
+				       bool first, int left_alignment,
+				       unsigned int print_opts, FILE *fp)
+{
+	int printed = 0;
+	struct inline_node *inline_node;
+	struct inline_list *ilist;
+	u64 addr;
+
+	if (!symbol_conf.inline_name)
+		return 0;
+
+	if (!node->map || !node->map->dso)
+		return 0;
+
+	addr = node->map->map_ip(node->map, node->ip);
+
+	inline_node = inlines__tree_find(&node->map->dso->inlined_nodes,
+					 addr);
+	if (!inline_node) {
+		inline_node = dso__parse_addr_inlines(node->map->dso,
+						      addr, node->sym);
+		if (!inline_node)
+			return 0;
+		inlines__tree_insert(&node->map->dso->inlined_nodes,
+				     inline_node);
+	}
+
+	list_for_each_entry(ilist, &inline_node->val, list) {
+		if (ilist->symbol == node->sym)
+			break;
+
+		printed += __fprintf_callchain_link(node->ip, node->map,
+						    ilist->symbol,
+						    ilist->srcline,
+						    first, left_alignment,
+						    print_opts, fp);
+		first = (first && printed == 0);
+	}
+
+	return printed;
+}
+
 int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
 			      unsigned int print_opts, struct callchain_cursor *cursor,
 			      FILE *fp)
@@ -183,6 +226,9 @@ int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
 			if (!node)
 				break;
 
+			printed += __fprintf_callchain_inlines(node, (printed ==  0),
+							       left_alignment,
+							       print_opts, fp);
 
 			printed += __fprintf_callchain_link(node->ip, node->map,
 							    node->sym, NULL,
-- 
2.19.2

Re: [PATCH 0/2] perf evsel: add support for inlined function in callchains

[Arnaldo Carvalho de Melo]

Em Tue, Feb 19, 2019 at 07:38:08PM +0100, Jonas Rabenstein escreveu:
> Hi,
> sample__fprintf_callchain currently did not use the already available
> code to get the symbols of an inlined function if such information is
> available in a dso. This patchset adds the required logic to add
> appropriate lines.

You forgot to add the person that did lots of work in this area more
recently, Milian, which I'm adding here so that he have the opportunity
to comment on your work,

Thanks for working on this!

- Arnaldo
 
> As I am quite new to the code base of perf I am not sure how to test
> that changeset in a correct way. At least the codes builds and the tools
> that make use of sample__fprintf_callchain (perf-script, perf-trace and
> perf-sched as far as I can see) did not fail to run - also I did not get
> into the details of perf-trace and perf-sched as I have never used them
> before.
> 
> Another thing I am not sure how to deal with are some warnings of
> checkpatch.pl due to the 80 character line limit. Due to the long
> function names in use the current implementation already exceeded that
> limit in the same spots by even more characters as I have taken the
> inner loop and put it into a separate function.
> 
> I hope to expand my work to perf-report but thought it might be usefull
> to get already early feedback on those patches.
> 
> Thank you,
>  Jonas
> 
> Jonas Rabenstein (2):
>   perf evsel: split sample__fprintf_callchain in output and iteration
>   perf evsel: add inline functions to sample callchain output
> 
>  tools/perf/util/evsel_fprintf.c | 157 ++++++++++++++++++++++----------
>  1 file changed, 109 insertions(+), 48 deletions(-)
> 
> -- 
> 2.19.2

-- 

- Arnaldo

Re: [PATCH 2/2] perf evsel: add inline functions to sample callchain output

[Jonas Rabenstein]

On Tue, Feb 19, 2019 at 07:38:10PM +0100, Jonas Rabenstein wrote:
> Whenever a callchain shall be printed search for each address whether
> inline information is available and add those symbols to the output
> if symbol_conf.inline_name is enabled.
> 
> Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
> ---
>  tools/perf/util/evsel_fprintf.c | 46 +++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/tools/perf/util/evsel_fprintf.c b/tools/perf/util/evsel_fprintf.c
> index c710f687ddf4..9f9ece453dc4 100644
> --- a/tools/perf/util/evsel_fprintf.c
> +++ b/tools/perf/util/evsel_fprintf.c
> @@ -168,6 +168,49 @@ static int __fprintf_callchain_link(u64 ip, struct map *map, struct symbol *symb
>  	return printed;
>  }
>  
> +static int __fprintf_callchain_inlines(struct callchain_cursor_node *node,
> +				       bool first, int left_alignment,
> +				       unsigned int print_opts, FILE *fp)
> +{
> +	int printed = 0;
> +	struct inline_node *inline_node;
> +	struct inline_list *ilist;
> +	u64 addr;
> +
> +	if (!symbol_conf.inline_name)
> +		return 0;
> +
> +	if (!node->map || !node->map->dso)
> +		return 0;
> +
> +	addr = node->map->map_ip(node->map, node->ip);
I stumbled across a problem with the usage of map_ip here. Using the map_ip
on dynamic binaries does resolve the inline symbols but it fails with static
ones. Using node->ip directly works for static binaries but fails for
static ones...
Is there a way to decide whether node->map->dso is a static or dynamic
object file? Furthermore, is it even correct for a static dso to have
map->map_ip set to map__map_ip instead of identity__map_ip?
> +
> +	inline_node = inlines__tree_find(&node->map->dso->inlined_nodes,
> +					 addr);
> +	if (!inline_node) {
> +		inline_node = dso__parse_addr_inlines(node->map->dso,
> +						      addr, node->sym);
> +		if (!inline_node)
> +			return 0;
> +		inlines__tree_insert(&node->map->dso->inlined_nodes,
> +				     inline_node);
> +	}
> +
> +	list_for_each_entry(ilist, &inline_node->val, list) {
> +		if (ilist->symbol == node->sym)
> +			break;
> +
> +		printed += __fprintf_callchain_link(node->ip, node->map,
> +						    ilist->symbol,
> +						    ilist->srcline,
> +						    first, left_alignment,
> +						    print_opts, fp);
> +		first = (first && printed == 0);
> +	}
> +
> +	return printed;
> +}
> +
>  int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
>  			      unsigned int print_opts, struct callchain_cursor *cursor,
>  			      FILE *fp)
> @@ -183,6 +226,9 @@ int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
>  			if (!node)
>  				break;
>  
> +			printed += __fprintf_callchain_inlines(node, (printed ==  0),
> +							       left_alignment,
> +							       print_opts, fp);
>  
>  			printed += __fprintf_callchain_link(node->ip, node->map,
>  							    node->sym, NULL,
> -- 
> 2.19.2
> 

[PATCHv2 2/2] perf evsel: add inline functions to sample callchain output

[Jonas Rabenstein]

Whenever a callchain shall be printed search for each address whether
inline information is available and add those symbols to the output
if symbol_conf.inline_name is enabled.

Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
---
v2:
 - fix handling of static binaries

 tools/perf/util/evsel_fprintf.c | 55 +++++++++++++++++++++++++++++----
 1 file changed, 49 insertions(+), 6 deletions(-)

diff --git a/tools/perf/util/evsel_fprintf.c b/tools/perf/util/evsel_fprintf.c
index c710f687ddf4..a8d6465af1aa 100644
--- a/tools/perf/util/evsel_fprintf.c
+++ b/tools/perf/util/evsel_fprintf.c
@@ -168,6 +168,52 @@ static int __fprintf_callchain_link(u64 ip, struct map *map, struct symbol *symb
 	return printed;
 }
 
+static int __fprintf_callchain(struct callchain_cursor_node *node, bool first,
+			       int left_alignment, unsigned int print_opts,
+			       FILE *fp)
+{
+	int printed = 0;
+	struct inline_node *inline_node;
+	struct inline_list *ilist;
+	u64 addr;
+
+	if (!symbol_conf.inline_name)
+		goto no_inline;
+
+	if (!node->map || !node->map->dso)
+		goto no_inline;
+
+	addr = node->map->map_ip(node->map, node->ip);
+	addr = map__rip_2objdump(node->map, addr);
+
+	inline_node = inlines__tree_find(&node->map->dso->inlined_nodes,
+					 addr);
+	if (!inline_node) {
+		inline_node = dso__parse_addr_inlines(node->map->dso,
+						      addr, node->sym);
+		if (!inline_node)
+			goto no_inline;
+		inlines__tree_insert(&node->map->dso->inlined_nodes,
+				     inline_node);
+	}
+
+	list_for_each_entry(ilist, &inline_node->val, list) {
+		printed += __fprintf_callchain_link(node->ip, node->map,
+						    ilist->symbol,
+						    ilist->srcline,
+						    first, left_alignment,
+						    print_opts, fp);
+		first = (first && printed == 0);
+	}
+
+	return printed;
+
+no_inline:
+	return __fprintf_callchain_link(node->ip, node->map, node->sym,
+					NULL, first, left_alignment,
+					print_opts, fp);
+}
+
 int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
 			      unsigned int print_opts, struct callchain_cursor *cursor,
 			      FILE *fp)
@@ -183,12 +229,9 @@ int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
 			if (!node)
 				break;
 
-
-			printed += __fprintf_callchain_link(node->ip, node->map,
-							    node->sym, NULL,
-							    (printed == 0),
-							    left_alignment,
-							    print_opts, fp);
+			printed += __fprintf_callchain(node, (printed == 0),
+						       left_alignment,
+						       print_opts, fp);
 
 			/* Add srccode here too? */
 			if (symbol_conf.bt_stop_list &&
-- 
2.19.2

Re: [PATCH 0/2] perf evsel: add support for inlined function in callchains

[Jiri Olsa]

On Tue, Feb 19, 2019 at 07:38:08PM +0100, Jonas Rabenstein wrote:
> Hi,
> sample__fprintf_callchain currently did not use the already available
> code to get the symbols of an inlined function if such information is
> available in a dso. This patchset adds the required logic to add
> appropriate lines.
> 
> As I am quite new to the code base of perf I am not sure how to test
> that changeset in a correct way. At least the codes builds and the tools
> that make use of sample__fprintf_callchain (perf-script, perf-trace and
> perf-sched as far as I can see) did not fail to run - also I did not get
> into the details of perf-trace and perf-sched as I have never used them
> before.

you could provide some examples that shows what u changed

I can see your changes change the perf script callchains,
displaying less or different callchains:

	--- old 2019-02-20 15:29:34.872312007 +0100
	+++ new 2019-02-20 15:30:47.784725456 +0100
	@@ -23,10 +23,9 @@ yes 11807 203482.590491:     246238 cycl
			      40 [unknown] ([unknown])

	 yes 11807 203482.590654:     534592 cycles:uppp:
	-           7f83e3c7fd0c _dl_addr+0x11c (/usr/lib64/libc-2.27.so)
	+           7f83e3c7fd0c __GI__dl_addr+0x11c (inlined)

	 yes 11807 203482.592371:     455976 cycles:uppp:
	-           55ddec8a3f6e [unknown] (/usr/bin/yes)
		 a790a790a790a79 [unknown] ([unknown])

	 yes 11807 203482.594176:     326514 cycles:uppp:
	@@ -38,11 +37,9 @@ yes 11807 203482.595464:     262867 cycl
		 a790a790a790a79 [unknown] ([unknown])

	 yes 11807 203482.596502:     214762 cycles:uppp:
	-           55ddec8a3f6e [unknown] (/usr/bin/yes)
		 a790a790a790a79 [unknown] ([unknown])


> 
> Another thing I am not sure how to deal with are some warnings of
> checkpatch.pl due to the 80 character line limit. Due to the long
> function names in use the current implementation already exceeded that
> limit in the same spots by even more characters as I have taken the
> inner loop and put it into a separate function.

don't worry much about the long lines warnings

> 
> I hope to expand my work to perf-report but thought it might be usefull
> to get already early feedback on those patches.

yep, good idea

thanks,
jirka