Re: [PATCH 10/55] perf stat: Forbid user passing improper config terms

[Arnaldo Carvalho de Melo]

Em Fri, Feb 19, 2016 at 11:43:58AM +0000, Wang Nan escreveu:
> 'perf stat' accepts some config terms but doesn't apply them. For
> example:
> 
>  # perf stat -e 'instructions/no-inherit/' -e 'instructions/inherit/' bash
>  # ls
>  # exit

This seems to do the trick, but I wonder if we couldn't instead pass a
callback or instead allow tools to provide a list of supported terms.

I'll apply the patch as it improves the situation, i.e. before people
were left wondering why the modifiers were not producing any effect, now
they get a nice message, but I'd like to hear from Jiri what are his
thoughts, when he's back.

- Arnaldo
 
>  Performance counter stats for 'bash':
> 
>          266258061      instructions/no-inherit/
>          266258061      instructions/inherit/
> 
>        1.402183915 seconds time elapsed
> 
> The result is confusing, because user may expect the first
> 'instructions' event exclude the 'ls' command.
> 
> This patch forbid most of these config terms for 'perf stat'.
> 
> Result:
> 
>  # ./perf stat -e 'instructions/no-inherit/' -e 'instructions/inherit/' bash
>  event syntax error: 'instructions/no-inherit/'
>                       \___ 'no-inherit' is not usable in 'perf stat'
>  ...
> 
> We can add blocked config terms back when 'perf stat' really supports them.
> 
> This patch also removes unavailable config term from error message:
> 
>  # ./perf stat -e 'instructions/badterm/' ls
>  event syntax error: 'instructions/badterm/'
>                                    \___ unknown term
> 
>  valid terms: config,config1,config2,name
> 
>  # ./perf stat -e 'cpu/badterm/' ls
>  event syntax error: 'cpu/badterm/'
>                           \___ unknown term
> 
>  valid terms: pc,any,inv,edge,cmask,event,in_tx,ldlat,umask,in_tx_cp,offcore_rsp,config,config1,config2,name
> 
> Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
> Cc: He Kuang <hekuang@xxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@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-stat.c      |  1 +
>  tools/perf/util/parse-events.c | 48 ++++++++++++++++++++++++++++++++++++++++++
>  tools/perf/util/parse-events.h |  1 +
>  3 files changed, 50 insertions(+)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 86289df..8c0bc0f 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1831,6 +1831,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
>  	if (evsel_list == NULL)
>  		return -ENOMEM;
>  
> +	parse_events__shrink_config_terms();
>  	argc = parse_options_subcommand(argc, argv, stat_options, stat_subcommands,
>  					(const char **) stat_usage,
>  					PARSE_OPT_STOP_AT_NON_OPTION);
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 3fe0d11..5b8a13b 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -816,6 +816,41 @@ static const char *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = {
>  	[PARSE_EVENTS__TERM_TYPE_INHERIT]		= "inherit",
>  };
>  
> +static bool config_term_shrinked;
> +
> +static bool
> +config_term_avail(int term_type, struct parse_events_error *err)
> +{
> +	if (term_type < 0 || term_type >= __PARSE_EVENTS__TERM_TYPE_NR) {
> +		err->str = strdup("Invalid term_type");
> +		return false;
> +	}
> +	if (!config_term_shrinked)
> +		return true;
> +
> +	switch (term_type) {
> +	case PARSE_EVENTS__TERM_TYPE_CONFIG:
> +	case PARSE_EVENTS__TERM_TYPE_CONFIG1:
> +	case PARSE_EVENTS__TERM_TYPE_CONFIG2:
> +	case PARSE_EVENTS__TERM_TYPE_NAME:
> +		return true;
> +	default:
> +		if (!err)
> +			return false;
> +
> +		/* term_type is validated so indexing is safe */
> +		if (asprintf(&err->str, "'%s' is not usable in 'perf stat'",
> +			     config_term_names[term_type]) < 0)
> +			err->str = NULL;
> +		return false;
> +	}
> +}
> +
> +void parse_events__shrink_config_terms(void)
> +{
> +	config_term_shrinked = true;
> +}
> +
>  typedef int config_term_func_t(struct perf_event_attr *attr,
>  			       struct parse_events_term *term,
>  			       struct parse_events_error *err);
> @@ -885,6 +920,17 @@ do {									   \
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * Check term availbility after basic checking so
> +	 * PARSE_EVENTS__TERM_TYPE_USER can be found and filtered.
> +	 *
> +	 * If check availbility at the entry of this function,
> +	 * user will see "'<sysfs term>' is not usable in 'perf stat'"
> +	 * if an invalid config term is provided for legacy events
> +	 * (for example, instructions/badterm/...), which is confusing.
> +	 */
> +	if (!config_term_avail(term->type_term, err))
> +		return -EINVAL;
>  	return 0;
>  #undef CHECK_TYPE_VAL
>  }
> @@ -2177,6 +2223,8 @@ static void config_terms_list(char *buf, size_t buf_sz)
>  	for (i = 0; i < __PARSE_EVENTS__TERM_TYPE_NR; i++) {
>  		const char *name = config_term_names[i];
>  
> +		if (!config_term_avail(i, NULL))
> +			continue;
>  		if (!name)
>  			continue;
>  		if (name[0] == '<')
> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> index 2196db9..b6afc57 100644
> --- a/tools/perf/util/parse-events.h
> +++ b/tools/perf/util/parse-events.h
> @@ -106,6 +106,7 @@ struct parse_events_terms {
>  	struct list_head *terms;
>  };
>  
> +void parse_events__shrink_config_terms(void);
>  int parse_events__is_hardcoded_term(struct parse_events_term *term);
>  int parse_events_term__num(struct parse_events_term **term,
>  			   int type_term, char *config, u64 num,
> -- 
> 1.8.3.4