[PATCH 0/3] perf inject: Add vmlinux and ignore-vmlinux arguments

[PATCH 0/3] perf inject: Add vmlinux and ignore-vmlinux arguments

[James Clark]

Perf inject was missing the --vmlinux argument so I've added it. At
the same time I tidied up the argument checking that already existed in
perf report and applied it to all tools.

What I'm not sure about is whether it would have been better
to check the accessibility of these files further down in a common place?
It seems like vmlinux_name is also used for a non user provided name at
some point so maybe this would be more complicated than just applying the
existing check everywhere.

Thanks
James

James Clark (3):
  perf tools: Refactor out kernel symbol argument sanity checking
  perf tools: Check vmlinux/kallsyms arguments in all tools
  perf inject: Add vmlinux and ignore-vmlinux arguments

 tools/perf/builtin-annotate.c |  4 ++++
 tools/perf/builtin-c2c.c      |  4 ++++
 tools/perf/builtin-inject.c   |  7 +++++++
 tools/perf/builtin-probe.c    |  5 +++++
 tools/perf/builtin-record.c   |  4 ++++
 tools/perf/builtin-report.c   | 13 ++-----------
 tools/perf/builtin-sched.c    |  4 ++++
 tools/perf/builtin-script.c   |  3 +++
 tools/perf/builtin-top.c      |  4 ++++
 tools/perf/util/symbol.c      | 22 ++++++++++++++++++++++
 tools/perf/util/symbol.h      |  2 ++
 11 files changed, 61 insertions(+), 11 deletions(-)

-- 
2.28.0

[PATCH 1/3] perf tools: Refactor out kernel symbol argument sanity checking

[James Clark]

User supplied values for vmlinux and kallsyms are checked before
continuing. Refactor this into a function so that it can be used
elsewhere.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/builtin-report.c | 13 ++-----------
 tools/perf/util/symbol.c    | 22 ++++++++++++++++++++++
 tools/perf/util/symbol.h    |  2 ++
 3 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index a0316ce910db..8167ebfe776a 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -1378,18 +1378,9 @@ int cmd_report(int argc, const char **argv)
 	if (quiet)
 		perf_quiet_option();
 
-	if (symbol_conf.vmlinux_name &&
-	    access(symbol_conf.vmlinux_name, R_OK)) {
-		pr_err("Invalid file: %s\n", symbol_conf.vmlinux_name);
-		ret = -EINVAL;
-		goto exit;
-	}
-	if (symbol_conf.kallsyms_name &&
-	    access(symbol_conf.kallsyms_name, R_OK)) {
-		pr_err("Invalid file: %s\n", symbol_conf.kallsyms_name);
-		ret = -EINVAL;
+	ret = symbol__validate_sym_arguments();
+	if (ret)
 		goto exit;
-	}
 
 	if (report.inverted_callchain)
 		callchain_param.order = ORDER_CALLER;
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 0fc9a5410739..8fad1f0d41cb 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2630,3 +2630,25 @@ struct mem_info *mem_info__new(void)
 		refcount_set(&mi->refcnt, 1);
 	return mi;
 }
+
+/*
+ * Checks that user supplied symbol kernel files are accessible because
+ * the default mechanism for accessing elf files fails silently. i.e. if
+ * debug syms for a build ID aren't found perf carries on normally. When
+ * they are user supplied we should assume that the user doesn't want to
+ * silently fail.
+ */
+int symbol__validate_sym_arguments(void)
+{
+	if (symbol_conf.vmlinux_name &&
+	    access(symbol_conf.vmlinux_name, R_OK)) {
+		pr_err("Invalid file: %s\n", symbol_conf.vmlinux_name);
+		return -EINVAL;
+	}
+	if (symbol_conf.kallsyms_name &&
+	    access(symbol_conf.kallsyms_name, R_OK)) {
+		pr_err("Invalid file: %s\n", symbol_conf.kallsyms_name);
+		return -EINVAL;
+	}
+	return 0;
+}
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 954d6a049ee2..166196686f2e 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -286,4 +286,6 @@ static inline void __mem_info__zput(struct mem_info **mi)
 
 #define mem_info__zput(mi) __mem_info__zput(&mi)
 
+int symbol__validate_sym_arguments(void);
+
 #endif /* __PERF_SYMBOL */
-- 
2.28.0

