[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <80d42369-3e23-453e-86a1-2fdb57bb78c8@linux.dev>
Date: Sat, 4 Oct 2025 16:19:48 -0700
From: Yonghong Song <yonghong.song@...ux.dev>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: Andrew Pinski <quic_apinski@...cinc.com>,
Namhyung Kim <namhyung@...nel.org>, Sam James <sam@...too.org>,
Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
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>,
"linux-perf-users@...r.kernel.org" <linux-perf-users@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"bpf@...r.kernel.org" <bpf@...r.kernel.org>
Subject: Re: [PATCH] perf: use __builtin_preserve_field_info for GCC
compatibility
On 10/2/25 10:31 AM, Arnaldo Carvalho de Melo wrote:
> On Wed, Aug 06, 2025 at 05:27:02PM -0700, Yonghong Song wrote:
>> On 8/6/25 4:57 PM, Andrew Pinski wrote:
>>>> -----Original Message-----
>>>> From: Namhyung Kim <namhyung@...nel.org>
>>>> Sent: Wednesday, August 6, 2025 4:34 PM
>>>> To: 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
>>>>
>>>> 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?
>>> Let's discuss how __builtin_preserve_field_info is handled in the first place for BPF. Right now it seems it is passed some expression as the first argument is never evaluated.
>>> The problem is GCC's implementation of __builtin_preserve_field_info is all in the backend and the front end does not understand of the special rules here.
>>>
>>> GCC implements some "special" builtins in the front-end but not by the normal function call rules but parsing them separately; this is how __builtin_offsetof and a few others are implemented in both the C and C++ front-ends (and implemented separately). Now we could have add a hook to allow a backend to something similar and maybe that is the best way forward here.
>>> But it won't be __builtin_preserve_field_info but rather `__builtin_preserve_field_type_info(type,field,kind)` instead.
>>>
>>> __builtin_preserve_enum_type_value would also be added with the following:
>>> __builtin_preserve_enum_type_value(enum_type, enum_value, kind)
>>>
>>> And change all of the rest of the builtins to accept a true type argument rather than having to cast an null pointer to that type.
>>>
>>> Will clang implement a similar builtin?
>> The clang only has one builtin for some related relocations:
>> __builtin_preserve_field_info(..., BPF_FIELD_EXISTS)
>> __builtin_preserve_field_info(..., BPF_FIELD_BYTE_OFFSET)
>> ...
>> They are all used in bpf_core_read.h.
>>
>>> Note this won't be done until at least GCC 16; maybe not until GCC 17 depending on if I or someone else gets time to implement the front-end parts which is acceptable to both the C and C++ front-ends.
> So I'm taking the patch as-is, ok?
>
> But first we need the Signed-off-by tag from Andrew Pinski as he is
> listed in a Co-authored-by, that I replaced with Co-developed-by as its
> the term used for this purpose in:
>
> Yonghong, can I add an Acked-by: you since you participated in this
> discussion agreeing with the original patch (If I'm not mistaken)?
LGTM.
Acked-by: Yonghong Song <yonghong.song@...ux.dev>
Powered by blists - more mailing lists