[GIT PULL 00/21] perf/core improvements and fixes

[GIT PULL 00/21] perf/core improvements and fixes

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>

Hi Ingo,

	Please consider pulling,

Best Regards,

-Arnaldo

The following changes since commit 6d65894bc028d0342829ea1e64c9e9efad571124:

  tools lib traceevent: Update kvm plugin with is_writable_pte helper (2013-12-04 15:38:14 -0300)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux tags/perf-core-for-mingo

for you to fetch changes up to e993d10caeb6dca690dbaf86e1981ba240d1414a:

  perf symbols: fix bug in usage of the basename() function (2013-12-09 15:41:59 -0300)

----------------------------------------------------------------
perf/core improvements and fixes:

. Add an option in 'perf script' to print the source line number, from Adrian Hunter

. Fix symoff printing in callchains in 'perf script', from Adrian Hunter.

. Assorted mmap_pages handling fixes, from Adrian Hunter.

. Fix summary percentage when processing files in 'perf trace', fom David Ahern.

. Handle old kernels where the "raw_syscalls" tracepoints were called plan "syscalls",
  in 'perf trace', from David Ahern.

. Several man pages typo fixes from Dongsheng Yang.

. Add '-v' option to 'perf kvm', from Dongsheng Yang.

. Make perf kvm diff support --guestmount, from Dongsheng Yang.

. Get rid of several die() calls in libtraceevent, from Namhyung Kim.

. Use basename() in a more robust way, to avoid problems related to different
  system library implementations for that function, from Stephane Eranian.

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

----------------------------------------------------------------
Adrian Hunter (6):
      perf script: Fix symoff printing in callchains
      perf script: Add an option to print the source line number
      perf record: Fix display of incorrect mmap pages
      perf evlist: Remove unnecessary parentheses
      perf evlist: Fix max mmap_pages
      perf evlist: Fix mmap pages rounding to power of 2

David Ahern (2):
      perf trace: Add support for syscalls vs raw_syscalls
      perf trace: Fix summary percentage when processing files

Dongsheng Yang (6):
      perf kvm: Introduce option -v for perf kvm command.
      perf kvm: Fix bug in 'stat report'
      perf archive: Remove duplicated 'runs' in man page
      perf annotate: Fix typo
      perf kvm: Move code to generate filename for perf-kvm to function.
      perf kvm: Make perf kvm diff support --guestmount.

Namhyung Kim (5):
      tools lib traceevent: Get rid of malloc_or_die() in pevent_filter_alloc()
      tools lib traceevent: Get rid of malloc_or_die() in add_event()
      tools lib traceevent: Get rid of die() in create_arg_item()
      tools lib traceevent: Get rid of malloc_or_die() in pevent_filter_add_filter_str()
      tools lib traceevent: Get rid of die() in pevent_filter_clear_trivial()

Stephane Eranian (1):
      perf symbols: fix bug in usage of the basename() function

Steven Rostedt (1):
      tools lib traceevent: Report better error message on bad function args

 tools/lib/traceevent/event-parse.c        | 28 +++++++++------
 tools/lib/traceevent/event-parse.h        |  2 +-
 tools/lib/traceevent/parse-filter.c       | 57 ++++++++++++++++++++++++-------
 tools/perf/Documentation/perf-archive.txt |  6 ++--
 tools/perf/Documentation/perf-kvm.txt     |  7 ++--
 tools/perf/Documentation/perf-script.txt  |  2 +-
 tools/perf/builtin-annotate.c             |  2 +-
 tools/perf/builtin-diff.c                 |  3 +-
 tools/perf/builtin-kvm.c                  | 11 +++---
 tools/perf/builtin-record.c               |  2 +-
 tools/perf/builtin-script.c               | 10 ++++++
 tools/perf/builtin-trace.c                | 32 +++++++++++++++--
 tools/perf/util/dso.c                     | 29 +++++++++++++++-
 tools/perf/util/evlist.c                  | 10 +++---
 tools/perf/util/map.c                     | 17 +++++++++
 tools/perf/util/map.h                     |  2 ++
 tools/perf/util/session.c                 | 15 +++++++-
 tools/perf/util/session.h                 |  1 +
 tools/perf/util/util.c                    | 14 ++++++++
 tools/perf/util/util.h                    | 14 ++++++++
 20 files changed, 214 insertions(+), 50 deletions(-)

[PATCH 01/21] perf trace: Add support for syscalls vs raw_syscalls

[Arnaldo Carvalho de Melo]

From: David Ahern <dsahern@gmail.com>

Older kernels (e.g., RHEL6) do system call tracing via
syscalls:sys_{enter,exit} rather than raw_syscalls. Update perf-trace to
detect lack of raw_syscalls support and try syscalls.

Signed-off-by: David Ahern <dsahern@gmail.com>
Link: http://lkml.kernel.org/r/1386211302-31303-2-git-send-email-dsahern@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-trace.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 56afe339661a..a7aa771a98e6 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -12,6 +12,7 @@
 #include "util/thread_map.h"
 #include "util/stat.h"
 #include "trace-event.h"
+#include "util/parse-events.h"
 
 #include <libaudit.h>
 #include <stdlib.h>
