From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f48.google.com (mail-wr1-f48.google.com [209.85.221.48]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9338A32E69A for ; Tue, 9 Dec 2025 17:55:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.48 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765302955; cv=none; b=Ag/2cW9UTIHabmwAA8SR/Z1TaUd4TXB+hSyTBnwA8HAc8h4+ew0JkTbqXAti2CPSvwYHG0wXHDYx5Dx9X57nxBFTiWrtB4qDjMqy0k9wa3ElMUTdOwvDUf6Ep02Eftj6rvpGgqNPhP788IBLGTlKF5ubE70fNtzR3vFhPkF+Iso= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765302955; c=relaxed/simple; bh=GS5/3sAnVWAW7aQ6tVZTTV2O+QddHpL6cXoTwdlo/lI=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=WqA6jCbqc7EEjiwHgT/pHMWn2AcQOTjiV/6huwe8wiZbTbjEDcl+KTB6/s964h8FZce9xy3dfJUyvmhxBFIJm8BPydeTgfVdOFnyIvqcT4xv1K0Ibnq3zi22/M5JIlYXAn/VnlYXKjGLfwUHkvG71t3QBpxbemBnQo2PgiDJzoA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=SjyRi9vJ; arc=none smtp.client-ip=209.85.221.48 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="SjyRi9vJ" Received: by mail-wr1-f48.google.com with SMTP id ffacd0b85a97d-42e2e628f8aso2828166f8f.1 for ; Tue, 09 Dec 2025 09:55:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1765302952; x=1765907752; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=ZUOgxr+7cSYt/btR8gXlm/+anfYygWBdHJEIEgIls30=; b=SjyRi9vJTV4Ncn3JYPNMxlV/fhXXKf+HLD/TsbiL2tNR6u+DZ0fMnivOrBlFF1trA4 N1xeUSOXcsoUgtMK2T2e+6F1wSGH/fu3MsDhRWR4dOIt7YjUsEK2CjYn4P5A+G525xdH 4986qG8X562t/5rXyMb9o3htMh7EAed85GzbdNQHUjfoy0kv9fdMuimYfRIZvD4bBEC2 5f/Ds0gRVYHqIujtz16OIASHPIa5tjjUsIlEr7flMmZi09awkFConqc87KPw589J3kmR hMyqDmpl3iWb6qNfQoAAcfrO/1c8YtZNVHN2A0LmfoQtiz7m8Y9QhO9EfF/mcPrsMcRy uUzg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1765302952; x=1765907752; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=ZUOgxr+7cSYt/btR8gXlm/+anfYygWBdHJEIEgIls30=; b=j4FQ4ARZGM/K4V4M1YFtk36QEWHtEmosY8zmAa9P9fZXEX5Ys1dtkZdklY+Lhwuwmp MgRrIQGELTUjGrbW0ADEPv5z7CngAiTW4FLiyVPpsgl5N/GfEBM9Sr7ZH0wwdI79ibjy 0gPZEENRUsqB/g1WkGWvJpBS+JOS8/s+uhFmz1iHcNFs0VTnu5K9dOiL6vuh23g3y78j wcyClttXcR/eFpQbNEDqwfuHQLczXrr+Ok5kOOFty+3mb/BxFA5TM/qM8DdTn/aH3ZDV yr7AmqfsoNBRTObkGlJmyy4ic2t3VnZlK8XkrGCnbeN/1RyYNkNR+H70QLzE29XGu4Tm trQg== X-Forwarded-Encrypted: i=1; AJvYcCUzGOi7Cy4LBJWj8ZhpBotXvWPo5BxBBFZtiGl2VjHYlgGDqL0cnErcoecvGmP3w523AYEisExkr7n9nAU=@vger.kernel.org X-Gm-Message-State: AOJu0YxWM9BIn3cCtMi5t2Bx0dTB4+XIu37fZUahZP8XleVSFAH18eBe d0s92yNQVjSQo2sGQ6S/tLXgOcv9vyeQ0dk3cWNkPrb+JYItfedwMJbAdPSeaCfP0NYAfkiDbU3 M3NoUrLw= X-Gm-Gg: AY/fxX4mFhjlV23iOd/7YjFhU7Q7Qlkg6nlXJK1+1BhrqKLr7I4aQt/S6zzZjeQvNPS AsnS+EzMp67qIU1dkU+ABUccugaBf7VOA3j/efk9+mylFrZ0MuSybpES3mkaxx9nNN7O9f5U77p GxsdbV89PFotF4GfXaYgHH0p3wmpzbm4U3QSswqY5AGsPzn9AcQnaHSmY6zPDahOjKPX7uAqnhK Yw/RaCIs+SF71YL58YEOyF/b265XCglh7UXDnB2MCUZSlRZw/jg01e0cim5flhW5c/HjyQyH/zS r7gXRm/LYHUloVs1ewNbctaD3h7UHr3fOdubCwTHMIQeV2bSGnuBY0vA26FjVe7y+3bWUoHUIe0 uT7FLYr7PhhjX86adEFkDOrACxSObS9Fwy+w4Fx1aWdLxUDnHd1VWs6/JHuurKzXF32E0HX/yfN 7hvhViytDE7KSbLh1EGQ== X-Google-Smtp-Source: AGHT+IGc4OjZMP1xzbym2F8zYcMdwysP1sgVkT7Ss3kIptxke0S8T0dAWuWUBtezxhVnNmhyFD50nQ== X-Received: by 2002:a05:6000:2884:b0:429:f14a:9807 with SMTP id ffacd0b85a97d-42f89f6348amr11888073f8f.40.1765302951729; Tue, 09 Dec 2025 09:55:51 -0800 (PST) Received: from [192.168.1.183] ([185.48.77.170]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-42f7d331af5sm31559379f8f.31.2025.12.09.09.55.50 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 09 Dec 2025 09:55:51 -0800 (PST) Message-ID: Date: Tue, 9 Dec 2025 17:55:51 +0000 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 01/12] perf parse-events: Refactor get_config_terms() to remove macros To: Ian Rogers Cc: Linux perf Profiling , linux-kernel@vger.kernel.org, coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Namhyung Kim , Mark Rutland , Alexander Shishkin , Jiri Olsa , Adrian Hunter , Suzuki K Poulose , Mike Leach , John Garry , Will Deacon , Leo Yan , leo.yan@arm.com References: <20251208-james-perf-config-bits-v2-0-4ac0281993b0@linaro.org> <20251208-james-perf-config-bits-v2-1-4ac0281993b0@linaro.org> <6ba6bde3-f56b-4354-b336-4739cb1edb71@linaro.org> Content-Language: en-US From: James Clark In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 09/12/2025 15:58, Ian Rogers wrote: > On Tue, Dec 9, 2025 at 4:48 AM James Clark 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 >>> --- >>> 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 > Maybe for C++, but not for C [1]: Accessing my_union.i after most recently writing to the other member, my_union.d, is an allowed form of type-punning in C, provided that the member read is not larger than the one whose value was set Type punning with unions is widely used in the kernel and there are a couple of instances in Perf. There are tons of discussions about it on the mailing lists too. I can check it with ubsan though. [1]: https://en.wikipedia.org/wiki/Type_punning#C_and_C++ >>> } >>> } >>> 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; >>> } >>> >>> >>