[PATCH 2/3] perf tools: Check vmlinux/kallsyms arguments in all tools

[James Clark]

Only perf report checked the validity of these arguments so apply the
same check to all tools that read them for consistency.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/builtin-annotate.c | 4 ++++
 tools/perf/builtin-c2c.c      | 4 ++++
 tools/perf/builtin-probe.c    | 5 +++++
 tools/perf/builtin-record.c   | 4 ++++
 tools/perf/builtin-sched.c    | 4 ++++
 tools/perf/builtin-script.c   | 3 +++
 tools/perf/builtin-top.c      | 4 ++++
 7 files changed, 28 insertions(+)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 05eb098cb0e3..490bb9b8cf17 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -591,6 +591,10 @@ int cmd_annotate(int argc, const char **argv)
 		return ret;
 	}
 
+	ret = symbol__validate_sym_arguments();
+	if (ret)
+		return ret;
+
 	if (quiet)
 		perf_quiet_option();
 
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index a192014fa52b..b5c67ef73862 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2768,6 +2768,10 @@ static int perf_c2c__report(int argc, const char **argv)
 	if (c2c.stats_only)
 		c2c.use_stdio = true;
 
+	err = symbol__validate_sym_arguments();
+	if (err)
+		goto out;
+
 	if (!input_name || !strlen(input_name))
 		input_name = "perf.data";
 
diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
index e1dd51f2874b..c31627af75d4 100644
--- a/tools/perf/builtin-probe.c
+++ b/tools/perf/builtin-probe.c
@@ -21,6 +21,7 @@
 #include "util/build-id.h"
 #include "util/strlist.h"
 #include "util/strfilter.h"
+#include "util/symbol.h"
 #include "util/symbol_conf.h"
 #include "util/debug.h"
 #include <subcmd/parse-options.h>
@@ -629,6 +630,10 @@ __cmd_probe(int argc, const char **argv)
 		params.command = 'a';
 	}
 
+	ret = symbol__validate_sym_arguments();
+	if (ret)
+		return ret;
+
 	if (params.quiet) {
 		if (verbose != 0) {
 			pr_err("  Error: -v and -q are exclusive.\n");
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 41bb884f5a74..9aff49915148 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -2712,6 +2712,10 @@ int cmd_record(int argc, const char **argv)
 	if (quiet)
 		perf_quiet_option();
 
+	err = symbol__validate_sym_arguments();
+	if (err)
+		return err;
+
 	/* Make system wide (-a) the default target. */
 	if (!argc && target__none(&rec->opts.target))
 		rec->opts.target.system_wide = true;
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 635a6b5a9ec9..4527f632ebe4 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -3538,6 +3538,7 @@ int cmd_sched(int argc, const char **argv)
 		.fork_event	    = replay_fork_event,
 	};
 	unsigned int i;
+	int ret;
 
 	for (i = 0; i < ARRAY_SIZE(sched.curr_pid); i++)
 		sched.curr_pid[i] = -1;
@@ -3598,6 +3599,9 @@ int cmd_sched(int argc, const char **argv)
 				parse_options_usage(NULL, timehist_options, "n", true);
 			return -EINVAL;
 		}
