Commit 64199ae4 authored by Ian Rogers's avatar Ian Rogers Committed by Arnaldo Carvalho de Melo
Browse files

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: 7a6e9164 ("perf parse-events: Make common term list to strbuf helper")
Fixes: 99e7138e ("perf tools: Fail on using multiple bits long terms without value")
Signed-off-by: default avatarIan Rogers <irogers@google.com>
Tested-by: default avatarKan Liang <kan.liang@linux.intel.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: James Clark <james.clark@arm.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rob Herring <robh@kernel.org>
Link: https://lore.kernel.org/r/20230831071421.2201358-4-irogers@google.com


Signed-off-by: default avatarArnaldo Carvalho de Melo <acme@redhat.com>
parent 58d3a4ce

tools/perf/util/parse-events.c

+13 −16
Original line number Diff line number Diff line
@@ -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)

tools/perf/util/parse-events.y

+5 −4
Original line number Diff line number Diff line
@@ -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);

tools/perf/util/pmu.c

+1 −1
Original line number Diff line number Diff line
@@ -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;