Re: [PATCH v2 2/3] perf parse-events: Improve error location of terms cloned from an event

[James Clark]

On 31/01/2024 13:49, Ian Rogers wrote:
> A PMU event/alias will have a set of format terms that replace it when
> an event is parsed. The location of the terms is their position when
> parsed for the event/alias either from sysfs or json. This location is
> of little use when an event fails to parse as the error will be given
> in terms of the location in the string of events parsed not the json
> or sysfs string. Fix this by making the cloned terms location that of
> the event/alias.
> 
> If a cloned term from an event/alias is invalid the bad format is hard
> to determine from the error string. Add the name of the bad format
> into the error string.
> 
> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>

Reviewed-by: James Clark <james.clark@xxxxxxx>

> ---
> These fixes were inspired by the poor error output in:
> https://lore.kernel.org/linux-perf-users/alpine.LRH.2.20.2401300733310.11354@Diego/
> ---
>  tools/perf/util/pmu.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 355f813f960d..437386dedd5c 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -657,7 +657,7 @@ static int pmu_aliases_parse(struct perf_pmu *pmu)
>  	return 0;
>  }
>  
> -static int pmu_alias_terms(struct perf_pmu_alias *alias, struct list_head *terms)
> +static int pmu_alias_terms(struct perf_pmu_alias *alias, int err_loc, struct list_head *terms)
>  {
>  	struct parse_events_term *term, *cloned;
>  	struct parse_events_terms clone_terms;
> @@ -675,6 +675,7 @@ static int pmu_alias_terms(struct perf_pmu_alias *alias, struct list_head *terms
>  		 * which we don't want for implicit terms in aliases.
>  		 */
>  		cloned->weak = true;
> +		cloned->err_term = cloned->err_val = err_loc;
>  		list_add_tail(&cloned->list, &clone_terms.terms);
>  	}
>  	list_splice_init(&clone_terms.terms, terms);
> @@ -1363,8 +1364,8 @@ static int pmu_config_term(const struct perf_pmu *pmu,
>  
>  			parse_events_error__handle(err, term->err_val,
>  				asprintf(&err_str,
> -				    "value too big for format, maximum is %llu",
> -				    (unsigned long long)max_val) < 0
> +				    "value too big for format (%s), maximum is %llu",
> +				    format->name, (unsigned long long)max_val) < 0
>  				    ? strdup("value too big for format")
>  				    : err_str,
>  				    NULL);
> @@ -1518,7 +1519,7 @@ int perf_pmu__check_alias(struct perf_pmu *pmu, struct parse_events_terms *head_
>  		alias = pmu_find_alias(pmu, term);
>  		if (!alias)
>  			continue;
> -		ret = pmu_alias_terms(alias, &term->list);
> +		ret = pmu_alias_terms(alias, term->err_term, &term->list);
>  		if (ret) {
>  			parse_events_error__handle(err, term->err_term,
>  						strdup("Failure to duplicate terms"),