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

[Arnaldo Carvalho de Melo]

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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> 
> 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 [<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 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 [<options>] 'PROBEDEF' ['PROBEDEF' ...]
>     or: perf probe [<options>] --add 'PROBEDEF' [--add 'PROBEDEF' ...]
> ...
>     -k, --vmlinux <file>  vmlinux pathname
>                           (not build because NO_DWARF=1)
>     -L, --line <FUNC[:RLN[+NUM|-RLN2]]|SRC:ALN[+NUM|-ALN2]>
>                           Show source code lines.
>                           (not build because NO_DWARF=1)
> ...
>     -V, --vars <FUNC[@SRC][+OFF|%return|:RL|;PT]|SRC:AL|SRC;PT>
>                           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 <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 | 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/