lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fVATGaoeaSEk5jjoGDY=pJkFThU2t2sixfwjouxisor=w@mail.gmail.com>
Date:   Mon, 4 Sep 2023 07:56:25 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     James Clark <james.clark@....com>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        "coresight@...ts.linaro.org" <coresight@...ts.linaro.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Eduard Zingerman <eddyz87@...il.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Rob Herring <robh@...nel.org>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        bpf@...r.kernel.org, Wang Nan <wangnan0@...wei.com>,
        Wang ShaoBo <bobo.shaobowang@...wei.com>,
        YueHaibing <yuehaibing@...wei.com>,
        He Kuang <hekuang@...wei.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH v1 2/3] perf tools: Revert enable indices setting syntax
 for BPF map

On Mon, Sep 4, 2023 at 4:02 AM James Clark <james.clark@....com> wrote:
>
>
>
> On 28/07/2023 01:12, Ian Rogers wrote:
> > This reverts commit e571e029bdbf ("perf tools: Enable indices setting
> > syntax for BPF map").
> >
> > The reverted commit added a notion of arrays that could be set as
> > event terms for BPF events. The parsing hasn't worked over multiple
> > Linux releases. Given the broken nature of the parsing it appears the
> > code isn't in use, nor could I find a way for it to be used to add a
> > test.
> >
> > The original commit contains a test in the commit message,
> > however, running it yields:
> > ```
> > $ perf record -e './test_bpf_map_3.c/map:channel.value[0,1,2,3...5]=101/' usleep 2
> > event syntax error: '..pf_map_3.c/map:channel.value[0,1,2,3...5]=101/'
> >                                   \___ parser error
> > Run 'perf list' for a list of valid events
> >
> >  Usage: perf record [<options>] [<command>]
> >     or: perf record [<options>] -- <command> [<options>]
> >
> >     -e, --event <event>   event selector. use 'perf list' to list available events
> > ```
> >
> > Given the code can't be used this commit reverts and removes it.
> >
>
> Hi Ian,
>
> Unfortunately this revert breaks Coresight sink argument parsing.
>
> Before:
>
>   $ perf record -e cs_etm/@..._etr0/ -- true
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 4.008 MB perf.data ]
>
> After:
>
>   $ perf record -e cs_etm/@..._etr0/ -- true
>   event syntax error: 'cs_etm/@..._etr0/'
>                            \___ parser error
>
> I can't really see how it's related to the array syntax that the commit
> messages mention, but it could either be that the revert wasn't applied
> cleanly or just some unintended side effect.
>
> We should probably add a cross platform parsing test for Coresight
> arguments, but I don't know whether we should just blindly revert the
> revert for now, or work on a new change that explicitly fixes the
> Coresight case.

Agreed, I'll take a look. Any chance you could post the full error
message? I suspect there's a first error hiding in there too.

Thanks,
Ian

