From 58d3a4cea4a45414a21a712078d95b39dcdda10d Mon Sep 17 00:00:00 2001 From: Ian Rogers Date: Thu, 31 Aug 2023 00:14:20 -0700 Subject: [PATCH 1/1] perf parse-events: Name the two term enums Name the enums used by 'struct parse_events_term' to parse_events__term_val_type and parse_events__term_type. This allows greater compile time error checking. Fix -Wswitch related issues by explicitly listing all enum values prior to default. Add config_term_name to safely look up a parse_events__term_type name, bounds checking the array access first. Add documentation to 'struct parse_events_terms' and reorder to save space. Signed-off-by: Ian Rogers Tested-by: Kan Liang Cc: Adrian Hunter Cc: Alexander Shishkin Cc: Ingo Molnar Cc: James Clark Cc: Jiri Olsa Cc: Mark Rutland Cc: Namhyung Kim Cc: Peter Zijlstra Cc: Rob Herring Link: https://lore.kernel.org/r/20230831071421.2201358-3-irogers@google.com Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/util/parse-events.c | 174 ++++++++++++++++++++++++++++++----------- tools/perf/util/parse-events.h | 60 ++++++++++---- tools/perf/util/parse-events.l | 2 +- tools/perf/util/parse-events.y | 18 +++-- 4 files changed, 187 insertions(+), 67 deletions(-) diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c index 0b941b5..51b7320 100644 --- a/tools/perf/util/parse-events.c +++ b/tools/perf/util/parse-events.c @@ -153,7 +153,7 @@ const char *event_type(int type) return "unknown"; } -static char *get_config_str(struct list_head *head_terms, int type_term) +static char *get_config_str(struct list_head *head_terms, enum parse_events__term_type type_term) { struct parse_events_term *term; @@ -722,7 +722,7 @@ int parse_events_add_breakpoint(struct parse_events_state *parse_state, static int check_type_val(struct parse_events_term *term, struct parse_events_error *err, - int type) + enum parse_events__term_val_type type) { if (type == term->type_val) return 0; @@ -737,42 +737,49 @@ static int check_type_val(struct parse_events_term *term, return -EINVAL; } -/* - * Update according to parse-events.l - */ -static const char *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = { - [PARSE_EVENTS__TERM_TYPE_USER] = "", - [PARSE_EVENTS__TERM_TYPE_CONFIG] = "config", - [PARSE_EVENTS__TERM_TYPE_CONFIG1] = "config1", - [PARSE_EVENTS__TERM_TYPE_CONFIG2] = "config2", - [PARSE_EVENTS__TERM_TYPE_CONFIG3] = "config3", - [PARSE_EVENTS__TERM_TYPE_NAME] = "name", - [PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD] = "period", - [PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ] = "freq", - [PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE] = "branch_type", - [PARSE_EVENTS__TERM_TYPE_TIME] = "time", - [PARSE_EVENTS__TERM_TYPE_CALLGRAPH] = "call-graph", - [PARSE_EVENTS__TERM_TYPE_STACKSIZE] = "stack-size", - [PARSE_EVENTS__TERM_TYPE_NOINHERIT] = "no-inherit", - [PARSE_EVENTS__TERM_TYPE_INHERIT] = "inherit", - [PARSE_EVENTS__TERM_TYPE_MAX_STACK] = "max-stack", - [PARSE_EVENTS__TERM_TYPE_MAX_EVENTS] = "nr", - [PARSE_EVENTS__TERM_TYPE_OVERWRITE] = "overwrite", - [PARSE_EVENTS__TERM_TYPE_NOOVERWRITE] = "no-overwrite", - [PARSE_EVENTS__TERM_TYPE_DRV_CFG] = "driver-config", - [PARSE_EVENTS__TERM_TYPE_PERCORE] = "percore", - [PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT] = "aux-output", - [PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE] = "aux-sample-size", - [PARSE_EVENTS__TERM_TYPE_METRIC_ID] = "metric-id", - [PARSE_EVENTS__TERM_TYPE_RAW] = "raw", - [PARSE_EVENTS__TERM_TYPE_LEGACY_CACHE] = "legacy-cache", - [PARSE_EVENTS__TERM_TYPE_HARDWARE] = "hardware", -}; - static bool config_term_shrinked; +static const char *config_term_name(enum parse_events__term_type term_type) +{ + /* + * Update according to parse-events.l + */ + static const char *config_term_names[__PARSE_EVENTS__TERM_TYPE_NR] = { + [PARSE_EVENTS__TERM_TYPE_USER] = "", + [PARSE_EVENTS__TERM_TYPE_CONFIG] = "config", + [PARSE_EVENTS__TERM_TYPE_CONFIG1] = "config1", + [PARSE_EVENTS__TERM_TYPE_CONFIG2] = "config2", + [PARSE_EVENTS__TERM_TYPE_CONFIG3] = "config3", + [PARSE_EVENTS__TERM_TYPE_NAME] = "name", + [PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD] = "period", + [PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ] = "freq", + [PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE] = "branch_type", + [PARSE_EVENTS__TERM_TYPE_TIME] = "time", + [PARSE_EVENTS__TERM_TYPE_CALLGRAPH] = "call-graph", + [PARSE_EVENTS__TERM_TYPE_STACKSIZE] = "stack-size", + [PARSE_EVENTS__TERM_TYPE_NOINHERIT] = "no-inherit", + [PARSE_EVENTS__TERM_TYPE_INHERIT] = "inherit", + [PARSE_EVENTS__TERM_TYPE_MAX_STACK] = "max-stack", + [PARSE_EVENTS__TERM_TYPE_MAX_EVENTS] = "nr", + [PARSE_EVENTS__TERM_TYPE_OVERWRITE] = "overwrite", + [PARSE_EVENTS__TERM_TYPE_NOOVERWRITE] = "no-overwrite", + [PARSE_EVENTS__TERM_TYPE_DRV_CFG] = "driver-config", + [PARSE_EVENTS__TERM_TYPE_PERCORE] = "percore", + [PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT] = "aux-output", + [PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE] = "aux-sample-size", + [PARSE_EVENTS__TERM_TYPE_METRIC_ID] = "metric-id", + [PARSE_EVENTS__TERM_TYPE_RAW] = "raw", + [PARSE_EVENTS__TERM_TYPE_LEGACY_CACHE] = "legacy-cache", + [PARSE_EVENTS__TERM_TYPE_HARDWARE] = "hardware", + }; + if ((unsigned int)term_type >= __PARSE_EVENTS__TERM_TYPE_NR) + return "unknown term"; + + return config_term_names[term_type]; +} + static bool -config_term_avail(int term_type, struct parse_events_error *err) +config_term_avail(enum parse_events__term_type term_type, struct parse_events_error *err) { char *err_str; @@ -794,13 +801,31 @@ config_term_avail(int term_type, struct parse_events_error *err) case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD: case PARSE_EVENTS__TERM_TYPE_PERCORE: return true; + case PARSE_EVENTS__TERM_TYPE_USER: + case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ: + case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE: + case PARSE_EVENTS__TERM_TYPE_TIME: + case PARSE_EVENTS__TERM_TYPE_CALLGRAPH: + case PARSE_EVENTS__TERM_TYPE_STACKSIZE: + case PARSE_EVENTS__TERM_TYPE_NOINHERIT: + case PARSE_EVENTS__TERM_TYPE_INHERIT: + case PARSE_EVENTS__TERM_TYPE_MAX_STACK: + case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS: + case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE: + case PARSE_EVENTS__TERM_TYPE_OVERWRITE: + case PARSE_EVENTS__TERM_TYPE_DRV_CFG: + case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT: + case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE: + case PARSE_EVENTS__TERM_TYPE_RAW: + case PARSE_EVENTS__TERM_TYPE_LEGACY_CACHE: + case PARSE_EVENTS__TERM_TYPE_HARDWARE: 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) + config_term_name(term_type)) >= 0) parse_events_error__handle(err, -1, err_str, NULL); return false; } @@ -918,10 +943,14 @@ do { \ return -EINVAL; } break; + case PARSE_EVENTS__TERM_TYPE_DRV_CFG: + case PARSE_EVENTS__TERM_TYPE_USER: + case PARSE_EVENTS__TERM_TYPE_LEGACY_CACHE: + case PARSE_EVENTS__TERM_TYPE_HARDWARE: default: parse_events_error__handle(err, term->err_term, - strdup("unknown term"), - parse_events_formats_error_string(NULL)); + strdup(config_term_name(term->type_term)), + parse_events_formats_error_string(NULL)); return -EINVAL; } @@ -1007,10 +1036,26 @@ static int config_term_tracepoint(struct perf_event_attr *attr, case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT: case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE: return config_term_common(attr, term, err); + case PARSE_EVENTS__TERM_TYPE_USER: + case PARSE_EVENTS__TERM_TYPE_CONFIG: + case PARSE_EVENTS__TERM_TYPE_CONFIG1: + case PARSE_EVENTS__TERM_TYPE_CONFIG2: + case PARSE_EVENTS__TERM_TYPE_CONFIG3: + case PARSE_EVENTS__TERM_TYPE_NAME: + case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD: + case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ: + case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE: + case PARSE_EVENTS__TERM_TYPE_TIME: + case PARSE_EVENTS__TERM_TYPE_DRV_CFG: + case PARSE_EVENTS__TERM_TYPE_PERCORE: + case PARSE_EVENTS__TERM_TYPE_METRIC_ID: + case PARSE_EVENTS__TERM_TYPE_RAW: + case PARSE_EVENTS__TERM_TYPE_LEGACY_CACHE: + case PARSE_EVENTS__TERM_TYPE_HARDWARE: default: if (err) { parse_events_error__handle(err, term->err_term, - strdup("unknown term"), + strdup(config_term_name(term->type_term)), strdup("valid terms: call-graph,stack-size\n")); } return -EINVAL; @@ -1128,6 +1173,16 @@ do { \ ADD_CONFIG_TERM_VAL(AUX_SAMPLE_SIZE, aux_sample_size, term->val.num, term->weak); break; + case PARSE_EVENTS__TERM_TYPE_USER: + case PARSE_EVENTS__TERM_TYPE_CONFIG: + case PARSE_EVENTS__TERM_TYPE_CONFIG1: + case PARSE_EVENTS__TERM_TYPE_CONFIG2: + case PARSE_EVENTS__TERM_TYPE_CONFIG3: + case PARSE_EVENTS__TERM_TYPE_NAME: + case PARSE_EVENTS__TERM_TYPE_METRIC_ID: + case PARSE_EVENTS__TERM_TYPE_RAW: + case PARSE_EVENTS__TERM_TYPE_LEGACY_CACHE: + case PARSE_EVENTS__TERM_TYPE_HARDWARE: default: break; } @@ -1157,6 +1212,30 @@ static int get_config_chgs(struct perf_pmu *pmu, struct list_head *head_config, case PARSE_EVENTS__TERM_TYPE_CONFIG: bits = ~(u64)0; break; + case PARSE_EVENTS__TERM_TYPE_CONFIG1: + case PARSE_EVENTS__TERM_TYPE_CONFIG2: + case PARSE_EVENTS__TERM_TYPE_CONFIG3: + case PARSE_EVENTS__TERM_TYPE_NAME: + case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD: + case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ: + case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE: + case PARSE_EVENTS__TERM_TYPE_TIME: + case PARSE_EVENTS__TERM_TYPE_CALLGRAPH: + case PARSE_EVENTS__TERM_TYPE_STACKSIZE: + case PARSE_EVENTS__TERM_TYPE_NOINHERIT: + case PARSE_EVENTS__TERM_TYPE_INHERIT: + case PARSE_EVENTS__TERM_TYPE_MAX_STACK: + case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS: + case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE: + case PARSE_EVENTS__TERM_TYPE_OVERWRITE: + case PARSE_EVENTS__TERM_TYPE_DRV_CFG: + case PARSE_EVENTS__TERM_TYPE_PERCORE: + case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT: + case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE: + case PARSE_EVENTS__TERM_TYPE_METRIC_ID: + case PARSE_EVENTS__TERM_TYPE_RAW: + case PARSE_EVENTS__TERM_TYPE_LEGACY_CACHE: + case PARSE_EVENTS__TERM_TYPE_HARDWARE: default: break; } @@ -2386,7 +2465,8 @@ static int new_term(struct parse_events_term **_term, } int parse_events_term__num(struct parse_events_term **term, - int type_term, const char *config, u64 num, + enum parse_events__term_type type_term, + const char *config, u64 num, bool no_value, void *loc_term_, void *loc_val_) { @@ -2396,7 +2476,7 @@ int parse_events_term__num(struct parse_events_term **term, struct parse_events_term temp = { .type_val = PARSE_EVENTS__TERM_TYPE_NUM, .type_term = type_term, - .config = config ? : strdup(config_term_names[type_term]), + .config = config ? : strdup(config_term_name(type_term)), .no_value = no_value, .err_term = loc_term ? loc_term->first_column : 0, .err_val = loc_val ? loc_val->first_column : 0, @@ -2406,7 +2486,8 @@ int parse_events_term__num(struct parse_events_term **term, } int parse_events_term__str(struct parse_events_term **term, - int type_term, char *config, char *str, + enum parse_events__term_type type_term, + char *config, char *str, void *loc_term_, void *loc_val_) { YYLTYPE *loc_term = loc_term_; @@ -2424,11 +2505,12 @@ int parse_events_term__str(struct parse_events_term **term, } int parse_events_term__term(struct parse_events_term **term, - int term_lhs, int term_rhs, + enum parse_events__term_type term_lhs, + enum parse_events__term_type term_rhs, void *loc_term, void *loc_val) { return parse_events_term__str(term, term_lhs, NULL, - strdup(config_term_names[term_rhs]), + strdup(config_term_name(term_rhs)), loc_term, loc_val); } @@ -2539,7 +2621,7 @@ int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb) if (ret < 0) return ret; } else if (term->type_term < __PARSE_EVENTS__TERM_TYPE_NR) { - ret = strbuf_addf(sb, "%s=", config_term_names[term->type_term]); + ret = strbuf_addf(sb, "%s=", config_term_name(term->type_term)); if (ret < 0) return ret; } @@ -2567,7 +2649,7 @@ static void config_terms_list(char *buf, size_t buf_sz) buf[0] = '\0'; for (i = 0; i < __PARSE_EVENTS__TERM_TYPE_NR; i++) { - const char *name = config_term_names[i]; + const char *name = config_term_name(i); if (!config_term_avail(i, NULL)) continue; diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h index 20bdc35..855b072 100644 --- a/tools/perf/util/parse-events.h +++ b/tools/perf/util/parse-events.h @@ -48,12 +48,12 @@ int parse_events_terms(struct list_head *terms, const char *str, FILE *input); int parse_filter(const struct option *opt, const char *str, int unset); int exclude_perf(const struct option *opt, const char *arg, int unset); -enum { +enum parse_events__term_val_type { PARSE_EVENTS__TERM_TYPE_NUM, PARSE_EVENTS__TERM_TYPE_STR, }; -enum { +enum parse_events__term_type { PARSE_EVENTS__TERM_TYPE_USER, PARSE_EVENTS__TERM_TYPE_CONFIG, PARSE_EVENTS__TERM_TYPE_CONFIG1, @@ -80,27 +80,54 @@ enum { PARSE_EVENTS__TERM_TYPE_RAW, PARSE_EVENTS__TERM_TYPE_LEGACY_CACHE, PARSE_EVENTS__TERM_TYPE_HARDWARE, - __PARSE_EVENTS__TERM_TYPE_NR, +#define __PARSE_EVENTS__TERM_TYPE_NR (PARSE_EVENTS__TERM_TYPE_HARDWARE + 1) }; struct parse_events_term { + /** @list: The term list the term is a part of. */ + struct list_head list; + /** + * @config: The left-hand side of a term assignment, so the term + * "event=8" would have the config be "event" + */ const char *config; + /** + * @val: The right-hand side of a term assignment that can either be a + * string or a number depending on type_val. + */ union { char *str; u64 num; } val; - int type_val; - int type_term; - struct list_head list; - bool used; - bool no_value; - - /* error string indexes for within parsed string */ + /** @type_val: The union variable in val to be used for the term. */ + enum parse_events__term_val_type type_val; + /** + * @type_term: A predefined term type or PARSE_EVENTS__TERM_TYPE_USER + * when not inbuilt. + */ + enum parse_events__term_type type_term; + /** + * @err_term: The column index of the term from parsing, used during + * error output. + */ int err_term; + /** + * @err_val: The column index of the val from parsing, used during error + * output. + */ int err_val; - - /* Coming from implicit alias */ + /** @used: Was the term used during parameterized-eval. */ + bool used; + /** + * @weak: A term from the sysfs or json encoding of an event that + * shouldn't override terms coming from the command line. + */ bool weak; + /** + * @no_value: Is there no value. TODO: this should really be part of + * type_val. + */ + bool no_value; }; struct parse_events_error { @@ -139,14 +166,17 @@ bool parse_events__filter_pmu(const struct parse_events_state *parse_state, 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, const char *config, u64 num, + enum parse_events__term_type type_term, + const char *config, u64 num, bool novalue, void *loc_term, void *loc_val); int parse_events_term__str(struct parse_events_term **term, - int type_term, char *config, char *str, + enum parse_events__term_type type_term, + char *config, char *str, void *loc_term, void *loc_val); int parse_events_term__term(struct parse_events_term **term, - int term_lhs, int term_rhs, + enum parse_events__term_type term_lhs, + enum parse_events__term_type term_rhs, void *loc_term, void *loc_val); int parse_events_term__clone(struct parse_events_term **new, struct parse_events_term *term); diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l index 1147084..4ef4b6f 100644 --- a/tools/perf/util/parse-events.l +++ b/tools/perf/util/parse-events.l @@ -116,7 +116,7 @@ static int tool(yyscan_t scanner, enum perf_tool_event event) return PE_VALUE_SYM_TOOL; } -static int term(yyscan_t scanner, int type) +static int term(yyscan_t scanner, enum parse_events__term_type type) { YYSTYPE *yylval = parse_events_get_lval(scanner); diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y index 4a370c3..534daed 100644 --- a/tools/perf/util/parse-events.y +++ b/tools/perf/util/parse-events.y @@ -777,7 +777,8 @@ PE_TERM_HW PE_TERM '=' name_or_legacy { struct parse_events_term *term; - int err = parse_events_term__str(&term, (int)$1, NULL, $3, &@1, &@3); + int err = parse_events_term__str(&term, (enum parse_events__term_type)$1, + /*config=*/NULL, $3, &@1, &@3); if (err) { free($3); @@ -789,7 +790,8 @@ PE_TERM '=' name_or_legacy PE_TERM '=' PE_TERM_HW { struct parse_events_term *term; - int err = parse_events_term__str(&term, (int)$1, NULL, $3.str, &@1, &@3); + int err = parse_events_term__str(&term, (enum parse_events__term_type)$1, + /*config=*/NULL, $3.str, &@1, &@3); if (err) { free($3.str); @@ -801,7 +803,10 @@ PE_TERM '=' PE_TERM_HW PE_TERM '=' PE_TERM { struct parse_events_term *term; - int err = parse_events_term__term(&term, (int)$1, (int)$3, &@1, &@3); + int err = parse_events_term__term(&term, + (enum parse_events__term_type)$1, + (enum parse_events__term_type)$3, + &@1, &@3); if (err) PE_ABORT(err); @@ -812,7 +817,8 @@ PE_TERM '=' PE_TERM PE_TERM '=' PE_VALUE { struct parse_events_term *term; - int err = parse_events_term__num(&term, (int)$1, NULL, $3, false, &@1, &@3); + int err = parse_events_term__num(&term, (enum parse_events__term_type)$1, + /*config=*/NULL, $3, /*novalue=*/false, &@1, &@3); if (err) PE_ABORT(err); @@ -823,7 +829,9 @@ PE_TERM '=' PE_VALUE PE_TERM { struct parse_events_term *term; - int err = parse_events_term__num(&term, (int)$1, NULL, 1, true, &@1, NULL); + int err = parse_events_term__num(&term, (enum parse_events__term_type)$1, + /*config=*/NULL, /*num=*/1, /*novalue=*/true, + &@1, /*loc_val=*/NULL); if (err) PE_ABORT(err); -- 2.7.4