| 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
| ||
|
Message-ID: <CAP-5=fV_Y5W2RyJhBgnk49JuTf8EsyisY-LfL3sVhC4TVRC_gg@mail.gmail.com>
Date: Tue, 9 Dec 2025 07:58:31 -0800
From: Ian Rogers <irogers@...gle.com>
To: James Clark <james.clark@...aro.org>
Cc: Linux perf Profiling <linux-perf-users@...r.kernel.org>, linux-kernel@...r.kernel.org,
coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>, Suzuki K Poulose <suzuki.poulose@....com>,
Mike Leach <mike.leach@...aro.org>, John Garry <john.g.garry@...cle.com>,
Will Deacon <will@...nel.org>, Leo Yan <leo.yan@...ux.dev>, leo.yan@....com
Subject: Re: [PATCH v2 01/12] perf parse-events: Refactor get_config_terms()
to remove macros
On Tue, Dec 9, 2025 at 4:48 AM James Clark <james.clark@...aro.org> wrote:
>
> On 08/12/2025 2:22 pm, James Clark wrote:
> > The ADD_CONFIG_TERM() macros build the __type argument out of a partial
> > EVSEL__CONFIG_TERM_x enum name. This means that they can't be called
> > from a function where __type is a variable and it's also impossible to
> > grep the codebase to find usages of these enums as they're never typed
> > in full.
> >
> > Fix this by removing the macros and replacing them with an
> > add_config_term() function. It seems the main reason these existed in
> > the first place was to avoid type punning and to write to a specific
> > field in the union, but the same thing can be achieved with a single
> > write to a u64 'val' field.
> >
> > Signed-off-by: James Clark <james.clark@...aro.org>
> > ---
> > tools/perf/util/evsel_config.h | 1 +
> > tools/perf/util/parse-events.c | 146 ++++++++++++++++++++++++-----------------
> > 2 files changed, 86 insertions(+), 61 deletions(-)
> >
> > diff --git a/tools/perf/util/evsel_config.h b/tools/perf/util/evsel_config.h
> > index bcd3a978f0c4..685fd8d5c4a8 100644
> > --- a/tools/perf/util/evsel_config.h
> > +++ b/tools/perf/util/evsel_config.h
> > @@ -50,6 +50,7 @@ struct evsel_config_term {
> > u64 cfg_chg;
> > char *str;
> > int cpu;
> > + u64 val;
> > } val;
> > bool weak;
> > };
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 17c1c36a7bf9..d5b009b4ebab 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1116,105 +1116,107 @@ static int config_attr(struct perf_event_attr *attr,
> > return 0;
> > }
> >
> > -static int get_config_terms(const struct parse_events_terms *head_config,
> > - struct list_head *head_terms)
> > +static struct evsel_config_term *add_config_term(enum evsel_term_type type,
> > + struct list_head *head_terms,
> > + bool weak)
> > {
> > -#define ADD_CONFIG_TERM(__type, __weak) \
> > - struct evsel_config_term *__t; \
> > - \
> > - __t = zalloc(sizeof(*__t)); \
> > - if (!__t) \
> > - return -ENOMEM; \
> > - \
> > - INIT_LIST_HEAD(&__t->list); \
> > - __t->type = EVSEL__CONFIG_TERM_ ## __type; \
> > - __t->weak = __weak; \
> > - list_add_tail(&__t->list, head_terms)
> > -
> > -#define ADD_CONFIG_TERM_VAL(__type, __name, __val, __weak) \
> > -do { \
> > - ADD_CONFIG_TERM(__type, __weak); \
> > - __t->val.__name = __val; \
> > -} while (0)
> > + struct evsel_config_term *t;
> >
> > -#define ADD_CONFIG_TERM_STR(__type, __val, __weak) \
> > -do { \
> > - ADD_CONFIG_TERM(__type, __weak); \
> > - __t->val.str = strdup(__val); \
> > - if (!__t->val.str) { \
> > - zfree(&__t); \
> > - return -ENOMEM; \
> > - } \
> > - __t->free_str = true; \
> > -} while (0)
> > + t = zalloc(sizeof(*t));
> > + if (!t)
> > + return NULL;
> > +
> > + INIT_LIST_HEAD(&t->list);
> > + t->type = type;
> > + t->weak = weak;
> > + list_add_tail(&t->list, head_terms);
> >
> > + return t;
> > +}
> > +
> > +static int get_config_terms(const struct parse_events_terms *head_config,
> > + struct list_head *head_terms)
> > +{
> > struct parse_events_term *term;
> >
> > list_for_each_entry(term, &head_config->terms, list) {
> > + struct evsel_config_term *new_term;
> > + enum evsel_term_type new_type;
> > + char *str = NULL;
> > + u64 val;
> > +
> > switch (term->type_term) {
> > case PARSE_EVENTS__TERM_TYPE_SAMPLE_PERIOD:
> > - ADD_CONFIG_TERM_VAL(PERIOD, period, term->val.num, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_PERIOD;
> > + val = term->val.num;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_SAMPLE_FREQ:
> > - ADD_CONFIG_TERM_VAL(FREQ, freq, term->val.num, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_FREQ;
> > + val = term->val.num;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_TIME:
> > - ADD_CONFIG_TERM_VAL(TIME, time, term->val.num, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_TIME;
> > + val = term->val.num;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_CALLGRAPH:
> > - ADD_CONFIG_TERM_STR(CALLGRAPH, term->val.str, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_CALLGRAPH;
> > + str = term->val.str;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_BRANCH_SAMPLE_TYPE:
> > - ADD_CONFIG_TERM_STR(BRANCH, term->val.str, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_BRANCH;
> > + str = term->val.str;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_STACKSIZE:
> > - ADD_CONFIG_TERM_VAL(STACK_USER, stack_user,
> > - term->val.num, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_STACK_USER;
> > + val = term->val.num;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_INHERIT:
> > - ADD_CONFIG_TERM_VAL(INHERIT, inherit,
> > - term->val.num ? 1 : 0, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_INHERIT;
> > + val = term->val.num ? 1 : 0;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_NOINHERIT:
> > - ADD_CONFIG_TERM_VAL(INHERIT, inherit,
> > - term->val.num ? 0 : 1, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_INHERIT;
> > + val = term->val.num ? 0 : 1;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_MAX_STACK:
> > - ADD_CONFIG_TERM_VAL(MAX_STACK, max_stack,
> > - term->val.num, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_MAX_STACK;
> > + val = term->val.num;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_MAX_EVENTS:
> > - ADD_CONFIG_TERM_VAL(MAX_EVENTS, max_events,
> > - term->val.num, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_MAX_EVENTS;
> > + val = term->val.num;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_OVERWRITE:
> > - ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite,
> > - term->val.num ? 1 : 0, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_OVERWRITE;
> > + val = term->val.num ? 1 : 0;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_NOOVERWRITE:
> > - ADD_CONFIG_TERM_VAL(OVERWRITE, overwrite,
> > - term->val.num ? 0 : 1, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_OVERWRITE;
> > + val = term->val.num ? 0 : 1;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_DRV_CFG:
> > - ADD_CONFIG_TERM_STR(DRV_CFG, term->val.str, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_DRV_CFG;
> > + str = term->val.str;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_PERCORE:
> > - ADD_CONFIG_TERM_VAL(PERCORE, percore,
> > - term->val.num ? true : false, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_PERCORE;
> > + val = term->val.num ? true : false;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_AUX_OUTPUT:
> > - ADD_CONFIG_TERM_VAL(AUX_OUTPUT, aux_output,
> > - term->val.num ? 1 : 0, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_AUX_OUTPUT;
> > + val = term->val.num ? 1 : 0;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_AUX_ACTION:
> > - ADD_CONFIG_TERM_STR(AUX_ACTION, term->val.str, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_AUX_ACTION;
> > + str = term->val.str;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_AUX_SAMPLE_SIZE:
> > - ADD_CONFIG_TERM_VAL(AUX_SAMPLE_SIZE, aux_sample_size,
> > - term->val.num, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_AUX_SAMPLE_SIZE;
> > + val = term->val.num;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_RATIO_TO_PREV:
> > - ADD_CONFIG_TERM_STR(RATIO_TO_PREV, term->val.str, term->weak);
> > + new_type = EVSEL__CONFIG_TERM_RATIO_TO_PREV;
> > + str = term->val.str;
> > break;
> > case PARSE_EVENTS__TERM_TYPE_USER:
> > case PARSE_EVENTS__TERM_TYPE_CONFIG:
> > @@ -1229,7 +1231,23 @@ do { \
> > case PARSE_EVENTS__TERM_TYPE_RAW:
> > case PARSE_EVENTS__TERM_TYPE_CPU:
> > default:
> > - break;
> > + /* Don't add a new term for these ones */
> > + continue;
> > + }
> > +
> > + new_term = add_config_term(new_type, head_terms, term->weak);
> > + if (!new_term)
> > + return -ENOMEM;
> > +
> > + if (str) {
> > + new_term->val.str = strdup(str);
> > + if (!new_term->val.str) {
> > + zfree(&new_term);
> > + return -ENOMEM;
> > + }
> > + new_term->free_str = true;
> > + } else {
>
> This will incorrectly hit the else if term->val.str is NULL. Not sure if
> that can happen but will fix anyway.
>
> > + new_term->val.val = val;
>
> There's an uninitialized variable warning for val here on release
> builds. Will fix too
I'm not sure how I feel about the change, but did you check that it is
ubsan clean? I worry it is introducing writes to "val" and then reads
from say "period" in the evsel_config_term val union. I believe ubsan
will flag this as:
https://en.cppreference.com/w/cpp/language/union.html
"It is undefined behavior to read from the member of the union that
wasn't most recently written."
Thanks,
Ian
> > }
> > }
> > return 0;
> > @@ -1290,10 +1308,16 @@ static int get_config_chgs(struct perf_pmu *pmu, struct parse_events_terms *head
> > }
> > }
> >
> > - if (bits)
> > - ADD_CONFIG_TERM_VAL(CFG_CHG, cfg_chg, bits, false);
> > + if (bits) {
> > + struct evsel_config_term *new_term;
> > +
> > + new_term = add_config_term(EVSEL__CONFIG_TERM_CFG_CHG,
> > + head_terms, false);
> > + if (!new_term)
> > + return -ENOMEM;
> > + new_term->val.cfg_chg = bits;
> > + }
> >
> > -#undef ADD_CONFIG_TERM
> > return 0;
> > }
> >
> >
>
Powered by blists - more mailing lists