[PATCH v2 01/12] perf parse-events: Refactor get_config_terms() to remove macros
James Clark
james.clark at linaro.org
Tue Dec 9 04:48:29 PST 2025
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 at linaro.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
> }
> }
> 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;
> }
>
>
More information about the linux-arm-kernel
mailing list