Re: [PATCH] perf: use __builtin_preserve_field_info for GCC compatibility

[Arnaldo Carvalho de Melo]

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@xxxxxxxxxx>
> > > Sent: Wednesday, August 6, 2025 4:34 PM
> > > To: Sam James <sam@xxxxxxxxxx>
> > > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>; Ingo Molnar
> > > <mingo@xxxxxxxxxx>; Arnaldo Carvalho de Melo
> > > <acme@xxxxxxxxxx>; Mark Rutland
> > > <mark.rutland@xxxxxxx>; Alexander Shishkin
> > > <alexander.shishkin@xxxxxxxxxxxxxxx>; Jiri Olsa
> > > <jolsa@xxxxxxxxxx>; Ian Rogers <irogers@xxxxxxxxxx>; Adrian
> > > Hunter <adrian.hunter@xxxxxxxxx>; Liang, Kan
> > > <kan.liang@xxxxxxxxxxxxxxx>; Andrew Pinski
> > > <quic_apinski@xxxxxxxxxxx>; linux-perf-
> > > users@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > > bpf@xxxxxxxxxxxxxxx
> > > 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)?