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

[James Clark]

On 31/01/2024 06:30, 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>
> ---
> 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);

Hi Ian,

I went to test this, but since b30d4f0b6954 ("perf parse-events:
Additional error reporting") I don't get this size error message
anymore, just a "bad event/PMU not found" type error. I'm not sure if
this is something Arm specific, or you're seeing the same thing?

Before b30d4f0b6954:

  $ perf record -e bus_access_rd/long=2
  event syntax error: '..ss_rd/long=2/'
                                  \___ value too big for format, maximum
                                       is 1

  Initial error:
  event syntax error: 'bus_access_rd/long=2/'
                     \___ Cannot find PMU `bus_access_rd'. Missing
                          kernel support?
  Run 'perf list' for a list of valid events

   Usage: perf record [<options>] [<command>]
    or: perf record [<options>] -- <command> [<options>]

    -e, --event <event>   event selector. use 'perf list' to list
      available events

After b30d4f0b6954:

  $ perf record -e bus_access_rd/long=2
  event syntax error: '..ss_rd/long=2/'
                                  \___ Bad event or PMU

  Unabled to find PMU or event on a PMU of 'bus_access_rd'

  Initial error:
  event syntax error: 'bus_access_rd/long=2/'
                     \___ Cannot find PMU `bus_access_rd'. Missing
                          kernel support?

  Run 'perf list' for a list of valid events

   Usage: perf record [<options>] [<command>]
    or: perf record [<options>] -- <command> [<options>]

    -e, --event <event>   event selector. use 'perf list' to list
      available events