[PATCH/RFC] perf report: Add option to change offset format when address is specified as a sort_key

[PATCH/RFC] perf report: Add option to change offset format when address is specified as a sort_key

[Aaron Tomlin]

With --call-graph option, a user can choose to display call chains with a
specific sort_key. The sort_key can be either: function, address or srcline.
By default, when the address is specified as the sort_key, the offset (i.e.
symbol + offset) is provided in decimal (base 10) form.
This is because in __get_srcline() we use PRIu64 to display the offset.
This may or may not be desirable.

This patch adds a new option --hex, to change the offset format to
hexidecimal. Note, this can only be used with the -g, --call-graph option
when address is specified as a sort_key.  When this option is not
specified, the default behavior is used, which is to display the offset
in decimal form.

Before:

$ ./perf report -n --fields=overhead,sample,period,pid,symbol --stdio \
> -g graph,callee,address --percent-limit 10 --no-children
[ ... ]
    10.87%             1     506940477     15:migration/3   [k] _spin_trylock
            |
            ---_spin_trylock +13
               double_lock_balance +48
               schedule +2007
               migration_thread +613
               kthread +158
               child_rip +10

After:

$ ./perf report -n --fields=overhead,sample,period,pid,symbol --stdio \
> -g graph,callee,address --percent-limit 10 --hex --no-children
[ ... ]
    10.87%             1     506940477     15:migration/3   [k] _spin_trylock
            |
            ---_spin_trylock +0xd
               double_lock_balance +0x30
               schedule +0x7d7
               migration_thread +0x265
               kthread +0x9e
               child_rip +0xa

Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 tools/perf/Documentation/perf-report.txt | 4 ++++
 tools/perf/builtin-report.c              | 2 ++
 tools/perf/util/srcline.c                | 6 ++++--
 tools/perf/util/srcline.h                | 1 +
 4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index ddde2b5..589435a 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -386,6 +386,10 @@ OPTIONS
 	sum of shown entries will be always 100%.  "absolute" means it retains
 	the original value before and after the filter is applied.
 
+--hex::
+	When address is specified as a sort_key, show the offset in
+	hexidecimal form. (Default: decimal)
+
 --header::
 	Show header information in the perf.data file.  This includes
 	various information like hostname, OS and perf version, cpu/mem
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index af5dd03..7820e55 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -866,6 +866,8 @@ int cmd_report(int argc, const char **argv)
 			    itrace_parse_synth_opts),
 	OPT_BOOLEAN(0, "full-source-path", &srcline_full_filename,
 			"Show full source file name path for source lines"),