> Thanks
> James
>
> > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > ---
> >  tools/perf/util/parse-events.c |   8 +--
> >  tools/perf/util/parse-events.l |  11 ---
> >  tools/perf/util/parse-events.y | 122 ---------------------------------
> >  3 files changed, 1 insertion(+), 140 deletions(-)
> >
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 02647313c918..0e2004511cf5 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -800,13 +800,7 @@ parse_events_config_bpf(struct parse_events_state *parse_state,
> >
> >                       parse_events_error__handle(parse_state->error, idx,
> >                                               strdup(errbuf),
> > -                                             strdup(
> > -"Hint:\tValid config terms:\n"
> > -"     \tmap:[<arraymap>].value<indices>=[value]\n"
> > -"     \tmap:[<eventmap>].event<indices>=[event]\n"
> > -"\n"
> > -"     \twhere <indices> is something like [0,3...5] or [all]\n"
> > -"     \t(add -v to see detail)"));
> > +                                             NULL);
> >                       return err;
> >               }
> >       }
> > diff --git a/tools/perf/util/parse-events.l b/tools/perf/util/parse-events.l
> > index 99335ec586ae..d7d084cc4140 100644
> > --- a/tools/perf/util/parse-events.l
> > +++ b/tools/perf/util/parse-events.l
> > @@ -175,7 +175,6 @@ do {                                                      \
> >  %x mem
> >  %s config
> >  %x event
> > -%x array
> >
> >  group                [^,{}/]*[{][^}]*[}][^,{}/]*
> >  event_pmu    [^,{}/]+[/][^/]*[/][^,{}/]*
> > @@ -251,14 +250,6 @@ non_digit        [^0-9]
> >               }
> >  }
> >
> > -<array>{
> > -"]"                  { BEGIN(config); return ']'; }
> > -{num_dec}            { return value(yyscanner, 10); }
> > -{num_hex}            { return value(yyscanner, 16); }
> > -,                    { return ','; }
> > -"\.\.\."             { return PE_ARRAY_RANGE; }
> > -}
> > -
> >  <config>{
> >       /*
> >        * Please update config_term_names when new static term is added.
> > @@ -302,8 +293,6 @@ r0x{num_raw_hex}  { return str(yyscanner, PE_RAW); }
> >  {lc_type}-{lc_op_result}     { return lc_str(yyscanner, _parse_state); }
> >  {lc_type}-{lc_op_result}-{lc_op_result}      { return lc_str(yyscanner, _parse_state); }
> >  {name_minus}         { return str(yyscanner, PE_NAME); }
> > -\[all\]                      { return PE_ARRAY_ALL; }
> > -"["                  { BEGIN(array); return '['; }
> >  @{drv_cfg_term}              { return drv_str(yyscanner, PE_DRV_CFG_TERM); }
> >  }
> >
> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > index 454577f7aff6..5a90e7874c59 100644
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -64,7 +64,6 @@ static void free_list_evsel(struct list_head* list_evsel)
> >  %token PE_LEGACY_CACHE
> >  %token PE_PREFIX_MEM
> >  %token PE_ERROR
> > -%token PE_ARRAY_ALL PE_ARRAY_RANGE
> >  %token PE_DRV_CFG_TERM
> >  %token PE_TERM_HW
> >  %type <num> PE_VALUE
> > @@ -108,11 +107,6 @@ static void free_list_evsel(struct list_head* list_evsel)
> >  %type <list_evsel> groups
> >  %destructor { free_list_evsel ($$); } <list_evsel>
> >  %type <tracepoint_name> tracepoint_name
> > -%destructor { free ($$.sys); free ($$.event); } <tracepoint_name>
> > -%type <array> array
> > -%type <array> array_term
> > -%type <array> array_terms
> > -%destructor { free ($$.ranges); } <array>
> >  %type <hardware_term> PE_TERM_HW
> >  %destructor { free ($$.str); } <hardware_term>
> >
> > @@ -127,7 +121,6 @@ static void free_list_evsel(struct list_head* list_evsel)
> >               char *sys;
> >               char *event;
> >       } tracepoint_name;
> > -     struct parse_events_array array;
> >       struct hardware_term {
> >               char *str;
> >               u64 num;
> > @@ -878,121 +871,6 @@ PE_TERM
> >
> >       $$ = term;
> >  }
> > -|
> > -name_or_raw array '=' name_or_legacy
> > -{
> > -     struct parse_events_term *term;
> > -     int err = parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_USER, $1, $4, &@1, &@4);
> > -
> > -     if (err) {
> > -             free($1);
> > -             free($4);
> > -             free($2.ranges);
> > -             PE_ABORT(err);
> > -     }
> > -     term->array = $2;
> > -     $$ = term;
> > -}
> > -|
> > -name_or_raw array '=' PE_VALUE
> > -{
> > -     struct parse_events_term *term;
> > -     int err = parse_events_term__num(&term, PARSE_EVENTS__TERM_TYPE_USER, $1, $4, false, &@1, &@4);
> > -
> > -     if (err) {
> > -             free($1);
> > -             free($2.ranges);
> > -             PE_ABORT(err);
> > -     }
> > -     term->array = $2;
> > -     $$ = term;
> > -}
> > -|
> > -PE_DRV_CFG_TERM
> > -{
> > -     struct parse_events_term *term;
> > -     char *config = strdup($1);
> > -     int err;
> > -
> > -     if (!config)
> > -             YYNOMEM;
> > -     err = parse_events_term__str(&term, PARSE_EVENTS__TERM_TYPE_DRV_CFG, config, $1, &@1, NULL);
> > -     if (err) {
> > -             free($1);
> > -             free(config);
> > -             PE_ABORT(err);
> > -     }
> > -     $$ = term;
> > -}
> > -
> > -array:
> > -'[' array_terms ']'
> > -{
> > -     $$ = $2;
> > -}
> > -|
> > -PE_ARRAY_ALL
> > -{
> > -     $$.nr_ranges = 0;
> > -     $$.ranges = NULL;
> > -}
> > -
> > -array_terms:
> > -array_terms ',' array_term
> > -{
> > -     struct parse_events_array new_array;
> > -
> > -     new_array.nr_ranges = $1.nr_ranges + $3.nr_ranges;
> > -     new_array.ranges = realloc($1.ranges,
> > -                             sizeof(new_array.ranges[0]) *
> > -                             new_array.nr_ranges);
> > -     if (!new_array.ranges)
> > -             YYNOMEM;
> > -     memcpy(&new_array.ranges[$1.nr_ranges], $3.ranges,
> > -            $3.nr_ranges * sizeof(new_array.ranges[0]));
> > -     free($3.ranges);
> > -     $$ = new_array;
> > -}
> > -|
> > -array_term
> > -
> > -array_term:
> > -PE_VALUE
> > -{
> > -     struct parse_events_array array;
> > -
> > -     array.nr_ranges = 1;
> > -     array.ranges = malloc(sizeof(array.ranges[0]));
> > -     if (!array.ranges)
> > -             YYNOMEM;
> > -     array.ranges[0].start = $1;
> > -     array.ranges[0].length = 1;
> > -     $$ = array;
> > -}
> > -|
> > -PE_VALUE PE_ARRAY_RANGE PE_VALUE
> > -{
> > -     struct parse_events_array array;
> > -
> > -     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)
> > -             YYNOMEM;
> > -     array.ranges[0].start = $1;
> > -     array.ranges[0].length = $3 - $1 + 1;
> > -     $$ = array;
> > -}
> >
> >  sep_dc: ':' |
> >

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