@@ -173,6 +174,10 @@ static struct perf_evsel *perf_evsel__syscall_newtp(const char *direction, void
 {
 	struct perf_evsel *evsel = perf_evsel__newtp("raw_syscalls", direction);
 
+	/* older kernel (e.g., RHEL6) use syscalls:{enter,exit} */
+	if (evsel == NULL)
+		evsel = perf_evsel__newtp("syscalls", direction);
+
 	if (evsel) {
 		if (perf_evsel__init_syscall_tp(evsel, handler))
 			goto out_delete;
@@ -1801,10 +1806,11 @@ static int trace__record(int argc, const char **argv)
 		"-R",
 		"-m", "1024",
 		"-c", "1",
-		"-e", "raw_syscalls:sys_enter,raw_syscalls:sys_exit",
+		"-e",
 	};
 
-	rec_argc = ARRAY_SIZE(record_args) + argc;
+	/* +1 is for the event string below */
+	rec_argc = ARRAY_SIZE(record_args) + 1 + argc;
 	rec_argv = calloc(rec_argc + 1, sizeof(char *));
 
 	if (rec_argv == NULL)
@@ -1813,6 +1819,17 @@ static int trace__record(int argc, const char **argv)
 	for (i = 0; i < ARRAY_SIZE(record_args); i++)
 		rec_argv[i] = record_args[i];
 
+	/* event string may be different for older kernels - e.g., RHEL6 */
+	if (is_valid_tracepoint("raw_syscalls:sys_enter"))
+		rec_argv[i] = "raw_syscalls:sys_enter,raw_syscalls:sys_exit";
+	else if (is_valid_tracepoint("syscalls:sys_enter"))
+		rec_argv[i] = "syscalls:sys_enter,syscalls:sys_exit";
+	else {
+		pr_err("Neither raw_syscalls nor syscalls events exist.\n");
+		return -1;
+	}
+	i++;
+
 	for (j = 0; j < (unsigned int)argc; j++, i++)
 		rec_argv[i] = argv[j];
 
@@ -2048,6 +2065,10 @@ static int trace__replay(struct trace *trace)
 
 	evsel = perf_evlist__find_tracepoint_by_name(session->evlist,
 						     "raw_syscalls:sys_enter");
+	/* older kernels have syscalls tp versus raw_syscalls */
+	if (evsel == NULL)
+		evsel = perf_evlist__find_tracepoint_by_name(session->evlist,
+							     "syscalls:sys_enter");
 	if (evsel == NULL) {
 		pr_err("Data file does not have raw_syscalls:sys_enter event\n");
 		goto out;
@@ -2061,6 +2082,9 @@ static int trace__replay(struct trace *trace)
 
 	evsel = perf_evlist__find_tracepoint_by_name(session->evlist,
 						     "raw_syscalls:sys_exit");
+	if (evsel == NULL)
+		evsel = perf_evlist__find_tracepoint_by_name(session->evlist,
+							     "syscalls:sys_exit");
 	if (evsel == NULL) {
 		pr_err("Data file does not have raw_syscalls:sys_exit event\n");
 		goto out;
-- 
1.8.1.4

[PATCH 02/21] perf trace: Fix summary percentage when processing files

[Arnaldo Carvalho de Melo]

From: David Ahern <dsahern@gmail.com>

Getting a divide by 0 when events are processed from a file:

   perf trace -i perf.data -s
   ...
   dnsmasq (1684), 10 events, inf%, 0.000 msec

The problem is that the event count is not incremented as events are
processed. With this patch:

   perf trace -i perf.data -s
   ...
   dnsmasq (1684), 10 events, 8.9%, 0.000 msec

Signed-off-by: David Ahern <dsahern@gmail.com>
Link: http://lkml.kernel.org/r/1386211302-31303-4-git-send-email-dsahern@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-trace.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index a7aa771a98e6..56bbca5bc2dc 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -1770,8 +1770,10 @@ static int trace__process_sample(struct perf_tool *tool,
 	if (!trace->full_time && trace->base_time == 0)
 		trace->base_time = sample->time;
 
-	if (handler)
+	if (handler) {
+		++trace->nr_events;
 		handler(trace, evsel, sample);
+	}
 
 	return err;
 }
-- 
1.8.1.4

[PATCH 03/21] tools lib traceevent: Report better error message on bad function args

[Arnaldo Carvalho de Melo]

From: Steven Rostedt <rostedt@goodmis.org>

When Jiri Olsa was writing a function callback for
scsi_trace_parse_cdb(), he thought that the traceevent library had a
bug in it because he was getting this error:

  Error: expected ')' but read ','
  Error: expected ')' but read ','
  Error: expected ')' but read ','
  Error: expected ')' but read ','

But in truth, he didn't have the write number of arguments for the
function callback, and the error was the library detecting the
discrepancy. A better error message would have prevented the confusion:

  Error: function 'scsi_trace_parse_cdb()' only expects 2 arguments but event scsi_dispatch_cmd_timeout has more
  Error: function 'scsi_trace_parse_cdb()' only expects 2 arguments but event scsi_dispatch_cmd_start has more
  Error: function 'scsi_trace_parse_cdb()' only expects 2 arguments but event scsi_dispatch_cmd_error has more
  Error: function 'scsi_trace_parse_cdb()' only expects 2 arguments but event scsi_dispatch_cmd_done has more

Or

  Error: function 'scsi_trace_parse_cdb()' expects 4 arguments but event scsi_dispatch_cmd_timeout only uses 3
  Error: function 'scsi_trace_parse_cdb()' expects 4 arguments but event scsi_dispatch_cmd_start only uses 3
  Error: function 'scsi_trace_parse_cdb()' expects 4 arguments but event scsi_dispatch_cmd_error only uses 3
  Error: function 'scsi_trace_parse_cdb()' expects 4 arguments but event scsi_dispatch_cmd_done only uses 3

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/n/tip-a4c34w62vl0diitvxb7bt3er@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/traceevent/event-parse.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.c b/tools/lib/traceevent/event-parse.c
index 9849873265d4..22566c271275 100644
--- a/tools/lib/traceevent/event-parse.c
+++ b/tools/lib/traceevent/event-parse.c
@@ -2710,7 +2710,6 @@ process_func_handler(struct event_format *event, struct pevent_function_handler
 	struct print_arg *farg;
 	enum event_type type;
 	char *token;
-	const char *test;
 	int i;
 
 	arg->type = PRINT_FUNC;
@@ -2727,15 +2726,19 @@ process_func_handler(struct event_format *event, struct pevent_function_handler
 		}
 
 		type = process_arg(event, farg, &token);
-		if (i < (func->nr_args - 1))
-			test = ",";
-		else
-			test = ")";
-
-		if (test_type_token(type, token, EVENT_DELIM, test)) {
-			free_arg(farg);
-			free_token(token);
-			return EVENT_ERROR;
+		if (i < (func->nr_args - 1)) {
+			if (type != EVENT_DELIM || strcmp(token, ",") != 0) {
+				warning("Error: function '%s()' expects %d arguments but event %s only uses %d",
+					func->name, func->nr_args,
+					event->name, i + 1);
+				goto err;
+			}
+		} else {
+			if (type != EVENT_DELIM || strcmp(token, ")") != 0) {
+				warning("Error: function '%s()' only expects %d arguments but event %s has more",
+					func->name, func->nr_args, event->name);
+				goto err;
+			}
 		}
 
 		*next_arg = farg;
@@ -2747,6 +2750,11 @@ process_func_handler(struct event_format *event, struct pevent_function_handler
 	*tok = token;
 
 	return type;
+
+err:
+	free_arg(farg);
+	free_token(token);
+	return EVENT_ERROR;
 }
 
 static enum event_type
-- 
1.8.1.4

[PATCH 04/21] perf script: Fix symoff printing in callchains

[Arnaldo Carvalho de Melo]

From: Adrian Hunter <adrian.hunter@intel.com>

The address being used to calculate the offset was the memory address
but the address needed is the address mapped to the dso. i.e. the 'addr'
member of 'struct addr_location'

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Acked-by: David Ahern <dsahern@gmail.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1386315778-11633-2-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/session.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 8a7da6f4a569..c236b38ed02b 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1515,6 +1515,8 @@ void perf_evsel__print_ip(struct perf_evsel *evsel, struct perf_sample *sample,
 			node_al = *al;
 
 		while (stack_depth) {
+			u64 addr = 0;
+
 			node = callchain_cursor_current(&callchain_cursor);
 			if (!node)
 				break;
@@ -1525,10 +1527,13 @@ void perf_evsel__print_ip(struct perf_evsel *evsel, struct perf_sample *sample,
 			if (print_ip)
 				printf("%c%16" PRIx64, s, node->ip);
 
+			if (node->map)
+				addr = node->map->map_ip(node->map, node->ip);
+
 			if (print_sym) {
 				printf(" ");
 				if (print_symoffset) {
-					node_al.addr = node->ip;
+					node_al.addr = addr;
 					node_al.map  = node->map;
 					symbol__fprintf_symname_offs(node->sym, &node_al, stdout);
 				} else
-- 
1.8.1.4

[PATCH 05/21] perf script: Add an option to print the source line number

[Arnaldo Carvalho de Melo]

From: Adrian Hunter <adrian.hunter@intel.com>

Add field 'srcline' that displays the source file name and line number
associated with the sample ip.  The information displayed is the same as
from addr2line.

 $ perf script -f comm,tid,pid,time,ip,sym,dso,symoff,srcline
            grep 10701/10701 2497321.421013:  ffffffff81043ffa native_write_msr_safe+0xa ([kernel.kallsyms])
  /usr/src/debug/kernel-3.9.fc17/linux-3.9.10-100.fc17.x86_64/arch/x86/include/asm/msr.h:95
            grep 10701/10701 2497321.421984:  ffffffff8165b6b3 _raw_spin_lock+0x13 ([kernel.kallsyms])
  /usr/src/debug/kernel-3.9.fc17/linux-3.9.10-100.fc17.x86_64/arch/x86/include/asm/spinlock.h:54
            grep 10701/10701 2497321.421990:  ffffffff810b64b3 tick_sched_timer+0x53 ([kernel.kallsyms])
  /usr/src/debug/kernel-3.9.fc17/linux-3.9.10-100.fc17.x86_64/kernel/time/tick-sched.c:840
            grep 10701/10701 2497321.421992:  ffffffff8106f63f run_timer_softirq+0x2f ([kernel.kallsyms])
  /usr/src/debug/kernel-3.9.fc17/linux-3.9.10-100.fc17.x86_64/kernel/timer.c:1372

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1386315778-11633-3-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Documentation/perf-script.txt |  2 +-
 tools/perf/builtin-script.c              | 10 ++++++++++
 tools/perf/util/map.c                    | 17 +++++++++++++++++
 tools/perf/util/map.h                    |  2 ++
 tools/perf/util/session.c                |  8 ++++++++
 tools/perf/util/session.h                |  1 +
 6 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/tools/perf/Documentation/perf-script.txt b/tools/perf/Documentation/perf-script.txt
index cfdbb1e045b5..c2a5071cf8f8 100644
--- a/tools/perf/Documentation/perf-script.txt
+++ b/tools/perf/Documentation/perf-script.txt
@@ -115,7 +115,7 @@ OPTIONS
 -f::
 --fields::
         Comma separated list of fields to print. Options are:
-        comm, tid, pid, time, cpu, event, trace, ip, sym, dso, addr, symoff.
+        comm, tid, pid, time, cpu, event, trace, ip, sym, dso, addr, symoff, srcline.
         Field list can be prepended with the type, trace, sw or hw,
         to indicate to which event type the field list applies.
         e.g., -f sw:comm,tid,time,ip,sym  and -f trace:time,cpu,trace
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 4484886dcf08..7a571fb7eb8a 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -43,6 +43,7 @@ enum perf_output_field {
 	PERF_OUTPUT_DSO             = 1U << 9,
 	PERF_OUTPUT_ADDR            = 1U << 10,
 	PERF_OUTPUT_SYMOFFSET       = 1U << 11,
+	PERF_OUTPUT_SRCLINE         = 1U << 12,
 };
 
 struct output_option {
@@ -61,6 +62,7 @@ struct output_option {
 	{.str = "dso",   .field = PERF_OUTPUT_DSO},
 	{.str = "addr",  .field = PERF_OUTPUT_ADDR},
 	{.str = "symoff", .field = PERF_OUTPUT_SYMOFFSET},
+	{.str = "srcline", .field = PERF_OUTPUT_SRCLINE},
 };
 
 /* default set to maintain compatibility with current format */
@@ -210,6 +212,11 @@ static int perf_evsel__check_attr(struct perf_evsel *evsel,
 		       "to DSO.\n");
 		return -EINVAL;
 	}
+	if (PRINT_FIELD(SRCLINE) && !PRINT_FIELD(IP)) {
+		pr_err("Display of source line number requested but sample IP is not\n"
+		       "selected. Hence, no address to lookup the source line number.\n");
+		return -EINVAL;
+	}
 
 	if ((PRINT_FIELD(PID) || PRINT_FIELD(TID)) &&
 		perf_evsel__check_stype(evsel, PERF_SAMPLE_TID, "TID",
@@ -245,6 +252,9 @@ static void set_print_ip_opts(struct perf_event_attr *attr)
 
 	if (PRINT_FIELD(SYMOFFSET))
 		output[type].print_ip_opts |= PRINT_IP_OPT_SYMOFFSET;
+
+	if (PRINT_FIELD(SRCLINE))
+		output[type].print_ip_opts |= PRINT_IP_OPT_SRCLINE;
 }
 
 /*
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index ef5bc913ca7a..9b9bd719aa19 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -11,6 +11,7 @@
 #include "strlist.h"
 #include "vdso.h"
 #include "build-id.h"
+#include "util.h"
 #include <linux/string.h>
 
 const char *map_type__name[MAP__NR_TYPES] = {
@@ -252,6 +253,22 @@ size_t map__fprintf_dsoname(struct map *map, FILE *fp)
 	return fprintf(fp, "%s", dsoname);
 }
 
+int map__fprintf_srcline(struct map *map, u64 addr, const char *prefix,
+			 FILE *fp)
+{
+	char *srcline;
+	int ret = 0;
+
+	if (map && map->dso) {
+		srcline = get_srcline(map->dso,
+				      map__rip_2objdump(map, addr));
+		if (srcline != SRCLINE_UNKNOWN)
+			ret = fprintf(fp, "%s%s", prefix, srcline);
+		free_srcline(srcline);
+	}
+	return ret;
+}
+
 /**
  * map__rip_2objdump - convert symbol start address to objdump address.
  * @map: memory map
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index e4e259c3ba16..18068c6b71c1 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -103,6 +103,8 @@ struct map *map__clone(struct map *map);
 int map__overlap(struct map *l, struct map *r);
 size_t map__fprintf(struct map *map, FILE *fp);
 size_t map__fprintf_dsoname(struct map *map, FILE *fp);
+int map__fprintf_srcline(struct map *map, u64 addr, const char *prefix,
+			 FILE *fp);
 
 int map__load(struct map *map, symbol_filter_t filter);
 struct symbol *map__find_symbol(struct map *map,
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index c236b38ed02b..e748f29c53cf 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -1497,6 +1497,7 @@ void perf_evsel__print_ip(struct perf_evsel *evsel, struct perf_sample *sample,
 	int print_dso = print_opts & PRINT_IP_OPT_DSO;
 	int print_symoffset = print_opts & PRINT_IP_OPT_SYMOFFSET;
 	int print_oneline = print_opts & PRINT_IP_OPT_ONELINE;
+	int print_srcline = print_opts & PRINT_IP_OPT_SRCLINE;
 	char s = print_oneline ? ' ' : '\t';
 
 	if (symbol_conf.use_callchain && sample->callchain) {
@@ -1546,6 +1547,10 @@ void perf_evsel__print_ip(struct perf_evsel *evsel, struct perf_sample *sample,
 				printf(")");
 			}
 
+			if (print_srcline)
+				map__fprintf_srcline(node->map, addr, "\n  ",
+						     stdout);
+
 			if (!print_oneline)
 				printf("\n");
 
@@ -1575,6 +1580,9 @@ next:
 			map__fprintf_dsoname(al->map, stdout);
 			printf(")");
 		}
+
+		if (print_srcline)
+			map__fprintf_srcline(al->map, al->addr, "\n  ", stdout);
 	}
 }
 
diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
index 004d3e8116aa..2a3955ea4fd8 100644
--- a/tools/perf/util/session.h
+++ b/tools/perf/util/session.h
@@ -45,6 +45,7 @@ struct perf_session {
 #define PRINT_IP_OPT_DSO		(1<<2)
 #define PRINT_IP_OPT_SYMOFFSET	(1<<3)
 #define PRINT_IP_OPT_ONELINE	(1<<4)
+#define PRINT_IP_OPT_SRCLINE	(1<<5)
 
 struct perf_tool;
 
-- 
1.8.1.4

[PATCH 06/21] perf record: Fix display of incorrect mmap pages

[Arnaldo Carvalho de Melo]

From: Adrian Hunter <adrian.hunter@intel.com>

'mmap_pages' is 'unsigned int' not 'int' e.g.

  perf record -m2147483648 uname
  Permission error mapping pages.
  Consider increasing /proc/sys/kernel/perf_event_mlock_kb,
  or try again with a smaller value of -m/--mmap_pages.
  (current value: -2147483648)

Fixed:

  perf record -m2147483648 uname
  Permission error mapping pages.
  Consider increasing /proc/sys/kernel/perf_event_mlock_kb,
  or try again with a smaller value of -m/--mmap_pages.
  (current value: 2147483648)

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1386595120-22978-5-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-record.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index d93e2eef0979..c1c1200d2f0a 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -224,7 +224,7 @@ try_again:
 			       "Consider increasing "
 			       "/proc/sys/kernel/perf_event_mlock_kb,\n"
 			       "or try again with a smaller value of -m/--mmap_pages.\n"
-			       "(current value: %d)\n", opts->mmap_pages);
+			       "(current value: %u)\n", opts->mmap_pages);
 			rc = -errno;
 		} else {
 			pr_err("failed to mmap with %d (%s)\n", errno, strerror(errno));
-- 
1.8.1.4

[PATCH 07/21] perf evlist: Remove unnecessary parentheses

[Arnaldo Carvalho de Melo]

From: Adrian Hunter <adrian.hunter@intel.com>

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1386595120-22978-2-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 7bb6ee1ca19f..4d0945c96ab2 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -732,7 +732,7 @@ static long parse_pages_arg(const char *str, unsigned long min,
 			return -EINVAL;
 	}
 
-	if ((pages == 0) && (min == 0)) {
+	if (pages == 0 && min == 0) {
 		/* leave number of pages at 0 */
 	} else if (pages < (1UL << 31) && !is_power_of_2(pages)) {
 		/* round pages up to next power of 2 */
-- 
1.8.1.4

[PATCH 08/21] perf evlist: Fix max mmap_pages

[Arnaldo Carvalho de Melo]

From: Adrian Hunter <adrian.hunter@intel.com>

'SIZE_MAX / page_size' is an upper limit for the maximum number of mmap
pages, not a lower limit.  Change the condition accordingly.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1386595120-22978-3-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 4d0945c96ab2..98ec96b3a744 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -754,7 +754,7 @@ int perf_evlist__parse_mmap_pages(const struct option *opt, const char *str,
 	unsigned long max = UINT_MAX;
 	long pages;
 
-	if (max < SIZE_MAX / page_size)
+	if (max > SIZE_MAX / page_size)
 		max = SIZE_MAX / page_size;
 
 	pages = parse_pages_arg(str, 1, max);
-- 
1.8.1.4

[PATCH 09/21] perf evlist: Fix mmap pages rounding to power of 2

[Arnaldo Carvalho de Melo]

From: Adrian Hunter <adrian.hunter@intel.com>

'next_pow2()' only works for 'unsigned int' but the argument is
'unsigned long'.  Checking for values less than (1 << 31) ensures that
'next_pow2()' is not passed a value out of range but lets anything else
go through unvalidated.

As a result mmap_pages of zero is used e.g.

  perf record -v -m2147483649 uname
  mmap size 0B
  failed to mmap with 22 (Invalid argument)

Fixed:

  perf record -m2147483649 uname
  rounding mmap pages size to 17592186044416 bytes (4294967296 pages)
  Invalid argument for --mmap_pages/-m

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
Link: http://lkml.kernel.org/r/1386595120-22978-4-git-send-email-adrian.hunter@intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/evlist.c |  6 ++++--
 tools/perf/util/util.h   | 12 ++++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 98ec96b3a744..af250556b33f 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -734,9 +734,11 @@ static long parse_pages_arg(const char *str, unsigned long min,
 
 	if (pages == 0 && min == 0) {
 		/* leave number of pages at 0 */
-	} else if (pages < (1UL << 31) && !is_power_of_2(pages)) {
+	} else if (!is_power_of_2(pages)) {
 		/* round pages up to next power of 2 */
-		pages = next_pow2(pages);
+		pages = next_pow2_l(pages);
+		if (!pages)
+			return -EINVAL;
 		pr_info("rounding mmap pages size to %lu bytes (%lu pages)\n",
 			pages * page_size, pages);
 	}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index adb39f251f90..659abf30e01b 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -73,6 +73,7 @@
 #include <sys/ttydefaults.h>
 #include <lk/debugfs.h>
 #include <termios.h>
+#include <linux/bitops.h>
 
 extern const char *graph_line;
 extern const char *graph_dotted_line;
@@ -281,6 +282,17 @@ static inline unsigned next_pow2(unsigned x)
 	return 1ULL << (32 - __builtin_clz(x - 1));
 }
 
+static inline unsigned long next_pow2_l(unsigned long x)
+{
+#if BITS_PER_LONG == 64
+	if (x <= (1UL << 31))
+		return next_pow2(x);
+	return (unsigned long)next_pow2(x >> 32) << 32;
+#else
+	return next_pow2(x);
+#endif
+}
+
 size_t hex_width(u64 v);
 int hex2u64(const char *ptr, u64 *val);
 
-- 
1.8.1.4

[PATCH 10/21] perf kvm: Introduce option -v for perf kvm command.

[Arnaldo Carvalho de Melo]

From: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>

As there is no -v option for perf kvm, the all debug message for perf
kvm will nerver be printed out to user.

Example:
	# perf kvm --guestmount /tmp/guestmount/ record -a
	Not enough memory for reading perf file header

It is confusing message for newbies such as me. With this patch applied,
we can use -v option to get the detail.

Example:
	# perf kvm --guestmount /tmp/guestmount/ record -a -v
	Can't access file /tmp/guestmount//15069/proc/kallsyms
	Not enough memory for reading perf file header

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Cc: David Ahern <dsahern@gmail.com>
Link: http://lkml.kernel.org/r/1386609311-23889-1-git-send-email-yangds.fnst@cn.fujitsu.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Documentation/perf-kvm.txt | 7 +++++--
 tools/perf/builtin-kvm.c              | 2 ++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Documentation/perf-kvm.txt b/tools/perf/Documentation/perf-kvm.txt
index 96a9a1dea727..52276a6d2b75 100644
--- a/tools/perf/Documentation/perf-kvm.txt
+++ b/tools/perf/Documentation/perf-kvm.txt
@@ -10,9 +10,9 @@ SYNOPSIS
 [verse]
 'perf kvm' [--host] [--guest] [--guestmount=<path>
 	[--guestkallsyms=<path> --guestmodules=<path> | --guestvmlinux=<path>]]
-	{top|record|report|diff|buildid-list}
+	{top|record|report|diff|buildid-list} [<options>]
 'perf kvm' [--host] [--guest] [--guestkallsyms=<path> --guestmodules=<path>
-	| --guestvmlinux=<path>] {top|record|report|diff|buildid-list|stat}
+	| --guestvmlinux=<path>] {top|record|report|diff|buildid-list|stat} [<options>]
 'perf kvm stat [record|report|live] [<options>]
 
 DESCRIPTION
@@ -93,6 +93,9 @@ OPTIONS
 	kernel module information. Users copy it out from guest os.
 --guestvmlinux=<path>::
 	Guest os kernel vmlinux.
+-v::
+--verbose::
+	Be more verbose (show counter open errors, etc).
 
 STAT REPORT OPTIONS
 -------------------
diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index f8bf5f244d77..d9cc0e3f284f 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1690,6 +1690,8 @@ int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
 			   "file", "file saving guest os /proc/kallsyms"),
 		OPT_STRING(0, "guestmodules", &symbol_conf.default_guest_modules,
 			   "file", "file saving guest os /proc/modules"),
+		OPT_INCR('v', "verbose", &verbose,
+			    "be more verbose (show counter open errors, etc)"),
 		OPT_END()
 	};
 
-- 
1.8.1.4

[PATCH 11/21] perf kvm: Fix bug in 'stat report'

[Arnaldo Carvalho de Melo]

From: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>

When we use perf kvm record-report, there is a bug in report subcommand.

Example:
	# perf kvm stat record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.678 MB perf.data.guest (~29641 samples) ]
	# perf kvm stat report
	failed to open perf.data: No such file or directory  (try 'perf record' first)
	Initializing perf session failed

This bug was introduced by f5fc14124.

+       struct perf_data_file file = {
+               .path = input_name,
+               .mode = PERF_DATA_MODE_READ,
+       };

        kvm->tool = eops;
-       kvm->session = perf_session__new(kvm->file_name, O_RDONLY, 0, false,
-                                        &kvm->tool);
+       kvm->session = perf_session__new(&file, false, &kvm->tool);

It changed the path from kvm->file_name to input_name, this patch change the path back to
'kvm->file_name', then it works well.

Verification:
	# perf kvm stat record -a sleep 1
	[ perf record: Woken up 1 times to write data ]
	[ perf record: Captured and wrote 0.807 MB perf.data.guest (~35264 samples) ]
	# perf kvm stat report

	Analyze events for all VCPUs:

		     VM-EXIT    Samples  Samples%     Time%   Min Time   Max Time         Avg time

	       EPT_VIOLATION        200    32.79%     1.25%        0us    12064us     62.35us ( +-  96.74% )
	       EPT_MISCONFIG        134    21.97%     0.21%        0us       35us     15.25us ( +-   4.14% )
	       EXCEPTION_NMI         96    15.74%     0.02%        0us       11us      1.95us ( +-   9.81% )
		 APIC_ACCESS         79    12.95%     0.02%        0us       13us      2.94us ( +-  11.20% )
			 HLT         65    10.66%    98.47%        0us    16706us  15084.86us ( +-   1.89% )
	      IO_INSTRUCTION         27     4.43%     0.02%        0us       29us      6.42us ( +-  15.53% )
	  EXTERNAL_INTERRUPT          5     0.82%     0.01%        0us       77us     23.65us ( +-  57.90% )
	 TPR_BELOW_THRESHOLD          4     0.66%     0.00%        0us        1us      1.22us ( +-   4.36% )

	Total Samples:610, Total events handled time:995745.54us.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Link: http://lkml.kernel.org/r/1386632823-17539-1-git-send-email-yangds.fnst@cn.fujitsu.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index d9cc0e3f284f..c2e5d56a291d 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1232,7 +1232,7 @@ static int read_events(struct perf_kvm_stat *kvm)
 		.ordered_samples	= true,
 	};
 	struct perf_data_file file = {
-		.path = input_name,
+		.path = kvm->file_name,
 		.mode = PERF_DATA_MODE_READ,
 	};
 
-- 
1.8.1.4

[PATCH 12/21] perf archive: Remove duplicated 'runs' in man page

[Arnaldo Carvalho de Melo]

From: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>

Two 'runs' here breaks the sentence in Description of 'perf archive'
command.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Link: http://lkml.kernel.org/r/78a15a9f4f500b6074a1e25917d6e8251f894628.1386629050.git.yangds.fnst@cn.fujitsu.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Documentation/perf-archive.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/Documentation/perf-archive.txt b/tools/perf/Documentation/perf-archive.txt
index 5032a142853e..ac6ecbb3e669 100644
--- a/tools/perf/Documentation/perf-archive.txt
+++ b/tools/perf/Documentation/perf-archive.txt
@@ -12,9 +12,9 @@ SYNOPSIS
 
 DESCRIPTION
 -----------
-This command runs runs perf-buildid-list --with-hits, and collects the files
-with the buildids found so that analysis of perf.data contents can be possible
-on another machine.
+This command runs perf-buildid-list --with-hits, and collects the files with the
+buildids found so that analysis of perf.data contents can be possible on another
+machine.
 
 
 SEE ALSO
-- 
1.8.1.4

[PATCH 13/21] perf annotate: Fix typo

[Arnaldo Carvalho de Melo]

From: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>

A typo in comment of builtin-annotate.c about 'that'.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Link: http://lkml.kernel.org/r/46cb069a4ce21141057a07c0b50baa9968e3228c.1386629050.git.yangds.fnst@cn.fujitsu.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-annotate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 4087ab19823c..6fd52c8fa682 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -373,7 +373,7 @@ int cmd_annotate(int argc, const char **argv, const char *prefix __maybe_unused)
 
 	if (argc) {
 		/*
-		 * Special case: if there's an argument left then assume tha
+		 * Special case: if there's an argument left then assume that
 		 * it's a symbol filter:
 		 */
 		if (argc > 1)
-- 
1.8.1.4

[PATCH 14/21] perf kvm: Move code to generate filename for perf-kvm to function.

[Arnaldo Carvalho de Melo]

From: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>

The code in builtin-kvm.c to generate filename for perf-kvm is useful to
other command such as builtin-diff.

This patch move the related code form builtin-kvm.c to util/util.c and
wrap them in a function named get_filename_for_perf_kvm.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Link: http://lkml.kernel.org/r/5e09a5c47e8a495e888cbdc65a6fafb2c950f529.1386368672.git.yangds.fnst@cn.fujitsu.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-kvm.c |  7 +------
 tools/perf/util/util.c   | 14 ++++++++++++++
 tools/perf/util/util.h   |  2 ++
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index c2e5d56a291d..c6fa3cbd45a9 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1713,12 +1713,7 @@ int cmd_kvm(int argc, const char **argv, const char *prefix __maybe_unused)
 		perf_guest = 1;
 
 	if (!file_name) {
-		if (perf_host && !perf_guest)
-			file_name = strdup("perf.data.host");
-		else if (!perf_host && perf_guest)
-			file_name = strdup("perf.data.guest");
-		else
-			file_name = strdup("perf.data.kvm");
+		file_name = get_filename_for_perf_kvm();
 
 		if (!file_name) {
 			pr_err("Failed to allocate memory for filename\n");
diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index bae8756a4eb1..4a57609c0b43 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -482,3 +482,17 @@ int filename__read_str(const char *filename, char **buf, size_t *sizep)
 	close(fd);
 	return err;
 }
+
+const char *get_filename_for_perf_kvm(void)
+{
+	const char *filename;
+
+	if (perf_host && !perf_guest)
+		filename = strdup("perf.data.host");
+	else if (!perf_host && perf_guest)
+		filename = strdup("perf.data.guest");
+	else
+		filename = strdup("perf.data.kvm");
+
+	return filename;
+}
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index 659abf30e01b..0171213d1d4d 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -321,4 +321,6 @@ void free_srcline(char *srcline);
 
 int filename__read_int(const char *filename, int *value);
 int filename__read_str(const char *filename, char **buf, size_t *sizep);
+
+const char *get_filename_for_perf_kvm(void);
 #endif /* GIT_COMPAT_UTIL_H */
-- 
1.8.1.4

[PATCH 15/21] perf kvm: Make perf kvm diff support --guestmount.

[Arnaldo Carvalho de Melo]

From: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>

In manpage of perf-kvm, --guestmount is supported by diff command, but
it does not work well.

This patch change the extend the checking in buildid-diff from
guestkallsyms or guestmodules to perf_guest. Then this checking can
cover the all cases perf kvm is used for.

Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Link: http://lkml.kernel.org/r/72857ed89642e0633f5e88f7e7abbc9645359e8e.1386368672.git.yangds.fnst@cn.fujitsu.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-diff.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index 3b67ea2444bd..2a85cc9a2d09 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -1000,8 +1000,7 @@ static int data_init(int argc, const char **argv)
 			data__files_cnt = argc;
 			use_default = false;
 		}
-	} else if (symbol_conf.default_guest_vmlinux_name ||
-		   symbol_conf.default_guest_kallsyms) {
+	} else if (perf_guest) {
 		defaults[0] = "perf.data.host";
 		defaults[1] = "perf.data.guest";
 	}
-- 
1.8.1.4

[PATCH 16/21] tools lib traceevent: Get rid of malloc_or_die() in pevent_filter_alloc()

[Arnaldo Carvalho de Melo]

From: Namhyung Kim <namhyung@kernel.org>

It returns NULL when allocation fails so the users should check the
return value from now on.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-4-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/traceevent/parse-filter.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 2500e75583fc..b3a61d4fe38e 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -182,7 +182,10 @@ struct event_filter *pevent_filter_alloc(struct pevent *pevent)
 {
 	struct event_filter *filter;
 
-	filter = malloc_or_die(sizeof(*filter));
+	filter = malloc(sizeof(*filter));
+	if (filter == NULL)
+		return NULL;
+
 	memset(filter, 0, sizeof(*filter));
 	filter->pevent = pevent;
 	pevent_ref(pevent);
-- 
1.8.1.4

[PATCH 17/21] tools lib traceevent: Get rid of malloc_or_die() in add_event()

[Arnaldo Carvalho de Melo]

From: Namhyung Kim <namhyung@kernel.org>

Make it return error value since its only caller find_event() now can
handle allocation error properly.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-8-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/traceevent/parse-filter.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index b3a61d4fe38e..2b73abfb0c9f 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -245,15 +245,19 @@ static void free_arg(struct filter_arg *arg)
 	free(arg);
 }
 
-static void add_event(struct event_list **events,
+static int add_event(struct event_list **events,
 		      struct event_format *event)
 {
 	struct event_list *list;
 
-	list = malloc_or_die(sizeof(*list));
+	list = malloc(sizeof(*list));
+	if (list == NULL)
+		return -1;
+
 	list->next = *events;
 	*events = list;
 	list->event = event;
+	return 0;
 }
 
 static int event_match(struct event_format *event,
@@ -276,6 +280,7 @@ find_event(struct pevent *pevent, struct event_list **events,
 	regex_t ereg;
 	regex_t sreg;
 	int match = 0;
+	int fail = 0;
 	char *reg;
 	int ret;
 	int i;
@@ -310,7 +315,10 @@ find_event(struct pevent *pevent, struct event_list **events,
 		event = pevent->events[i];
 		if (event_match(event, sys_name ? &sreg : NULL, &ereg)) {
 			match = 1;
-			add_event(events, event);
+			if (add_event(events, event) < 0) {
+				fail = 1;
+				break;
+			}
 		}
 	}
 
@@ -320,6 +328,8 @@ find_event(struct pevent *pevent, struct event_list **events,
 
 	if (!match)
 		return -1;
+	if (fail)
+		return -2;
 
 	return 0;
 }
-- 
1.8.1.4

[PATCH 18/21] tools lib traceevent: Get rid of die() in create_arg_item()

[Arnaldo Carvalho de Melo]

From: Namhyung Kim <namhyung@kernel.org>

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-9-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/traceevent/parse-filter.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 2b73abfb0c9f..53e48eb112c3 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -362,8 +362,11 @@ create_arg_item(struct event_format *event, const char *token,
 		arg->value.type =
 			type == EVENT_DQUOTE ? FILTER_STRING : FILTER_CHAR;
 		arg->value.str = strdup(token);
-		if (!arg->value.str)
-			die("malloc string");
+		if (!arg->value.str) {
+			free_arg(arg);
+			show_error(error_str, "failed to allocate string filter arg");
+			return NULL;
+		}
 		break;
 	case EVENT_ITEM:
 		/* if it is a number, then convert it */
-- 
1.8.1.4

[PATCH 19/21] tools lib traceevent: Get rid of malloc_or_die() in pevent_filter_add_filter_str()

[Arnaldo Carvalho de Melo]

From: Namhyung Kim <namhyung@kernel.org>

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-12-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/traceevent/parse-filter.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index 53e48eb112c3..a4d5bb23a110 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -1226,7 +1226,13 @@ int pevent_filter_add_filter_str(struct event_filter *filter,
 		else
 			len = strlen(filter_str);
 
-		this_event = malloc_or_die(len + 1);
+		this_event = malloc(len + 1);
+		if (this_event == NULL) {
+			show_error(error_str, "Memory allocation failure");
+			/* This can only happen when events is NULL, but still */
+			free_events(events);
+			return -1;
+		}
 		memcpy(this_event, filter_str, len);
 		this_event[len] = 0;
 
-- 
1.8.1.4

[PATCH 20/21] tools lib traceevent: Get rid of die() in pevent_filter_clear_trivial()

[Arnaldo Carvalho de Melo]

From: Namhyung Kim <namhyung@kernel.org>

Change the function signature to return error code and not call die()
anymore.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Reviewed-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Link: http://lkml.kernel.org/r/1386567251-22751-13-git-send-email-namhyung@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/lib/traceevent/event-parse.h  |  2 +-
 tools/lib/traceevent/parse-filter.c | 21 +++++++++++++++------
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
index 620c27a72960..6e23f197175f 100644
--- a/tools/lib/traceevent/event-parse.h
+++ b/tools/lib/traceevent/event-parse.h
@@ -860,7 +860,7 @@ int pevent_event_filtered(struct event_filter *filter,
 
 void pevent_filter_reset(struct event_filter *filter);
 
-void pevent_filter_clear_trivial(struct event_filter *filter,
+int pevent_filter_clear_trivial(struct event_filter *filter,
 				 enum filter_trivial_type type);
 
 void pevent_filter_free(struct event_filter *filter);
diff --git a/tools/lib/traceevent/parse-filter.c b/tools/lib/traceevent/parse-filter.c
index a4d5bb23a110..ab402fb2dcf7 100644
--- a/tools/lib/traceevent/parse-filter.c
+++ b/tools/lib/traceevent/parse-filter.c
@@ -1504,8 +1504,10 @@ int pevent_update_trivial(struct event_filter *dest, struct event_filter *source
  * @type: remove only true, false, or both
  *
  * Removes filters that only contain a TRUE or FALES boolean arg.
+ *
+ * Returns 0 on success and -1 if there was a problem.
  */
-void pevent_filter_clear_trivial(struct event_filter *filter,
+int pevent_filter_clear_trivial(struct event_filter *filter,
 				 enum filter_trivial_type type)
 {
 	struct filter_type *filter_type;
@@ -1514,13 +1516,15 @@ void pevent_filter_clear_trivial(struct event_filter *filter,
 	int i;
 
 	if (!filter->filters)
-		return;
+		return 0;
 
 	/*
 	 * Two steps, first get all ids with trivial filters.
 	 *  then remove those ids.
 	 */
 	for (i = 0; i < filter->filters; i++) {
+		int *new_ids;
+
 		filter_type = &filter->event_filters[i];
 		if (filter_type->filter->type != FILTER_ARG_BOOLEAN)
 			continue;
@@ -1535,19 +1539,24 @@ void pevent_filter_clear_trivial(struct event_filter *filter,
 			break;
 		}
 
-		ids = realloc(ids, sizeof(*ids) * (count + 1));
-		if (!ids)
-			die("Can't allocate ids");
+		new_ids = realloc(ids, sizeof(*ids) * (count + 1));
+		if (!new_ids) {
+			free(ids);
+			return -1;
+		}
+
+		ids = new_ids;
 		ids[count++] = filter_type->event_id;
 	}
 
 	if (!count)
-		return;
+		return 0;
 
 	for (i = 0; i < count; i++)
 		pevent_filter_remove_event(filter, ids[i]);
 
 	free(ids);
+	return 0;
 }
 
 /**
-- 
1.8.1.4

[PATCH 21/21] perf symbols: fix bug in usage of the basename() function

[Arnaldo Carvalho de Melo]

From: Stephane Eranian <eranian@google.com>

The basename() implementation varies a lot between systems.

The Linux man page says: "basename may modify the content of the path,
so it may be desirable to pass a copy when calling the function".

On some other systems, the returned address may come from an internal
buffer which can be reused in subsequent calls, thus the results should
also be copied.

The dso__set_basename() function was not doing this causing problems
on some systems with wrong library names being shown by perf report,
such as on Android systems.

This patch fixes the problem.
The patch is relative to tip.git.

In v2, we clean up the comments based on Ingo's feedback.

Reported-by: Ben Cheng <bccheng@google.com>
Signed-off-by: Stephane Eranian <eranian@google.com>
Acked-by: Ingo Molnar <mingo@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: bccheng@google.com
Link: http://lkml.kernel.org/r/20131205182642.GA14614@quad
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/dso.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index a0c7c591f4b2..9fae484853c1 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -404,7 +404,34 @@ void dso__set_short_name(struct dso *dso, const char *name)
 
 static void dso__set_basename(struct dso *dso)
 {
-	dso__set_short_name(dso, basename(dso->long_name));
+	char *lname, *base;
+
+	/*
+	 * basename() may modify path buffer, so we must pass
+	 * a copy.
+	 */
+	lname = strdup(dso->long_name);
+	if (!lname)
+		return;
+
+	/*
+	 * basename() may return a pointer to internal
+	 * storage which is reused in subsequent calls
+	 * so copy the result.
+	 */
+	base = strdup(basename(lname));
+
+	free(lname);
+
+	if (!base)
+		return;
+
+	if (dso->sname_alloc)
+		free((char *)dso->short_name);
+	else
+		dso->sname_alloc = 1;
+
+	dso__set_short_name(dso, base);
 }
 
 int dso__name_len(const struct dso *dso)
-- 
1.8.1.4

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@infradead.org> wrote:

> From: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> 
> Hi Ingo,
> 
> 	Please consider pulling,
> 
> Best Regards,
> 
> -Arnaldo
> 
> The following changes since commit 6d65894bc028d0342829ea1e64c9e9efad571124:
> 
>   tools lib traceevent: Update kvm plugin with is_writable_pte helper (2013-12-04 15:38:14 -0300)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux tags/perf-core-for-mingo
> 
> for you to fetch changes up to e993d10caeb6dca690dbaf86e1981ba240d1414a:
> 
>   perf symbols: fix bug in usage of the basename() function (2013-12-09 15:41:59 -0300)
> 
> ----------------------------------------------------------------
> perf/core improvements and fixes:
> 
> . Add an option in 'perf script' to print the source line number, from Adrian Hunter
> 
> . Fix symoff printing in callchains in 'perf script', from Adrian Hunter.
> 
> . Assorted mmap_pages handling fixes, from Adrian Hunter.
> 
> . Fix summary percentage when processing files in 'perf trace', fom David Ahern.
> 
> . Handle old kernels where the "raw_syscalls" tracepoints were called plan "syscalls",
>   in 'perf trace', from David Ahern.
> 
> . Several man pages typo fixes from Dongsheng Yang.
> 
> . Add '-v' option to 'perf kvm', from Dongsheng Yang.
> 
> . Make perf kvm diff support --guestmount, from Dongsheng Yang.
> 
> . Get rid of several die() calls in libtraceevent, from Namhyung Kim.
> 
> . Use basename() in a more robust way, to avoid problems related to different
>   system library implementations for that function, from Stephane Eranian.
> 
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> ----------------------------------------------------------------
> Adrian Hunter (6):
>       perf script: Fix symoff printing in callchains
>       perf script: Add an option to print the source line number
>       perf record: Fix display of incorrect mmap pages
>       perf evlist: Remove unnecessary parentheses
>       perf evlist: Fix max mmap_pages
>       perf evlist: Fix mmap pages rounding to power of 2
> 
> David Ahern (2):
>       perf trace: Add support for syscalls vs raw_syscalls
>       perf trace: Fix summary percentage when processing files
> 
> Dongsheng Yang (6):
>       perf kvm: Introduce option -v for perf kvm command.
>       perf kvm: Fix bug in 'stat report'
>       perf archive: Remove duplicated 'runs' in man page
>       perf annotate: Fix typo
>       perf kvm: Move code to generate filename for perf-kvm to function.
>       perf kvm: Make perf kvm diff support --guestmount.
> 
> Namhyung Kim (5):
>       tools lib traceevent: Get rid of malloc_or_die() in pevent_filter_alloc()
>       tools lib traceevent: Get rid of malloc_or_die() in add_event()
>       tools lib traceevent: Get rid of die() in create_arg_item()
>       tools lib traceevent: Get rid of malloc_or_die() in pevent_filter_add_filter_str()
>       tools lib traceevent: Get rid of die() in pevent_filter_clear_trivial()
> 
> Stephane Eranian (1):
>       perf symbols: fix bug in usage of the basename() function
> 
> Steven Rostedt (1):
>       tools lib traceevent: Report better error message on bad function args
> 
>  tools/lib/traceevent/event-parse.c        | 28 +++++++++------
>  tools/lib/traceevent/event-parse.h        |  2 +-
>  tools/lib/traceevent/parse-filter.c       | 57 ++++++++++++++++++++++++-------
>  tools/perf/Documentation/perf-archive.txt |  6 ++--
>  tools/perf/Documentation/perf-kvm.txt     |  7 ++--
>  tools/perf/Documentation/perf-script.txt  |  2 +-
>  tools/perf/builtin-annotate.c             |  2 +-
>  tools/perf/builtin-diff.c                 |  3 +-
>  tools/perf/builtin-kvm.c                  | 11 +++---
>  tools/perf/builtin-record.c               |  2 +-
>  tools/perf/builtin-script.c               | 10 ++++++
>  tools/perf/builtin-trace.c                | 32 +++++++++++++++--
>  tools/perf/util/dso.c                     | 29 +++++++++++++++-
>  tools/perf/util/evlist.c                  | 10 +++---
>  tools/perf/util/map.c                     | 17 +++++++++
>  tools/perf/util/map.h                     |  2 ++
>  tools/perf/util/session.c                 | 15 +++++++-
>  tools/perf/util/session.h                 |  1 +
>  tools/perf/util/util.c                    | 14 ++++++++
>  tools/perf/util/util.h                    | 14 ++++++++
>  20 files changed, 214 insertions(+), 50 deletions(-)

Pulled, thanks Arnaldo!

There's one detail I noticed about the recent trace-plugin changes:

comet:~/tip/tools/perf> make install
  BUILD:   Doing 'make -j12' parallel build
  SUBDIR   Documentation
  INSTALL  Documentation-man
  INSTALL  GTK UI
  SUBDIR   /home/mingo/tip/tools/lib/traceevent/
  INSTALL  binaries
  INSTALL  plugin_jbd2.so
  INSTALL  plugin_hrtimer.so
  INSTALL  plugin_kmem.so
  INSTALL  plugin_kvm.so
  INSTALL  plugin_mac80211.so
  INSTALL  plugin_sched_switch.so
  INSTALL  plugin_function.so
  INSTALL  plugin_xen.so
  INSTALL  plugin_scsi.so
  INSTALL  plugin_cfg80211.so
  INSTALL  libexec
  INSTALL  perf-archive
  INSTALL  perl-scripts
  INSTALL  python-scripts
  INSTALL  perf_completion-script
  INSTALL  tests

those plugin installs are way too verbose, they should really be in a 
single summarized line, only saying something like:

  INSTALL  plugins

Just like we already sum up 'binaries', 'libexec', 'tests', etc.

Thanks,

	Ingo

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Ingo Molnar]

Hm, I've unpulled it because 'perf top' crashes on exit, in 
dso__delete():

[Thread 0x7ffff70df700 (LWP 29561) exited]
*** Error in `/fast/mingo/tip/tools/perf/perf': munmap_chunk(): invalid pointer: 0x0000000000587371 ***
======= Backtrace: =========
/lib64/libc.so.6[0x3e5907bbe7]
/fast/mingo/tip/tools/perf/perf(dso__delete+0xd9)[0x46da89]
/fast/mingo/tip/tools/perf/perf(machines__exit+0xad)[0x482e7d]
/fast/mingo/tip/tools/perf/perf(perf_session__delete+0xb6)[0x488c66]
/fast/mingo/tip/tools/perf/perf(cmd_top+0xf44)[0x4345f4]
/fast/mingo/tip/tools/perf/perf[0x419f95]
/fast/mingo/tip/tools/perf/perf(main+0x600)[0x419830]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x3e59021b45]
/fast/mingo/tip/tools/perf/perf[0x4198fd]
======= Memory map: ========

Program received signal SIGABRT, Aborted.
0x0000003e590359e9 in raise () from /lib64/libc.so.6
Missing separate debuginfos, use: debuginfo-install audit-libs-2.3.2-1.fc19.x86_64 bzip2-libs-1.0.6-8.fc19.x86_64 elfutils-libelf-0.156-5.fc19.x86_64 elfutils-libs-0.156-5.fc19.x86_64 glibc-2.17-19.fc19.x86_64 libgcc-4.8.2-1.fc19.x86_64 libunwind-1.1-2.fc19.x86_64 nss-softokn-freebl-3.15.2-2.fc19.x86_64 numactl-libs-2.0.8-4.fc19.x86_64 perl-libs-5.16.3-266.fc19.x86_64 python-libs-2.7.5-9.fc19.x86_64 slang-2.2.4-8.fc19.x86_64 xz-libs-5.1.2-4alpha.fc19.x86_64 zlib-1.2.7-10.fc19.x86_64
(gdb) 
(gdb) bt
#0  0x0000003e590359e9 in raise () from /lib64/libc.so.6
#1  0x0000003e590370f8 in abort () from /lib64/libc.so.6
#2  0x0000003e59075d17 in __libc_message () from /lib64/libc.so.6
#3  0x0000003e5907bbe7 in malloc_printerr () from /lib64/libc.so.6
#4  0x000000000046da89 in dso__delete (dso=0x8e46f0) at util/dso.c:496
#5  0x0000000000482e7d in dsos__delete (dsos=0x8e4490) at util/machine.c:72
#6  machine__exit (machine=<optimized out>) at util/machine.c:103
#7  machines__exit (machines=machines@entry=0x8e4438) at util/machine.c:123
#8  0x0000000000488c66 in perf_session__delete (session=0x8e4360) at util/session.c:155
#9  0x00000000004345f4 in __cmd_top (top=0x7fffffffb140) at builtin-top.c:985
#10 cmd_top (argc=<optimized out>, argv=<optimized out>, prefix=<optimized out>) at builtin-top.c:1210
#11 0x0000000000419f95 in run_builtin (p=p@entry=0x7ece88 <commands+264>, argc=argc@entry=2, argv=argv@entry=0x7fffffffe420) at perf.c:319
#12 0x0000000000419830 in handle_internal_command (argv=0x7fffffffe420, argc=2) at perf.c:376
#13 run_argv (argv=0x7fffffffe220, argcp=0x7fffffffe22c) at perf.c:420
#14 main (argc=2, argv=0x7fffffffe420) at perf.c:529
(gdb) 

Running it on an up-to-date installation of Fedora 19.

Thanks,

	Ingo

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Arnaldo Carvalho de Melo]

Em Tue, Dec 10, 2013 at 12:12:29PM +0100, Ingo Molnar escreveu:
> 
> Hm, I've unpulled it because 'perf top' crashes on exit, in 
> dso__delete():

495		if (dso->sname_alloc)
496			free((char *)dso->short_name)

Yeah, must be that basename() patch from Stephane, I'll work on a fix
and resubmit this batch, thanks for the report.

- Arnaldo
 
> [Thread 0x7ffff70df700 (LWP 29561) exited]
> *** Error in `/fast/mingo/tip/tools/perf/perf': munmap_chunk(): invalid pointer: 0x0000000000587371 ***
> ======= Backtrace: =========
> /lib64/libc.so.6[0x3e5907bbe7]
> /fast/mingo/tip/tools/perf/perf(dso__delete+0xd9)[0x46da89]
> /fast/mingo/tip/tools/perf/perf(machines__exit+0xad)[0x482e7d]
> /fast/mingo/tip/tools/perf/perf(perf_session__delete+0xb6)[0x488c66]
> /fast/mingo/tip/tools/perf/perf(cmd_top+0xf44)[0x4345f4]
> /fast/mingo/tip/tools/perf/perf[0x419f95]
> /fast/mingo/tip/tools/perf/perf(main+0x600)[0x419830]
> /lib64/libc.so.6(__libc_start_main+0xf5)[0x3e59021b45]
> /fast/mingo/tip/tools/perf/perf[0x4198fd]
> ======= Memory map: ========
> 
> Program received signal SIGABRT, Aborted.
> 0x0000003e590359e9 in raise () from /lib64/libc.so.6
> Missing separate debuginfos, use: debuginfo-install audit-libs-2.3.2-1.fc19.x86_64 bzip2-libs-1.0.6-8.fc19.x86_64 elfutils-libelf-0.156-5.fc19.x86_64 elfutils-libs-0.156-5.fc19.x86_64 glibc-2.17-19.fc19.x86_64 libgcc-4.8.2-1.fc19.x86_64 libunwind-1.1-2.fc19.x86_64 nss-softokn-freebl-3.15.2-2.fc19.x86_64 numactl-libs-2.0.8-4.fc19.x86_64 perl-libs-5.16.3-266.fc19.x86_64 python-libs-2.7.5-9.fc19.x86_64 slang-2.2.4-8.fc19.x86_64 xz-libs-5.1.2-4alpha.fc19.x86_64 zlib-1.2.7-10.fc19.x86_64
> (gdb) 
> (gdb) bt
> #0  0x0000003e590359e9 in raise () from /lib64/libc.so.6
> #1  0x0000003e590370f8 in abort () from /lib64/libc.so.6
> #2  0x0000003e59075d17 in __libc_message () from /lib64/libc.so.6
> #3  0x0000003e5907bbe7 in malloc_printerr () from /lib64/libc.so.6
> #4  0x000000000046da89 in dso__delete (dso=0x8e46f0) at util/dso.c:496
> #5  0x0000000000482e7d in dsos__delete (dsos=0x8e4490) at util/machine.c:72
> #6  machine__exit (machine=<optimized out>) at util/machine.c:103
> #7  machines__exit (machines=machines@entry=0x8e4438) at util/machine.c:123
> #8  0x0000000000488c66 in perf_session__delete (session=0x8e4360) at util/session.c:155
> #9  0x00000000004345f4 in __cmd_top (top=0x7fffffffb140) at builtin-top.c:985
> #10 cmd_top (argc=<optimized out>, argv=<optimized out>, prefix=<optimized out>) at builtin-top.c:1210
> #11 0x0000000000419f95 in run_builtin (p=p@entry=0x7ece88 <commands+264>, argc=argc@entry=2, argv=argv@entry=0x7fffffffe420) at perf.c:319
> #12 0x0000000000419830 in handle_internal_command (argv=0x7fffffffe420, argc=2) at perf.c:376
> #13 run_argv (argv=0x7fffffffe220, argcp=0x7fffffffe22c) at perf.c:420
> #14 main (argc=2, argv=0x7fffffffe420) at perf.c:529
> (gdb) 
> 
> Running it on an up-to-date installation of Fedora 19.
> 
> Thanks,
> 
> 	Ingo

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:

> Em Tue, Dec 10, 2013 at 12:12:29PM +0100, Ingo Molnar escreveu:
> > 
> > Hm, I've unpulled it because 'perf top' crashes on exit, in 
> > dso__delete():
> 
> 495		if (dso->sname_alloc)
> 496			free((char *)dso->short_name)

Btw., instead of trusting flags I'd argue that using the pointer as a 
flag and clearing the pointer too is a much more robust freeing 
pattern in general:

		if (dso->short_name) {
			free(dso->short_name);
			dso->short_name = NULL;
		}

or so ...

Thanks,

	Ingo

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Arnaldo Carvalho de Melo]

Em Tue, Dec 10, 2013 at 12:47:57PM +0100, Ingo Molnar escreveu:
> * Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:
> > Em Tue, Dec 10, 2013 at 12:12:29PM +0100, Ingo Molnar escreveu:
> > > Hm, I've unpulled it because 'perf top' crashes on exit, in 
> > > dso__delete():

> > 495		if (dso->sname_alloc)
> > 496			free((char *)dso->short_name)

> Btw., instead of trusting flags I'd argue that using the pointer as a 
> flag and clearing the pointer too is a much more robust freeing 
> pattern in general:

> 		if (dso->short_name) {
> 			free(dso->short_name);
> 			dso->short_name = NULL;
> 		}
> 
> or so ...

This is not an unusual idiom, if you look at tools/perf/util/ev{list,sel}.c,
for instance, you'll see it in many destructors.

In this case there is a micro optimization where sometimes the shortname
is just a pointer to the tail part of the long name, hence the flag.

- Arnaldo

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:

> Em Tue, Dec 10, 2013 at 12:47:57PM +0100, Ingo Molnar escreveu:
> > * Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:
> > > Em Tue, Dec 10, 2013 at 12:12:29PM +0100, Ingo Molnar escreveu:
> > > > Hm, I've unpulled it because 'perf top' crashes on exit, in 
> > > > dso__delete():
> 
> > > 495		if (dso->sname_alloc)
> > > 496			free((char *)dso->short_name)
> 
> > Btw., instead of trusting flags I'd argue that using the pointer as a 
> > flag and clearing the pointer too is a much more robust freeing 
> > pattern in general:
> 
> > 		if (dso->short_name) {
> > 			free(dso->short_name);
> > 			dso->short_name = NULL;
> > 		}
> > 
> > or so ...
> 
> This is not an unusual idiom, if you look at 
> tools/perf/util/ev{list,sel}.c, for instance, you'll see it in many 
> destructors.
> 
> In this case there is a micro optimization where sometimes the 
> shortname is just a pointer to the tail part of the long name, hence 
> the flag.

Sounds fair.

[btw., a tiny nit remains: the cast is probably unnecessary, free() 
will take any pointer.]

Thanks,

	Ingo

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Arnaldo Carvalho de Melo]

Em Tue, Dec 10, 2013 at 02:13:12PM +0200, Adrian Hunter escreveu:
> On 10/12/13 13:44, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Dec 10, 2013 at 12:12:29PM +0100, Ingo Molnar escreveu:
> >>
> >> Hm, I've unpulled it because 'perf top' crashes on exit, in 
> >> dso__delete():
> > 
> > 495		if (dso->sname_alloc)
> > 496			free((char *)dso->short_name)
> > 
> > Yeah, must be that basename() patch from Stephane, I'll work on a fix
> > and resubmit this batch, thanks for the report.
> 
> The problem is sname_alloc is not maintained.  Perhaps it should be
> set in dso__set_short_name() e.g.

Yeah, sounds better than having all callers manage that thing, quickie,
was this with Stephane's patch applied?

I think it should be done as a prep, then apply a modified version of
Stephanes, that doesn't deal with the alloc flag (more than using 'true'
to say it is a malloc'ed chunk).

- Arnaldo
 
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 9fae484..54ed980 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -379,7 +379,7 @@ struct dso *dso__kernel_findnew(struct machine *machine,
> const char *name,
>  	 * processing we had no idea this was the kernel dso.
>  	 */
>  	if (dso != NULL) {
> -		dso__set_short_name(dso, short_name);
> +		dso__set_short_name(dso, short_name, false);
>  		dso->kernel = dso_type;
>  	}
> 
> @@ -394,10 +394,13 @@ void dso__set_long_name(struct dso *dso, char *name)
>  	dso->long_name_len = strlen(name);
>  }
> 
> -void dso__set_short_name(struct dso *dso, const char *name)
> +void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc)
>  {
>  	if (name == NULL)
>  		return;
> +	if (dso->sname_alloc)
> +		free((char *)dso->short_name);
> +	dso->sname_alloc = sname_alloc;
>  	dso->short_name = name;
>  	dso->short_name_len = strlen(name);
>  }
> @@ -426,12 +429,7 @@ static void dso__set_basename(struct dso *dso)
>  	if (!base)
>  		return;
> 
> -	if (dso->sname_alloc)
> -		free((char *)dso->short_name);
> -	else
> -		dso->sname_alloc = 1;
> -
> -	dso__set_short_name(dso, base);
> +	dso__set_short_name(dso, base, true);
>  }
> 
>  int dso__name_len(const struct dso *dso)
> @@ -467,7 +465,7 @@ struct dso *dso__new(const char *name)
>  		int i;
>  		strcpy(dso->name, name);
>  		dso__set_long_name(dso, dso->name);
> -		dso__set_short_name(dso, dso->name);
> +		dso__set_short_name(dso, dso->name, false);
>  		for (i = 0; i < MAP__NR_TYPES; ++i)
>  			dso->symbols[i] = dso->symbol_names[i] = RB_ROOT;
>  		dso->cache = RB_ROOT;
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index 384f2d9..166463e 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -110,7 +110,7 @@ static inline void dso__set_loaded(struct dso *dso, enum
> map_type type)
>  struct dso *dso__new(const char *name);
>  void dso__delete(struct dso *dso);
> 
> -void dso__set_short_name(struct dso *dso, const char *name);
> +void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc);
>  void dso__set_long_name(struct dso *dso, char *name);
> 
>  int dso__name_len(const struct dso *dso);
> 
> 
> > 
> > - Arnaldo
> >  
> >> [Thread 0x7ffff70df700 (LWP 29561) exited]
> >> *** Error in `/fast/mingo/tip/tools/perf/perf': munmap_chunk(): invalid pointer: 0x0000000000587371 ***
> >> ======= Backtrace: =========
> >> /lib64/libc.so.6[0x3e5907bbe7]
> >> /fast/mingo/tip/tools/perf/perf(dso__delete+0xd9)[0x46da89]
> >> /fast/mingo/tip/tools/perf/perf(machines__exit+0xad)[0x482e7d]
> >> /fast/mingo/tip/tools/perf/perf(perf_session__delete+0xb6)[0x488c66]
> >> /fast/mingo/tip/tools/perf/perf(cmd_top+0xf44)[0x4345f4]
> >> /fast/mingo/tip/tools/perf/perf[0x419f95]
> >> /fast/mingo/tip/tools/perf/perf(main+0x600)[0x419830]
> >> /lib64/libc.so.6(__libc_start_main+0xf5)[0x3e59021b45]
> >> /fast/mingo/tip/tools/perf/perf[0x4198fd]
> >> ======= Memory map: ========
> >>
> >> Program received signal SIGABRT, Aborted.
> >> 0x0000003e590359e9 in raise () from /lib64/libc.so.6
> >> Missing separate debuginfos, use: debuginfo-install audit-libs-2.3.2-1.fc19.x86_64 bzip2-libs-1.0.6-8.fc19.x86_64 elfutils-libelf-0.156-5.fc19.x86_64 elfutils-libs-0.156-5.fc19.x86_64 glibc-2.17-19.fc19.x86_64 libgcc-4.8.2-1.fc19.x86_64 libunwind-1.1-2.fc19.x86_64 nss-softokn-freebl-3.15.2-2.fc19.x86_64 numactl-libs-2.0.8-4.fc19.x86_64 perl-libs-5.16.3-266.fc19.x86_64 python-libs-2.7.5-9.fc19.x86_64 slang-2.2.4-8.fc19.x86_64 xz-libs-5.1.2-4alpha.fc19.x86_64 zlib-1.2.7-10.fc19.x86_64
> >> (gdb) 
> >> (gdb) bt
> >> #0  0x0000003e590359e9 in raise () from /lib64/libc.so.6
> >> #1  0x0000003e590370f8 in abort () from /lib64/libc.so.6
> >> #2  0x0000003e59075d17 in __libc_message () from /lib64/libc.so.6
> >> #3  0x0000003e5907bbe7 in malloc_printerr () from /lib64/libc.so.6
> >> #4  0x000000000046da89 in dso__delete (dso=0x8e46f0) at util/dso.c:496
> >> #5  0x0000000000482e7d in dsos__delete (dsos=0x8e4490) at util/machine.c:72
> >> #6  machine__exit (machine=<optimized out>) at util/machine.c:103
> >> #7  machines__exit (machines=machines@entry=0x8e4438) at util/machine.c:123
> >> #8  0x0000000000488c66 in perf_session__delete (session=0x8e4360) at util/session.c:155
> >> #9  0x00000000004345f4 in __cmd_top (top=0x7fffffffb140) at builtin-top.c:985
> >> #10 cmd_top (argc=<optimized out>, argv=<optimized out>, prefix=<optimized out>) at builtin-top.c:1210
> >> #11 0x0000000000419f95 in run_builtin (p=p@entry=0x7ece88 <commands+264>, argc=argc@entry=2, argv=argv@entry=0x7fffffffe420) at perf.c:319
> >> #12 0x0000000000419830 in handle_internal_command (argv=0x7fffffffe420, argc=2) at perf.c:376
> >> #13 run_argv (argv=0x7fffffffe220, argcp=0x7fffffffe22c) at perf.c:420
> >> #14 main (argc=2, argv=0x7fffffffe420) at perf.c:529
> >> (gdb) 
> >>
> >> Running it on an up-to-date installation of Fedora 19.
> >>
> >> Thanks,
> >>
> >> 	Ingo
> > 
> > 

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Adrian Hunter]

On 10/12/13 13:44, Arnaldo Carvalho de Melo wrote:
> Em Tue, Dec 10, 2013 at 12:12:29PM +0100, Ingo Molnar escreveu:
>>
>> Hm, I've unpulled it because 'perf top' crashes on exit, in 
>> dso__delete():
> 
> 495		if (dso->sname_alloc)
> 496			free((char *)dso->short_name)
> 
> Yeah, must be that basename() patch from Stephane, I'll work on a fix
> and resubmit this batch, thanks for the report.

The problem is sname_alloc is not maintained.  Perhaps it should be
set in dso__set_short_name() e.g.

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 9fae484..54ed980 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -379,7 +379,7 @@ struct dso *dso__kernel_findnew(struct machine *machine,
const char *name,
 	 * processing we had no idea this was the kernel dso.
 	 */
 	if (dso != NULL) {
-		dso__set_short_name(dso, short_name);
+		dso__set_short_name(dso, short_name, false);
 		dso->kernel = dso_type;
 	}

@@ -394,10 +394,13 @@ void dso__set_long_name(struct dso *dso, char *name)
 	dso->long_name_len = strlen(name);
 }

-void dso__set_short_name(struct dso *dso, const char *name)
+void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc)
 {
 	if (name == NULL)
 		return;
+	if (dso->sname_alloc)
+		free((char *)dso->short_name);
+	dso->sname_alloc = sname_alloc;
 	dso->short_name = name;
 	dso->short_name_len = strlen(name);
 }
@@ -426,12 +429,7 @@ static void dso__set_basename(struct dso *dso)
 	if (!base)
 		return;

-	if (dso->sname_alloc)
-		free((char *)dso->short_name);
-	else
-		dso->sname_alloc = 1;
-
-	dso__set_short_name(dso, base);
+	dso__set_short_name(dso, base, true);
 }

 int dso__name_len(const struct dso *dso)
@@ -467,7 +465,7 @@ struct dso *dso__new(const char *name)
 		int i;
 		strcpy(dso->name, name);
 		dso__set_long_name(dso, dso->name);
-		dso__set_short_name(dso, dso->name);
+		dso__set_short_name(dso, dso->name, false);
 		for (i = 0; i < MAP__NR_TYPES; ++i)
 			dso->symbols[i] = dso->symbol_names[i] = RB_ROOT;
 		dso->cache = RB_ROOT;
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 384f2d9..166463e 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -110,7 +110,7 @@ static inline void dso__set_loaded(struct dso *dso, enum
map_type type)
 struct dso *dso__new(const char *name);
 void dso__delete(struct dso *dso);

-void dso__set_short_name(struct dso *dso, const char *name);
+void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc);
 void dso__set_long_name(struct dso *dso, char *name);

 int dso__name_len(const struct dso *dso);


> 
> - Arnaldo
>  
>> [Thread 0x7ffff70df700 (LWP 29561) exited]
>> *** Error in `/fast/mingo/tip/tools/perf/perf': munmap_chunk(): invalid pointer: 0x0000000000587371 ***
>> ======= Backtrace: =========
>> /lib64/libc.so.6[0x3e5907bbe7]
>> /fast/mingo/tip/tools/perf/perf(dso__delete+0xd9)[0x46da89]
>> /fast/mingo/tip/tools/perf/perf(machines__exit+0xad)[0x482e7d]
>> /fast/mingo/tip/tools/perf/perf(perf_session__delete+0xb6)[0x488c66]
>> /fast/mingo/tip/tools/perf/perf(cmd_top+0xf44)[0x4345f4]
>> /fast/mingo/tip/tools/perf/perf[0x419f95]
>> /fast/mingo/tip/tools/perf/perf(main+0x600)[0x419830]
>> /lib64/libc.so.6(__libc_start_main+0xf5)[0x3e59021b45]
>> /fast/mingo/tip/tools/perf/perf[0x4198fd]
>> ======= Memory map: ========
>>
>> Program received signal SIGABRT, Aborted.
>> 0x0000003e590359e9 in raise () from /lib64/libc.so.6
>> Missing separate debuginfos, use: debuginfo-install audit-libs-2.3.2-1.fc19.x86_64 bzip2-libs-1.0.6-8.fc19.x86_64 elfutils-libelf-0.156-5.fc19.x86_64 elfutils-libs-0.156-5.fc19.x86_64 glibc-2.17-19.fc19.x86_64 libgcc-4.8.2-1.fc19.x86_64 libunwind-1.1-2.fc19.x86_64 nss-softokn-freebl-3.15.2-2.fc19.x86_64 numactl-libs-2.0.8-4.fc19.x86_64 perl-libs-5.16.3-266.fc19.x86_64 python-libs-2.7.5-9.fc19.x86_64 slang-2.2.4-8.fc19.x86_64 xz-libs-5.1.2-4alpha.fc19.x86_64 zlib-1.2.7-10.fc19.x86_64
>> (gdb) 
>> (gdb) bt
>> #0  0x0000003e590359e9 in raise () from /lib64/libc.so.6
>> #1  0x0000003e590370f8 in abort () from /lib64/libc.so.6
>> #2  0x0000003e59075d17 in __libc_message () from /lib64/libc.so.6
>> #3  0x0000003e5907bbe7 in malloc_printerr () from /lib64/libc.so.6
>> #4  0x000000000046da89 in dso__delete (dso=0x8e46f0) at util/dso.c:496
>> #5  0x0000000000482e7d in dsos__delete (dsos=0x8e4490) at util/machine.c:72
>> #6  machine__exit (machine=<optimized out>) at util/machine.c:103
>> #7  machines__exit (machines=machines@entry=0x8e4438) at util/machine.c:123
>> #8  0x0000000000488c66 in perf_session__delete (session=0x8e4360) at util/session.c:155
>> #9  0x00000000004345f4 in __cmd_top (top=0x7fffffffb140) at builtin-top.c:985
>> #10 cmd_top (argc=<optimized out>, argv=<optimized out>, prefix=<optimized out>) at builtin-top.c:1210
>> #11 0x0000000000419f95 in run_builtin (p=p@entry=0x7ece88 <commands+264>, argc=argc@entry=2, argv=argv@entry=0x7fffffffe420) at perf.c:319
>> #12 0x0000000000419830 in handle_internal_command (argv=0x7fffffffe420, argc=2) at perf.c:376
>> #13 run_argv (argv=0x7fffffffe220, argcp=0x7fffffffe22c) at perf.c:420
>> #14 main (argc=2, argv=0x7fffffffe420) at perf.c:529
>> (gdb) 
>>
>> Running it on an up-to-date installation of Fedora 19.
>>
>> Thanks,
>>
>> 	Ingo
> 
> 

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Ingo Molnar]

* Adrian Hunter <adrian.hunter@intel.com> wrote:

> -void dso__set_short_name(struct dso *dso, const char *name)
> +void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc)
>  {
>  	if (name == NULL)
>  		return;
> +	if (dso->sname_alloc)
> +		free((char *)dso->short_name);
> +	dso->sname_alloc = sname_alloc;

Calling the function option the same as the field name is asking for 
trouble - I'd suggest 'new_sname_alloc' for the parameter, or so.

And I'd also remove the 'const' from struct dso::short_name, it 
probably does not help code generation, because 'dso' is passed in as 
const in all the non-lifetime methods anyway.

That way the cast can be dropped from the free().

Similar problems exist with the usage of 'short_name' - it overloads 
the field name which makes it somewhat confusing, and it's also 
sometimes inconsistently named, such as 'name' in 
dso__set_short_name().

Ditto for 'long_name' handling.

Also, the 'sname_alloc' name sucks, it does not make it obvious that 
it's related to 'short_name', hiding its true significance (and hiding 
the broken life time handling of the flag/pointer combo). I'd rename 
it to something more descriptive, like ->short_name_allocated - or I'd 
rename everything to 'sname'/'lname' naming for short/long names.

Every time one runs into a crash like this it's a canary signal that 
cleanliness principles need hardening.

Thanks,

	Ingo

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Arnaldo Carvalho de Melo]

Em Tue, Dec 10, 2013 at 02:22:58PM +0200, Adrian Hunter escreveu:
> On 10/12/13 14:10, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Dec 10, 2013 at 02:13:12PM +0200, Adrian Hunter escreveu:
> >> On 10/12/13 13:44, Arnaldo Carvalho de Melo wrote:
> >>> Em Tue, Dec 10, 2013 at 12:12:29PM +0100, Ingo Molnar escreveu:
> >>>>
> >>>> Hm, I've unpulled it because 'perf top' crashes on exit, in 
> >>>> dso__delete():
> >>>
> >>> 495		if (dso->sname_alloc)
> >>> 496			free((char *)dso->short_name)
> >>>
> >>> Yeah, must be that basename() patch from Stephane, I'll work on a fix
> >>> and resubmit this batch, thanks for the report.
> >>
> >> The problem is sname_alloc is not maintained.  Perhaps it should be
> >> set in dso__set_short_name() e.g.
> > 
> > Yeah, sounds better than having all callers manage that thing, quickie,
> > was this with Stephane's patch applied?
> 
> Yes it was at
> 	e993d10caeb6dca690dbaf86e1981ba240d1414a
> 	perf symbols: fix bug in usage of the basename() function

Yes, this is the buggy patch, my question was if Ingo did the changes
that streamlined the dso->sname_alloc management with e993d10caeb6
applied to his working tree.

- Arnaldo
 
> > I think it should be done as a prep, then apply a modified version of
> > Stephanes, that doesn't deal with the alloc flag (more than using 'true'
> > to say it is a malloc'ed chunk).
> > 
> > - Arnaldo
> >  
> >> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> >> index 9fae484..54ed980 100644
> >> --- a/tools/perf/util/dso.c
> >> +++ b/tools/perf/util/dso.c
> >> @@ -379,7 +379,7 @@ struct dso *dso__kernel_findnew(struct machine *machine,
> >> const char *name,
> >>  	 * processing we had no idea this was the kernel dso.
> >>  	 */
> >>  	if (dso != NULL) {
> >> -		dso__set_short_name(dso, short_name);
> >> +		dso__set_short_name(dso, short_name, false);
> >>  		dso->kernel = dso_type;
> >>  	}
> >>
> >> @@ -394,10 +394,13 @@ void dso__set_long_name(struct dso *dso, char *name)
> >>  	dso->long_name_len = strlen(name);
> >>  }
> >>
> >> -void dso__set_short_name(struct dso *dso, const char *name)
> >> +void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc)
> >>  {
> >>  	if (name == NULL)
> >>  		return;
> >> +	if (dso->sname_alloc)
> >> +		free((char *)dso->short_name);
> >> +	dso->sname_alloc = sname_alloc;
> >>  	dso->short_name = name;
> >>  	dso->short_name_len = strlen(name);
> >>  }
> >> @@ -426,12 +429,7 @@ static void dso__set_basename(struct dso *dso)
> >>  	if (!base)
> >>  		return;
> >>
> >> -	if (dso->sname_alloc)
> >> -		free((char *)dso->short_name);
> >> -	else
> >> -		dso->sname_alloc = 1;
> >> -
> >> -	dso__set_short_name(dso, base);
> >> +	dso__set_short_name(dso, base, true);
> >>  }
> >>
> >>  int dso__name_len(const struct dso *dso)
> >> @@ -467,7 +465,7 @@ struct dso *dso__new(const char *name)
> >>  		int i;
> >>  		strcpy(dso->name, name);
> >>  		dso__set_long_name(dso, dso->name);
> >> -		dso__set_short_name(dso, dso->name);
> >> +		dso__set_short_name(dso, dso->name, false);
> >>  		for (i = 0; i < MAP__NR_TYPES; ++i)
> >>  			dso->symbols[i] = dso->symbol_names[i] = RB_ROOT;
> >>  		dso->cache = RB_ROOT;
> >> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> >> index 384f2d9..166463e 100644
> >> --- a/tools/perf/util/dso.h
> >> +++ b/tools/perf/util/dso.h
> >> @@ -110,7 +110,7 @@ static inline void dso__set_loaded(struct dso *dso, enum
> >> map_type type)
> >>  struct dso *dso__new(const char *name);
> >>  void dso__delete(struct dso *dso);
> >>
> >> -void dso__set_short_name(struct dso *dso, const char *name);
> >> +void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc);
> >>  void dso__set_long_name(struct dso *dso, char *name);
> >>
> >>  int dso__name_len(const struct dso *dso);
> >>
> >>
> >>>
> >>> - Arnaldo
> >>>  
> >>>> [Thread 0x7ffff70df700 (LWP 29561) exited]
> >>>> *** Error in `/fast/mingo/tip/tools/perf/perf': munmap_chunk(): invalid pointer: 0x0000000000587371 ***
> >>>> ======= Backtrace: =========
> >>>> /lib64/libc.so.6[0x3e5907bbe7]
> >>>> /fast/mingo/tip/tools/perf/perf(dso__delete+0xd9)[0x46da89]
> >>>> /fast/mingo/tip/tools/perf/perf(machines__exit+0xad)[0x482e7d]
> >>>> /fast/mingo/tip/tools/perf/perf(perf_session__delete+0xb6)[0x488c66]
> >>>> /fast/mingo/tip/tools/perf/perf(cmd_top+0xf44)[0x4345f4]
> >>>> /fast/mingo/tip/tools/perf/perf[0x419f95]
> >>>> /fast/mingo/tip/tools/perf/perf(main+0x600)[0x419830]
> >>>> /lib64/libc.so.6(__libc_start_main+0xf5)[0x3e59021b45]
> >>>> /fast/mingo/tip/tools/perf/perf[0x4198fd]
> >>>> ======= Memory map: ========
> >>>>
> >>>> Program received signal SIGABRT, Aborted.
> >>>> 0x0000003e590359e9 in raise () from /lib64/libc.so.6
> >>>> Missing separate debuginfos, use: debuginfo-install audit-libs-2.3.2-1.fc19.x86_64 bzip2-libs-1.0.6-8.fc19.x86_64 elfutils-libelf-0.156-5.fc19.x86_64 elfutils-libs-0.156-5.fc19.x86_64 glibc-2.17-19.fc19.x86_64 libgcc-4.8.2-1.fc19.x86_64 libunwind-1.1-2.fc19.x86_64 nss-softokn-freebl-3.15.2-2.fc19.x86_64 numactl-libs-2.0.8-4.fc19.x86_64 perl-libs-5.16.3-266.fc19.x86_64 python-libs-2.7.5-9.fc19.x86_64 slang-2.2.4-8.fc19.x86_64 xz-libs-5.1.2-4alpha.fc19.x86_64 zlib-1.2.7-10.fc19.x86_64
> >>>> (gdb) 
> >>>> (gdb) bt
> >>>> #0  0x0000003e590359e9 in raise () from /lib64/libc.so.6
> >>>> #1  0x0000003e590370f8 in abort () from /lib64/libc.so.6
> >>>> #2  0x0000003e59075d17 in __libc_message () from /lib64/libc.so.6
> >>>> #3  0x0000003e5907bbe7 in malloc_printerr () from /lib64/libc.so.6
> >>>> #4  0x000000000046da89 in dso__delete (dso=0x8e46f0) at util/dso.c:496
> >>>> #5  0x0000000000482e7d in dsos__delete (dsos=0x8e4490) at util/machine.c:72
> >>>> #6  machine__exit (machine=<optimized out>) at util/machine.c:103
> >>>> #7  machines__exit (machines=machines@entry=0x8e4438) at util/machine.c:123
> >>>> #8  0x0000000000488c66 in perf_session__delete (session=0x8e4360) at util/session.c:155
> >>>> #9  0x00000000004345f4 in __cmd_top (top=0x7fffffffb140) at builtin-top.c:985
> >>>> #10 cmd_top (argc=<optimized out>, argv=<optimized out>, prefix=<optimized out>) at builtin-top.c:1210
> >>>> #11 0x0000000000419f95 in run_builtin (p=p@entry=0x7ece88 <commands+264>, argc=argc@entry=2, argv=argv@entry=0x7fffffffe420) at perf.c:319
> >>>> #12 0x0000000000419830 in handle_internal_command (argv=0x7fffffffe420, argc=2) at perf.c:376
> >>>> #13 run_argv (argv=0x7fffffffe220, argcp=0x7fffffffe22c) at perf.c:420
> >>>> #14 main (argc=2, argv=0x7fffffffe420) at perf.c:529
> >>>> (gdb) 
> >>>>
> >>>> Running it on an up-to-date installation of Fedora 19.
> >>>>
> >>>> Thanks,
> >>>>
> >>>> 	Ingo
> >>>
> >>>
> > 
> > 

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Adrian Hunter]

On 10/12/13 14:10, Arnaldo Carvalho de Melo wrote:
> Em Tue, Dec 10, 2013 at 02:13:12PM +0200, Adrian Hunter escreveu:
>> On 10/12/13 13:44, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, Dec 10, 2013 at 12:12:29PM +0100, Ingo Molnar escreveu:
>>>>
>>>> Hm, I've unpulled it because 'perf top' crashes on exit, in 
>>>> dso__delete():
>>>
>>> 495		if (dso->sname_alloc)
>>> 496			free((char *)dso->short_name)
>>>
>>> Yeah, must be that basename() patch from Stephane, I'll work on a fix
>>> and resubmit this batch, thanks for the report.
>>
>> The problem is sname_alloc is not maintained.  Perhaps it should be
>> set in dso__set_short_name() e.g.
> 
> Yeah, sounds better than having all callers manage that thing, quickie,
> was this with Stephane's patch applied?

Yes it was at
	e993d10caeb6dca690dbaf86e1981ba240d1414a
	perf symbols: fix bug in usage of the basename() function

> 
> I think it should be done as a prep, then apply a modified version of
> Stephanes, that doesn't deal with the alloc flag (more than using 'true'
> to say it is a malloc'ed chunk).
> 
> - Arnaldo
>  
>> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
>> index 9fae484..54ed980 100644
>> --- a/tools/perf/util/dso.c
>> +++ b/tools/perf/util/dso.c
>> @@ -379,7 +379,7 @@ struct dso *dso__kernel_findnew(struct machine *machine,
>> const char *name,
>>  	 * processing we had no idea this was the kernel dso.
>>  	 */
>>  	if (dso != NULL) {
>> -		dso__set_short_name(dso, short_name);
>> +		dso__set_short_name(dso, short_name, false);
>>  		dso->kernel = dso_type;
>>  	}
>>
>> @@ -394,10 +394,13 @@ void dso__set_long_name(struct dso *dso, char *name)
>>  	dso->long_name_len = strlen(name);
>>  }
>>
>> -void dso__set_short_name(struct dso *dso, const char *name)
>> +void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc)
>>  {
>>  	if (name == NULL)
>>  		return;
>> +	if (dso->sname_alloc)
>> +		free((char *)dso->short_name);
>> +	dso->sname_alloc = sname_alloc;
>>  	dso->short_name = name;
>>  	dso->short_name_len = strlen(name);
>>  }
>> @@ -426,12 +429,7 @@ static void dso__set_basename(struct dso *dso)
>>  	if (!base)
>>  		return;
>>
>> -	if (dso->sname_alloc)
>> -		free((char *)dso->short_name);
>> -	else
>> -		dso->sname_alloc = 1;
>> -
>> -	dso__set_short_name(dso, base);
>> +	dso__set_short_name(dso, base, true);
>>  }
>>
>>  int dso__name_len(const struct dso *dso)
>> @@ -467,7 +465,7 @@ struct dso *dso__new(const char *name)
>>  		int i;
>>  		strcpy(dso->name, name);
>>  		dso__set_long_name(dso, dso->name);
>> -		dso__set_short_name(dso, dso->name);
>> +		dso__set_short_name(dso, dso->name, false);
>>  		for (i = 0; i < MAP__NR_TYPES; ++i)
>>  			dso->symbols[i] = dso->symbol_names[i] = RB_ROOT;
>>  		dso->cache = RB_ROOT;
>> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
>> index 384f2d9..166463e 100644
>> --- a/tools/perf/util/dso.h
>> +++ b/tools/perf/util/dso.h
>> @@ -110,7 +110,7 @@ static inline void dso__set_loaded(struct dso *dso, enum
>> map_type type)
>>  struct dso *dso__new(const char *name);
>>  void dso__delete(struct dso *dso);
>>
>> -void dso__set_short_name(struct dso *dso, const char *name);
>> +void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc);
>>  void dso__set_long_name(struct dso *dso, char *name);
>>
>>  int dso__name_len(const struct dso *dso);
>>
>>
>>>
>>> - Arnaldo
>>>  
>>>> [Thread 0x7ffff70df700 (LWP 29561) exited]
>>>> *** Error in `/fast/mingo/tip/tools/perf/perf': munmap_chunk(): invalid pointer: 0x0000000000587371 ***
>>>> ======= Backtrace: =========
>>>> /lib64/libc.so.6[0x3e5907bbe7]
>>>> /fast/mingo/tip/tools/perf/perf(dso__delete+0xd9)[0x46da89]
>>>> /fast/mingo/tip/tools/perf/perf(machines__exit+0xad)[0x482e7d]
>>>> /fast/mingo/tip/tools/perf/perf(perf_session__delete+0xb6)[0x488c66]
>>>> /fast/mingo/tip/tools/perf/perf(cmd_top+0xf44)[0x4345f4]
>>>> /fast/mingo/tip/tools/perf/perf[0x419f95]
>>>> /fast/mingo/tip/tools/perf/perf(main+0x600)[0x419830]
>>>> /lib64/libc.so.6(__libc_start_main+0xf5)[0x3e59021b45]
>>>> /fast/mingo/tip/tools/perf/perf[0x4198fd]
>>>> ======= Memory map: ========
>>>>
>>>> Program received signal SIGABRT, Aborted.
>>>> 0x0000003e590359e9 in raise () from /lib64/libc.so.6
>>>> Missing separate debuginfos, use: debuginfo-install audit-libs-2.3.2-1.fc19.x86_64 bzip2-libs-1.0.6-8.fc19.x86_64 elfutils-libelf-0.156-5.fc19.x86_64 elfutils-libs-0.156-5.fc19.x86_64 glibc-2.17-19.fc19.x86_64 libgcc-4.8.2-1.fc19.x86_64 libunwind-1.1-2.fc19.x86_64 nss-softokn-freebl-3.15.2-2.fc19.x86_64 numactl-libs-2.0.8-4.fc19.x86_64 perl-libs-5.16.3-266.fc19.x86_64 python-libs-2.7.5-9.fc19.x86_64 slang-2.2.4-8.fc19.x86_64 xz-libs-5.1.2-4alpha.fc19.x86_64 zlib-1.2.7-10.fc19.x86_64
>>>> (gdb) 
>>>> (gdb) bt
>>>> #0  0x0000003e590359e9 in raise () from /lib64/libc.so.6
>>>> #1  0x0000003e590370f8 in abort () from /lib64/libc.so.6
>>>> #2  0x0000003e59075d17 in __libc_message () from /lib64/libc.so.6
>>>> #3  0x0000003e5907bbe7 in malloc_printerr () from /lib64/libc.so.6
>>>> #4  0x000000000046da89 in dso__delete (dso=0x8e46f0) at util/dso.c:496
>>>> #5  0x0000000000482e7d in dsos__delete (dsos=0x8e4490) at util/machine.c:72
>>>> #6  machine__exit (machine=<optimized out>) at util/machine.c:103
>>>> #7  machines__exit (machines=machines@entry=0x8e4438) at util/machine.c:123
>>>> #8  0x0000000000488c66 in perf_session__delete (session=0x8e4360) at util/session.c:155
>>>> #9  0x00000000004345f4 in __cmd_top (top=0x7fffffffb140) at builtin-top.c:985
>>>> #10 cmd_top (argc=<optimized out>, argv=<optimized out>, prefix=<optimized out>) at builtin-top.c:1210
>>>> #11 0x0000000000419f95 in run_builtin (p=p@entry=0x7ece88 <commands+264>, argc=argc@entry=2, argv=argv@entry=0x7fffffffe420) at perf.c:319
>>>> #12 0x0000000000419830 in handle_internal_command (argv=0x7fffffffe420, argc=2) at perf.c:376
>>>> #13 run_argv (argv=0x7fffffffe220, argcp=0x7fffffffe22c) at perf.c:420
>>>> #14 main (argc=2, argv=0x7fffffffe420) at perf.c:529
>>>> (gdb) 
>>>>
>>>> Running it on an up-to-date installation of Fedora 19.
>>>>
>>>> Thanks,
>>>>
>>>> 	Ingo
>>>
>>>
> 
> 

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Arnaldo Carvalho de Melo]

Em Tue, Dec 10, 2013 at 09:22:13AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Dec 10, 2013 at 02:22:58PM +0200, Adrian Hunter escreveu:
> > On 10/12/13 14:10, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, Dec 10, 2013 at 02:13:12PM +0200, Adrian Hunter escreveu:
> > >> On 10/12/13 13:44, Arnaldo Carvalho de Melo wrote:
> > >>> Em Tue, Dec 10, 2013 at 12:12:29PM +0100, Ingo Molnar escreveu:
> > >>>>
> > >>>> Hm, I've unpulled it because 'perf top' crashes on exit, in 
> > >>>> dso__delete():
> > >>>
> > >>> 495		if (dso->sname_alloc)
> > >>> 496			free((char *)dso->short_name)
> > >>>
> > >>> Yeah, must be that basename() patch from Stephane, I'll work on a fix
> > >>> and resubmit this batch, thanks for the report.
> > >>
> > >> The problem is sname_alloc is not maintained.  Perhaps it should be
> > >> set in dso__set_short_name() e.g.
> > > 
> > > Yeah, sounds better than having all callers manage that thing, quickie,
> > > was this with Stephane's patch applied?
> > 
> > Yes it was at
> > 	e993d10caeb6dca690dbaf86e1981ba240d1414a
> > 	perf symbols: fix bug in usage of the basename() function
> 
> Yes, this is the buggy patch, my question was if Ingo did the changes
> that streamlined the dso->sname_alloc management with e993d10caeb6
> applied to his working tree.

Sorry Adrian, my bad, I thought Ingo had provided the patch, now I
realized my mistake, it was you 8-)
 
> - Arnaldo
>  
> > > I think it should be done as a prep, then apply a modified version of
> > > Stephanes, that doesn't deal with the alloc flag (more than using 'true'
> > > to say it is a malloc'ed chunk).
> > > 
> > > - Arnaldo
> > >  
> > >> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > >> index 9fae484..54ed980 100644
> > >> --- a/tools/perf/util/dso.c
> > >> +++ b/tools/perf/util/dso.c
> > >> @@ -379,7 +379,7 @@ struct dso *dso__kernel_findnew(struct machine *machine,
> > >> const char *name,
> > >>  	 * processing we had no idea this was the kernel dso.
> > >>  	 */
> > >>  	if (dso != NULL) {
> > >> -		dso__set_short_name(dso, short_name);
> > >> +		dso__set_short_name(dso, short_name, false);
> > >>  		dso->kernel = dso_type;
> > >>  	}
> > >>
> > >> @@ -394,10 +394,13 @@ void dso__set_long_name(struct dso *dso, char *name)
> > >>  	dso->long_name_len = strlen(name);
> > >>  }
> > >>
> > >> -void dso__set_short_name(struct dso *dso, const char *name)
> > >> +void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc)
> > >>  {
> > >>  	if (name == NULL)
> > >>  		return;
> > >> +	if (dso->sname_alloc)
> > >> +		free((char *)dso->short_name);
> > >> +	dso->sname_alloc = sname_alloc;
> > >>  	dso->short_name = name;
> > >>  	dso->short_name_len = strlen(name);
> > >>  }
> > >> @@ -426,12 +429,7 @@ static void dso__set_basename(struct dso *dso)
> > >>  	if (!base)
> > >>  		return;
> > >>
> > >> -	if (dso->sname_alloc)
> > >> -		free((char *)dso->short_name);
> > >> -	else
> > >> -		dso->sname_alloc = 1;
> > >> -
> > >> -	dso__set_short_name(dso, base);
> > >> +	dso__set_short_name(dso, base, true);
> > >>  }
> > >>
> > >>  int dso__name_len(const struct dso *dso)
> > >> @@ -467,7 +465,7 @@ struct dso *dso__new(const char *name)
> > >>  		int i;
> > >>  		strcpy(dso->name, name);
> > >>  		dso__set_long_name(dso, dso->name);
> > >> -		dso__set_short_name(dso, dso->name);
> > >> +		dso__set_short_name(dso, dso->name, false);
> > >>  		for (i = 0; i < MAP__NR_TYPES; ++i)
> > >>  			dso->symbols[i] = dso->symbol_names[i] = RB_ROOT;
> > >>  		dso->cache = RB_ROOT;
> > >> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> > >> index 384f2d9..166463e 100644
> > >> --- a/tools/perf/util/dso.h
> > >> +++ b/tools/perf/util/dso.h
> > >> @@ -110,7 +110,7 @@ static inline void dso__set_loaded(struct dso *dso, enum
> > >> map_type type)
> > >>  struct dso *dso__new(const char *name);
> > >>  void dso__delete(struct dso *dso);
> > >>
> > >> -void dso__set_short_name(struct dso *dso, const char *name);
> > >> +void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc);
> > >>  void dso__set_long_name(struct dso *dso, char *name);
> > >>
> > >>  int dso__name_len(const struct dso *dso);
> > >>
> > >>
> > >>>
> > >>> - Arnaldo
> > >>>  
> > >>>> [Thread 0x7ffff70df700 (LWP 29561) exited]
> > >>>> *** Error in `/fast/mingo/tip/tools/perf/perf': munmap_chunk(): invalid pointer: 0x0000000000587371 ***
> > >>>> ======= Backtrace: =========
> > >>>> /lib64/libc.so.6[0x3e5907bbe7]
> > >>>> /fast/mingo/tip/tools/perf/perf(dso__delete+0xd9)[0x46da89]
> > >>>> /fast/mingo/tip/tools/perf/perf(machines__exit+0xad)[0x482e7d]
> > >>>> /fast/mingo/tip/tools/perf/perf(perf_session__delete+0xb6)[0x488c66]
> > >>>> /fast/mingo/tip/tools/perf/perf(cmd_top+0xf44)[0x4345f4]
> > >>>> /fast/mingo/tip/tools/perf/perf[0x419f95]
> > >>>> /fast/mingo/tip/tools/perf/perf(main+0x600)[0x419830]
> > >>>> /lib64/libc.so.6(__libc_start_main+0xf5)[0x3e59021b45]
> > >>>> /fast/mingo/tip/tools/perf/perf[0x4198fd]
> > >>>> ======= Memory map: ========
> > >>>>
> > >>>> Program received signal SIGABRT, Aborted.
> > >>>> 0x0000003e590359e9 in raise () from /lib64/libc.so.6
> > >>>> Missing separate debuginfos, use: debuginfo-install audit-libs-2.3.2-1.fc19.x86_64 bzip2-libs-1.0.6-8.fc19.x86_64 elfutils-libelf-0.156-5.fc19.x86_64 elfutils-libs-0.156-5.fc19.x86_64 glibc-2.17-19.fc19.x86_64 libgcc-4.8.2-1.fc19.x86_64 libunwind-1.1-2.fc19.x86_64 nss-softokn-freebl-3.15.2-2.fc19.x86_64 numactl-libs-2.0.8-4.fc19.x86_64 perl-libs-5.16.3-266.fc19.x86_64 python-libs-2.7.5-9.fc19.x86_64 slang-2.2.4-8.fc19.x86_64 xz-libs-5.1.2-4alpha.fc19.x86_64 zlib-1.2.7-10.fc19.x86_64
> > >>>> (gdb) 
> > >>>> (gdb) bt
> > >>>> #0  0x0000003e590359e9 in raise () from /lib64/libc.so.6
> > >>>> #1  0x0000003e590370f8 in abort () from /lib64/libc.so.6
> > >>>> #2  0x0000003e59075d17 in __libc_message () from /lib64/libc.so.6
> > >>>> #3  0x0000003e5907bbe7 in malloc_printerr () from /lib64/libc.so.6
> > >>>> #4  0x000000000046da89 in dso__delete (dso=0x8e46f0) at util/dso.c:496
> > >>>> #5  0x0000000000482e7d in dsos__delete (dsos=0x8e4490) at util/machine.c:72
> > >>>> #6  machine__exit (machine=<optimized out>) at util/machine.c:103
> > >>>> #7  machines__exit (machines=machines@entry=0x8e4438) at util/machine.c:123
> > >>>> #8  0x0000000000488c66 in perf_session__delete (session=0x8e4360) at util/session.c:155
> > >>>> #9  0x00000000004345f4 in __cmd_top (top=0x7fffffffb140) at builtin-top.c:985
> > >>>> #10 cmd_top (argc=<optimized out>, argv=<optimized out>, prefix=<optimized out>) at builtin-top.c:1210
> > >>>> #11 0x0000000000419f95 in run_builtin (p=p@entry=0x7ece88 <commands+264>, argc=argc@entry=2, argv=argv@entry=0x7fffffffe420) at perf.c:319
> > >>>> #12 0x0000000000419830 in handle_internal_command (argv=0x7fffffffe420, argc=2) at perf.c:376
> > >>>> #13 run_argv (argv=0x7fffffffe220, argcp=0x7fffffffe22c) at perf.c:420
> > >>>> #14 main (argc=2, argv=0x7fffffffe420) at perf.c:529
> > >>>> (gdb) 
> > >>>>
> > >>>> Running it on an up-to-date installation of Fedora 19.
> > >>>>
> > >>>> Thanks,
> > >>>>
> > >>>> 	Ingo
> > >>>
> > >>>
> > > 
> > > 

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:

> Em Tue, Dec 10, 2013 at 02:22:58PM +0200, Adrian Hunter escreveu:
> > On 10/12/13 14:10, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, Dec 10, 2013 at 02:13:12PM +0200, Adrian Hunter escreveu:
> > >> On 10/12/13 13:44, Arnaldo Carvalho de Melo wrote:
> > >>> Em Tue, Dec 10, 2013 at 12:12:29PM +0100, Ingo Molnar escreveu:
> > >>>>
> > >>>> Hm, I've unpulled it because 'perf top' crashes on exit, in 
> > >>>> dso__delete():
> > >>>
> > >>> 495		if (dso->sname_alloc)
> > >>> 496			free((char *)dso->short_name)
> > >>>
> > >>> Yeah, must be that basename() patch from Stephane, I'll work on a fix
> > >>> and resubmit this batch, thanks for the report.
> > >>
> > >> The problem is sname_alloc is not maintained.  Perhaps it should be
> > >> set in dso__set_short_name() e.g.
> > > 
> > > Yeah, sounds better than having all callers manage that thing, quickie,
> > > was this with Stephane's patch applied?
> > 
> > Yes it was at
> > 	e993d10caeb6dca690dbaf86e1981ba240d1414a
> > 	perf symbols: fix bug in usage of the basename() function
> 
> Yes, this is the buggy patch, my question was if Ingo did the 
> changes that streamlined the dso->sname_alloc management with 
> e993d10caeb6 applied to his working tree.

My current perf/core head is:

  789790791ad2 tools/perf/build: Fix install dependency

which does not have e993d10c.

[ Btw., a small nit: the capitalization of the commit title looks 
  inconsistent. ]

Thanks,

	Ingo

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
> > -void dso__set_short_name(struct dso *dso, const char *name)
> > +void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc)
> >  {
> >  	if (name == NULL)
> >  		return;
> > +	if (dso->sname_alloc)
> > +		free((char *)dso->short_name);
> > +	dso->sname_alloc = sname_alloc;
> 
> Calling the function option the same as the field name is asking for 
> trouble - I'd suggest 'new_sname_alloc' for the parameter, or so.
> 
> And I'd also remove the 'const' from struct dso::short_name, it 
> probably does not help code generation, because 'dso' is passed in as 
> const in all the non-lifetime methods anyway.
> 
> That way the cast can be dropped from the free().
> 
> Similar problems exist with the usage of 'short_name' - it overloads 
> the field name which makes it somewhat confusing, and it's also 
> sometimes inconsistently named, such as 'name' in 
> dso__set_short_name().
> 
> Ditto for 'long_name' handling.
> 
> Also, the 'sname_alloc' name sucks, it does not make it obvious that 
> it's related to 'short_name', hiding its true significance (and hiding 
> the broken life time handling of the flag/pointer combo). I'd rename 
> it to something more descriptive, like ->short_name_allocated - or I'd 
> rename everything to 'sname'/'lname' naming for short/long names.
> 
> Every time one runs into a crash like this it's a canary signal that 
> cleanliness principles need hardening.

More observations about util/dso.c:

 - dso__binary_type_file() should probably pass in 'const struct dso'

 - dso__binary_type_file()'s filename string parameter should be named 
   'filename', not 'file' ...

 - build_id__sprintf() looks fragile: every single use of it appears 
   to follow this pattern:

	build_id__sprintf(x, sizeof(x), ...)

   this could be simplified (and eliminating the possibility to typo a 
   bug) by changing the function to __build_id__snprintf() and adding 
   a build_id__sprintf() wrapper macro around it:

	build_id__sprintf(x, ...)

   that generates the size itself.

 - dso__binary_type_file() is a method without a verb, so it's unclear 
   what it does. It probably wants to be renamed to 
   dso__set_binary_type_file() or so?

 - dso_cache__find() probably wants to pass in a const rb_root.

 - 'struct dso *pos' should probably be named 'struct dso *dso_pos' or 
   so - 'pos' is frequently used for integer variable names so its use 
   for an object iterator feels confusing.

 - the 'head' argument of dsos__find() wants to be constified too I 
   guess

Thanks,

	Ingo

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Arnaldo Carvalho de Melo]

Em Tue, Dec 10, 2013 at 01:46:58PM +0100, Ingo Molnar escreveu:
> * Ingo Molnar <mingo@kernel.org> wrote:
> > Every time one runs into a crash like this it's a canary signal that 
> > cleanliness principles need hardening.
> 
> More observations about util/dso.c:
> 
>  - dso__binary_type_file() should probably pass in 'const struct dso'
> 
>  - dso__binary_type_file()'s filename string parameter should be named 
>    'filename', not 'file' ...
> 
>  - build_id__sprintf() looks fragile: every single use of it appears 
>    to follow this pattern:
> 
> 	build_id__sprintf(x, sizeof(x), ...)
> 
>    this could be simplified (and eliminating the possibility to typo a 
>    bug) by changing the function to __build_id__snprintf() and adding 
>    a build_id__sprintf() wrapper macro around it:
> 
> 	build_id__sprintf(x, ...)
> 
>    that generates the size itself.

Right, like:

int __perf_evlist__add_default_attrs(struct perf_evlist *evlist,
                                     struct perf_event_attr *attrs, size_t nr_attrs);

#define perf_evlist__add_default_attrs(evlist, array) \
        __perf_evlist__add_default_attrs(evlist, array, ARRAY_SIZE(array))

This is all a matter of being more dilligent and judicious at employing
these and other good practices.

But don't be shy to point anything (like you did here), as time permits
we can go on doing patchkits to address things people notice.

- Arnaldo

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Arnaldo Carvalho de Melo]

Em Tue, Dec 10, 2013 at 01:18:01PM +0100, Ingo Molnar escreveu:
> 
> * Adrian Hunter <adrian.hunter@intel.com> wrote:
> 
> > -void dso__set_short_name(struct dso *dso, const char *name)
> > +void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc)
> >  {
> >  	if (name == NULL)
> >  		return;
> > +	if (dso->sname_alloc)
> > +		free((char *)dso->short_name);
> > +	dso->sname_alloc = sname_alloc;
> 
> Calling the function option the same as the field name is asking for 
> trouble - I'd suggest 'new_sname_alloc' for the parameter, or so.
> 
> And I'd also remove the 'const' from struct dso::short_name, it 
> probably does not help code generation, because 'dso' is passed in as 
> const in all the non-lifetime methods anyway.
 
> That way the cast can be dropped from the free().

Not that simple, there are multiple places that pass a constant
short_name, for instance:

	machine__get_kernel()
                kernel = dso__kernel_findnew(machine, vmlinux_name,
					     "[kernel]", DSO_TYPE_KERNEL);
			dso__set_short_name(dso, short_name);

So dso->short_name will point to "[kernel]", which is a const char *.

> Similar problems exist with the usage of 'short_name' - it overloads 
> the field name which makes it somewhat confusing, and it's also 
> sometimes inconsistently named, such as 'name' in 
> dso__set_short_name().
> 
> Ditto for 'long_name' handling.
> 
> Also, the 'sname_alloc' name sucks, it does not make it obvious that 

> it's related to 'short_name', hiding its true significance (and hiding 
> the broken life time handling of the flag/pointer combo). I'd rename 
> it to something more descriptive, like ->short_name_allocated - or I'd 
> rename everything to 'sname'/'lname' naming for short/long names.

Ok, we can use rename it to short_name_alloc, like we have
short_name_len.
 
> Every time one runs into a crash like this it's a canary signal that 
> cleanliness principles need hardening.

Hardening we go then!
 
> Thanks,
> 
> 	Ingo

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:

> Em Tue, Dec 10, 2013 at 01:18:01PM +0100, Ingo Molnar escreveu:
> > 
> > * Adrian Hunter <adrian.hunter@intel.com> wrote:
> > 
> > > -void dso__set_short_name(struct dso *dso, const char *name)
> > > +void dso__set_short_name(struct dso *dso, const char *name, bool sname_alloc)
> > >  {
> > >  	if (name == NULL)
> > >  		return;
> > > +	if (dso->sname_alloc)
> > > +		free((char *)dso->short_name);
> > > +	dso->sname_alloc = sname_alloc;
> > 
> > Calling the function option the same as the field name is asking for 
> > trouble - I'd suggest 'new_sname_alloc' for the parameter, or so.
> > 
> > And I'd also remove the 'const' from struct dso::short_name, it 
> > probably does not help code generation, because 'dso' is passed in as 
> > const in all the non-lifetime methods anyway.
>  
> > That way the cast can be dropped from the free().
> 
> Not that simple, there are multiple places that pass a constant
> short_name, for instance:
> 
> 	machine__get_kernel()
>                 kernel = dso__kernel_findnew(machine, vmlinux_name,
> 					     "[kernel]", DSO_TYPE_KERNEL);
> 			dso__set_short_name(dso, short_name);
> 
> So dso->short_name will point to "[kernel]", which is a const char *.

Okay, I guess the free() cast is fine then.

Thanks,

	Ingo

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Jiri Olsa]

On Tue, Dec 10, 2013 at 12:07:59PM +0100, Ingo Molnar wrote:
> 

SNIP

> 
> Pulled, thanks Arnaldo!
> 
> There's one detail I noticed about the recent trace-plugin changes:
> 
> comet:~/tip/tools/perf> make install
>   BUILD:   Doing 'make -j12' parallel build
>   SUBDIR   Documentation
>   INSTALL  Documentation-man
>   INSTALL  GTK UI
>   SUBDIR   /home/mingo/tip/tools/lib/traceevent/
>   INSTALL  binaries
>   INSTALL  plugin_jbd2.so
>   INSTALL  plugin_hrtimer.so
>   INSTALL  plugin_kmem.so
>   INSTALL  plugin_kvm.so
>   INSTALL  plugin_mac80211.so
>   INSTALL  plugin_sched_switch.so
>   INSTALL  plugin_function.so
>   INSTALL  plugin_xen.so
>   INSTALL  plugin_scsi.so
>   INSTALL  plugin_cfg80211.so
>   INSTALL  libexec
>   INSTALL  perf-archive
>   INSTALL  perl-scripts
>   INSTALL  python-scripts
>   INSTALL  perf_completion-script
>   INSTALL  tests
> 
> those plugin installs are way too verbose, they should really be in a 
> single summarized line, only saying something like:
> 
>   INSTALL  plugins
> 
> Just like we already sum up 'binaries', 'libexec', 'tests', etc.

ok, TODO updated ;-)

thanks,
jirka

Re: [GIT PULL 00/21] perf/core improvements and fixes

[Ingo Molnar]

* Jiri Olsa <jolsa@redhat.com> wrote:

> On Tue, Dec 10, 2013 at 12:07:59PM +0100, Ingo Molnar wrote:
> > 
> 
> SNIP
> 
> > 
> > Pulled, thanks Arnaldo!
> > 
> > There's one detail I noticed about the recent trace-plugin changes:
> > 
> > comet:~/tip/tools/perf> make install
> >   BUILD:   Doing 'make -j12' parallel build
> >   SUBDIR   Documentation
> >   INSTALL  Documentation-man
> >   INSTALL  GTK UI
> >   SUBDIR   /home/mingo/tip/tools/lib/traceevent/
> >   INSTALL  binaries
> >   INSTALL  plugin_jbd2.so
> >   INSTALL  plugin_hrtimer.so
> >   INSTALL  plugin_kmem.so
> >   INSTALL  plugin_kvm.so
> >   INSTALL  plugin_mac80211.so
> >   INSTALL  plugin_sched_switch.so
> >   INSTALL  plugin_function.so
> >   INSTALL  plugin_xen.so
> >   INSTALL  plugin_scsi.so
> >   INSTALL  plugin_cfg80211.so
> >   INSTALL  libexec
> >   INSTALL  perf-archive
> >   INSTALL  perl-scripts
> >   INSTALL  python-scripts
> >   INSTALL  perf_completion-script
> >   INSTALL  tests
> > 
> > those plugin installs are way too verbose, they should really be in a 
> > single summarized line, only saying something like:
> > 
> >   INSTALL  plugins
> > 
> > Just like we already sum up 'binaries', 'libexec', 'tests', etc.
> 
> ok, TODO updated ;-)

Consider it a regression! ;-)

Thanks,

	Ingo

[tip:perf/core] perf symbols: Remove open coded management of short_name_allocated member

[tip-bot for Adrian Hunter]

Commit-ID:  58a98c9cc583435784a93f23754128363b4cca94
Gitweb:     http://git.kernel.org/tip/58a98c9cc583435784a93f23754128363b4cca94
Author:     Adrian Hunter <adrian.hunter@intel.com>
AuthorDate: Tue, 10 Dec 2013 11:11:46 -0300
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Tue, 10 Dec 2013 16:51:08 -0300

perf symbols: Remove open coded management of  short_name_allocated member

Instead of expecting callers to set this member accodingly so that later
at dso destruction it can, if needed, be correctly free()d, make it a
requirement by passing it as a parameter to dso__set_short_name.

Cc: Andi Kleen <ak@linux.intel.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
CC: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Namhyung Kim <namhyung.kim@lge.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Link: http://lkml.kernel.org/r/52A707A2.5020802@intel.com
[ Renamed the 'allocated' parameter to clearly indicate to which variable it refers to. ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/dso.c     | 17 +++++++++++------
 tools/perf/util/dso.h     |  2 +-
 tools/perf/util/machine.c |  3 +--
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 55c9835..f8c8497 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -379,7 +379,7 @@ struct dso *dso__kernel_findnew(struct machine *machine, const char *name,
 	 * processing we had no idea this was the kernel dso.
 	 */
 	if (dso != NULL) {
-		dso__set_short_name(dso, short_name);
+		dso__set_short_name(dso, short_name, false);
 		dso->kernel = dso_type;
 	}
 
@@ -394,17 +394,22 @@ void dso__set_long_name(struct dso *dso, char *name)
 	dso->long_name_len = strlen(name);
 }
 
-void dso__set_short_name(struct dso *dso, const char *name)
+void dso__set_short_name(struct dso *dso, const char *name, bool name_allocated)
 {
 	if (name == NULL)
 		return;
-	dso->short_name = name;
-	dso->short_name_len = strlen(name);
+
+	if (dso->short_name_allocated)
+		free((char *)dso->short_name);
+
+	dso->short_name		  = name;
+	dso->short_name_len	  = strlen(name);
+	dso->short_name_allocated = name_allocated;
 }
 
 static void dso__set_basename(struct dso *dso)
 {
-	dso__set_short_name(dso, basename(dso->long_name));
+	dso__set_short_name(dso, basename(dso->long_name), false);
 }
 
 int dso__name_len(const struct dso *dso)
@@ -440,7 +445,7 @@ struct dso *dso__new(const char *name)
 		int i;
 		strcpy(dso->name, name);
 		dso__set_long_name(dso, dso->name);
-		dso__set_short_name(dso, dso->name);
+		dso__set_short_name(dso, dso->name, false);
 		for (i = 0; i < MAP__NR_TYPES; ++i)
 			dso->symbols[i] = dso->symbol_names[i] = RB_ROOT;
 		dso->cache = RB_ROOT;
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 00a232d..8eceab7 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -110,7 +110,7 @@ static inline void dso__set_loaded(struct dso *dso, enum map_type type)
 struct dso *dso__new(const char *name);
 void dso__delete(struct dso *dso);
 
-void dso__set_short_name(struct dso *dso, const char *name);
+void dso__set_short_name(struct dso *dso, const char *name, bool name_allocated);
 void dso__set_long_name(struct dso *dso, char *name);
 
 int dso__name_len(const struct dso *dso);
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index f85da9a..09d5c66 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -935,8 +935,7 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
 		if (name == NULL)
 			goto out_problem;
 
-		dso__set_short_name(map->dso, name);
-		map->dso->short_name_allocated = 1;
+		dso__set_short_name(map->dso, name, true);
 		map->end = map->start + event->mmap.len;
 	} else if (is_kernel_mmap) {
 		const char *symbol_name = (event->mmap.filename +