+	OPT_BOOLEAN(0, "hex", &show_hex_offset,
+			"When address is specified as a sort_key, show the offset in hexidecimal form"),
 	OPT_BOOLEAN(0, "show-ref-call-graph", &symbol_conf.show_ref_callgraph,
 		    "Show callgraph from reference event"),
 	OPT_INTEGER(0, "socket-filter", &report.socket_filter,
diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index d19f05c..98fc911 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -15,6 +15,7 @@
 #include "symbol.h"
 
 bool srcline_full_filename;
+bool show_hex_offset;
 
 static const char *dso__name(struct dso *dso)
 {
@@ -535,8 +536,9 @@ char *__get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
 			    strndup(sym->name, sym->namelen) : NULL;
 
 	if (sym) {
-		if (asprintf(&srcline, "%s+%" PRIu64, show_sym ? sym->name : "",
-					addr - sym->start) < 0)
+		if (asprintf(&srcline,
+			show_hex_offset ? "%s+%#" PRIx64 : "%s+%" PRIu64,
+			show_sym ? sym->name : "", addr - sym->start) < 0)
 			return SRCLINE_UNKNOWN;
 	} else if (asprintf(&srcline, "%s[%" PRIx64 "]", dso->short_name, addr) < 0)
 		return SRCLINE_UNKNOWN;
diff --git a/tools/perf/util/srcline.h b/tools/perf/util/srcline.h
index 847b708..1bca068 100644
--- a/tools/perf/util/srcline.h
+++ b/tools/perf/util/srcline.h
@@ -10,6 +10,7 @@ struct dso;
 struct symbol;
 
 extern bool srcline_full_filename;
+extern bool show_hex_offset;
 char *get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
 		  bool show_sym, bool show_addr);
 char *__get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
-- 
2.9.5

Re: [PATCH/RFC] perf report: Add option to change offset format when address is specified as a sort_key

[Jiri Olsa]

On Fri, Dec 15, 2017 at 03:35:10PM +0000, Aaron Tomlin wrote:
> With --call-graph option, a user can choose to display call chains with a
> specific sort_key. The sort_key can be either: function, address or srcline.
> By default, when the address is specified as the sort_key, the offset (i.e.
> symbol + offset) is provided in decimal (base 10) form.
> This is because in __get_srcline() we use PRIu64 to display the offset.
> This may or may not be desirable.
> 
> This patch adds a new option --hex, to change the offset format to
> hexidecimal. Note, this can only be used with the -g, --call-graph option
> when address is specified as a sort_key.  When this option is not
> specified, the default behavior is used, which is to display the offset
> in decimal form.
> 
> Before:
> 
> $ ./perf report -n --fields=overhead,sample,period,pid,symbol --stdio \
> > -g graph,callee,address --percent-limit 10 --no-children
> [ ... ]
>     10.87%             1     506940477     15:migration/3   [k] _spin_trylock
>             |
>             ---_spin_trylock +13
>                double_lock_balance +48
>                schedule +2007
>                migration_thread +613
>                kthread +158
>                child_rip +10
> 
> After:
> 
> $ ./perf report -n --fields=overhead,sample,period,pid,symbol --stdio \
> > -g graph,callee,address --percent-limit 10 --hex --no-children
> [ ... ]
>     10.87%             1     506940477     15:migration/3   [k] _spin_trylock
>             |
>             ---_spin_trylock +0xd
>                double_lock_balance +0x30
>                schedule +0x7d7
>                migration_thread +0x265
>                kthread +0x9e
>                child_rip +0xa

not sure we've already discussed that, but this could be default?

if not, I think the hex option should be part of -g option arg, maybe like:

  -g graph,callee,xaddress

but default hex seems better to me, thoughts?

jirka

Re: [PATCH/RFC] perf report: Add option to change offset format when address is specified as a sort_key

[Aaron Tomlin]

[-- Attachment #1: Type: text/plain, Size: 634 bytes --]

On Sat 2017-12-23 00:59 +0100, Jiri Olsa wrote:
[ ... ]
> not sure we've already discussed that, but this could be default?

Probably it is best to keep the default behaviour.

I'd prefer a hexadecimal address offset, as the default, however perhaps
someone is happy with the current default (decimal).

> if not, I think the hex option should be part of -g option arg, maybe like:

Does it have to be?

>   -g graph,callee,xaddress

Not sure - adding another sort_key seems odd to me.

But if you insist, perhaps address[=hex] would be cleaner?

Otherwise I would prefer --hex.


Regards,

-- 
Aaron Tomlin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH/RFC] perf report: Add option to change offset format when address is specified as a sort_key

[Jiri Olsa]

On Sun, Dec 24, 2017 at 06:23:15PM +0000, Aaron Tomlin wrote:
> On Sat 2017-12-23 00:59 +0100, Jiri Olsa wrote:
> [ ... ]
> > not sure we've already discussed that, but this could be default?
> 
> Probably it is best to keep the default behaviour.
> 
> I'd prefer a hexadecimal address offset, as the default, however perhaps
> someone is happy with the current default (decimal).

Arnaldo?

> 
> > if not, I think the hex option should be part of -g option arg, maybe like:
> 
> Does it have to be?
> 
> >   -g graph,callee,xaddress
> 
> Not sure - adding another sort_key seems odd to me.

it's not sort key, it's -g option key.. but if we go for default
there's no need.. let's see ;-)

> 
> But if you insist, perhaps address[=hex] would be cleaner?

but yes, that looks better to me

