On 2024-04-18 5:07 p.m., Ian Rogers wrote:
> On Thu, Apr 18, 2024 at 1:27 PM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>>
>>
>>
>> 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
>
> Right, for hexadecimal we say the number of digits is at most 16, so
> when you exceed this the token is no longer recognized. It just
> becomes input that can't be parsed, hence parser error. Doing this
> means we can simplify other strtoull checks, but I agree having a
> better error message for hexadecimal would be good. Let's do it as
> follow up.
OK. There is already a warning. It's fine to provide a follow-up for a
better error message later.
Thanks,
Kan
>
> Thanks,
> Ian
>
>>> 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@xxxxxxxxxx>
>>> ---
>>> 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); }
>
