[PATCH v1 0/6] config file/command line for objdump & addr2line

[PATCH v1 0/6] config file/command line for objdump & addr2line

[Ian Rogers]

Allow objdump to be set as a perf config file variable. As previously
objdump was set via the command line, the string was owned by
argv. Now the string must be strdup-ed, so the corresponding logic
needs changing with an annotation_options__init/exit.

Add command line and config file options for addr2line, set in
symbol_conf for convenience. This doesn't allow the setting of
llvm-addr2line due to a bug, but could in the future.

Ian Rogers (6):
  perf annotate: Delete session for debug builds
  perf report: Additional config warnings
  perf annotate: Add init/exit to annotation_options remove default
  perf annotate: Own objdump_path and disassembler_style strings
  perf annotate: Allow objdump to be set in perfconfig
  perf symbol: Add command line support for addr2line path

 tools/perf/Documentation/perf-annotate.txt |  3 ++
 tools/perf/Documentation/perf-config.txt   |  8 +++-
 tools/perf/Documentation/perf-report.txt   |  3 ++
 tools/perf/Documentation/perf-top.txt      |  6 +++
 tools/perf/arch/common.c                   |  4 +-
 tools/perf/arch/common.h                   |  2 +-
 tools/perf/builtin-annotate.c              | 42 +++++++++++++-------
 tools/perf/builtin-report.c                | 35 ++++++++++++++---
 tools/perf/builtin-top.c                   | 27 +++++++++++--
 tools/perf/util/annotate.c                 | 45 +++++++++++++++++-----
 tools/perf/util/annotate.h                 |  9 +++--
 tools/perf/util/srcline.c                  | 26 ++++++++-----
 tools/perf/util/symbol_conf.h              |  1 +
 13 files changed, 163 insertions(+), 48 deletions(-)

-- 
2.40.0.348.gf938b09366-goog

[PATCH v1 1/6] perf annotate: Delete session for debug builds

[Ian Rogers]

Use the debug build indicator as the guide to free the session. This
implements a behavior described in a comment, which is consequentially
removed.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-annotate.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 4750fac7bf93..98d1b6379230 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -692,16 +692,12 @@ int cmd_annotate(int argc, const char **argv)
 
 out_delete:
 	/*
-	 * Speed up the exit process, for large files this can
-	 * take quite a while.
-	 *
-	 * XXX Enable this when using valgrind or if we ever
-	 * librarize this command.
-	 *
-	 * Also experiment with obstacks to see how much speed
-	 * up we'll get here.
-	 *
-	 * perf_session__delete(session);
+	 * Speed up the exit process by only deleting for debug builds. For
+	 * large files this can save time.
 	 */
+#ifndef NDEBUG
+	perf_session__delete(annotate.session);
+#endif
+
 	return ret;
 }
-- 
2.40.0.348.gf938b09366-goog

[PATCH v1 2/6] perf report: Additional config warnings

[Ian Rogers]

If the default_sort_order isn't correctly strdup-ed warn and return an
error. Debug warn if no option is matched.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-report.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 6400615b5e98..500f9d8902e7 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -143,6 +143,10 @@ static int report__config(const char *var, const char *value, void *cb)
 
 	if (!strcmp(var, "report.sort_order")) {
 		default_sort_order = strdup(value);
+		if (!default_sort_order) {
+			pr_err("Not enough memory for report.sort_order\n");
+			return -1;
+		}
 		return 0;
 	}
 
@@ -151,6 +155,7 @@ static int report__config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	pr_debug("%s variable unknown, ignoring...", var);
 	return 0;
 }
 
-- 
2.40.0.348.gf938b09366-goog

[PATCH v1 3/6] perf annotate: Add init/exit to annotation_options remove default

[Ian Rogers]

annotation__default_options was used to initialize
annotation_options. Switch to the init/exit pattern as later changes
will give ownership over strings and this will be necessary to avoid
memory leaks.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-annotate.c |  3 ++-
 tools/perf/builtin-report.c   |  7 ++++---
 tools/perf/builtin-top.c      |  4 +++-
 tools/perf/util/annotate.c    | 25 +++++++++++++++++--------
 tools/perf/util/annotate.h    |  5 +++--
 5 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 98d1b6379230..0ceb41f16663 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -509,7 +509,6 @@ int cmd_annotate(int argc, const char **argv)
 			.ordered_events = true,
 			.ordering_requires_timestamps = true,
 		},
-		.opts = annotation__default_options,
 	};
 	struct perf_data data = {
 		.mode  = PERF_DATA_MODE_READ,
@@ -598,6 +597,7 @@ int cmd_annotate(int argc, const char **argv)
 	set_option_flag(options, 0, "show-total-period", PARSE_OPT_EXCLUSIVE);
 	set_option_flag(options, 0, "show-nr-samples", PARSE_OPT_EXCLUSIVE);
 
+	annotation_options__init(&annotate.opts);
 
 	ret = hists__init();
 	if (ret < 0)
@@ -698,6 +698,7 @@ int cmd_annotate(int argc, const char **argv)
 #ifndef NDEBUG
 	perf_session__delete(annotate.session);
 #endif
+	annotation_options__exit(&annotate.opts);
 
 	return ret;
 }
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 500f9d8902e7..b41e1219d153 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -728,8 +728,7 @@ static int hists__resort_cb(struct hist_entry *he, void *arg)
 	if (rep->symbol_ipc && sym && !sym->annotate2) {
 		struct evsel *evsel = hists_to_evsel(he->hists);
 
-		symbol__annotate2(&he->ms, evsel,
-				  &annotation__default_options, NULL);
+		symbol__annotate2(&he->ms, evsel, &rep->annotation_opts, NULL);
 	}
 
 	return 0;
@@ -1223,7 +1222,6 @@ int cmd_report(int argc, const char **argv)
 		.max_stack		 = PERF_MAX_STACK_DEPTH,
 		.pretty_printing_style	 = "normal",
 		.socket_filter		 = -1,
-		.annotation_opts	 = annotation__default_options,
 		.skip_empty		 = true,
 	};
 	char *sort_order_help = sort_help("sort by key(s):");
@@ -1403,6 +1401,8 @@ int cmd_report(int argc, const char **argv)
 	if (ret < 0)
 		goto exit;
 
+	annotation_options__init(&report.annotation_opts);
+
 	ret = perf_config(report__config, &report);
 	if (ret)
 		goto exit;
@@ -1706,6 +1706,7 @@ int cmd_report(int argc, const char **argv)
 	zstd_fini(&(session->zstd_data));
 	perf_session__delete(session);
 exit:
+	annotation_options__exit(&report.annotation_opts);
 	free(sort_order_help);
 	free(field_order_help);
 	return ret;
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index d4b5b02bab73..592eb827fba9 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1435,7 +1435,6 @@ int cmd_top(int argc, const char **argv)
 			.sample_time_set = true,
 		},
 		.max_stack	     = sysctl__max_stack(),
-		.annotation_opts     = annotation__default_options,
 		.nr_threads_synthesize = UINT_MAX,
 	};
 	struct record_opts *opts = &top.record_opts;
@@ -1587,6 +1586,8 @@ int cmd_top(int argc, const char **argv)
 	if (status < 0)
 		return status;
 
+	annotation_options__init(&top.annotation_opts);
+
 	top.annotation_opts.min_pcnt = 5;
 	top.annotation_opts.context  = 4;
 
@@ -1783,6 +1784,7 @@ int cmd_top(int argc, const char **argv)
 out_delete_evlist:
 	evlist__delete(top.evlist);
 	perf_session__delete(top.session);
+	annotation_options__exit(&top.annotation_opts);
 
 	return status;
 }
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index db475e44f42f..a984bdae7811 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -55,14 +55,6 @@
 
 #include <linux/ctype.h>
 