+		ret = symbol__validate_sym_arguments();
+		if (ret)
+			return ret;
 
 		return perf_sched__timehist(&sched);
 	} else {
diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 6211d0b84b7a..53f8f7b408be 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -3836,6 +3836,9 @@ int cmd_script(int argc, const char **argv)
 	data.path  = input_name;
 	data.force = symbol_conf.force;
 
+	if (symbol__validate_sym_arguments())
+		return -1;
+
 	if (argc > 1 && !strncmp(argv[0], "rec", strlen("rec"))) {
 		rec_script_path = get_script_path(argv[1], RECORD_SUFFIX);
 		if (!rec_script_path)
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 020c4f110c10..1fc390f136dd 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1618,6 +1618,10 @@ int cmd_top(int argc, const char **argv)
 	if (argc)
 		usage_with_options(top_usage, options);
 
+	status = symbol__validate_sym_arguments();
+	if (status)
+		goto out_delete_evlist;
+
 	if (annotate_check_args(&top.annotation_opts) < 0)
 		goto out_delete_evlist;
 
-- 
2.28.0

[PATCH 3/3] perf inject: Add vmlinux and ignore-vmlinux arguments

[James Clark]

Other perf tools allow specifying the path to vmlinux. Perf inject
didn't have this argument which made some auxtrace workflows difficult.

Also add ignore-vmlinux for consistency with other tools.

Suggested-by: Denis Nitikin <denik@chromium.org>
Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/builtin-inject.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 6ad191e731fc..4261ad89730f 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -938,6 +938,10 @@ int cmd_inject(int argc, const char **argv)
 #endif
 		OPT_INCR('v', "verbose", &verbose,
 			 "be more verbose (show build ids, etc)"),
+		OPT_STRING('k', "vmlinux", &symbol_conf.vmlinux_name,
+			   "file", "vmlinux pathname"),
+		OPT_BOOLEAN(0, "ignore-vmlinux", &symbol_conf.ignore_vmlinux,
+			    "don't load vmlinux even if found"),
 		OPT_STRING(0, "kallsyms", &symbol_conf.kallsyms_name, "file",
 			   "kallsyms pathname"),
 		OPT_BOOLEAN('f', "force", &data.force, "don't complain, do it"),
@@ -972,6 +976,9 @@ int cmd_inject(int argc, const char **argv)
 		return -1;
 	}
 
+	if (symbol__validate_sym_arguments())
+		return -1;
+
 	if (inject.in_place_update) {
 		if (!strcmp(inject.input_name, "-")) {
 			pr_err("Input file name required for in-place updating\n");
-- 
2.28.0

Re: [PATCH 1/3] perf tools: Refactor out kernel symbol argument sanity checking

[Denis Nikitin]

On Mon, Oct 18, 2021 at 6:48 AM James Clark <james.clark@arm.com> wrote:
>
> User supplied values for vmlinux and kallsyms are checked before
> continuing. Refactor this into a function so that it can be used
> elsewhere.
>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/builtin-report.c | 13 ++-----------
>  tools/perf/util/symbol.c    | 22 ++++++++++++++++++++++
>  tools/perf/util/symbol.h    |  2 ++
>  3 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index a0316ce910db..8167ebfe776a 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -1378,18 +1378,9 @@ int cmd_report(int argc, const char **argv)
>         if (quiet)
>                 perf_quiet_option();
>
> -       if (symbol_conf.vmlinux_name &&
> -           access(symbol_conf.vmlinux_name, R_OK)) {
> -               pr_err("Invalid file: %s\n", symbol_conf.vmlinux_name);
> -               ret = -EINVAL;
> -               goto exit;
> -       }
> -       if (symbol_conf.kallsyms_name &&
> -           access(symbol_conf.kallsyms_name, R_OK)) {
> -               pr_err("Invalid file: %s\n", symbol_conf.kallsyms_name);
> -               ret = -EINVAL;
> +       ret = symbol__validate_sym_arguments();
> +       if (ret)
>                 goto exit;
> -       }
>
>         if (report.inverted_callchain)
>                 callchain_param.order = ORDER_CALLER;
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 0fc9a5410739..8fad1f0d41cb 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -2630,3 +2630,25 @@ struct mem_info *mem_info__new(void)
>                 refcount_set(&mi->refcnt, 1);
>         return mi;
>  }
> +
> +/*
> + * Checks that user supplied symbol kernel files are accessible because
> + * the default mechanism for accessing elf files fails silently. i.e. if
> + * debug syms for a build ID aren't found perf carries on normally. When
> + * they are user supplied we should assume that the user doesn't want to
> + * silently fail.
> + */
> +int symbol__validate_sym_arguments(void)
> +{
> +       if (symbol_conf.vmlinux_name &&
> +           access(symbol_conf.vmlinux_name, R_OK)) {
> +               pr_err("Invalid file: %s\n", symbol_conf.vmlinux_name);
> +               return -EINVAL;
> +       }
> +       if (symbol_conf.kallsyms_name &&
> +           access(symbol_conf.kallsyms_name, R_OK)) {
> +               pr_err("Invalid file: %s\n", symbol_conf.kallsyms_name);
> +               return -EINVAL;
> +       }
> +       return 0;
> +}
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 954d6a049ee2..166196686f2e 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -286,4 +286,6 @@ static inline void __mem_info__zput(struct mem_info **mi)
>
>  #define mem_info__zput(mi) __mem_info__zput(&mi)
>
> +int symbol__validate_sym_arguments(void);
> +
>  #endif /* __PERF_SYMBOL */
> --
> 2.28.0
>

Reviewed-by: Denis Nikitin <denik@chromium.org>

Re: [PATCH 3/3] perf inject: Add vmlinux and ignore-vmlinux arguments

[Denis Nikitin]

On Mon, Oct 18, 2021 at 6:49 AM James Clark <james.clark@arm.com> wrote:
>
> Other perf tools allow specifying the path to vmlinux. Perf inject
> didn't have this argument which made some auxtrace workflows difficult.
>
> Also add ignore-vmlinux for consistency with other tools.
>
> Suggested-by: Denis Nitikin <denik@chromium.org>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/builtin-inject.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 6ad191e731fc..4261ad89730f 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -938,6 +938,10 @@ int cmd_inject(int argc, const char **argv)
>  #endif
>                 OPT_INCR('v', "verbose", &verbose,
>                          "be more verbose (show build ids, etc)"),
> +               OPT_STRING('k', "vmlinux", &symbol_conf.vmlinux_name,
> +                          "file", "vmlinux pathname"),
> +               OPT_BOOLEAN(0, "ignore-vmlinux", &symbol_conf.ignore_vmlinux,
> +                           "don't load vmlinux even if found"),

I think we also need to update documentation at Documentation/perf-inject.txt

>                 OPT_STRING(0, "kallsyms", &symbol_conf.kallsyms_name, "file",
>                            "kallsyms pathname"),
>                 OPT_BOOLEAN('f', "force", &data.force, "don't complain, do it"),
> @@ -972,6 +976,9 @@ int cmd_inject(int argc, const char **argv)
>                 return -1;
>         }
>
> +       if (symbol__validate_sym_arguments())
> +               return -1;
> +
>         if (inject.in_place_update) {
>                 if (!strcmp(inject.input_name, "-")) {
>                         pr_err("Input file name required for in-place updating\n");
> --
> 2.28.0
>

Tested-by: Denis Nikitin <denik@chromium.org>

Re: [PATCH 3/3] perf inject: Add vmlinux and ignore-vmlinux arguments

[Arnaldo Carvalho de Melo]

Em Fri, Nov 05, 2021 at 09:19:10PM -0700, Denis Nikitin escreveu:
> On Mon, Oct 18, 2021 at 6:49 AM James Clark <james.clark@arm.com> wrote:
> >
> > Other perf tools allow specifying the path to vmlinux. Perf inject
> > didn't have this argument which made some auxtrace workflows difficult.
> >
> > Also add ignore-vmlinux for consistency with other tools.
> >
> > Suggested-by: Denis Nitikin <denik@chromium.org>
> > Signed-off-by: James Clark <james.clark@arm.com>
> > ---
> >  tools/perf/builtin-inject.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> > index 6ad191e731fc..4261ad89730f 100644
> > --- a/tools/perf/builtin-inject.c
> > +++ b/tools/perf/builtin-inject.c
> > @@ -938,6 +938,10 @@ int cmd_inject(int argc, const char **argv)
> >  #endif
> >                 OPT_INCR('v', "verbose", &verbose,
> >                          "be more verbose (show build ids, etc)"),
> > +               OPT_STRING('k', "vmlinux", &symbol_conf.vmlinux_name,
> > +                          "file", "vmlinux pathname"),
> > +               OPT_BOOLEAN(0, "ignore-vmlinux", &symbol_conf.ignore_vmlinux,
> > +                           "don't load vmlinux even if found"),
> 
> I think we also need to update documentation at Documentation/perf-inject.txt

I can replicate what is in 'perf report' where this came from, will do
it now.
 
> >                 OPT_STRING(0, "kallsyms", &symbol_conf.kallsyms_name, "file",
> >                            "kallsyms pathname"),
> >                 OPT_BOOLEAN('f', "force", &data.force, "don't complain, do it"),
> > @@ -972,6 +976,9 @@ int cmd_inject(int argc, const char **argv)
> >                 return -1;
> >         }
> >
> > +       if (symbol__validate_sym_arguments())
> > +               return -1;
> > +
> >         if (inject.in_place_update) {
> >                 if (!strcmp(inject.input_name, "-")) {
> >                         pr_err("Input file name required for in-place updating\n");
> > --
> > 2.28.0
> >
> 
> Tested-by: Denis Nikitin <denik@chromium.org>

Thanks,

- Arnaldo