jirka

Re: [PATCH/RFC] perf report: Add option to change offset format when address is specified as a sort_key

[Arnaldo Carvalho de Melo]

Em Fri, Dec 15, 2017 at 03:35:10PM +0000, Aaron Tomlin escreveu:
> With --call-graph option, a user can choose to display call chains with a
> specific sort_key. The sort_key can be either: function, address or srcline.
> By default, when the address is specified as the sort_key, the offset (i.e.
> symbol + offset) is provided in decimal (base 10) form.

> This is because in __get_srcline() we use PRIu64 to display the offset.
> This may or may not be desirable.
> 
> This patch adds a new option --hex, to change the offset format to
> hexidecimal. Note, this can only be used with the -g, --call-graph option
> when address is specified as a sort_key.  When this option is not
> specified, the default behavior is used, which is to display the offset
> in decimal form.
> 
> Before:
> 
> $ ./perf report -n --fields=overhead,sample,period,pid,symbol --stdio \
> > -g graph,callee,address --percent-limit 10 --no-children
> [ ... ]
>     10.87%             1     506940477     15:migration/3   [k] _spin_trylock
>             |
>             ---_spin_trylock +13
>                double_lock_balance +48
>                schedule +2007
>                migration_thread +613
>                kthread +158
>                child_rip +10

Humm, trying to reproduce this with the command line you provided above,
in your 'Before:' case:

# perf record -F 10000 -g --pid 10,15,21,27
# perf report -n --fields=overhead,sample,period,pid,symbol --stdio -g graph,callee,address --no-children

     7.31%             1        205216       10:migration/0  [k] _raw_spin_lock
            |
            ---_raw_spin_lock atomic.h:187
               scheduler_tick sched.h:935
               update_process_times timer.c:1637
               tick_sched_handle tick-sched.c:163
               tick_sched_timer tick-sched.c:1171
               __hrtimer_run_queues hrtimer.c:1211
               hrtimer_interrupt hrtimer.c:469
               smp_apic_timer_interrupt apic.c:1025
               apic_timer_interrupt entry_64.S:795
               finish_task_switch current.h:15
               __sched_text_start core.c:2802
               schedule current.h:15
               smpboot_thread_fn smpboot.c:160
               kthread kthread.c:238
               ret_from_fork entry_64.S:447

[root@jouet ~]# perf --version
perf version 4.15.rc4.g6c7919a

This is with is in my perf/core branch, but I bet you'll get the same
result with 4.15-rc4.

Humm, so this is because you're not using a vmlinux file, then the
srcline gets resolved and get_srcline() returns before getting to the
fallback where offsets are shown, if I do:

[root@jouet ~]# mv /lib/modules/4.15.0-rc3+/build/vmlinux /lib/modules/4.15.0-rc3+/build/vmlinux.OFF

and remove the cached entry in my ~/.debug/ build-id cache, I get to:

# symbol: _raw_spin_lock
#
# Total Lost Samples: 0
#
# Samples: 54  of event 'cycles:ppp'
# Event count (approx.): 2805651
#
# Overhead       Samples        Period      Pid:Command      Source:Line                        
# ........  ............  ............  ...................  ...................................
#
     7.31%             1        205216       10:migration/0  _raw_spin_lock+18446603336221184012
            |
            ---_raw_spin_lock +18446603336221184012
               scheduler_tick +18446603336221184060
               update_process_times +18446603336221184064
               tick_sched_handle +18446603336221184034
               tick_sched_timer +18446603336221184052
               __hrtimer_run_queues +18446603336221184214
               hrtimer_interrupt +18446603336221184166
               smp_apic_timer_interrupt +18446603336221184086
               __irqentry_text_start +18446603336221184150
               finish_task_switch +18446603336221184123
               __sched_text_start +18446603336221184577
               schedule +18446603336221184040
               smpboot_thread_fn +18446603336221184323
               kthread +18446603336221184273
               ret_from_fork +18446603336221184031



#
[root@jouet ~]#

Which looks buggy... bisecting...

- Arnaldo