-struct annotation_options annotation__default_options = {
-	.use_offset     = true,
-	.jump_arrows    = true,
-	.annotate_src	= true,
-	.offset_level	= ANNOTATION__OFFSET_JUMP_TARGETS,
-	.percent_type	= PERCENT_PERIOD_LOCAL,
-};
-
 static regex_t	 file_lineno;
 
 static struct ins_ops *ins__find(struct arch *arch, const char *name);
@@ -3226,6 +3218,23 @@ static int annotation__config(const char *var, const char *value, void *data)
 	return 0;
 }
 
+void annotation_options__init(struct annotation_options *opt)
+{
+	memset(opt, 0, sizeof(*opt));
+
+	/* Default values. */
+	opt->use_offset = true;
+	opt->jump_arrows = true;
+	opt->annotate_src = true;
+	opt->offset_level = ANNOTATION__OFFSET_JUMP_TARGETS;
+	opt->percent_type = PERCENT_PERIOD_LOCAL;
+}
+
+
+void annotation_options__exit(struct annotation_options *opt __maybe_unused)
+{
+}
+
 void annotation_config__init(struct annotation_options *opt)
 {
 	perf_config(annotation__config, opt);
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 8934072c39e6..e7238c694465 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -109,8 +109,6 @@ enum {
 
 #define ANNOTATION__MIN_OFFSET_LEVEL ANNOTATION__OFFSET_JUMP_TARGETS
 
-extern struct annotation_options annotation__default_options;
-
 struct annotation;
 
 struct sym_hist_entry {
@@ -418,6 +416,9 @@ static inline int symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
 }
 #endif
 
+void annotation_options__init(struct annotation_options *opt);
+void annotation_options__exit(struct annotation_options *opt);
+
 void annotation_config__init(struct annotation_options *opt);
 
 int annotate_parse_percent_type(const struct option *opt, const char *_str,
-- 
2.40.0.348.gf938b09366-goog

[PATCH v1 4/6] perf annotate: Own objdump_path and disassembler_style strings

[Ian Rogers]

Make struct annotation_options own the strings objdump_path and
disassembler_style, freeing them on exit. Add missing strdup for
disassembler_style when read from a config file.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/arch/common.c      |  4 ++--
 tools/perf/arch/common.h      |  2 +-
 tools/perf/builtin-annotate.c | 16 ++++++++++++++--
 tools/perf/builtin-report.c   | 16 ++++++++++++++--
 tools/perf/builtin-top.c      | 17 +++++++++++++++--
 tools/perf/util/annotate.c    | 10 ++++++++--
 tools/perf/util/annotate.h    |  4 ++--
 7 files changed, 56 insertions(+), 13 deletions(-)

diff --git a/tools/perf/arch/common.c b/tools/perf/arch/common.c
index 59dd875fd5e4..28ac09997928 100644
--- a/tools/perf/arch/common.c
+++ b/tools/perf/arch/common.c
@@ -130,7 +130,7 @@ static int lookup_triplets(const char *const *triplets, const char *name)
 }
 
 static int perf_env__lookup_binutils_path(struct perf_env *env,
-					  const char *name, const char **path)
+					  const char *name, char **path)
 {
 	int idx;
 	const char *arch = perf_env__arch(env), *cross_env;
@@ -202,7 +202,7 @@ static int perf_env__lookup_binutils_path(struct perf_env *env,
 	return -1;
 }
 
-int perf_env__lookup_objdump(struct perf_env *env, const char **path)
+int perf_env__lookup_objdump(struct perf_env *env, char **path)
 {
 	/*
 	 * For live mode, env->arch will be NULL and we can use
diff --git a/tools/perf/arch/common.h b/tools/perf/arch/common.h
index e965ed8bb328..4224c299cc70 100644
--- a/tools/perf/arch/common.h
+++ b/tools/perf/arch/common.h
@@ -6,7 +6,7 @@
 
 struct perf_env;
 
-int perf_env__lookup_objdump(struct perf_env *env, const char **path);
+int perf_env__lookup_objdump(struct perf_env *env, char **path);
 bool perf_env__single_address_space(struct perf_env *env);
 
 #endif /* ARCH_PERF_COMMON_H */
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 0ceb41f16663..d781639b644f 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -516,6 +516,7 @@ int cmd_annotate(int argc, const char **argv)
 	struct itrace_synth_opts itrace_synth_opts = {
 		.set = 0,
 	};
+	const char *disassembler_style = NULL, *objdump_path = NULL;
 	struct option options[] = {
 	OPT_STRING('i', "input", &input_name, "file",
 		    "input file name"),
@@ -560,13 +561,13 @@ int cmd_annotate(int argc, const char **argv)
 		    "Interleave source code with assembly code (default)"),
 	OPT_BOOLEAN(0, "asm-raw", &annotate.opts.show_asm_raw,
 		    "Display raw encoding of assembly instructions (default)"),
-	OPT_STRING('M', "disassembler-style", &annotate.opts.disassembler_style, "disassembler style",
+	OPT_STRING('M', "disassembler-style", &disassembler_style, "disassembler style",
 		   "Specify disassembler style (e.g. -M intel for intel syntax)"),
 	OPT_STRING(0, "prefix", &annotate.opts.prefix, "prefix",
 		    "Add prefix to source file path names in programs (with --prefix-strip)"),
 	OPT_STRING(0, "prefix-strip", &annotate.opts.prefix_strip, "N",
 		    "Strip first N entries of source file path name in programs (with --prefix)"),
-	OPT_STRING(0, "objdump", &annotate.opts.objdump_path, "path",
+	OPT_STRING(0, "objdump", &objdump_path, "path",
 		   "objdump binary to use for disassembly and annotations"),
 	OPT_BOOLEAN(0, "demangle", &symbol_conf.demangle,
 		    "Enable symbol demangling"),
@@ -617,6 +618,17 @@ int cmd_annotate(int argc, const char **argv)
 		annotate.sym_hist_filter = argv[0];
 	}
 
+	if (disassembler_style) {
+		annotate.opts.disassembler_style = strdup(disassembler_style);
+		if (!annotate.opts.disassembler_style)
+			return -ENOMEM;
+	}
+	if (objdump_path) {
+		annotate.opts.objdump_path = strdup(objdump_path);
+		if (!annotate.opts.objdump_path)
+			return -ENOMEM;
+	}
+
 	if (annotate_check_args(&annotate.opts) < 0)
 		return -EINVAL;
 
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index b41e1219d153..15b0cf649e1a 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -1226,6 +1226,7 @@ int cmd_report(int argc, const char **argv)
 	};
 	char *sort_order_help = sort_help("sort by key(s):");
 	char *field_order_help = sort_help("output field(s): overhead period sample ");
+	const char *disassembler_style = NULL, *objdump_path = NULL;
 	const struct option options[] = {
 	OPT_STRING('i', "input", &input_name, "file",
 		    "input file name"),
@@ -1322,7 +1323,7 @@ int cmd_report(int argc, const char **argv)
 		    "Interleave source code with assembly code (default)"),
 	OPT_BOOLEAN(0, "asm-raw", &report.annotation_opts.show_asm_raw,
 		    "Display raw encoding of assembly instructions (default)"),
-	OPT_STRING('M', "disassembler-style", &report.annotation_opts.disassembler_style, "disassembler style",
+	OPT_STRING('M', "disassembler-style", &disassembler_style, "disassembler style",
 		   "Specify disassembler style (e.g. -M intel for intel syntax)"),
 	OPT_STRING(0, "prefix", &report.annotation_opts.prefix, "prefix",
 		    "Add prefix to source file path names in programs (with --prefix-strip)"),
@@ -1341,7 +1342,7 @@ int cmd_report(int argc, const char **argv)
 		    parse_branch_mode),
 	OPT_BOOLEAN(0, "branch-history", &branch_call_mode,
 		    "add last branch records to call history"),
-	OPT_STRING(0, "objdump", &report.annotation_opts.objdump_path, "path",
+	OPT_STRING(0, "objdump", &objdump_path, "path",
 		   "objdump binary to use for disassembly and annotations"),
 	OPT_BOOLEAN(0, "demangle", &symbol_conf.demangle,
 		    "Disable symbol demangling"),
@@ -1419,6 +1420,17 @@ int cmd_report(int argc, const char **argv)
 		report.symbol_filter_str = argv[0];
 	}
 
+	if (disassembler_style) {
+		report.annotation_opts.disassembler_style = strdup(disassembler_style);
+		if (!report.annotation_opts.disassembler_style)
+			return -ENOMEM;
+	}
+	if (objdump_path) {
+		report.annotation_opts.objdump_path = strdup(objdump_path);
+		if (!report.annotation_opts.objdump_path)
+			return -ENOMEM;
+	}
+
 	if (annotate_check_args(&report.annotation_opts) < 0) {
 		ret = -EINVAL;
 		goto exit;
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 592eb827fba9..57a273cd03de 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1439,6 +1439,7 @@ int cmd_top(int argc, const char **argv)
 	};
 	struct record_opts *opts = &top.record_opts;
 	struct target *target = &opts->target;
+	const char *disassembler_style = NULL, *objdump_path = NULL;
 	const struct option options[] = {
 	OPT_CALLBACK('e', "event", &top.evlist, "event",
 		     "event selector. use 'perf list' to list available events",
@@ -1524,9 +1525,9 @@ int cmd_top(int argc, const char **argv)
 	OPT_BOOLEAN(0, "demangle-kernel", &symbol_conf.demangle_kernel,
 		    "Enable kernel symbol demangling"),
 	OPT_BOOLEAN(0, "no-bpf-event", &top.record_opts.no_bpf_event, "do not record bpf events"),
-	OPT_STRING(0, "objdump", &top.annotation_opts.objdump_path, "path",
+	OPT_STRING(0, "objdump", &objdump_path, "path",
 		    "objdump binary to use for disassembly and annotations"),
-	OPT_STRING('M', "disassembler-style", &top.annotation_opts.disassembler_style, "disassembler style",
+	OPT_STRING('M', "disassembler-style", &disassembler_style, "disassembler style",
 		   "Specify disassembler style (e.g. -M intel for intel syntax)"),
 	OPT_STRING(0, "prefix", &top.annotation_opts.prefix, "prefix",
 		    "Add prefix to source file path names in programs (with --prefix-strip)"),
@@ -1618,6 +1619,18 @@ int cmd_top(int argc, const char **argv)
 	if (argc)
 		usage_with_options(top_usage, options);
 
+	if (disassembler_style) {
+		top.annotation_opts.disassembler_style = strdup(disassembler_style);
+		if (!top.annotation_opts.disassembler_style)
+			return -ENOMEM;
+	}
+	if (objdump_path) {
+		top.annotation_opts.objdump_path = strdup(objdump_path);
+		if (!top.annotation_opts.objdump_path)
+			return -ENOMEM;
+	}
+
+
 	status = symbol__validate_sym_arguments();
 	if (status)
 		goto out_delete_evlist;
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index a984bdae7811..7338249dfdd9 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3206,7 +3206,11 @@ static int annotation__config(const char *var, const char *value, void *data)
 	} else if (!strcmp(var, "annotate.use_offset")) {
 		opt->use_offset = perf_config_bool("use_offset", value);
 	} else if (!strcmp(var, "annotate.disassembler_style")) {
-		opt->disassembler_style = value;
+		opt->disassembler_style = strdup(value);
+		if (!opt->disassembler_style) {
+			pr_err("Not enough memory for annotate.disassembler_style\n");
+			return -1;
+		}
 	} else if (!strcmp(var, "annotate.demangle")) {
 		symbol_conf.demangle = perf_config_bool("demangle", value);
 	} else if (!strcmp(var, "annotate.demangle_kernel")) {
@@ -3231,8 +3235,10 @@ void annotation_options__init(struct annotation_options *opt)
 }
 
 
-void annotation_options__exit(struct annotation_options *opt __maybe_unused)
+void annotation_options__exit(struct annotation_options *opt)
 {
+	free(opt->disassembler_style);
+	free(opt->objdump_path);
 }
 
 void annotation_config__init(struct annotation_options *opt)
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index e7238c694465..1c6335b8333a 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -94,8 +94,8 @@ struct annotation_options {
 	int  min_pcnt;
 	int  max_lines;
 	int  context;
-	const char *objdump_path;
-	const char *disassembler_style;
+	char *objdump_path;
+	char *disassembler_style;
 	const char *prefix;
 	const char *prefix_strip;
 	unsigned int percent_type;
-- 
2.40.0.348.gf938b09366-goog

[PATCH v1 5/6] perf annotate: Allow objdump to be set in perfconfig

[Ian Rogers]

Allow the setting of the objdump command in the perfconfig. Update man
page for this new option.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/Documentation/perf-config.txt | 5 ++++-
 tools/perf/util/annotate.c               | 6 ++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
index 39c890ead2dc..697f7f924545 100644
--- a/tools/perf/Documentation/perf-config.txt
+++ b/tools/perf/Documentation/perf-config.txt
@@ -250,7 +250,10 @@ annotate.*::
 	These are in control of addresses, jump function, source code
 	in lines of assembly code from a specific program.
 
-	annotate.disassembler_style:
+	annotate.objdump::
+		objdump binary to use for disassembly and annotations.
+
+	annotate.disassembler_style::
 		Use this to change the default disassembler style to some other value
 		supported by binutils, such as "intel", see the '-M' option help in the
 		'objdump' man page.
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 7338249dfdd9..3eaa9b2df6c4 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3211,6 +3211,12 @@ static int annotation__config(const char *var, const char *value, void *data)
 			pr_err("Not enough memory for annotate.disassembler_style\n");
 			return -1;
 		}
+	} else if (!strcmp(var, "annotate.objdump")) {
+		opt->objdump_path = strdup(value);
+		if (!opt->objdump_path) {
+			pr_err("Not enough memory for annotate.objdump\n");
+			return -1;
+		}
 	} else if (!strcmp(var, "annotate.demangle")) {
 		symbol_conf.demangle = perf_config_bool("demangle", value);
 	} else if (!strcmp(var, "annotate.demangle_kernel")) {
-- 
2.40.0.348.gf938b09366-goog

[PATCH v1 6/6] perf symbol: Add command line support for addr2line path

[Ian Rogers]

Allow addr2line to be set either on the command line or via the
perfconfig file. This doesn't currently work with llvm-addr2line as
the addr2line code emits two things:
1) the address to decode,
2) a bogus ',' value.
The expectation is the bogus value will generate:
??
??:0
that terminates the addr2line reading. However, the output from
llvm-addr2line is a single line with just the input ',' locking up the
addr2line reading that is expecting a second line.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/Documentation/perf-annotate.txt |  3 +++
 tools/perf/Documentation/perf-config.txt   |  3 +++
 tools/perf/Documentation/perf-report.txt   |  3 +++
 tools/perf/Documentation/perf-top.txt      |  6 +++++
 tools/perf/builtin-annotate.c              |  9 +++++++-
 tools/perf/builtin-report.c                |  9 +++++++-
 tools/perf/builtin-top.c                   | 10 +++++++--
 tools/perf/util/annotate.c                 |  6 +++++
 tools/perf/util/srcline.c                  | 26 +++++++++++++---------
 tools/perf/util/symbol_conf.h              |  1 +
 10 files changed, 62 insertions(+), 14 deletions(-)

diff --git a/tools/perf/Documentation/perf-annotate.txt b/tools/perf/Documentation/perf-annotate.txt
index 980fe2c29275..fe168e8165c8 100644
--- a/tools/perf/Documentation/perf-annotate.txt
+++ b/tools/perf/Documentation/perf-annotate.txt
@@ -116,6 +116,9 @@ include::itrace.txt[]
 -M::
 --disassembler-style=:: Set disassembler style for objdump.
 
+--addr2line=<path>::
+        Path to addr2line binary.
+
 --objdump=<path>::
         Path to objdump binary.
 
diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
index 697f7f924545..e56ae54805a8 100644
--- a/tools/perf/Documentation/perf-config.txt
+++ b/tools/perf/Documentation/perf-config.txt
@@ -250,6 +250,9 @@ annotate.*::
 	These are in control of addresses, jump function, source code
 	in lines of assembly code from a specific program.
 
+	annotate.addr2line::
+		addr2line binary to use for file names and line numbers.
+
 	annotate.objdump::
 		objdump binary to use for disassembly and annotations.
 
diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
index cfd502f7e6da..af068b4f1e5a 100644
--- a/tools/perf/Documentation/perf-report.txt
+++ b/tools/perf/Documentation/perf-report.txt
@@ -381,6 +381,9 @@ OPTIONS
 	This allows to examine the path the program took to each sample.
 	The data collection must have used -b (or -j) and -g.
 
+--addr2line=<path>::
+        Path to addr2line binary.
+
 --objdump=<path>::
         Path to objdump binary.
 
diff --git a/tools/perf/Documentation/perf-top.txt b/tools/perf/Documentation/perf-top.txt
index c60e615b7183..619cc8143ad5 100644
--- a/tools/perf/Documentation/perf-top.txt
+++ b/tools/perf/Documentation/perf-top.txt
@@ -161,6 +161,12 @@ Default is to monitor all CPUS.
 -M::
 --disassembler-style=:: Set disassembler style for objdump.
 
+--addr2line=<path>::
+        Path to addr2line binary.
+
+--objdump=<path>::
+        Path to objdump binary.
+
 --prefix=PREFIX::
 --prefix-strip=N::
         Remove first N entries from source file path names in executables
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index d781639b644f..ec276db3e5dd 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -516,7 +516,7 @@ int cmd_annotate(int argc, const char **argv)
 	struct itrace_synth_opts itrace_synth_opts = {
 		.set = 0,
 	};
-	const char *disassembler_style = NULL, *objdump_path = NULL;
+	const char *disassembler_style = NULL, *objdump_path = NULL, *addr2line_path = NULL;
 	struct option options[] = {
 	OPT_STRING('i', "input", &input_name, "file",
 		    "input file name"),
@@ -569,6 +569,8 @@ int cmd_annotate(int argc, const char **argv)
 		    "Strip first N entries of source file path name in programs (with --prefix)"),
 	OPT_STRING(0, "objdump", &objdump_path, "path",
 		   "objdump binary to use for disassembly and annotations"),
+	OPT_STRING(0, "addr2line", &addr2line_path, "path",
+		   "addr2line binary to use for line numbers"),
 	OPT_BOOLEAN(0, "demangle", &symbol_conf.demangle,
 		    "Enable symbol demangling"),
 	OPT_BOOLEAN(0, "demangle-kernel", &symbol_conf.demangle_kernel,
@@ -628,6 +630,11 @@ int cmd_annotate(int argc, const char **argv)
 		if (!annotate.opts.objdump_path)
 			return -ENOMEM;
 	}
+	if (addr2line_path) {
+		symbol_conf.addr2line_path = strdup(addr2line_path);
+		if (!symbol_conf.addr2line_path)
+			return -ENOMEM;
+	}
 
 	if (annotate_check_args(&annotate.opts) < 0)
 		return -EINVAL;
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 15b0cf649e1a..4011abc03d0d 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -1226,7 +1226,7 @@ int cmd_report(int argc, const char **argv)
 	};
 	char *sort_order_help = sort_help("sort by key(s):");
 	char *field_order_help = sort_help("output field(s): overhead period sample ");
-	const char *disassembler_style = NULL, *objdump_path = NULL;
+	const char *disassembler_style = NULL, *objdump_path = NULL, *addr2line_path = NULL;
 	const struct option options[] = {
 	OPT_STRING('i', "input", &input_name, "file",
 		    "input file name"),
@@ -1344,6 +1344,8 @@ int cmd_report(int argc, const char **argv)
 		    "add last branch records to call history"),
 	OPT_STRING(0, "objdump", &objdump_path, "path",
 		   "objdump binary to use for disassembly and annotations"),
+	OPT_STRING(0, "addr2line", &addr2line_path, "path",
+		   "addr2line binary to use for line numbers"),
 	OPT_BOOLEAN(0, "demangle", &symbol_conf.demangle,
 		    "Disable symbol demangling"),
 	OPT_BOOLEAN(0, "demangle-kernel", &symbol_conf.demangle_kernel,
@@ -1430,6 +1432,11 @@ int cmd_report(int argc, const char **argv)
 		if (!report.annotation_opts.objdump_path)
 			return -ENOMEM;
 	}
+	if (addr2line_path) {
+		symbol_conf.addr2line_path = strdup(addr2line_path);
+		if (!symbol_conf.addr2line_path)
+			return -ENOMEM;
+	}
 
 	if (annotate_check_args(&report.annotation_opts) < 0) {
 		ret = -EINVAL;
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 57a273cd03de..82c6c065830d 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1439,7 +1439,7 @@ int cmd_top(int argc, const char **argv)
 	};
 	struct record_opts *opts = &top.record_opts;
 	struct target *target = &opts->target;
-	const char *disassembler_style = NULL, *objdump_path = NULL;
+	const char *disassembler_style = NULL, *objdump_path = NULL, *addr2line_path = NULL;
 	const struct option options[] = {
 	OPT_CALLBACK('e', "event", &top.evlist, "event",
 		     "event selector. use 'perf list' to list available events",
@@ -1527,6 +1527,8 @@ int cmd_top(int argc, const char **argv)
 	OPT_BOOLEAN(0, "no-bpf-event", &top.record_opts.no_bpf_event, "do not record bpf events"),
 	OPT_STRING(0, "objdump", &objdump_path, "path",
 		    "objdump binary to use for disassembly and annotations"),
+	OPT_STRING(0, "addr2line", &addr2line_path, "path",
+		   "addr2line binary to use for line numbers"),
 	OPT_STRING('M', "disassembler-style", &disassembler_style, "disassembler style",
 		   "Specify disassembler style (e.g. -M intel for intel syntax)"),
 	OPT_STRING(0, "prefix", &top.annotation_opts.prefix, "prefix",
@@ -1629,7 +1631,11 @@ int cmd_top(int argc, const char **argv)
 		if (!top.annotation_opts.objdump_path)
 			return -ENOMEM;
 	}
-
+	if (addr2line_path) {
+		symbol_conf.addr2line_path = strdup(addr2line_path);
+		if (!symbol_conf.addr2line_path)
+			return -ENOMEM;
+	}
 
 	status = symbol__validate_sym_arguments();
 	if (status)
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 3eaa9b2df6c4..3e5df90e11f8 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3217,6 +3217,12 @@ static int annotation__config(const char *var, const char *value, void *data)
 			pr_err("Not enough memory for annotate.objdump\n");
 			return -1;
 		}
+	} else if (!strcmp(var, "annotate.addr2line")) {
+		symbol_conf.addr2line_path = strdup(value);
+		if (!symbol_conf.addr2line_path) {
+			pr_err("Not enough memory for annotate.addr2line\n");
+			return -1;
+		}
 	} else if (!strcmp(var, "annotate.demangle")) {
 		symbol_conf.demangle = perf_config_bool("demangle", value);
 	} else if (!strcmp(var, "annotate.demangle_kernel")) {
diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index 33321867416b..f0a96a834e4b 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -414,9 +414,14 @@ static void addr2line_subprocess_cleanup(struct a2l_subprocess *a2l)
 	free(a2l);
 }
 
-static struct a2l_subprocess *addr2line_subprocess_init(const char *path)
-{
-	const char *argv[] = { "addr2line", "-e", path, "-i", "-f", NULL };
+static struct a2l_subprocess *addr2line_subprocess_init(const char *addr2line_path,
+							const char *binary_path)
+{
+	const char *argv[] = {
+		addr2line_path ?: "addr2line",
+		"-e", binary_path,
+		"-i", "-f", NULL
+	};
 	struct a2l_subprocess *a2l = zalloc(sizeof(*a2l));
 	int start_command_status = 0;
 
@@ -436,21 +441,22 @@ static struct a2l_subprocess *addr2line_subprocess_init(const char *path)
 	a2l->addr2line.argv = NULL; /* it's not used after start_command; avoid dangling pointers */
 
 	if (start_command_status != 0) {
-		pr_warning("could not start addr2line for %s: start_command return code %d\n",
-			   path,
-			   start_command_status);
+		pr_warning("could not start addr2line (%s) for %s: start_command return code %d\n",
+			addr2line_path, binary_path, start_command_status);
 		goto out;
 	}
 
 	a2l->to_child = fdopen(a2l->addr2line.in, "w");
 	if (a2l->to_child == NULL) {
-		pr_warning("could not open write-stream to addr2line of %s\n", path);
+		pr_warning("could not open write-stream to addr2line (%s) of %s\n",
+			addr2line_path, binary_path);
 		goto out;
 	}
 
 	a2l->from_child = fdopen(a2l->addr2line.out, "r");
 	if (a2l->from_child == NULL) {
-		pr_warning("could not open read-stream from addr2line of %s\n", path);
+		pr_warning("could not open read-stream from addr2line (%s) of %s\n",
+			addr2line_path, binary_path);
 		goto out;
 	}
 
@@ -490,7 +496,6 @@ static int read_addr2line_record(struct a2l_subprocess *a2l,
 
 	if (getline(&line, &line_len, a2l->from_child) < 0 || !line_len)
 		goto error;
-
 	if (function != NULL)
 		*function = strdup(strim(line));
 
@@ -553,7 +558,8 @@ static int addr2line(const char *dso_name, u64 addr,
 		if (!filename__has_section(dso_name, ".debug_line"))
 			goto out;
 
-		dso->a2l = addr2line_subprocess_init(dso_name);
+		dso->a2l = addr2line_subprocess_init(symbol_conf.addr2line_path,
+						     dso_name);
 		a2l = dso->a2l;
 	}
 
diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
index bc3d046fbb63..5accd8e69ad2 100644
--- a/tools/perf/util/symbol_conf.h
+++ b/tools/perf/util/symbol_conf.h
@@ -61,6 +61,7 @@ struct symbol_conf {
 			*sym_list_str,
 			*col_width_list_str,
 			*bt_stop_list_str;
+	char		*addr2line_path;
 	unsigned long	time_quantum;
        struct strlist	*dso_list,
 			*comm_list,
-- 
2.40.0.348.gf938b09366-goog

Re: [PATCH v1 1/6] perf annotate: Delete session for debug builds

[Arnaldo Carvalho de Melo]

Em Tue, Mar 28, 2023 at 04:55:38PM -0700, Ian Rogers escreveu:
> Use the debug build indicator as the guide to free the session. This
> implements a behavior described in a comment, which is consequentially
> removed.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-annotate.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 4750fac7bf93..98d1b6379230 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -692,16 +692,12 @@ int cmd_annotate(int argc, const char **argv)
>  
>  out_delete:
>  	/*
> -	 * Speed up the exit process, for large files this can
> -	 * take quite a while.
> -	 *
> -	 * XXX Enable this when using valgrind or if we ever
> -	 * librarize this command.
> -	 *
> -	 * Also experiment with obstacks to see how much speed
> -	 * up we'll get here.
> -	 *
> -	 * perf_session__delete(session);
> +	 * Speed up the exit process by only deleting for debug builds. For
> +	 * large files this can save time.
>  	 */
> +#ifndef NDEBUG
> +	perf_session__delete(annotate.session);
> +#endif

So now, but default, we will call this, as we don't have this defined
only if DEBUG=1 is set, right?

⬢[acme@toolbox perf-tools-next]$ find tools/perf/ -type f | xargs grep NDEBUG
tools/perf/util/mutex.c:#ifndef NDEBUG
⬢[acme@toolbox perf-tools-next]$

- Arnaldo

Re: [PATCH v1 4/6] perf annotate: Own objdump_path and disassembler_style strings

[Arnaldo Carvalho de Melo]

Em Tue, Mar 28, 2023 at 04:55:41PM -0700, Ian Rogers escreveu:
> @@ -3231,8 +3235,10 @@ void annotation_options__init(struct annotation_options *opt)
>  }
>  
>  
> -void annotation_options__exit(struct annotation_options *opt __maybe_unused)
> +void annotation_options__exit(struct annotation_options *opt)
>  {
> +	free(opt->disassembler_style);
> +	free(opt->objdump_path);
>  }


Changed this:

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 7338249dfdd94390..08e041a9b9cc279e 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -3237,8 +3237,8 @@ void annotation_options__init(struct annotation_options *opt)
 
 void annotation_options__exit(struct annotation_options *opt)
 {
-	free(opt->disassembler_style);
-	free(opt->objdump_path);
+	zfree(&opt->disassembler_style);
+	zfree(&opt->objdump_path);
 }
 
 void annotation_config__init(struct annotation_options *opt)

Re: [PATCH v1 1/6] perf annotate: Delete session for debug builds

[Arnaldo Carvalho de Melo]

Em Wed, Mar 29, 2023 at 10:09:48AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Mar 28, 2023 at 04:55:38PM -0700, Ian Rogers escreveu:
> > Use the debug build indicator as the guide to free the session. This
> > implements a behavior described in a comment, which is consequentially
> > removed.
> > 
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/builtin-annotate.c | 16 ++++++----------
> >  1 file changed, 6 insertions(+), 10 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> > index 4750fac7bf93..98d1b6379230 100644
> > --- a/tools/perf/builtin-annotate.c
> > +++ b/tools/perf/builtin-annotate.c
> > @@ -692,16 +692,12 @@ int cmd_annotate(int argc, const char **argv)
> >  
> >  out_delete:
> >  	/*
> > -	 * Speed up the exit process, for large files this can
> > -	 * take quite a while.
> > -	 *
> > -	 * XXX Enable this when using valgrind or if we ever
> > -	 * librarize this command.
> > -	 *
> > -	 * Also experiment with obstacks to see how much speed
> > -	 * up we'll get here.
> > -	 *
> > -	 * perf_session__delete(session);
> > +	 * Speed up the exit process by only deleting for debug builds. For
> > +	 * large files this can save time.
> >  	 */
> > +#ifndef NDEBUG
> > +	perf_session__delete(annotate.session);
> > +#endif
> 
> So now, but default, we will call this, as we don't have this defined
> only if DEBUG=1 is set, right?
> 
> ⬢[acme@toolbox perf-tools-next]$ find tools/perf/ -type f | xargs grep NDEBUG
> tools/perf/util/mutex.c:#ifndef NDEBUG
> ⬢[acme@toolbox perf-tools-next]$

We can discuss this later, applied the series with just that zfree()
change to annotation_options__exit().

- Arnaldo

Re: [PATCH v1 0/6] config file/command line for objdump & addr2line

[Arnaldo Carvalho de Melo]

Em Tue, Mar 28, 2023 at 04:55:37PM -0700, Ian Rogers escreveu:
> Allow objdump to be set as a perf config file variable. As previously
> objdump was set via the command line, the string was owned by
> argv. Now the string must be strdup-ed, so the corresponding logic
> needs changing with an annotation_options__init/exit.

ui/gtk/annotate.c: In function ‘symbol__gtk_annotate’:
ui/gtk/annotate.c:177:44: error: ‘annotation__default_options’ undeclared (first use in this function); did you mean ‘annotation_options’?
  177 |         err = symbol__annotate(ms, evsel, &annotation__default_options, NULL);
      |                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                            annotation_options
ui/gtk/annotate.c:177:44: note: each undeclared identifier is reported only once for each function it appears in
make[6]: *** [/var/home/acme/git/perf-tools-next/tools/build/Makefile.build:97: ui/gtk/annotate.o] Error 1
make[6]: *** Waiting for unfinished jobs....


I'll fix this later...

- Arnaldo
 
> Add command line and config file options for addr2line, set in
> symbol_conf for convenience. This doesn't allow the setting of
> llvm-addr2line due to a bug, but could in the future.
> 
> Ian Rogers (6):
>   perf annotate: Delete session for debug builds
>   perf report: Additional config warnings
>   perf annotate: Add init/exit to annotation_options remove default
>   perf annotate: Own objdump_path and disassembler_style strings
>   perf annotate: Allow objdump to be set in perfconfig
>   perf symbol: Add command line support for addr2line path
> 
>  tools/perf/Documentation/perf-annotate.txt |  3 ++
>  tools/perf/Documentation/perf-config.txt   |  8 +++-
>  tools/perf/Documentation/perf-report.txt   |  3 ++
>  tools/perf/Documentation/perf-top.txt      |  6 +++
>  tools/perf/arch/common.c                   |  4 +-
>  tools/perf/arch/common.h                   |  2 +-
>  tools/perf/builtin-annotate.c              | 42 +++++++++++++-------
>  tools/perf/builtin-report.c                | 35 ++++++++++++++---
>  tools/perf/builtin-top.c                   | 27 +++++++++++--
>  tools/perf/util/annotate.c                 | 45 +++++++++++++++++-----
>  tools/perf/util/annotate.h                 |  9 +++--
>  tools/perf/util/srcline.c                  | 26 ++++++++-----
>  tools/perf/util/symbol_conf.h              |  1 +
>  13 files changed, 163 insertions(+), 48 deletions(-)
> 
> -- 
> 2.40.0.348.gf938b09366-goog
> 

-- 

- Arnaldo

Re: [PATCH v1 1/6] perf annotate: Delete session for debug builds

[Namhyung Kim]

On Wed, Mar 29, 2023 at 6:18 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Wed, Mar 29, 2023 at 10:09:48AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Mar 28, 2023 at 04:55:38PM -0700, Ian Rogers escreveu:
> > > Use the debug build indicator as the guide to free the session. This
> > > implements a behavior described in a comment, which is consequentially
> > > removed.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/builtin-annotate.c | 16 ++++++----------
> > >  1 file changed, 6 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> > > index 4750fac7bf93..98d1b6379230 100644
> > > --- a/tools/perf/builtin-annotate.c
> > > +++ b/tools/perf/builtin-annotate.c
> > > @@ -692,16 +692,12 @@ int cmd_annotate(int argc, const char **argv)
> > >
> > >  out_delete:
> > >     /*
> > > -    * Speed up the exit process, for large files this can
> > > -    * take quite a while.
> > > -    *
> > > -    * XXX Enable this when using valgrind or if we ever
> > > -    * librarize this command.
> > > -    *
> > > -    * Also experiment with obstacks to see how much speed
> > > -    * up we'll get here.
> > > -    *
> > > -    * perf_session__delete(session);
> > > +    * Speed up the exit process by only deleting for debug builds. For
> > > +    * large files this can save time.
> > >      */
> > > +#ifndef NDEBUG
> > > +   perf_session__delete(annotate.session);
> > > +#endif
> >
> > So now, but default, we will call this, as we don't have this defined
> > only if DEBUG=1 is set, right?
> >
> > ⬢[acme@toolbox perf-tools-next]$ find tools/perf/ -type f | xargs grep NDEBUG
> > tools/perf/util/mutex.c:#ifndef NDEBUG
> > ⬢[acme@toolbox perf-tools-next]$
>
> We can discuss this later, applied the series with just that zfree()
> change to annotation_options__exit().

I don't think it's just an issue in the perf annotate.  Maybe we can
do the same for perf report and so on.

Anyway we could define NDEBUG=1 for release builds from now on.

Thanks,
Namhyung

Re: [PATCH v1 6/6] perf symbol: Add command line support for addr2line path

[Namhyung Kim]

Hi ian,

On Tue, Mar 28, 2023 at 4:56 PM Ian Rogers <irogers@google.com> wrote:
>
> Allow addr2line to be set either on the command line or via the
> perfconfig file. This doesn't currently work with llvm-addr2line as
> the addr2line code emits two things:
> 1) the address to decode,
> 2) a bogus ',' value.
> The expectation is the bogus value will generate:
> ??
> ??:0
> that terminates the addr2line reading. However, the output from
> llvm-addr2line is a single line with just the input ',' locking up the
> addr2line reading that is expecting a second line.

Yeah, I remember that when I tried to llvm-addr2line support.
Thanks for pointing it out.  Have you reported it to the LLVM
community or even sent a fix? :)

Thanks,
Namhyung


>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/Documentation/perf-annotate.txt |  3 +++
>  tools/perf/Documentation/perf-config.txt   |  3 +++
>  tools/perf/Documentation/perf-report.txt   |  3 +++
>  tools/perf/Documentation/perf-top.txt      |  6 +++++
>  tools/perf/builtin-annotate.c              |  9 +++++++-
>  tools/perf/builtin-report.c                |  9 +++++++-
>  tools/perf/builtin-top.c                   | 10 +++++++--
>  tools/perf/util/annotate.c                 |  6 +++++
>  tools/perf/util/srcline.c                  | 26 +++++++++++++---------
>  tools/perf/util/symbol_conf.h              |  1 +
>  10 files changed, 62 insertions(+), 14 deletions(-)
>
> diff --git a/tools/perf/Documentation/perf-annotate.txt b/tools/perf/Documentation/perf-annotate.txt
> index 980fe2c29275..fe168e8165c8 100644
> --- a/tools/perf/Documentation/perf-annotate.txt
> +++ b/tools/perf/Documentation/perf-annotate.txt
> @@ -116,6 +116,9 @@ include::itrace.txt[]
>  -M::
>  --disassembler-style=:: Set disassembler style for objdump.
>
> +--addr2line=<path>::
> +        Path to addr2line binary.
> +
>  --objdump=<path>::
>          Path to objdump binary.
>
> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
> index 697f7f924545..e56ae54805a8 100644
> --- a/tools/perf/Documentation/perf-config.txt
> +++ b/tools/perf/Documentation/perf-config.txt
> @@ -250,6 +250,9 @@ annotate.*::
>         These are in control of addresses, jump function, source code
>         in lines of assembly code from a specific program.
>
> +       annotate.addr2line::
> +               addr2line binary to use for file names and line numbers.
> +
>         annotate.objdump::
>                 objdump binary to use for disassembly and annotations.
>
> diff --git a/tools/perf/Documentation/perf-report.txt b/tools/perf/Documentation/perf-report.txt
> index cfd502f7e6da..af068b4f1e5a 100644
> --- a/tools/perf/Documentation/perf-report.txt
> +++ b/tools/perf/Documentation/perf-report.txt
> @@ -381,6 +381,9 @@ OPTIONS
>         This allows to examine the path the program took to each sample.
>         The data collection must have used -b (or -j) and -g.
>
> +--addr2line=<path>::
> +        Path to addr2line binary.
> +
>  --objdump=<path>::
>          Path to objdump binary.
>
> diff --git a/tools/perf/Documentation/perf-top.txt b/tools/perf/Documentation/perf-top.txt
> index c60e615b7183..619cc8143ad5 100644
> --- a/tools/perf/Documentation/perf-top.txt
> +++ b/tools/perf/Documentation/perf-top.txt
> @@ -161,6 +161,12 @@ Default is to monitor all CPUS.
>  -M::
>  --disassembler-style=:: Set disassembler style for objdump.
>
> +--addr2line=<path>::
> +        Path to addr2line binary.
> +
> +--objdump=<path>::
> +        Path to objdump binary.
> +
>  --prefix=PREFIX::
>  --prefix-strip=N::
>          Remove first N entries from source file path names in executables
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index d781639b644f..ec276db3e5dd 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -516,7 +516,7 @@ int cmd_annotate(int argc, const char **argv)
>         struct itrace_synth_opts itrace_synth_opts = {
>                 .set = 0,
>         };
> -       const char *disassembler_style = NULL, *objdump_path = NULL;
> +       const char *disassembler_style = NULL, *objdump_path = NULL, *addr2line_path = NULL;
>         struct option options[] = {
>         OPT_STRING('i', "input", &input_name, "file",
>                     "input file name"),
> @@ -569,6 +569,8 @@ int cmd_annotate(int argc, const char **argv)
>                     "Strip first N entries of source file path name in programs (with --prefix)"),
>         OPT_STRING(0, "objdump", &objdump_path, "path",
>                    "objdump binary to use for disassembly and annotations"),
> +       OPT_STRING(0, "addr2line", &addr2line_path, "path",
> +                  "addr2line binary to use for line numbers"),
>         OPT_BOOLEAN(0, "demangle", &symbol_conf.demangle,
>                     "Enable symbol demangling"),
>         OPT_BOOLEAN(0, "demangle-kernel", &symbol_conf.demangle_kernel,
> @@ -628,6 +630,11 @@ int cmd_annotate(int argc, const char **argv)
>                 if (!annotate.opts.objdump_path)
>                         return -ENOMEM;
>         }
> +       if (addr2line_path) {
> +               symbol_conf.addr2line_path = strdup(addr2line_path);
> +               if (!symbol_conf.addr2line_path)
> +                       return -ENOMEM;
> +       }
>
>         if (annotate_check_args(&annotate.opts) < 0)
>                 return -EINVAL;
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 15b0cf649e1a..4011abc03d0d 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -1226,7 +1226,7 @@ int cmd_report(int argc, const char **argv)
>         };
>         char *sort_order_help = sort_help("sort by key(s):");
>         char *field_order_help = sort_help("output field(s): overhead period sample ");
> -       const char *disassembler_style = NULL, *objdump_path = NULL;
> +       const char *disassembler_style = NULL, *objdump_path = NULL, *addr2line_path = NULL;
>         const struct option options[] = {
>         OPT_STRING('i', "input", &input_name, "file",
>                     "input file name"),
> @@ -1344,6 +1344,8 @@ int cmd_report(int argc, const char **argv)
>                     "add last branch records to call history"),
>         OPT_STRING(0, "objdump", &objdump_path, "path",
>                    "objdump binary to use for disassembly and annotations"),
> +       OPT_STRING(0, "addr2line", &addr2line_path, "path",
> +                  "addr2line binary to use for line numbers"),
>         OPT_BOOLEAN(0, "demangle", &symbol_conf.demangle,
>                     "Disable symbol demangling"),
>         OPT_BOOLEAN(0, "demangle-kernel", &symbol_conf.demangle_kernel,
> @@ -1430,6 +1432,11 @@ int cmd_report(int argc, const char **argv)
>                 if (!report.annotation_opts.objdump_path)
>                         return -ENOMEM;
>         }
> +       if (addr2line_path) {
> +               symbol_conf.addr2line_path = strdup(addr2line_path);
> +               if (!symbol_conf.addr2line_path)
> +                       return -ENOMEM;
> +       }
>
>         if (annotate_check_args(&report.annotation_opts) < 0) {
>                 ret = -EINVAL;
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 57a273cd03de..82c6c065830d 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1439,7 +1439,7 @@ int cmd_top(int argc, const char **argv)
>         };
>         struct record_opts *opts = &top.record_opts;
>         struct target *target = &opts->target;
> -       const char *disassembler_style = NULL, *objdump_path = NULL;
> +       const char *disassembler_style = NULL, *objdump_path = NULL, *addr2line_path = NULL;
>         const struct option options[] = {
>         OPT_CALLBACK('e', "event", &top.evlist, "event",
>                      "event selector. use 'perf list' to list available events",
> @@ -1527,6 +1527,8 @@ int cmd_top(int argc, const char **argv)
>         OPT_BOOLEAN(0, "no-bpf-event", &top.record_opts.no_bpf_event, "do not record bpf events"),
>         OPT_STRING(0, "objdump", &objdump_path, "path",
>                     "objdump binary to use for disassembly and annotations"),
> +       OPT_STRING(0, "addr2line", &addr2line_path, "path",
> +                  "addr2line binary to use for line numbers"),
>         OPT_STRING('M', "disassembler-style", &disassembler_style, "disassembler style",
>                    "Specify disassembler style (e.g. -M intel for intel syntax)"),
>         OPT_STRING(0, "prefix", &top.annotation_opts.prefix, "prefix",
> @@ -1629,7 +1631,11 @@ int cmd_top(int argc, const char **argv)
>                 if (!top.annotation_opts.objdump_path)
>                         return -ENOMEM;
>         }
> -
> +       if (addr2line_path) {
> +               symbol_conf.addr2line_path = strdup(addr2line_path);
> +               if (!symbol_conf.addr2line_path)
> +                       return -ENOMEM;
> +       }
>
>         status = symbol__validate_sym_arguments();
>         if (status)
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 3eaa9b2df6c4..3e5df90e11f8 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -3217,6 +3217,12 @@ static int annotation__config(const char *var, const char *value, void *data)
>                         pr_err("Not enough memory for annotate.objdump\n");
>                         return -1;
>                 }
> +       } else if (!strcmp(var, "annotate.addr2line")) {
> +               symbol_conf.addr2line_path = strdup(value);
> +               if (!symbol_conf.addr2line_path) {
> +                       pr_err("Not enough memory for annotate.addr2line\n");
> +                       return -1;
> +               }
>         } else if (!strcmp(var, "annotate.demangle")) {
>                 symbol_conf.demangle = perf_config_bool("demangle", value);
>         } else if (!strcmp(var, "annotate.demangle_kernel")) {
> diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
> index 33321867416b..f0a96a834e4b 100644
> --- a/tools/perf/util/srcline.c
> +++ b/tools/perf/util/srcline.c
> @@ -414,9 +414,14 @@ static void addr2line_subprocess_cleanup(struct a2l_subprocess *a2l)
>         free(a2l);
>  }
>
> -static struct a2l_subprocess *addr2line_subprocess_init(const char *path)
> -{
> -       const char *argv[] = { "addr2line", "-e", path, "-i", "-f", NULL };
> +static struct a2l_subprocess *addr2line_subprocess_init(const char *addr2line_path,
> +                                                       const char *binary_path)
> +{
> +       const char *argv[] = {
> +               addr2line_path ?: "addr2line",
> +               "-e", binary_path,
> +               "-i", "-f", NULL
> +       };
>         struct a2l_subprocess *a2l = zalloc(sizeof(*a2l));
>         int start_command_status = 0;
>
> @@ -436,21 +441,22 @@ static struct a2l_subprocess *addr2line_subprocess_init(const char *path)
>         a2l->addr2line.argv = NULL; /* it's not used after start_command; avoid dangling pointers */
>
>         if (start_command_status != 0) {
> -               pr_warning("could not start addr2line for %s: start_command return code %d\n",
> -                          path,
> -                          start_command_status);
> +               pr_warning("could not start addr2line (%s) for %s: start_command return code %d\n",
> +                       addr2line_path, binary_path, start_command_status);
>                 goto out;
>         }
>
>         a2l->to_child = fdopen(a2l->addr2line.in, "w");
>         if (a2l->to_child == NULL) {
> -               pr_warning("could not open write-stream to addr2line of %s\n", path);
> +               pr_warning("could not open write-stream to addr2line (%s) of %s\n",
> +                       addr2line_path, binary_path);
>                 goto out;
>         }
>
>         a2l->from_child = fdopen(a2l->addr2line.out, "r");
>         if (a2l->from_child == NULL) {
> -               pr_warning("could not open read-stream from addr2line of %s\n", path);
> +               pr_warning("could not open read-stream from addr2line (%s) of %s\n",
> +                       addr2line_path, binary_path);
>                 goto out;
>         }
>
> @@ -490,7 +496,6 @@ static int read_addr2line_record(struct a2l_subprocess *a2l,
>
>         if (getline(&line, &line_len, a2l->from_child) < 0 || !line_len)
>                 goto error;
> -
>         if (function != NULL)
>                 *function = strdup(strim(line));
>
> @@ -553,7 +558,8 @@ static int addr2line(const char *dso_name, u64 addr,
>                 if (!filename__has_section(dso_name, ".debug_line"))
>                         goto out;
>
> -               dso->a2l = addr2line_subprocess_init(dso_name);
> +               dso->a2l = addr2line_subprocess_init(symbol_conf.addr2line_path,
> +                                                    dso_name);
>                 a2l = dso->a2l;
>         }
>
> diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
> index bc3d046fbb63..5accd8e69ad2 100644
> --- a/tools/perf/util/symbol_conf.h
> +++ b/tools/perf/util/symbol_conf.h
> @@ -61,6 +61,7 @@ struct symbol_conf {
>                         *sym_list_str,
>                         *col_width_list_str,
>                         *bt_stop_list_str;
> +       char            *addr2line_path;
>         unsigned long   time_quantum;
>         struct strlist  *dso_list,
>                         *comm_list,
> --
> 2.40.0.348.gf938b09366-goog
>

Re: [PATCH v1 1/6] perf annotate: Delete session for debug builds

[Arnaldo Carvalho de Melo]

Em Wed, Mar 29, 2023 at 05:13:17PM -0700, Namhyung Kim escreveu:
> On Wed, Mar 29, 2023 at 6:18 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Wed, Mar 29, 2023 at 10:09:48AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Tue, Mar 28, 2023 at 04:55:38PM -0700, Ian Rogers escreveu:
> > > > Use the debug build indicator as the guide to free the session. This
> > > > implements a behavior described in a comment, which is consequentially
> > > > removed.
> > > >
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > >  tools/perf/builtin-annotate.c | 16 ++++++----------
> > > >  1 file changed, 6 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> > > > index 4750fac7bf93..98d1b6379230 100644
> > > > --- a/tools/perf/builtin-annotate.c
> > > > +++ b/tools/perf/builtin-annotate.c
> > > > @@ -692,16 +692,12 @@ int cmd_annotate(int argc, const char **argv)
> > > >
> > > >  out_delete:
> > > >     /*
> > > > -    * Speed up the exit process, for large files this can
> > > > -    * take quite a while.
> > > > -    *
> > > > -    * XXX Enable this when using valgrind or if we ever
> > > > -    * librarize this command.
> > > > -    *
> > > > -    * Also experiment with obstacks to see how much speed
> > > > -    * up we'll get here.
> > > > -    *
> > > > -    * perf_session__delete(session);
> > > > +    * Speed up the exit process by only deleting for debug builds. For
> > > > +    * large files this can save time.
> > > >      */
> > > > +#ifndef NDEBUG
> > > > +   perf_session__delete(annotate.session);
> > > > +#endif
> > >
> > > So now, but default, we will call this, as we don't have this defined
> > > only if DEBUG=1 is set, right?
> > >
> > > ⬢[acme@toolbox perf-tools-next]$ find tools/perf/ -type f | xargs grep NDEBUG
> > > tools/perf/util/mutex.c:#ifndef NDEBUG
> > > ⬢[acme@toolbox perf-tools-next]$
> >
> > We can discuss this later, applied the series with just that zfree()
> > change to annotation_options__exit().
> 
> I don't think it's just an issue in the perf annotate.  Maybe we can
> do the same for perf report and so on.

Yes, I thought at some point of setting some flag, perf_exiting, and
then we would stop releasing memory, zfree comes to mind, but then we
would still be traversing data structures, taking locks, etc.

And we can't just exit() as we may need to flush caches, etc.

IIRC this specific case appeared in profiles, so was commented out.
 
> Anyway we could define NDEBUG=1 for release builds from now on.

Yes, we should do it.

- Arnaldo

Re: [PATCH v1 1/6] perf annotate: Delete session for debug builds

[Ian Rogers]

On Thu, Mar 30, 2023 at 4:24 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Wed, Mar 29, 2023 at 05:13:17PM -0700, Namhyung Kim escreveu:
> > On Wed, Mar 29, 2023 at 6:18 AM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > Em Wed, Mar 29, 2023 at 10:09:48AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Tue, Mar 28, 2023 at 04:55:38PM -0700, Ian Rogers escreveu:
> > > > > Use the debug build indicator as the guide to free the session. This
> > > > > implements a behavior described in a comment, which is consequentially
> > > > > removed.
> > > > >
> > > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > > ---
> > > > >  tools/perf/builtin-annotate.c | 16 ++++++----------
> > > > >  1 file changed, 6 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> > > > > index 4750fac7bf93..98d1b6379230 100644
> > > > > --- a/tools/perf/builtin-annotate.c
> > > > > +++ b/tools/perf/builtin-annotate.c
> > > > > @@ -692,16 +692,12 @@ int cmd_annotate(int argc, const char **argv)
> > > > >
> > > > >  out_delete:
> > > > >     /*
> > > > > -    * Speed up the exit process, for large files this can
> > > > > -    * take quite a while.
> > > > > -    *
> > > > > -    * XXX Enable this when using valgrind or if we ever
> > > > > -    * librarize this command.
> > > > > -    *
> > > > > -    * Also experiment with obstacks to see how much speed
> > > > > -    * up we'll get here.
> > > > > -    *
> > > > > -    * perf_session__delete(session);
> > > > > +    * Speed up the exit process by only deleting for debug builds. For
> > > > > +    * large files this can save time.
> > > > >      */
> > > > > +#ifndef NDEBUG
> > > > > +   perf_session__delete(annotate.session);
> > > > > +#endif
> > > >
> > > > So now, but default, we will call this, as we don't have this defined
> > > > only if DEBUG=1 is set, right?
> > > >
> > > > ⬢[acme@toolbox perf-tools-next]$ find tools/perf/ -type f | xargs grep NDEBUG
> > > > tools/perf/util/mutex.c:#ifndef NDEBUG
> > > > ⬢[acme@toolbox perf-tools-next]$
> > >
> > > We can discuss this later, applied the series with just that zfree()
> > > change to annotation_options__exit().
> >
> > I don't think it's just an issue in the perf annotate.  Maybe we can
> > do the same for perf report and so on.
>
> Yes, I thought at some point of setting some flag, perf_exiting, and
> then we would stop releasing memory, zfree comes to mind, but then we
> would still be traversing data structures, taking locks, etc.
>
> And we can't just exit() as we may need to flush caches, etc.
>
> IIRC this specific case appeared in profiles, so was commented out.
>
> > Anyway we could define NDEBUG=1 for release builds from now on.
>
> Yes, we should do it.

I'll send out a patch. Just a heads up that this will also disable
asserts for non-debug builds.

Thanks,
Ian

> - Arnaldo