[PATCH v1 0/3] perf list/debug output fixes

[PATCH v1 0/3] perf list/debug output fixes

[Ian Rogers]

Fix a long standing parse_events_term cloning bug so that the bad
display of terms can be fixed and the code somewhat more intuitive:
https://lore.kernel.org/lkml/20230830070753.1821629-2-irogers@google.com/

Fix a bug caused by the rename of 'cpu' to 'default_core' in perf list.

Add more documentation, increase type safety and fix some related bugs
where terms weren't initialized properly.

Ian Rogers (3):
  perf list: Don't print Unit for default_core
  perf parse-events: Name the two term enums
  perf parse-events: Fix propagation of term's no_value when cloning

 tools/perf/builtin-list.c      |   2 +-
 tools/perf/util/parse-events.c | 203 +++++++++++++++++++++++----------
 tools/perf/util/parse-events.h |  60 +++++++---
 tools/perf/util/parse-events.l |   2 +-
 tools/perf/util/parse-events.y |  27 +++--
 tools/perf/util/pmu.c          |   2 +-
 6 files changed, 207 insertions(+), 89 deletions(-)

-- 
2.42.0.rc2.253.gd59a3bf2b4-goog

[PATCH v1 1/3] perf list: Don't print Unit for default_core

[Ian Rogers]

default_core was added as a way to demark json events whose PMU should
be whatever the default core PMU is, previously this had been assumed
to be "cpu" but that fails on s390 and ARM. perf list displays the PMU
in the event description to save storing it in json, but was still
comparing against "cpu" and not "default_core", so update this.

