Re: [PATCH v2] bpf/scripts: Generate GCC compatible helpers

[Jose E. Marchesi]

> CC Quentin as well
>
> On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
> <james.hilliard1@xxxxxxxxx> wrote:
>>
>> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@xxxxxx> wrote:
>> >
>> >
>> >
>> > On 7/6/22 10:28 AM, James Hilliard wrote:
>> > > The current bpf_helper_defs.h helpers are llvm specific and don't work
>> > > correctly with gcc.
>> > >
>> > > GCC appears to required kernel helper funcs to have the following
>> > > attribute set: __attribute__((kernel_helper(NUM)))
>> > >
>> > > Generate gcc compatible headers based on the format in bpf-helpers.h.
>> > >
>> > > This adds conditional blocks for GCC while leaving clang codepaths
>> > > unchanged, for example:
>> > >       #if __GNUC__ && !__clang__
>> > >       void *bpf_map_lookup_elem(void *map, const void *key)
>> > > __attribute__((kernel_helper(1)));
>> > >       #else
>> > >       static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
>> > >       #endif
>> >
>> > It does look like that gcc kernel_helper attribute is better than
>> > '(void *) 1' style. The original clang uses '(void *) 1' style is
>> > just for simplicity.
>>
>> Isn't the original style going to be needed for backwards compatibility with
>> older clang versions for a while?
>
> I'm curious, is there any added benefit to having this special
> kernel_helper attribute vs what we did in Clang for a long time?
> Did GCC do it just to be different and require workarounds like this
> or there was some technical benefit to this?

We did it that way so we could make trouble and piss you off.

Nah :)

We did it that way because technically speaking the clang construction
works relying on particular optimizations to happen to get correct
compiled programs, which is not guaranteed to happen and _may_ break in
the future.

In fact, if you compile a call to such a function prototype with clang
with -O0 the compiler will try to load the function's address in a
register and then emit an invalid BPF instruction:

  28:   8d 00 00 00 03 00 00 00         *unknown*

On the other hand the kernel_helper attribute is bullet-proof: will work
with any optimization level, with any version of the compiler, and in
our opinion it is also more readable, more tidy and more correct.

Note I'm not saying what you do in clang is not reasonable; it may be,
obviously it works well enough for you in practice.  Only that we have
good reasons for doing it differently in GCC.

> This duplication of definitions with #if for each one looks really
> awful, IMO. I'd rather have a macro invocation like below (or
> something along those lines) for each helper:
>
> BPF_HELPER_DEF(2, void *, bpf_map_update_elem, void *map, const void
> *key, const void *value, __u64 flags);
>
> And then define BPF_HELPER_DEF() once based on whether it's Clang or GCC.
>
>>
>> >
>> > Do you mind to help implement similar attribute in clang so we
>> > don't need "#if" here?
>>
>> That's well outside my area of expertise unfortunately.
>>
>> >
>> > >
>> > >       #if __GNUC__ && !__clang__
>> > >       long bpf_map_update_elem(void *map, const void *key, const void *value, __u64 flags) __attribute__((kernel_helper(2)));
>> > >       #else
>> > >       static long (*bpf_map_update_elem)(void *map, const void *key, const void *value, __u64 flags) = (void *) 2;
>> > >       #endif
>> > >
>> > > See:
>> > > https://github.com/gcc-mirror/gcc/blob/releases/gcc-12.1.0/gcc/config/bpf/bpf-helpers.h#L24-L27
>> > >
>> > > This fixes the following build error:
>> > > error: indirect call in function, which are not supported by eBPF
>> > >
>> > > Signed-off-by: James Hilliard <james.hilliard1@xxxxxxxxx>
>> > > ---
>> > > Changes v1 -> v2:
>> > >    - more details in commit log
>> > > ---
>> > >   scripts/bpf_doc.py | 43 ++++++++++++++++++++++++++-----------------
>> > >   1 file changed, 26 insertions(+), 17 deletions(-)
>> > >
>> > [...]