Re: [PATCH 1/4] perf expr: Add parsing support for multiple expressions

[Jiri Olsa <jolsa@redhat.com>]

On Tue, May 12, 2020 at 11:50:18PM -0700, Ian Rogers wrote:
> On Mon, May 11, 2020 at 1:54 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding support to parse metric difinitions in following form:
> 
> Typo on definitions.

right

SNIP

> > +int expr__parse_custom(struct expr_parse_ctx *ctx, const char *expr)
> > +{
> > +       return __expr__parse(NULL, ctx, expr, EXPR_CUSTOM, 0);
> > +}
> > +
> >  static bool
> >  already_seen(const char *val, const char *one, const char **other,
> >              int num_other)
> > diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> > index 40fc452b0f2b..ef116b58a5d4 100644
> > --- a/tools/perf/util/expr.h
> > +++ b/tools/perf/util/expr.h
> > @@ -4,15 +4,29 @@
> >
> >  #define EXPR_MAX_OTHER 64
> >  #define MAX_PARSE_ID EXPR_MAX_OTHER
> > +#define EXPR_MAX 20
> 
> Currently deduplication of ids is done after rather than during
> expression pasing, meaning hitting these limits is quite easy. This is
> fixed in:
> https://lore.kernel.org/lkml/20200508053629.210324-8-irogers@google.com/
> But not for custom expressions being added here. I plan to rebase that
> work and clone hashmap from libbpf into libapi to workaround the
> dependency issue.
> That patch also adds expr__ctx_clear as a convenience for cleaning up
> the context, and passes the context around inside of metricgroup
> rather than ids.

ok

SNIP

> >  symbol         ({spec}|{sym})+
> > +all            [^;]+
> >
> >  %%
> >         struct expr_scanner_ctx *sctx = expr_get_extra(yyscanner);
> > @@ -100,6 +103,12 @@ symbol             ({spec}|{sym})+
> >                 }
> >         }
> >
> > +<custom>{
> > +
> > +{all}          { BEGIN(INITIAL); return str(yyscanner, ALL, sctx->runtime); }
> > +
> > +}
> > +
> >  max            { return MAX; }
> >  min            { return MIN; }
> >  if             { return IF; }
> > @@ -118,6 +127,9 @@ else                { return ELSE; }
> >  "("            { return '('; }
> >  ")"            { return ')'; }
> >  ","            { return ','; }
> > +";"            { return ';'; }
> > +"="            { BEGIN(custom); return '='; }
> 
> Will this interfere with the \\= encoded in MetricExpr? Could you test with:
> https://lore.kernel.org/lkml/20200513062752.3681-2-irogers@google.com/

it shouldn't, the escape is matched first.. but I'll put it on top
of the new tests to be sure

> 
> > +\n             { }
> >  .              { }
> >  %%
> >
> > diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> > index 21e82a1e11a2..0521e48fa5e3 100644
> > --- a/tools/perf/util/expr.y
> > +++ b/tools/perf/util/expr.y
> > @@ -24,9 +24,10 @@
> >         char    *str;
> >  }
> >
> > -%token EXPR_PARSE EXPR_OTHER EXPR_ERROR
> > +%token EXPR_PARSE EXPR_OTHER EXPR_CUSTOM EXPR_ERROR
> >  %token <num> NUMBER
> >  %token <str> ID
> > +%token <str> ALL
> 
> Missing %destructor, fix is here:
> https://lore.kernel.org/lkml/20200513000318.15166-1-irogers@google.com/

oops, right.. thanks

jirka