On Tue, May 12, 2020 at 11:50:18PM -0700, Ian Rogers wrote:
> On Mon, May 11, 2020 at 1:54 PM Jiri Olsa <jo...@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-irog...@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-irog...@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-irog...@google.com/
oops, right.. thanks
jirka
