Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755046AbbLKMka (ORCPT ); Fri, 11 Dec 2015 07:40:30 -0500 Received: from mail.kernel.org ([198.145.29.136]:47874 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754554AbbLKMjj (ORCPT ); Fri, 11 Dec 2015 07:39:39 -0500 Date: Fri, 11 Dec 2015 09:39:32 -0300 From: Arnaldo Carvalho de Melo To: Wang Nan Cc: namhyung@kernel.org, linux-kernel@vger.kernel.org, pi3orama@163.com, mingo@kernel.org, lizefan@huawei.com, Alexei Starovoitov , Masami Hiramatsu Subject: Re: [PATCH v4 13/16] perf tools: Always give options even it not compiled Message-ID: <20151211123932.GA6843@kernel.org> References: <1449541544-67621-1-git-send-email-wangnan0@huawei.com> <1449541544-67621-14-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1449541544-67621-14-git-send-email-wangnan0@huawei.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13298 Lines: 384 Em Tue, Dec 08, 2015 at 02:25:41AM +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@GSjpTKYDCembx32.service.hitachi.net > > Test result: > > Normal case: > > # ./perf probe --vmlinux /tmp/vmlinux sys_write > 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 built because NO_DWARF=1 This should be: Error: switch `L' is not built-in because NO_DWARF=1 But it would be better as: Error: switch `L' is not available because NO_DWARF=1 What makes this an error is the fact that this option doesn't have that CAN_SKIP flag, right? I.e. this SKIP flag determines the error/warning message, for errors this should be "... is not available ..." for warnings it should instead be "... is being ignored ...". > Usage: perf probe [] 'PROBEDEF' ['PROBEDEF' ...] > or: perf probe [] --add 'PROBEDEF' [--add 'PROBEDEF' ...] > or: perf probe [] --del '[GROUP:]EVENT' ... > or: perf probe --list [GROUP:]EVENT ... > or: perf probe [] --funcs > > -L, --line > Show source code lines. > (not build because NO_DWARF=1) > > # ./perf probe -k /tmp/vmlinux sys_write > Warning: switch `k' is not built 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 not built because NO_DWARF=1 > Added new event: > [SNIP] > > # ./perf probe -l > Usage: perf probe [] 'PROBEDEF' ['PROBEDEF' ...] > or: perf probe [] --add 'PROBEDEF' [--add 'PROBEDEF' ...] > ... > -k, --vmlinux vmlinux pathname > (not build because NO_DWARF=1) > -L, --line > Show source code lines. > (not build because NO_DWARF=1) > ... > -V, --vars > Show accessible variables on PROBEDEF > (not build because NO_DWARF=1) > --externs Show external variables too (with --vars only) > (not build because NO_DWARF=1) > --no-inlines Don't search inlined functions > (not build because NO_DWARF=1) > --range Show variables location range in scope (with --vars only) > (not build because NO_DWARF=1) > > Signed-off-by: Wang Nan > Cc: Alexei Starovoitov > Cc: Arnaldo Carvalho de Melo > Cc: Masami Hiramatsu > Cc: Namhyung Kim > Cc: Zefan Li > Cc: pi3orama@163.com > --- > tools/perf/builtin-probe.c | 15 +++++- > tools/perf/builtin-record.c | 9 +++- > tools/perf/util/parse-options.c | 113 ++++++++++++++++++++++++++++++++++++---- > tools/perf/util/parse-options.h | 5 ++ > 4 files changed, 129 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 9fca092..105e357 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,59 @@ 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), > + "is not built because %s", > + opt->build_opt); > + reason[sizeof(reason) - 1] = '\0'; > + > + if (err < 0) > + strncpy(reason, "is not built", 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) > @@ -647,6 +714,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 build because %s)\n", > + USAGE_OPTS_WIDTH + USAGE_GAP, "", > + opts->build_opt); > } > > static int option__cmp(const void *va, const void *vb) > @@ -853,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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/