Received-SPF: pass (google.com: domain of linux-kernel+bounces-150745-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <ac8835f8-0ea5-4f28-941c-aa43f0da92fd@linux.intel.com>
Date: Thu, 18 Apr 2024 16:27:33 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 11/16] perf parse-events: Improve error message for bad
 numbers
To: Ian Rogers <irogers@google.com>, Peter Zijlstra <peterz@infradead.org>,
 Ingo Molnar <mingo@redhat.com>, Arnaldo Carvalho de Melo <acme@kernel.org>,
 Namhyung Kim <namhyung@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
 Alexander Shishkin <alexander.shishkin@linux.intel.com>,
 Jiri Olsa <jolsa@kernel.org>, Adrian Hunter <adrian.hunter@intel.com>,
 James Clark <james.clark@arm.com>, linux-perf-users@vger.kernel.org,
 linux-kernel@vger.kernel.org, bpf@vger.kernel.org,
 Atish Patra <atishp@rivosinc.com>, linux-riscv@lists.infradead.org,
 Beeman Strong <beeman@rivosinc.com>
References: <20240416061533.921723-1-irogers@google.com>
 <20240416061533.921723-12-irogers@google.com>
Content-Language: en-US
From: "Liang, Kan" <kan.liang@linux.intel.com>
In-Reply-To: <20240416061533.921723-12-irogers@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


On 2024-04-16 2:15 a.m., Ian Rogers wrote:
> Use the error handler from the parse_state to give a more informative
> error message.
> 
> Before:
> ```
> $ perf stat -e 'cycles/period=99999999999999999999/' true
> event syntax error: 'cycles/period=99999999999999999999/'
>                                   \___ parser error
> Run 'perf list' for a list of valid events
> 
>  Usage: perf stat [<options>] [<command>]
> 
>     -e, --event <event>   event selector. use 'perf list' to list available events
> ```
> 
> After:
> ```
> $ perf stat -e 'cycles/period=99999999999999999999/' true
> event syntax error: 'cycles/period=99999999999999999999/'
>                                   \___ parser error
> 
> event syntax error: '..les/period=99999999999999999999/'
>                                   \___ Bad base 10 number "99999999999999999999"


It seems the patch only works for decimal?

/perf stat -e 'cycles/period=0xaaaaaaaaaaaaaaaaaaaaaa/' true
event syntax error: '..les/period=0xaaaaaaaaaaaaaaaaaaaaaa/'
                                   \___ parser error
 Run 'perf list' for a list of valid events

  Usage: perf stat [<options>] [<command>]

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

Thanks,
Kan

> Run 'perf list' for a list of valid events
> 
>  Usage: perf stat [<options>] [<command>]
> 
>     -e, --event <event>   event selector. use 'perf list' to list available events
> ```
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/parse-events.l | 40 ++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> index 6fe37003ab7b..0cd68c9f0d4f 100644
> --- a/tools/perf/util/parse-events.l
> +++ b/tools/perf/util/parse-events.l
> @@ -18,26 +18,34 @@
>  
>  char *parse_events_get_text(yyscan_t yyscanner);
>  YYSTYPE *parse_events_get_lval(yyscan_t yyscanner);
> +int parse_events_get_column(yyscan_t yyscanner);
> +int parse_events_get_leng(yyscan_t yyscanner);
>  
> -static int __value(YYSTYPE *yylval, char *str, int base, int token)
> +static int get_column(yyscan_t scanner)
>  {
> -	u64 num;
> -
> -	errno = 0;
> -	num = strtoull(str, NULL, base);
> -	if (errno)
> -		return PE_ERROR;
> -
> -	yylval->num = num;
> -	return token;
> +	return parse_events_get_column(scanner) - parse_events_get_leng(scanner);
>  }
>  
> -static int value(yyscan_t scanner, int base)
> +static int value(struct parse_events_state *parse_state, yyscan_t scanner, int base)
>  {
>  	YYSTYPE *yylval = parse_events_get_lval(scanner);
>  	char *text = parse_events_get_text(scanner);
> +	u64 num;
>  
> -	return __value(yylval, text, base, PE_VALUE);
> +	errno = 0;
> +	num = strtoull(text, NULL, base);
> +	if (errno) {
> +		struct parse_events_error *error = parse_state->error;
> +		char *help = NULL;
> +
> +		if (asprintf(&help, "Bad base %d number \"%s\"", base, text) > 0)
> +			parse_events_error__handle(error, get_column(scanner), help , NULL);
> +
> +		return PE_ERROR;
> +	}
> +
> +	yylval->num = num;
> +	return PE_VALUE;
>  }
>  
>  static int str(yyscan_t scanner, int token)
> @@ -283,8 +291,8 @@ r0x{num_raw_hex}	{ return str(yyscanner, PE_RAW); }
>  	 */
>  "/"/{digit}		{ return PE_BP_SLASH; }
>  "/"/{non_digit}		{ BEGIN(config); return '/'; }
> -{num_dec}		{ return value(yyscanner, 10); }
> -{num_hex}		{ return value(yyscanner, 16); }
> +{num_dec}		{ return value(_parse_state, yyscanner, 10); }
> +{num_hex}		{ return value(_parse_state, yyscanner, 16); }
>  	/*
>  	 * We need to separate 'mem:' scanner part, in order to get specific
>  	 * modifier bits parsed out. Otherwise we would need to handle PE_NAME
> @@ -330,8 +338,8 @@ cgroup-switches					{ return sym(yyscanner, PERF_COUNT_SW_CGROUP_SWITCHES); }
>  {lc_type}-{lc_op_result}-{lc_op_result}	{ return str(yyscanner, PE_LEGACY_CACHE); }
>  mem:			{ BEGIN(mem); return PE_PREFIX_MEM; }
>  r{num_raw_hex}		{ return str(yyscanner, PE_RAW); }
> -{num_dec}		{ return value(yyscanner, 10); }
> -{num_hex}		{ return value(yyscanner, 16); }
> +{num_dec}		{ return value(_parse_state, yyscanner, 10); }
> +{num_hex}		{ return value(_parse_state, yyscanner, 16); }
>  
>  {modifier_event}	{ return str(yyscanner, PE_MODIFIER_EVENT); }
>  {name}			{ return str(yyscanner, PE_NAME); }