Fixes: d2045f87154b ("perf jevents: Use "default_core" for events with no Unit")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-list.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-list.c b/tools/perf/builtin-list.c
index 1ac47db4d66a..a343823c8ddf 100644
--- a/tools/perf/builtin-list.c
+++ b/tools/perf/builtin-list.c
@@ -148,7 +148,7 @@ static void default_print_event(void *ps, const char *pmu_name, const char *topi
 		char *desc_with_unit = NULL;
 		int desc_len = -1;
 
-		if (pmu_name && strcmp(pmu_name, "cpu")) {
+		if (pmu_name && strcmp(pmu_name, "default_core")) {
 			desc_len = strlen(desc);
 			desc_len = asprintf(&desc_with_unit,
 					    desc[desc_len - 1] != '.'
-- 
2.42.0.rc2.253.gd59a3bf2b4-goog

[PATCH v1 2/3] perf parse-events: Name the two term enums

[Ian Rogers]

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 <irogers@google.com>
---
 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 0b941b58bdc0..51b73207e9f4 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]			= "<sysfs term>",
-	[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]			= "<sysfs term>",
+		[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 20bdc35d6112..855b0725c5d4 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 1147084b2c76..4ef4b6f171a0 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 4a370c36a0d5..534daed91c50 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.42.0.rc2.253.gd59a3bf2b4-goog

[PATCH v1 3/3] perf parse-events: Fix propagation of term's no_value when cloning

[Ian Rogers]

The no_value field in parse_events_term indicates that the val
variable isn't used, the case for an event name. Cloning wasn't
propagating this, making cloned event name terms appearing to have a
constant assinged to them. Working around the bug would check for a
value of 1 assigned to value, but then this meant a user value of 1
couldn't be differentiated causing the value to be lost in debug
printing and perf list.

The change fixes the cloning and updates the "val.num ==/!= 1" tests
to use no_value instead. To better check the no_value is set
appropriately parameter comments are added for constant values. This
found that no_value wasn't set correctly in
parse_events_multi_pmu_add, which matters now that no_value is used to
indicate an event name.

Fixes: 7a6e91644708 ("perf parse-events: Make common term list to strbuf helper")
Fixes: 99e7138eb789 ("perf tools: Fail on using multiple bits long terms without value")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/parse-events.c | 29 +++++++++++++----------------
 tools/perf/util/parse-events.y |  9 +++++----
 tools/perf/util/pmu.c          |  2 +-
 3 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index 51b73207e9f4..68fe2c4ff49f 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -1510,8 +1510,8 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state,
 
 	if (parse_events_term__num(&term,
 				   PARSE_EVENTS__TERM_TYPE_USER,
-				   config, 1, false, NULL,
-					NULL) < 0) {
+				   config, /*num=*/1, /*novalue=*/true,
+				   loc, /*loc_val=*/NULL) < 0) {
 		zfree(&config);
 		goto out_err;
 	}
@@ -2482,7 +2482,7 @@ int parse_events_term__num(struct parse_events_term **term,
 		.err_val   = loc_val  ? loc_val->first_column  : 0,
 	};
 
-	return new_term(term, &temp, NULL, num);
+	return new_term(term, &temp, /*str=*/NULL, num);
 }
 
 int parse_events_term__str(struct parse_events_term **term,
@@ -2501,7 +2501,7 @@ int parse_events_term__str(struct parse_events_term **term,
 		.err_val   = loc_val  ? loc_val->first_column  : 0,
 	};
 
-	return new_term(term, &temp, str, 0);
+	return new_term(term, &temp, str, /*num=*/0);
 }
 
 int parse_events_term__term(struct parse_events_term **term,
@@ -2518,26 +2518,21 @@ int parse_events_term__clone(struct parse_events_term **new,
 			     struct parse_events_term *term)
 {
 	char *str;
-	struct parse_events_term temp = {
-		.type_val  = term->type_val,
-		.type_term = term->type_term,
-		.config    = NULL,
-		.err_term  = term->err_term,
-		.err_val   = term->err_val,
-	};
+	struct parse_events_term temp = *term;
 
+	temp.used = false;
 	if (term->config) {
 		temp.config = strdup(term->config);
 		if (!temp.config)
 			return -ENOMEM;
 	}
 	if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
-		return new_term(new, &temp, NULL, term->val.num);
+		return new_term(new, &temp, /*str=*/NULL, term->val.num);
 
 	str = strdup(term->val.str);
 	if (!str)
 		return -ENOMEM;
-	return new_term(new, &temp, str, 0);
+	return new_term(new, &temp, str, /*num=*/0);
 }
 
 void parse_events_term__delete(struct parse_events_term *term)
@@ -2611,20 +2606,22 @@ int parse_events_term__to_strbuf(struct list_head *term_list, struct strbuf *sb)
 		first = false;
 
 		if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM)
-			if (term->type_term == PARSE_EVENTS__TERM_TYPE_USER && term->val.num == 1)
+			if (term->no_value) {
+				assert(term->type_term == PARSE_EVENTS__TERM_TYPE_USER);
 				ret = strbuf_addf(sb, "%s", term->config);
-			else
+			} else
 				ret = strbuf_addf(sb, "%s=%#"PRIx64, term->config, term->val.num);
 		else if (term->type_val == PARSE_EVENTS__TERM_TYPE_STR) {
 			if (term->config) {
 				ret = strbuf_addf(sb, "%s=", term->config);
 				if (ret < 0)
 					return ret;
-			} else if (term->type_term < __PARSE_EVENTS__TERM_TYPE_NR) {
+			} else if ((unsigned int)term->type_term < __PARSE_EVENTS__TERM_TYPE_NR) {
 				ret = strbuf_addf(sb, "%s=", config_term_name(term->type_term));
 				if (ret < 0)
 					return ret;
 			}
+			assert(!term->no_value);
 			ret = strbuf_addf(sb, "%s", term->val.str);
 		}
 		if (ret < 0)
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 534daed91c50..4a305df61f74 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -712,7 +712,7 @@ name_or_raw '=' PE_VALUE
 {
 	struct parse_events_term *term;
 	int err = parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
-					 $1, $3, false, &@1, &@3);
+					 $1, $3, /*novalue=*/false, &@1, &@3);
 
 	if (err) {
 		free($1);
@@ -739,7 +739,7 @@ PE_LEGACY_CACHE
 {
 	struct parse_events_term *term;
 	int err = parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_LEGACY_CACHE,
-					 $1, 1, true, &@1, NULL);
+					 $1, /*num=*/1, /*novalue=*/true, &@1, /*loc_val=*/NULL);
 
 	if (err) {
 		free($1);
@@ -752,7 +752,7 @@ PE_NAME
 {
 	struct parse_events_term *term;
 	int err = parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER,
-					 $1, 1, true, &@1, NULL);
+					 $1, /*num=*/1, /*novalue=*/true, &@1, /*loc_val=*/NULL);
 
 	if (err) {
 		free($1);
@@ -765,7 +765,8 @@ PE_TERM_HW
 {
 	struct parse_events_term *term;
 	int err = parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_HARDWARE,
-					 $1.str, $1.num & 255, false, &@1, NULL);
+					 $1.str, $1.num & 255, /*novalue=*/false,
+					 &@1, /*loc_val=*/NULL);
 
 	if (err) {
 		free($1.str);
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 152cda84f273..d85602aa4b9f 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1396,7 +1396,7 @@ static struct perf_pmu_alias *pmu_find_alias(struct perf_pmu *pmu,
 		return NULL;
 
 	if (term->type_val == PARSE_EVENTS__TERM_TYPE_NUM) {
-		if (term->val.num != 1)
+		if (!term->no_value)
 			return NULL;
 		if (pmu_find_format(&pmu->format, term->config))
 			return NULL;
-- 
2.42.0.rc2.253.gd59a3bf2b4-goog

Re: [PATCH v1 0/3] perf list/debug output fixes

[Liang, Kan]

On 2023-08-31 3:14 a.m., Ian Rogers wrote:
> Fix a long standing parse_events_term cloning bug so that the bad
> display of terms can be fixed and the code somewhat more intuitive:
> https://lore.kernel.org/lkml/20230830070753.1821629-2-irogers@google.com/
> 

Tested-by: Kan Liang <kan.liang@linux.intel.com>

Thanks,
Kan

> Fix a bug caused by the rename of 'cpu' to 'default_core' in perf list.
> 
> Add more documentation, increase type safety and fix some related bugs
> where terms weren't initialized properly.
> 
> Ian Rogers (3):
>   perf list: Don't print Unit for default_core
>   perf parse-events: Name the two term enums
>   perf parse-events: Fix propagation of term's no_value when cloning
> 
>  tools/perf/builtin-list.c      |   2 +-
>  tools/perf/util/parse-events.c | 203 +++++++++++++++++++++++----------
>  tools/perf/util/parse-events.h |  60 +++++++---
>  tools/perf/util/parse-events.l |   2 +-
>  tools/perf/util/parse-events.y |  27 +++--
>  tools/perf/util/pmu.c          |   2 +-
>  6 files changed, 207 insertions(+), 89 deletions(-)
> 

Re: [PATCH v1 0/3] perf list/debug output fixes

[Ian Rogers]

On Thu, Aug 31, 2023 at 11:28 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2023-08-31 3:14 a.m., Ian Rogers wrote:
> > Fix a long standing parse_events_term cloning bug so that the bad
> > display of terms can be fixed and the code somewhat more intuitive:
> > https://lore.kernel.org/lkml/20230830070753.1821629-2-irogers@google.com/
> >
>
> Tested-by: Kan Liang <kan.liang@linux.intel.com>

Thanks Kan!
Ian

> Thanks,
> Kan
>
> > Fix a bug caused by the rename of 'cpu' to 'default_core' in perf list.
> >
> > Add more documentation, increase type safety and fix some related bugs
> > where terms weren't initialized properly.
> >
> > Ian Rogers (3):
> >   perf list: Don't print Unit for default_core
> >   perf parse-events: Name the two term enums
> >   perf parse-events: Fix propagation of term's no_value when cloning
> >
> >  tools/perf/builtin-list.c      |   2 +-
> >  tools/perf/util/parse-events.c | 203 +++++++++++++++++++++++----------
> >  tools/perf/util/parse-events.h |  60 +++++++---
> >  tools/perf/util/parse-events.l |   2 +-
> >  tools/perf/util/parse-events.y |  27 +++--
> >  tools/perf/util/pmu.c          |   2 +-
> >  6 files changed, 207 insertions(+), 89 deletions(-)
> >

Re: [PATCH v1 0/3] perf list/debug output fixes

[Arnaldo Carvalho de Melo]

Em Thu, Aug 31, 2023 at 11:41:51AM -0700, Ian Rogers escreveu:
> On Thu, Aug 31, 2023 at 11:28 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >
> >
> >
> > On 2023-08-31 3:14 a.m., Ian Rogers wrote:
> > > Fix a long standing parse_events_term cloning bug so that the bad
> > > display of terms can be fixed and the code somewhat more intuitive:
> > > https://lore.kernel.org/lkml/20230830070753.1821629-2-irogers@google.com/
> > >
> >
> > Tested-by: Kan Liang <kan.liang@linux.intel.com>
> 
> Thanks Kan!
> Ian

Thanks, applied.

- Arnaldo
 
> > Thanks,
> > Kan
> >
> > > Fix a bug caused by the rename of 'cpu' to 'default_core' in perf list.
> > >
> > > Add more documentation, increase type safety and fix some related bugs
> > > where terms weren't initialized properly.
> > >
> > > Ian Rogers (3):
> > >   perf list: Don't print Unit for default_core
> > >   perf parse-events: Name the two term enums
> > >   perf parse-events: Fix propagation of term's no_value when cloning
> > >
> > >  tools/perf/builtin-list.c      |   2 +-
> > >  tools/perf/util/parse-events.c | 203 +++++++++++++++++++++++----------
> > >  tools/perf/util/parse-events.h |  60 +++++++---
> > >  tools/perf/util/parse-events.l |   2 +-
> > >  tools/perf/util/parse-events.y |  27 +++--
> > >  tools/perf/util/pmu.c          |   2 +-
> > >  6 files changed, 207 insertions(+), 89 deletions(-)
> > >

-- 

- Arnaldo