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

[Alexei Starovoitov]

On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi
<jose.marchesi@xxxxxxxxxx> wrote:
>
>
> > 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.

Not questioning the validity of the reasons, but they created
the unnecessary difference between compilers.
We have to avoid forking.
Meaning we're not going to work around that by ifdefs in
bpf_helper_defs.h
Because gcc community will not learn the lesson and will keep
the bad practice of unnecessary forks.
The best path forward here is to support both (void *) 1 style
and kernel_helper attribute in both gcc and llvm.
Moving forward the bpf_helper_defs.h will stay with (void *)1 style
and when both compilers support both options we will start
transitioning to the new kernel_helpers style.