Re: [PATCH v5 13/14] perf tools: Always give options even it not compiled

From: Arnaldo Carvalho de Melo
Date: Mon Dec 14 2015 - 11:00:58 EST

Next message: Josh Poimboeuf: "Re: [PATCH v3.1 02/17] tools build: Fix feature Makefile dependencies for 'O='"
Previous message: Thomas Gleixner: "Re: [RFC 3/3] irq: Privatize irq_common_data::state_use_accessors"
In reply to: Wang Nan: "[PATCH v5 13/14] perf tools: Always give options even it not compiled"
Next in thread: Wang Nan: "[PATCH v5 10/14] perf tools: Enable indices setting syntax for BPF maps"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Em Mon, Dec 14, 2015 at 10:39:22AM +0000, Wang Nan escreveu:
> This patch keeps options of perf builtins same in all conditions. If
> one option is disabled because of compiling options, users should be
> notified.
>
> Masami suggested another implementation in [1] that, by adding a
> OPTION_NEXT_DEPENDS option before those options in the 'struct option'
> array, options parser knows an option is disabled. However, in some
> cases this array is reordered (options__order()). In addition, in
> parse-option.c that array is const, so we can't simply merge
> information in decorator option into the affacted option.
>
> This patch chooses a simpler implementation that, introducing a
> set_option_nobuild() function and two option parsing flags. Builtins
> with such options should call set_option_nobuild() before option
> parsing. The complexity of this patch is because we want some of options
> can be skipped safely. In this case their arguments should also be
> consumed.
>
> Options in 'perf record' and 'perf probe' are fixed in this patch.
>
> [1] http://lkml.kernel.org/g/50399556C9727B4D88A595C8584AAB3752627CD4@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>
> Test result:
>
> Normal case:
>
> # ./perf probe --vmlinux /tmp/vmlinux sys_write

Please add two spaces before command output, even more so when the first
character is a '#', because that will make git consider it as comments
and will then suppress it from the commit logs.

Thanks, tested and applied!

- Arnaldo

