From 64199ae4b8a36038dd5b69904a29bd48ef11ca8b Mon Sep 17 00:00:00 2001 From: Ian Rogers Date: Thu, 31 Aug 2023 00:14:21 -0700 Subject: perf parse-events: Fix propagation of term's no_value when cloning The no_value field in 'struct 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: 7a6e91644708d514 ("perf parse-events: Make common term list to strbuf helper") Fixes: 99e7138eb7897aa0 ("perf tools: Fail on using multiple bits long terms without value") 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-4-irogers@google.com Signed-off-by: Arnaldo Carvalho de Melo --- 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(-) (limited to 'tools') 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; -- cgit