linux-kernel - Re: [PATCH] perf: use __builtin_preserve_field

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <425a7914-a653-45fe-800a-1da0108bb580@linux.dev>
Date: Wed, 6 Aug 2025 17:15:37 -0700
From: Yonghong Song <yonghong.song@...ux.dev>
To: Namhyung Kim <namhyung@...nel.org>, Sam James <sam@...too.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
 Adrian Hunter <adrian.hunter@...el.com>,
 "Liang, Kan" <kan.liang@...ux.intel.com>,
 Andrew Pinski <quic_apinski@...cinc.com>, linux-perf-users@...r.kernel.org,
 linux-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH] perf: use __builtin_preserve_field_info for GCC
 compatibility



On 8/6/25 4:33 PM, Namhyung Kim wrote:
> Hello,
>
> On Wed, Aug 06, 2025 at 01:03:01AM +0100, Sam James wrote:
>> When exploring building bpf_skel with GCC's BPF support, there was a
>> buid failure because of bpf_core_field_exists vs the mem_hops bitfield:
>> ```
>>   In file included from util/bpf_skel/sample_filter.bpf.c:6:
>> util/bpf_skel/sample_filter.bpf.c: In function 'perf_get_sample':
>> tools/perf/libbpf/include/bpf/bpf_core_read.h:169:42: error: cannot take address of bit-field 'mem_hops'
>>    169 | #define ___bpf_field_ref1(field)        (&(field))
>>        |                                          ^
>> tools/perf/libbpf/include/bpf/bpf_helpers.h:222:29: note: in expansion of macro '___bpf_field_ref1'
>>    222 | #define ___bpf_concat(a, b) a ## b
>>        |                             ^
>> tools/perf/libbpf/include/bpf/bpf_helpers.h:225:29: note: in expansion of macro '___bpf_concat'
>>    225 | #define ___bpf_apply(fn, n) ___bpf_concat(fn, n)
>>        |                             ^~~~~~~~~~~~~
>> tools/perf/libbpf/include/bpf/bpf_core_read.h:173:9: note: in expansion of macro '___bpf_apply'
>>    173 |         ___bpf_apply(___bpf_field_ref, ___bpf_narg(args))(args)
>>        |         ^~~~~~~~~~~~
>> tools/perf/libbpf/include/bpf/bpf_core_read.h:188:39: note: in expansion of macro '___bpf_field_ref'
>>    188 |         __builtin_preserve_field_info(___bpf_field_ref(field), BPF_FIELD_EXISTS)
>>        |                                       ^~~~~~~~~~~~~~~~
>> util/bpf_skel/sample_filter.bpf.c:167:29: note: in expansion of macro 'bpf_core_field_exists'
>>    167 |                         if (bpf_core_field_exists(data->mem_hops))
>>        |                             ^~~~~~~~~~~~~~~~~~~~~
>> cc1: error: argument is not a field access
>> ```
>>
>> ___bpf_field_ref1 was adapted for GCC in 12bbcf8e840f40b82b02981e96e0a5fbb0703ea9
>> but the trick added for compatibility in 3a8b8fc3174891c4c12f5766d82184a82d4b2e3e
>> isn't compatible with that as an address is used as an argument.
>>
>> Workaround this by calling __builtin_preserve_field_info directly as the
>> bpf_core_field_exists macro does, but without the ___bpf_field_ref use.
> IIUC GCC doesn't support bpf_core_fields_exists() for bitfield members,
> right?  Is it gonna change in the future?
>
>> Link: https://gcc.gnu.org/PR121420
>> Co-authored-by: Andrew Pinski <quic_apinski@...cinc.com>
>> Signed-off-by: Sam James <sam@...too.org>
>> ---
>>   tools/perf/util/bpf_skel/sample_filter.bpf.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/util/bpf_skel/sample_filter.bpf.c b/tools/perf/util/bpf_skel/sample_filter.bpf.c
>> index b195e6efeb8be..e5666d4c17228 100644
>> --- a/tools/perf/util/bpf_skel/sample_filter.bpf.c
>> +++ b/tools/perf/util/bpf_skel/sample_filter.bpf.c
>> @@ -164,7 +164,7 @@ static inline __u64 perf_get_sample(struct bpf_perf_event_data_kern *kctx,
>>   		if (entry->part == 8) {
>>   			union perf_mem_data_src___new *data = (void *)&kctx->data->data_src;
>>   
>> -			if (bpf_core_field_exists(data->mem_hops))
>> +			if (__builtin_preserve_field_info(data->mem_hops, BPF_FIELD_EXISTS))
> I believe those two are equivalent (maybe worth a comment?).  But it'd
> be great if BPF/clang folks can review if it's ok.

Yes, from clang side, they are almost equnivalent. See tools/lib/bpf/bpf_core_read.h.

#define bpf_core_field_exists(field...)                                     \
         __builtin_preserve_field_info(___bpf_field_ref(field), BPF_FIELD_EXISTS)

bpf_core_field_exists actually relies on clang builtin function
__builtin_preserve_field_info(). This builtin is handled in frontend and
also at early IR stage.

So your above code is okay to me although bpf_core_field_exists() is much
easy to understand the intent.

>
> Anyway, I can build it with clang.
>
> Tested-by: Namhyung Kim <namhyung@...nel.org>
>
> Thanks,
> Namhyung
>
>
>>   				return data->mem_hops;
>>   
>>   			return 0;
>> -- 
>> 2.50.1
>>