> Added new event:
> probe:sys_write (on sys_write)
>
> You can now use it in all perf tools, such as:
>
> perf record -e probe:sys_write -aR sleep 1
>
>
> Build with NO_DWARF=1:
>
> # ./perf probe -L sys_write
> Error: switch `L' is not available because NO_DWARF=1
>
> Usage: perf probe [<options>] 'PROBEDEF' ['PROBEDEF' ...]
> or: perf probe [<options>] --add 'PROBEDEF' [--add 'PROBEDEF' ...]
> or: perf probe [<options>] --del '[GROUP:]EVENT' ...
> or: perf probe --list [GROUP:]EVENT ...
> or: perf probe [<options>] --funcs
>
> -L, --line <FUNC[:RLN[+NUM|-RLN2]]|SRC:ALN[+NUM|-ALN2]>
> Show source code lines.
> (not built-in because NO_DWARF=1)
>
> # ./perf probe -k /tmp/vmlinux sys_write
> Warning: switch `k' is being ignored because NO_DWARF=1
> Added new event:
> probe:sys_write (on sys_write)
>
> You can now use it in all perf tools, such as:
>
> perf record -e probe:sys_write -aR sleep 1
>
> # ./perf probe --vmlinux /tmp/vmlinux sys_write
> Warning: option `vmlinux' is being ignored because NO_DWARF=1
> Added new event:
> [SNIP]
>
> # ./perf probe -l
> Usage: perf probe [<options>] 'PROBEDEF' ['PROBEDEF' ...]
> or: perf probe [<options>] --add 'PROBEDEF' [--add 'PROBEDEF' ...]
> ...
> -k, --vmlinux <file> vmlinux pathname
> (not built-in because NO_DWARF=1)
> -L, --line <FUNC[:RLN[+NUM|-RLN2]]|SRC:ALN[+NUM|-ALN2]>
> Show source code lines.
> (not built-in because NO_DWARF=1)
> ...
> -V, --vars <FUNC[@SRC][+OFF|%return|:RL|;PT]|SRC:AL|SRC;PT>
> Show accessible variables on PROBEDEF
> (not built-in because NO_DWARF=1)
> --externs Show external variables too (with --vars only)
> (not built-in because NO_DWARF=1)
> --no-inlines Don't search inlined functions
> (not built-in because NO_DWARF=1)
> --range Show variables location range in scope (with --vars only)
> (not built-in because NO_DWARF=1)
>
> Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
> Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx>
> Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> Cc: Zefan Li <lizefan@xxxxxxxxxx>
> Cc: pi3orama@xxxxxxx
> ---
> tools/perf/builtin-probe.c | 15 ++++-
> tools/perf/builtin-record.c | 9 ++-
> tools/perf/util/parse-options.c | 118 +++++++++++++++++++++++++++++++++++++---
> tools/perf/util/parse-options.h | 5 ++
> 4 files changed, 134 insertions(+), 13 deletions(-)
>
> diff --git a/tools/perf/builtin-probe.c b/tools/perf/builtin-probe.c
> index 132afc9..dbe2ea5 100644
> --- a/tools/perf/builtin-probe.c
> +++ b/tools/perf/builtin-probe.c
> @@ -249,6 +249,9 @@ static int opt_show_vars(const struct option *opt,
>
> return ret;
> }
> +#else
> +# define opt_show_lines NULL
> +# define opt_show_vars NULL
> #endif
> static int opt_add_probe_event(const struct option *opt,
> const char *str, int unset __maybe_unused)
> @@ -473,7 +476,6 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> opt_add_probe_event),
> OPT_BOOLEAN('f', "force", &probe_conf.force_add, "forcibly add events"
> " with existing name"),
> -#ifdef HAVE_DWARF_SUPPORT
> OPT_CALLBACK('L', "line", NULL,
> "FUNC[:RLN[+NUM|-RLN2]]|SRC:ALN[+NUM|-ALN2]",
> "Show source code lines.", opt_show_lines),
> @@ -490,7 +492,6 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> "directory", "path to kernel source"),
> OPT_BOOLEAN('\0', "no-inlines", &probe_conf.no_inlines,
> "Don't search inlined functions"),
> -#endif
> OPT__DRY_RUN(&probe_event_dry_run),
> OPT_INTEGER('\0', "max-probes", &probe_conf.max_probes,
> "Set how many probe points can be found for a probe."),
> @@ -521,6 +522,16 @@ __cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused)
> #ifdef HAVE_DWARF_SUPPORT
> set_option_flag(options, 'L', "line", PARSE_OPT_EXCLUSIVE);
> set_option_flag(options, 'V', "vars", PARSE_OPT_EXCLUSIVE);
> +#else
> +# define set_nobuild(s, l, c) set_option_nobuild(options, s, l, "NO_DWARF=1", c)
> + set_nobuild('L', "line", false);
> + set_nobuild('V', "vars", false);
> + set_nobuild('\0', "externs", false);
> + set_nobuild('\0', "range", false);
> + set_nobuild('k', "vmlinux", true);
> + set_nobuild('s', "source", true);
> + set_nobuild('\0', "no-inlines", true);
> +# undef set_nobuild
> #endif
> set_option_flag(options, 'F', "funcs", PARSE_OPT_EXCLUSIVE);
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 8479821..11bf32d 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1124,12 +1124,10 @@ struct option __record_options[] = {
> "per thread proc mmap processing timeout in ms"),
> OPT_BOOLEAN(0, "switch-events", &record.opts.record_switch_events,
> "Record context switch events"),
> -#ifdef HAVE_LIBBPF_SUPPORT
> OPT_STRING(0, "clang-path", &llvm_param.clang_path, "clang path",
> "clang binary to use for compiling BPF scriptlets"),
> OPT_STRING(0, "clang-opt", &llvm_param.clang_opt, "clang options",
> "options passed to clang when compiling BPF scriptlets"),
> -#endif
> OPT_END()
> };
>
> @@ -1141,6 +1139,13 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
> struct record *rec = &record;
> char errbuf[BUFSIZ];
>
> +#ifndef HAVE_LIBBPF_SUPPORT
> +# define set_nobuild(s, l, c) set_option_nobuild(record_options, s, l, "NO_LIBBPF=1", c)
> + set_nobuild('\0', "clang-path", true);
> + set_nobuild('\0', "clang-opt", true);
> +# undef set_nobuild
> +#endif
> +
> rec->evlist = perf_evlist__new();
> if (rec->evlist == NULL)
> return -ENOMEM;
> diff --git a/tools/perf/util/parse-options.c b/tools/perf/util/parse-options.c
> index de3290b..e22fbde 100644
> --- a/tools/perf/util/parse-options.c
> +++ b/tools/perf/util/parse-options.c
> @@ -18,20 +18,34 @@ static int opterror(const struct option *opt, const char *reason, int flags)
> return error("option `%s' %s", opt->long_name, reason);
> }
>
> +static void optwarning(const struct option *opt, const char *reason, int flags)
> +{
> + if (flags & OPT_SHORT)
> + warning("switch `%c' %s", opt->short_name, reason);
> + else if (flags & OPT_UNSET)
> + warning("option `no-%s' %s", opt->long_name, reason);
> + else
> + warning("option `%s' %s", opt->long_name, reason);
> +}
> +
> static int get_arg(struct parse_opt_ctx_t *p, const struct option *opt,
> int flags, const char **arg)
> {
> + const char *res;
> +
> if (p->opt) {
> - *arg = p->opt;
> + res = p->opt;
> p->opt = NULL;
> } else if ((opt->flags & PARSE_OPT_LASTARG_DEFAULT) && (p->argc == 1 ||
> **(p->argv + 1) == '-')) {
> - *arg = (const char *)opt->defval;
> + res = (const char *)opt->defval;
> } else if (p->argc > 1) {
> p->argc--;
> - *arg = *++p->argv;
> + res = *++p->argv;
> } else
> return opterror(opt, "requires a value", flags);
> + if (arg)
> + *arg = res;
> return 0;
> }
>
> @@ -91,6 +105,64 @@ static int get_value(struct parse_opt_ctx_t *p,
> }
> }
>
> + if (opt->flags & PARSE_OPT_NOBUILD) {
> + char reason[128];
> + bool noarg = false;
> +
> + err = snprintf(reason, sizeof(reason),
> + opt->flags & PARSE_OPT_CANSKIP ?
> + "is being ignored because %s " :
> + "is not available because %s",
> + opt->build_opt);
> + reason[sizeof(reason) - 1] = '\0';
> +
> + if (err < 0)
> + strncpy(reason, opt->flags & PARSE_OPT_CANSKIP ?
> + "is being ignored" :
> + "is not available",
> + sizeof(reason));
> +
> + if (!(opt->flags & PARSE_OPT_CANSKIP))
> + return opterror(opt, reason, flags);
> +
> + err = 0;
> + if (unset)
> + noarg = true;
> + if (opt->flags & PARSE_OPT_NOARG)
> + noarg = true;
> + if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
> + noarg = true;
> +
> + switch (opt->type) {
> + case OPTION_BOOLEAN:
> + case OPTION_INCR:
> + case OPTION_BIT:
> + case OPTION_SET_UINT:
> + case OPTION_SET_PTR:
> + case OPTION_END:
> + case OPTION_ARGUMENT:
> + case OPTION_GROUP:
> + noarg = true;
> + break;
> + case OPTION_CALLBACK:
> + case OPTION_STRING:
> + case OPTION_INTEGER:
> + case OPTION_UINTEGER:
> + case OPTION_LONG:
> + case OPTION_U64:
> + default:
> + break;
> + }
> +
> + if (!noarg)
> + err = get_arg(p, opt, flags, NULL);
> + if (err)
> + return err;
> +
> + optwarning(opt, reason, flags);
> + return 0;
> + }
> +
> switch (opt->type) {
> case OPTION_BIT:
> if (unset)
> @@ -645,6 +717,10 @@ static void print_option_help(const struct option *opts, int full)
> pad = USAGE_OPTS_WIDTH;
> }
> fprintf(stderr, "%*s%s\n", pad + USAGE_GAP, "", opts->help);
> + if (opts->flags & PARSE_OPT_NOBUILD)
> + fprintf(stderr, "%*s(not built-in because %s)\n",
> + USAGE_OPTS_WIDTH + USAGE_GAP, "",
> + opts->build_opt);
> }
>
> static int option__cmp(const void *va, const void *vb)
> @@ -848,15 +924,39 @@ int parse_opt_verbosity_cb(const struct option *opt,
> return 0;
> }
>
> -void set_option_flag(struct option *opts, int shortopt, const char *longopt,
> - int flag)
> +static struct option *
> +find_option(struct option *opts, int shortopt, const char *longopt)
> {
> for (; opts->type != OPTION_END; opts++) {
> if ((shortopt && opts->short_name == shortopt) ||
> (opts->long_name && longopt &&
> - !strcmp(opts->long_name, longopt))) {
> - opts->flags |= flag;
> - break;
> - }
> + !strcmp(opts->long_name, longopt)))
> + return opts;
> }
> + return NULL;
> +}
> +
> +void set_option_flag(struct option *opts, int shortopt, const char *longopt,
> + int flag)
> +{
> + struct option *opt = find_option(opts, shortopt, longopt);
> +
> + if (opt)
> + opt->flags |= flag;
> + return;
> +}
> +
> +void set_option_nobuild(struct option *opts, int shortopt,
> + const char *longopt,
> + const char *build_opt,
> + bool can_skip)
> +{
> + struct option *opt = find_option(opts, shortopt, longopt);
> +
> + if (!opt)
> + return;
> +
> + opt->flags |= PARSE_OPT_NOBUILD;
> + opt->flags |= can_skip ? PARSE_OPT_CANSKIP : 0;
> + opt->build_opt = build_opt;
> }
> diff --git a/tools/perf/util/parse-options.h b/tools/perf/util/parse-options.h
> index a8e407b..2cac2aa 100644
> --- a/tools/perf/util/parse-options.h
> +++ b/tools/perf/util/parse-options.h
> @@ -41,6 +41,8 @@ enum parse_opt_option_flags {
> PARSE_OPT_DISABLED = 32,
> PARSE_OPT_EXCLUSIVE = 64,
> PARSE_OPT_NOEMPTY = 128,
> + PARSE_OPT_NOBUILD = 256,
> + PARSE_OPT_CANSKIP = 512,
> };
>
> struct option;
> @@ -96,6 +98,7 @@ struct option {
> void *value;
> const char *argh;
> const char *help;
> + const char *build_opt;
>
> int flags;
> parse_opt_cb *callback;
> @@ -226,4 +229,6 @@ extern int parse_opt_verbosity_cb(const struct option *, const char *, int);
> extern const char *parse_options_fix_filename(const char *prefix, const char *file);
>
> void set_option_flag(struct option *opts, int sopt, const char *lopt, int flag);
> +void set_option_nobuild(struct option *opts, int shortopt, const char *longopt,
> + const char *build_opt, bool can_skip);
> #endif /* __PERF_PARSE_OPTIONS_H */
> --
> 1.8.3.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Josh Poimboeuf: "Re: [PATCH v3.1 02/17] tools build: Fix feature Makefile dependencies for 'O='"
Previous message: Thomas Gleixner: "Re: [RFC 3/3] irq: Privatize irq_common_data::state_use_accessors"
In reply to: Wang Nan: "[PATCH v5 13/14] perf tools: Always give options even it not compiled"
Next in thread: Wang Nan: "[PATCH v5 10/14] perf tools: Enable indices setting syntax for BPF maps"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]