* [PATCH 1/3] perf tools: Refactor out kernel symbol argument sanity checking
2021-10-18 13:48 [PATCH 0/3] perf inject: Add vmlinux and ignore-vmlinux arguments James Clark
@ 2021-10-18 13:48 ` James Clark
2021-11-06 1:40 ` Denis Nikitin
2021-10-18 13:48 ` [PATCH 2/3] perf tools: Check vmlinux/kallsyms arguments in all tools James Clark
2021-10-18 13:48 ` [PATCH 3/3] perf inject: Add vmlinux and ignore-vmlinux arguments James Clark
2 siblings, 1 reply; 7+ messages in thread
From: James Clark @ 2021-10-18 13:48 UTC (permalink / raw)
To: acme, linux-perf-users
Cc: denik, James Clark, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, linux-kernel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] perf tools: Refactor out kernel symbol argument sanity checking
2021-10-18 13:48 ` [PATCH 1/3] perf tools: Refactor out kernel symbol argument sanity checking James Clark
@ 2021-11-06 1:40 ` Denis Nikitin
0 siblings, 0 replies; 7+ messages in thread
From: Denis Nikitin @ 2021-11-06 1:40 UTC (permalink / raw)
To: James Clark
Cc: acme, linux-perf-users, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, linux-kernel
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/3] perf tools: Check vmlinux/kallsyms arguments in all tools
2021-10-18 13:48 [PATCH 0/3] perf inject: Add vmlinux and ignore-vmlinux arguments James Clark
2021-10-18 13:48 ` [PATCH 1/3] perf tools: Refactor out kernel symbol argument sanity checking James Clark
@ 2021-10-18 13:48 ` James Clark
2021-10-18 13:48 ` [PATCH 3/3] perf inject: Add vmlinux and ignore-vmlinux arguments James Clark
2 siblings, 0 replies; 7+ messages in thread
From: James Clark @ 2021-10-18 13:48 UTC (permalink / raw)
To: acme, linux-perf-users
Cc: denik, James Clark, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, linux-kernel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] perf inject: Add vmlinux and ignore-vmlinux arguments
2021-10-18 13:48 [PATCH 0/3] perf inject: Add vmlinux and ignore-vmlinux arguments James Clark
2021-10-18 13:48 ` [PATCH 1/3] perf tools: Refactor out kernel symbol argument sanity checking James Clark
2021-10-18 13:48 ` [PATCH 2/3] perf tools: Check vmlinux/kallsyms arguments in all tools James Clark
@ 2021-10-18 13:48 ` James Clark
2021-11-06 4:19 ` Denis Nikitin
2 siblings, 1 reply; 7+ messages in thread
From: James Clark @ 2021-10-18 13:48 UTC (permalink / raw)
To: acme, linux-perf-users
Cc: denik, James Clark, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, linux-kernel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] perf inject: Add vmlinux and ignore-vmlinux arguments
2021-10-18 13:48 ` [PATCH 3/3] perf inject: Add vmlinux and ignore-vmlinux arguments James Clark
@ 2021-11-06 4:19 ` Denis Nikitin
2021-11-06 19:53 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 7+ messages in thread
From: Denis Nikitin @ 2021-11-06 4:19 UTC (permalink / raw)
To: James Clark
Cc: acme, linux-perf-users, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, linux-kernel
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] perf inject: Add vmlinux and ignore-vmlinux arguments
2021-11-06 4:19 ` Denis Nikitin
@ 2021-11-06 19:53 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 7+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-11-06 19:53 UTC (permalink / raw)
To: Denis Nikitin
Cc: James Clark, linux-perf-users, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread