Prefer informative messages rather than none with ABORT_ON. Document
one failure mode and add an error message for another.
Signed-off-by: Ian Rogers <[email protected]>
---
tools/perf/util/parse-events.y | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
index 844646752462..454577f7aff6 100644
--- a/tools/perf/util/parse-events.y
+++ b/tools/perf/util/parse-events.y
@@ -22,12 +22,6 @@
void parse_events_error(YYLTYPE *loc, void *parse_state, void *scanner, char const *msg);
-#define ABORT_ON(val) \
-do { \
- if (val) \
- YYABORT; \
-} while (0)
-
#define PE_ABORT(val) \
do { \
if (val == -ENOMEM) \
@@ -618,7 +612,9 @@ PE_RAW opt_event_config
YYNOMEM;
errno = 0;
num = strtoull($1 + 1, NULL, 16);
- ABORT_ON(errno);
+ /* Given the lexer will only give [a-fA-F0-9]+ a failure here should be impossible. */
+ if (errno)
+ YYABORT;
free($1);
err = parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, num, $2,
/*wildcard=*/false);
@@ -978,7 +974,17 @@ PE_VALUE PE_ARRAY_RANGE PE_VALUE
{
struct parse_events_array array;
- ABORT_ON($3 < $1);
+ if ($3 < $1) {
+ struct parse_events_state *parse_state = _parse_state;
+ struct parse_events_error *error = parse_state->error;
+ char *err_str;
+
+ if (asprintf(&err_str, "Expected '%ld' to be less-than '%ld'", $3, $1) < 0)
+ err_str = NULL;
+
+ parse_events_error__handle(error, @1.first_column, err_str, NULL);
+ YYABORT;
+ }
array.nr_ranges = 1;
array.ranges = malloc(sizeof(array.ranges[0]));
if (!array.ranges)
--
2.41.0.162.gfafddb0af9-goog
On Tue, Jun 27, 2023 at 11:11 AM Ian Rogers <[email protected]> wrote:
>
> Prefer informative messages rather than none with ABORT_ON. Document
> one failure mode and add an error message for another.
>
> Signed-off-by: Ian Rogers <[email protected]>
> ---
> tools/perf/util/parse-events.y | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> index 844646752462..454577f7aff6 100644
> --- a/tools/perf/util/parse-events.y
> +++ b/tools/perf/util/parse-events.y
> @@ -22,12 +22,6 @@
>
> void parse_events_error(YYLTYPE *loc, void *parse_state, void *scanner, char const *msg);
>
> -#define ABORT_ON(val) \
> -do { \
> - if (val) \
> - YYABORT; \
> -} while (0)
> -
> #define PE_ABORT(val) \
> do { \
> if (val == -ENOMEM) \
> @@ -618,7 +612,9 @@ PE_RAW opt_event_config
> YYNOMEM;
> errno = 0;
> num = strtoull($1 + 1, NULL, 16);
> - ABORT_ON(errno);
> + /* Given the lexer will only give [a-fA-F0-9]+ a failure here should be impossible. */
> + if (errno)
> + YYABORT;
> free($1);
> err = parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, num, $2,
> /*wildcard=*/false);
> @@ -978,7 +974,17 @@ PE_VALUE PE_ARRAY_RANGE PE_VALUE
> {
> struct parse_events_array array;
>
> - ABORT_ON($3 < $1);
> + if ($3 < $1) {
> + struct parse_events_state *parse_state = _parse_state;
> + struct parse_events_error *error = parse_state->error;
> + char *err_str;
> +
> + if (asprintf(&err_str, "Expected '%ld' to be less-than '%ld'", $3, $1) < 0)
Isn't it to be "greater-than or equal" ?
Thanks,
Namhyung
> + err_str = NULL;
> +
> + parse_events_error__handle(error, @1.first_column, err_str, NULL);
> + YYABORT;
> + }
> array.nr_ranges = 1;
> array.ranges = malloc(sizeof(array.ranges[0]));
> if (!array.ranges)
> --
> 2.41.0.162.gfafddb0af9-goog
>
On Thu, Jun 29, 2023 at 2:49 PM Namhyung Kim <[email protected]> wrote:
>
> On Tue, Jun 27, 2023 at 11:11 AM Ian Rogers <[email protected]> wrote:
> >
> > Prefer informative messages rather than none with ABORT_ON. Document
> > one failure mode and add an error message for another.
> >
> > Signed-off-by: Ian Rogers <[email protected]>
> > ---
> > tools/perf/util/parse-events.y | 22 ++++++++++++++--------
> > 1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > index 844646752462..454577f7aff6 100644
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -22,12 +22,6 @@
> >
> > void parse_events_error(YYLTYPE *loc, void *parse_state, void *scanner, char const *msg);
> >
> > -#define ABORT_ON(val) \
> > -do { \
> > - if (val) \
> > - YYABORT; \
> > -} while (0)
> > -
> > #define PE_ABORT(val) \
> > do { \
> > if (val == -ENOMEM) \
> > @@ -618,7 +612,9 @@ PE_RAW opt_event_config
> > YYNOMEM;
> > errno = 0;
> > num = strtoull($1 + 1, NULL, 16);
> > - ABORT_ON(errno);
> > + /* Given the lexer will only give [a-fA-F0-9]+ a failure here should be impossible. */
> > + if (errno)
> > + YYABORT;
> > free($1);
> > err = parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, num, $2,
> > /*wildcard=*/false);
> > @@ -978,7 +974,17 @@ PE_VALUE PE_ARRAY_RANGE PE_VALUE
> > {
> > struct parse_events_array array;
> >
> > - ABORT_ON($3 < $1);
> > + if ($3 < $1) {
> > + struct parse_events_state *parse_state = _parse_state;
> > + struct parse_events_error *error = parse_state->error;
> > + char *err_str;
> > +
> > + if (asprintf(&err_str, "Expected '%ld' to be less-than '%ld'", $3, $1) < 0)
>
> Isn't it to be "greater-than or equal" ?
I think the order is right. From the man page:
When successful, these functions return the number of bytes printed,
just like sprintf(3). If memory allocation wasn't possible, or some
other error occurs, these functions will return -1, and the contents of
strp are undefined.
So here we need to catch -1 and ensure strp (&err_str) is NULL before
passing it to parse_events_error__handle.
Thanks,
Ian
> Thanks,
> Namhyung
>
>
> > + err_str = NULL;
> > +
> > + parse_events_error__handle(error, @1.first_column, err_str, NULL);
> > + YYABORT;
> > + }
> > array.nr_ranges = 1;
> > array.ranges = malloc(sizeof(array.ranges[0]));
> > if (!array.ranges)
> > --
> > 2.41.0.162.gfafddb0af9-goog
> >
On Fri, Jun 30, 2023 at 9:56 AM Markus Elfring <[email protected]> wrote:
>
> > Prefer informative messages rather than none with ABORT_ON. Document
> > one failure mode and add an error message for another.
>
> Does such a wording really fit to the known requirement “Solve only one problem per patch.”?
>
> See also:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n81
Sorry your explanation isn't clear. Please can you elaborate.
Thanks,
Ian
>
> Regards,
> Markus
On Fri, Jun 30, 2023 at 8:14 AM Ian Rogers <[email protected]> wrote:
>
> On Thu, Jun 29, 2023 at 2:49 PM Namhyung Kim <[email protected]> wrote:
> >
> > On Tue, Jun 27, 2023 at 11:11 AM Ian Rogers <[email protected]> wrote:
> > >
> > > Prefer informative messages rather than none with ABORT_ON. Document
> > > one failure mode and add an error message for another.
> > >
> > > Signed-off-by: Ian Rogers <[email protected]>
> > > ---
> > > tools/perf/util/parse-events.y | 22 ++++++++++++++--------
> > > 1 file changed, 14 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > > index 844646752462..454577f7aff6 100644
> > > --- a/tools/perf/util/parse-events.y
> > > +++ b/tools/perf/util/parse-events.y
> > > @@ -22,12 +22,6 @@
> > >
> > > void parse_events_error(YYLTYPE *loc, void *parse_state, void *scanner, char const *msg);
> > >
> > > -#define ABORT_ON(val) \
> > > -do { \
> > > - if (val) \
> > > - YYABORT; \
> > > -} while (0)
> > > -
> > > #define PE_ABORT(val) \
> > > do { \
> > > if (val == -ENOMEM) \
> > > @@ -618,7 +612,9 @@ PE_RAW opt_event_config
> > > YYNOMEM;
> > > errno = 0;
> > > num = strtoull($1 + 1, NULL, 16);
> > > - ABORT_ON(errno);
> > > + /* Given the lexer will only give [a-fA-F0-9]+ a failure here should be impossible. */
> > > + if (errno)
> > > + YYABORT;
> > > free($1);
> > > err = parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, num, $2,
> > > /*wildcard=*/false);
> > > @@ -978,7 +974,17 @@ PE_VALUE PE_ARRAY_RANGE PE_VALUE
> > > {
> > > struct parse_events_array array;
> > >
> > > - ABORT_ON($3 < $1);
> > > + if ($3 < $1) {
> > > + struct parse_events_state *parse_state = _parse_state;
> > > + struct parse_events_error *error = parse_state->error;
> > > + char *err_str;
> > > +
> > > + if (asprintf(&err_str, "Expected '%ld' to be less-than '%ld'", $3, $1) < 0)
> >
> > Isn't it to be "greater-than or equal" ?
>
> I think the order is right. From the man page:
>
> When successful, these functions return the number of bytes printed,
> just like sprintf(3). If memory allocation wasn't possible, or some
> other error occurs, these functions will return -1, and the contents of
> strp are undefined.
>
> So here we need to catch -1 and ensure strp (&err_str) is NULL before
> passing it to parse_events_error__handle.
Oh, I meant the message not the condition in the if statement.
It seems it aborts if $3 < $1, then it expects $3 >= $1 in the
normal condition, right?
Thanks,
Namhyung
On Sat, Jul 1, 2023 at 11:43 AM Namhyung Kim <[email protected]> wrote:
>
> On Fri, Jun 30, 2023 at 8:14 AM Ian Rogers <[email protected]> wrote:
> >
> > On Thu, Jun 29, 2023 at 2:49 PM Namhyung Kim <[email protected]> wrote:
> > >
> > > On Tue, Jun 27, 2023 at 11:11 AM Ian Rogers <[email protected]> wrote:
> > > >
> > > > Prefer informative messages rather than none with ABORT_ON. Document
> > > > one failure mode and add an error message for another.
> > > >
> > > > Signed-off-by: Ian Rogers <[email protected]>
> > > > ---
> > > > tools/perf/util/parse-events.y | 22 ++++++++++++++--------
> > > > 1 file changed, 14 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > > > index 844646752462..454577f7aff6 100644
> > > > --- a/tools/perf/util/parse-events.y
> > > > +++ b/tools/perf/util/parse-events.y
> > > > @@ -22,12 +22,6 @@
> > > >
> > > > void parse_events_error(YYLTYPE *loc, void *parse_state, void *scanner, char const *msg);
> > > >
> > > > -#define ABORT_ON(val) \
> > > > -do { \
> > > > - if (val) \
> > > > - YYABORT; \
> > > > -} while (0)
> > > > -
> > > > #define PE_ABORT(val) \
> > > > do { \
> > > > if (val == -ENOMEM) \
> > > > @@ -618,7 +612,9 @@ PE_RAW opt_event_config
> > > > YYNOMEM;
> > > > errno = 0;
> > > > num = strtoull($1 + 1, NULL, 16);
> > > > - ABORT_ON(errno);
> > > > + /* Given the lexer will only give [a-fA-F0-9]+ a failure here should be impossible. */
> > > > + if (errno)
> > > > + YYABORT;
> > > > free($1);
> > > > err = parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, num, $2,
> > > > /*wildcard=*/false);
> > > > @@ -978,7 +974,17 @@ PE_VALUE PE_ARRAY_RANGE PE_VALUE
> > > > {
> > > > struct parse_events_array array;
> > > >
> > > > - ABORT_ON($3 < $1);
> > > > + if ($3 < $1) {
> > > > + struct parse_events_state *parse_state = _parse_state;
> > > > + struct parse_events_error *error = parse_state->error;
> > > > + char *err_str;
> > > > +
> > > > + if (asprintf(&err_str, "Expected '%ld' to be less-than '%ld'", $3, $1) < 0)
> > >
> > > Isn't it to be "greater-than or equal" ?
> >
> > I think the order is right. From the man page:
> >
> > When successful, these functions return the number of bytes printed,
> > just like sprintf(3). If memory allocation wasn't possible, or some
> > other error occurs, these functions will return -1, and the contents of
> > strp are undefined.
> >
> > So here we need to catch -1 and ensure strp (&err_str) is NULL before
> > passing it to parse_events_error__handle.
>
> Oh, I meant the message not the condition in the if statement.
> It seems it aborts if $3 < $1, then it expects $3 >= $1 in the
> normal condition, right?
In the old code with the macro expanded it did:
if ($3 < $1) YYABORT
in the new code it fills in parse_state->error if the same error
condition applies. The change is to get rid of the macro and add an
error message. The asprintf is just added to make the error message
more informative.
Thanks,
Ian
> Thanks,
> Namhyung