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
#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 <[email protected]>
---
Changes v1 -> v2:
- more details in commit log
---
scripts/bpf_doc.py | 43 ++++++++++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index a0ec321469bd..36fb400a5731 100755
--- a/scripts/bpf_doc.py
+++ b/scripts/bpf_doc.py
@@ -739,6 +739,24 @@ class PrinterHelpers(Printer):
seen_helpers = set()
+ def print_args(self, proto):
+ comma = ''
+ for i, a in enumerate(proto['args']):
+ t = a['type']
+ n = a['name']
+ if proto['name'] in self.overloaded_helpers and i == 0:
+ t = 'void'
+ n = 'ctx'
+ one_arg = '{}{}'.format(comma, self.map_type(t))
+ if n:
+ if a['star']:
+ one_arg += ' {}'.format(a['star'])
+ else:
+ one_arg += ' '
+ one_arg += '{}'.format(n)
+ comma = ', '
+ print(one_arg, end='')
+
def print_one(self, helper):
proto = helper.proto_break_down()
@@ -762,26 +780,17 @@ class PrinterHelpers(Printer):
print(' *{}{}'.format(' \t' if line else '', line))
print(' */')
+ print('#if __GNUC__ && !__clang__')
+ print('%s %s%s(' % (self.map_type(proto['ret_type']),
+ proto['ret_star'], proto['name']), end='')
+ self.print_args(proto)
+ print(') __attribute__((kernel_helper(%d)));' % len(self.seen_helpers))
+ print('#else')
print('static %s %s(*%s)(' % (self.map_type(proto['ret_type']),
proto['ret_star'], proto['name']), end='')
- comma = ''
- for i, a in enumerate(proto['args']):
- t = a['type']
- n = a['name']
- if proto['name'] in self.overloaded_helpers and i == 0:
- t = 'void'
- n = 'ctx'
- one_arg = '{}{}'.format(comma, self.map_type(t))
- if n:
- if a['star']:
- one_arg += ' {}'.format(a['star'])
- else:
- one_arg += ' '
- one_arg += '{}'.format(n)
- comma = ', '
- print(one_arg, end='')
-
+ self.print_args(proto)
print(') = (void *) %d;' % len(self.seen_helpers))
+ print('#endif')
print('')
###############################################################################
--
2.34.1
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.
Do you mind to help implement similar attribute in clang so we
don't need "#if" here?
>
> #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 <[email protected]>
> ---
> Changes v1 -> v2:
> - more details in commit log
> ---
> scripts/bpf_doc.py | 43 ++++++++++++++++++++++++++-----------------
> 1 file changed, 26 insertions(+), 17 deletions(-)
>
[...]
On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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?
>
> 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 <[email protected]>
> > ---
> > Changes v1 -> v2:
> > - more details in commit log
> > ---
> > scripts/bpf_doc.py | 43 ++++++++++++++++++++++++++-----------------
> > 1 file changed, 26 insertions(+), 17 deletions(-)
> >
> [...]
CC Quentin as well
On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
<[email protected]> wrote:
>
> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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?
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 <[email protected]>
> > > ---
> > > Changes v1 -> v2:
> > > - more details in commit log
> > > ---
> > > scripts/bpf_doc.py | 43 ++++++++++++++++++++++++++-----------------
> > > 1 file changed, 26 insertions(+), 17 deletions(-)
> > >
> > [...]
On 12/07/2022 05:40, Andrii Nakryiko wrote:
> CC Quentin as well
>
> On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
> <[email protected]> wrote:
>>
>> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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?
>
> 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.
Hi, for what it's worth I agree with Andrii, I would rather avoid the
#if/else/endif and dual definition for each helper in the header, using
a macro should keep it more readable indeed. The existing one
(BPF_HELPER(return_type, name, args, id)) can likely be adapted.
Also I note that contrarily to clang's helpers, you don't declare GCC's
as "static" (although I'm not sure of the effect of declaring them
static in this case).
Thanks,
Quentin
> CC Quentin as well
>
> On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
> <[email protected]> wrote:
>>
>> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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 <[email protected]>
>> > > ---
>> > > Changes v1 -> v2:
>> > > - more details in commit log
>> > > ---
>> > > scripts/bpf_doc.py | 43 ++++++++++++++++++++++++++-----------------
>> > > 1 file changed, 26 insertions(+), 17 deletions(-)
>> > >
>> > [...]
> On 12/07/2022 05:40, Andrii Nakryiko wrote:
>> CC Quentin as well
>>
>> On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
>> <[email protected]> wrote:
>>>
>>> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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?
>>
>> 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.
>
> Hi, for what it's worth I agree with Andrii, I would rather avoid the
> #if/else/endif and dual definition for each helper in the header, using
> a macro should keep it more readable indeed. The existing one
> (BPF_HELPER(return_type, name, args, id)) can likely be adapted.
>
> Also I note that contrarily to clang's helpers, you don't declare GCC's
> as "static" (although I'm not sure of the effect of declaring them
> static in this case).
That's because in the clang line bpf_map_lookup_elem is a static
variable, a pointer to a function type, initialized to 1.
On the other hand, in the GCC line bpf_map_lookup_elem is just a normal
function declaration. No variable, and thus no need for `static'.
>
> Thanks,
> Quentin
On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi
<[email protected]> wrote:
>
>
> > CC Quentin as well
> >
> > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
> > <[email protected]> wrote:
> >>
> >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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.
On Tue, Jul 12, 2022 at 3:48 AM Quentin Monnet <[email protected]> wrote:
>
> On 12/07/2022 05:40, Andrii Nakryiko wrote:
> > CC Quentin as well
> >
> > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
> > <[email protected]> wrote:
> >>
> >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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?
> >
> > 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.
>
> Hi, for what it's worth I agree with Andrii, I would rather avoid the
> #if/else/endif and dual definition for each helper in the header, using
> a macro should keep it more readable indeed. The existing one
> (BPF_HELPER(return_type, name, args, id)) can likely be adapted.
Yeah, seems a bit cleaner, think I got it working:
https://lore.kernel.org/bpf/[email protected]/
>
> Also I note that contrarily to clang's helpers, you don't declare GCC's
> as "static" (although I'm not sure of the effect of declaring them
> static in this case).
>
> Thanks,
> Quentin
On Tue, Jul 12, 2022 at 10:48 AM Alexei Starovoitov
<[email protected]> wrote:
>
> On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi
> <[email protected]> wrote:
> >
> >
> > > CC Quentin as well
> > >
> > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
> > > <[email protected]> wrote:
> > >>
> > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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.
Sounds to me like clang is relying on an unreliable hack that may
be difficult to implement in GCC, so let's see what's the best option
moving forwards in terms of a migration path for both GCC and clang.
> We have to avoid forking.
Avoiding having to maintain a separate libbpf fork for GCC compatibility
is precisely the purpose of this patch.
> 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.
Is there some background I'm missing here? What's your definition
of an unnecessary fork?
> The best path forward here is to support both (void *) 1 style
> and kernel_helper attribute in both gcc and llvm.
How about we use a guard like this:
#if __has_attribute(kernel_helper)
new kernel_helper variant macro
#else
legacy clang variant macro
#endif
> 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.
Or we can just feature detect kernel_helper and leave the (void *)1 style
fallback in place until we drop support for clang variants that don't support
kernel_helper. This would provide GCC compatibility and a better migration
path for clang as well as clang will then automatically use the new variant
whenever support for kernel_helper is introduced.
On Tue, Jul 12, 2022 at 7:18 PM Alexei Starovoitov
<[email protected]> wrote:
>
> On Tue, Jul 12, 2022 at 07:10:27PM -0600, James Hilliard wrote:
> > On Tue, Jul 12, 2022 at 10:48 AM Alexei Starovoitov
> > <[email protected]> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi
> > > <[email protected]> wrote:
> > > >
> > > >
> > > > > CC Quentin as well
> > > > >
> > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
> > > > > <[email protected]> wrote:
> > > > >>
> > > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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.
> >
> > Sounds to me like clang is relying on an unreliable hack that may
> > be difficult to implement in GCC, so let's see what's the best option
> > moving forwards in terms of a migration path for both GCC and clang.
>
> The following is a valid C code:
> static long (*foo) (void) = (void *) 1234;
> foo();
>
> and GCC has to generate correct assembly assuming it runs at -O1 or higher.
Providing -O1 or higher with gcc-bpf does not seem to work at the moment.
> There is no indirect call insn defined in BPF ISA yet,
> so the -O0 behavior is undefined.
Well GCC at least seems to be able to compile BPF programs with -O0 using
kernel_helper. I assume -O0 is probably just targeting the minimum BPF ISA
optimization level or something like that which avoids indirect calls.
>
> > Or we can just feature detect kernel_helper and leave the (void *)1 style
> > fallback in place until we drop support for clang variants that don't support
> > kernel_helper. This would provide GCC compatibility and a better migration
> > path for clang as well as clang will then automatically use the new variant
> > whenever support for kernel_helper is introduced.
>
> Support for valid C code will not be dropped from clang.
That wasn't what I was suggesting, I was suggesting adding support for
kernel_helper to clang, and then in the future libbpf(not clang) can
drop support
for the (void *)1 style in the future if desired(or can just keep the
fallback). By
feature detecting kernel_helper and providing a fallback we get a nice clean
migration path.
On Tue, Jul 12, 2022 at 6:29 PM James Hilliard
<[email protected]> wrote:
>
> On Tue, Jul 12, 2022 at 7:18 PM Alexei Starovoitov
> <[email protected]> wrote:
> >
> > On Tue, Jul 12, 2022 at 07:10:27PM -0600, James Hilliard wrote:
> > > On Tue, Jul 12, 2022 at 10:48 AM Alexei Starovoitov
> > > <[email protected]> wrote:
> > > >
> > > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi
> > > > <[email protected]> wrote:
> > > > >
> > > > >
> > > > > > CC Quentin as well
> > > > > >
> > > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
> > > > > > <[email protected]> wrote:
> > > > > >>
> > > > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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.
> > >
> > > Sounds to me like clang is relying on an unreliable hack that may
> > > be difficult to implement in GCC, so let's see what's the best option
> > > moving forwards in terms of a migration path for both GCC and clang.
> >
> > The following is a valid C code:
> > static long (*foo) (void) = (void *) 1234;
> > foo();
> >
> > and GCC has to generate correct assembly assuming it runs at -O1 or higher.
>
> Providing -O1 or higher with gcc-bpf does not seem to work at the moment.
Let's fix gcc first.
> > There is no indirect call insn defined in BPF ISA yet,
> > so the -O0 behavior is undefined.
>
> Well GCC at least seems to be able to compile BPF programs with -O0 using
> kernel_helper. I assume -O0 is probably just targeting the minimum BPF ISA
> optimization level or something like that which avoids indirect calls.
There are other reasons why -O0 compiled progs will
fail in the verifier.
> >
> > > Or we can just feature detect kernel_helper and leave the (void *)1 style
> > > fallback in place until we drop support for clang variants that don't support
> > > kernel_helper. This would provide GCC compatibility and a better migration
> > > path for clang as well as clang will then automatically use the new variant
> > > whenever support for kernel_helper is introduced.
> >
> > Support for valid C code will not be dropped from clang.
>
> That wasn't what I was suggesting, I was suggesting adding support for
> kernel_helper to clang, and then in the future libbpf(not clang) can
> drop support
> for the (void *)1 style in the future if desired(or can just keep the
> fallback). By
> feature detecting kernel_helper and providing a fallback we get a nice clean
> migration path.
Makes sense. That deprecation step is far away though.
Assuming that kernel_helper attr is actually necessary
we have to add its support to clang as well.
We have to keep compilers in sync.
gcc-bpf is a niche. If gcc devs want it to become a real
alternative to clang they have to always aim for feature parity
instead of inventing their own ways of doing things.
On Tue, Jul 12, 2022 at 07:10:27PM -0600, James Hilliard wrote:
> On Tue, Jul 12, 2022 at 10:48 AM Alexei Starovoitov
> <[email protected]> wrote:
> >
> > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi
> > <[email protected]> wrote:
> > >
> > >
> > > > CC Quentin as well
> > > >
> > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
> > > > <[email protected]> wrote:
> > > >>
> > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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.
>
> Sounds to me like clang is relying on an unreliable hack that may
> be difficult to implement in GCC, so let's see what's the best option
> moving forwards in terms of a migration path for both GCC and clang.
The following is a valid C code:
static long (*foo) (void) = (void *) 1234;
foo();
and GCC has to generate correct assembly assuming it runs at -O1 or higher.
There is no indirect call insn defined in BPF ISA yet,
so the -O0 behavior is undefined.
> Or we can just feature detect kernel_helper and leave the (void *)1 style
> fallback in place until we drop support for clang variants that don't support
> kernel_helper. This would provide GCC compatibility and a better migration
> path for clang as well as clang will then automatically use the new variant
> whenever support for kernel_helper is introduced.
Support for valid C code will not be dropped from clang.
On Tue, Jul 12, 2022 at 7:45 PM Alexei Starovoitov
<[email protected]> wrote:
>
> On Tue, Jul 12, 2022 at 6:29 PM James Hilliard
> <[email protected]> wrote:
> >
> > On Tue, Jul 12, 2022 at 7:18 PM Alexei Starovoitov
> > <[email protected]> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 07:10:27PM -0600, James Hilliard wrote:
> > > > On Tue, Jul 12, 2022 at 10:48 AM Alexei Starovoitov
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > >
> > > > > > > CC Quentin as well
> > > > > > >
> > > > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
> > > > > > > <[email protected]> wrote:
> > > > > > >>
> > > > > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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.
> > > >
> > > > Sounds to me like clang is relying on an unreliable hack that may
> > > > be difficult to implement in GCC, so let's see what's the best option
> > > > moving forwards in terms of a migration path for both GCC and clang.
> > >
> > > The following is a valid C code:
> > > static long (*foo) (void) = (void *) 1234;
> > > foo();
> > >
> > > and GCC has to generate correct assembly assuming it runs at -O1 or higher.
> >
> > Providing -O1 or higher with gcc-bpf does not seem to work at the moment.
>
> Let's fix gcc first.
If the intention is to migrate to kernel_helper for clang as well it
seems kind of
redundant, is there a real world use case for supporting the '(void *)
1' style in
GCC rather than just adding feature detection+kernel_helper support to libbpf?
My assumption is that kernel helpers are in practice always used via libbpf
which appears to be sufficient in terms of being able to provide a compatibility
layer via feature detection. Or is there some use case I'm missing here?
>
> > > There is no indirect call insn defined in BPF ISA yet,
> > > so the -O0 behavior is undefined.
> >
> > Well GCC at least seems to be able to compile BPF programs with -O0 using
> > kernel_helper. I assume -O0 is probably just targeting the minimum BPF ISA
> > optimization level or something like that which avoids indirect calls.
>
> There are other reasons why -O0 compiled progs will
> fail in the verifier.
Why would -O0 generate code that isn't compatible with the selected
target BPF ISA?
I mean according to GCC docs "-O0 means (almost) no optimization":
https://gcc.gnu.org/onlinedocs/gnat_ugn/Optimization-Levels.html
My understanding is that -O0 does not actually mean no optimization in practice.
>
> > >
> > > > Or we can just feature detect kernel_helper and leave the (void *)1 style
> > > > fallback in place until we drop support for clang variants that don't support
> > > > kernel_helper. This would provide GCC compatibility and a better migration
> > > > path for clang as well as clang will then automatically use the new variant
> > > > whenever support for kernel_helper is introduced.
> > >
> > > Support for valid C code will not be dropped from clang.
> >
> > That wasn't what I was suggesting, I was suggesting adding support for
> > kernel_helper to clang, and then in the future libbpf(not clang) can
> > drop support
> > for the (void *)1 style in the future if desired(or can just keep the
> > fallback). By
> > feature detecting kernel_helper and providing a fallback we get a nice clean
> > migration path.
>
> Makes sense. That deprecation step is far away though.
Sure, feature detection/fallback would probably make sense to keep indefinitely
or until libbpf drops support entirely for non-kernel_helper clang versions for
other reasons.
> Assuming that kernel_helper attr is actually necessary
> we have to add its support to clang as well.
I mean, I'd argue there's a difference between something being arguably a better
alternative(optional) and actually being necessary(non-optional).
> We have to keep compilers in sync.
The end goal here IMO is kernel helper implementation convergence, since
it's fairly trivial to support both via feature detection the one that we want
the implementations to converge to is the one that's most important for both
compilers to support.
> gcc-bpf is a niche. If gcc devs want it to become a real
> alternative to clang they have to always aim for feature parity
> instead of inventing their own ways of doing things.
What's ultimately going to help the most in regards to helping gcc-bpf reach
feature parity with clang is getting it minimally usable in the real
world, because
that's how you're going to get more people testing+fixing bugs so that all these
differences/incompatibilities can be worked though/fixed.
If nobody can compile a real world BPF program with gcc-bpf it's likely going to
lag further behind. There's a lot of ecosystem
tooling/infrastructure(build system
support and such) that needs to add support for it as well and that
ends up being
rather difficult without at least a minimally functional toolchain.
For example I've
been working on integrating gcc-bpf into
buildroot(https://buildroot.org/)'s cross
compilation toolchain infrastructure which will then get tested in our
autobuilder
CI(http://autobuild.buildroot.net/) which tends to help surface a lot
of obscure bugs
and incompatibilities.
With embedded system cpu architecture support using clang toolchains tend to
lag behind GCC in general so getting gcc-bpf working there is
helpful(and reduces
the need for larger and more complex hybrid clang/gcc toolchain setups/builds).
On Tue, Jul 12, 2022 at 08:56:35PM -0600, James Hilliard wrote:
> On Tue, Jul 12, 2022 at 7:45 PM Alexei Starovoitov
> <[email protected]> wrote:
> >
> > On Tue, Jul 12, 2022 at 6:29 PM James Hilliard
> > <[email protected]> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 7:18 PM Alexei Starovoitov
> > > <[email protected]> wrote:
> > > >
> > > > On Tue, Jul 12, 2022 at 07:10:27PM -0600, James Hilliard wrote:
> > > > > On Tue, Jul 12, 2022 at 10:48 AM Alexei Starovoitov
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > >
> > > > > > > > CC Quentin as well
> > > > > > > >
> > > > > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
> > > > > > > > <[email protected]> wrote:
> > > > > > > >>
> > > > > > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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.
> > > > >
> > > > > Sounds to me like clang is relying on an unreliable hack that may
> > > > > be difficult to implement in GCC, so let's see what's the best option
> > > > > moving forwards in terms of a migration path for both GCC and clang.
> > > >
> > > > The following is a valid C code:
> > > > static long (*foo) (void) = (void *) 1234;
> > > > foo();
> > > >
> > > > and GCC has to generate correct assembly assuming it runs at -O1 or higher.
> > >
> > > Providing -O1 or higher with gcc-bpf does not seem to work at the moment.
> >
> > Let's fix gcc first.
>
> If the intention is to migrate to kernel_helper for clang as well it
> seems kind of
> redundant, is there a real world use case for supporting the '(void *)
> 1' style in
> GCC rather than just adding feature detection+kernel_helper support to libbpf?
>
> My assumption is that kernel helpers are in practice always used via libbpf
> which appears to be sufficient in terms of being able to provide a compatibility
> layer via feature detection. Or is there some use case I'm missing here?
static long (*foo) (void) = (void *) 1234;
is not about calling into "kernel helpers".
There is no concept of "kernel" in BPF ISA.
'call 1234' insn means call a function with that absolute address.
The gcc named that attribute incorrectly.
It should be renamed to something like __attribute__((fixed_address(1234))).
It's a linux kernel abi choice to interpret 'call abs_addr' as a call to a kernel
provided function at that address. 1,2,3,... are addresses of functions.
> >
> > > > There is no indirect call insn defined in BPF ISA yet,
> > > > so the -O0 behavior is undefined.
> > >
> > > Well GCC at least seems to be able to compile BPF programs with -O0 using
> > > kernel_helper. I assume -O0 is probably just targeting the minimum BPF ISA
> > > optimization level or something like that which avoids indirect calls.
> >
> > There are other reasons why -O0 compiled progs will
> > fail in the verifier.
>
> Why would -O0 generate code that isn't compatible with the selected
> target BPF ISA?
llvm has no issue producing valid BPF code with -O0.
It's the kernel verifier that doesn't understand such code.
For the following code:
static long (*foo) (void) = (void *) 1234;
long bar(void)
{
return foo();
}
With -O[12] llvm will generate
call 1234
exit
With -O0
r1 = foo ll
r1 = *(u64 *)(r1 + 0)
callx r1
exit
Both codes are valid and equivalent.
'callx' here is a reserved insn. The kernel verifier doesn't know about it yet,
but llvm was generting such code for 8+ years.
> > Assuming that kernel_helper attr is actually necessary
> > we have to add its support to clang as well.
>
> I mean, I'd argue there's a difference between something being arguably a better
> alternative(optional) and actually being necessary(non-optional).
gcc's attribute is not better.
It's just a different way to tell compiler about fixed function address.
> > gcc-bpf is a niche. If gcc devs want it to become a real
> > alternative to clang they have to always aim for feature parity
> > instead of inventing their own ways of doing things.
>
> What's ultimately going to help the most in regards to helping gcc-bpf reach
> feature parity with clang is getting it minimally usable in the real
> world, because
> that's how you're going to get more people testing+fixing bugs so that all these
> differences/incompatibilities can be worked though/fixed.
Can gcc-bpf compile all of selftests/bpf ?
How many of compiled programs will pass the verifier ?
> If nobody can compile a real world BPF program with gcc-bpf it's likely going to
> lag further behind.
selftest/bpf is a first milestone that gcc-bpf has to pass before talking about
'real world' bpf progs.
On Tue, Jul 12, 2022 at 10:25 PM Alexei Starovoitov
<[email protected]> wrote:
>
> On Tue, Jul 12, 2022 at 08:56:35PM -0600, James Hilliard wrote:
> > On Tue, Jul 12, 2022 at 7:45 PM Alexei Starovoitov
> > <[email protected]> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 6:29 PM James Hilliard
> > > <[email protected]> wrote:
> > > >
> > > > On Tue, Jul 12, 2022 at 7:18 PM Alexei Starovoitov
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Tue, Jul 12, 2022 at 07:10:27PM -0600, James Hilliard wrote:
> > > > > > On Tue, Jul 12, 2022 at 10:48 AM Alexei Starovoitov
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi
> > > > > > > <[email protected]> wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > > CC Quentin as well
> > > > > > > > >
> > > > > > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
> > > > > > > > > <[email protected]> wrote:
> > > > > > > > >>
> > > > > > > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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.
> > > > > >
> > > > > > Sounds to me like clang is relying on an unreliable hack that may
> > > > > > be difficult to implement in GCC, so let's see what's the best option
> > > > > > moving forwards in terms of a migration path for both GCC and clang.
> > > > >
> > > > > The following is a valid C code:
> > > > > static long (*foo) (void) = (void *) 1234;
> > > > > foo();
> > > > >
> > > > > and GCC has to generate correct assembly assuming it runs at -O1 or higher.
> > > >
> > > > Providing -O1 or higher with gcc-bpf does not seem to work at the moment.
> > >
> > > Let's fix gcc first.
> >
> > If the intention is to migrate to kernel_helper for clang as well it
> > seems kind of
> > redundant, is there a real world use case for supporting the '(void *)
> > 1' style in
> > GCC rather than just adding feature detection+kernel_helper support to libbpf?
> >
> > My assumption is that kernel helpers are in practice always used via libbpf
> > which appears to be sufficient in terms of being able to provide a compatibility
> > layer via feature detection. Or is there some use case I'm missing here?
>
> static long (*foo) (void) = (void *) 1234;
> is not about calling into "kernel helpers".
> There is no concept of "kernel" in BPF ISA.
I thought GCC at least had a somewhat kernel specific BPF ISA target,
I presume clang's bpf target is more generalized.
> 'call 1234' insn means call a function with that absolute address.
> The gcc named that attribute incorrectly.
> It should be renamed to something like __attribute__((fixed_address(1234))).
>
> It's a linux kernel abi choice to interpret 'call abs_addr' as a call to a kernel
> provided function at that address. 1,2,3,... are addresses of functions.
The impression I got was that GCC's BPF support was designed for targeting
the kernel ISA effectively, at least going off of the gcc-bpf docs gave me that
impression, although I might be wrong about that.
>
> > >
> > > > > There is no indirect call insn defined in BPF ISA yet,
> > > > > so the -O0 behavior is undefined.
> > > >
> > > > Well GCC at least seems to be able to compile BPF programs with -O0 using
> > > > kernel_helper. I assume -O0 is probably just targeting the minimum BPF ISA
> > > > optimization level or something like that which avoids indirect calls.
> > >
> > > There are other reasons why -O0 compiled progs will
> > > fail in the verifier.
> >
> > Why would -O0 generate code that isn't compatible with the selected
> > target BPF ISA?
>
> llvm has no issue producing valid BPF code with -O0.
> It's the kernel verifier that doesn't understand such code.
> For the following code:
> static long (*foo) (void) = (void *) 1234;
> long bar(void)
> {
> return foo();
> }
>
> With -O[12] llvm will generate
> call 1234
> exit
> With -O0
> r1 = foo ll
> r1 = *(u64 *)(r1 + 0)
> callx r1
> exit
>
> Both codes are valid and equivalent.
> 'callx' here is a reserved insn. The kernel verifier doesn't know about it yet,
> but llvm was generting such code for 8+ years.
Hmm, I thought GCC gates non-kernel compatible BPF behind -mxbpf(for
use with GCC's internal test suite mostly AFAIU):
https://gcc.gnu.org/onlinedocs/gcc/eBPF-Options.html
>
> > > Assuming that kernel_helper attr is actually necessary
> > > we have to add its support to clang as well.
> >
> > I mean, I'd argue there's a difference between something being arguably a better
> > alternative(optional) and actually being necessary(non-optional).
>
> gcc's attribute is not better.
> It's just a different way to tell compiler about fixed function address.
I presume it's a lot simpler implementation wise than the clang
version, but I could
be wrong about that though. I mostly work with compiler integration testing and
build fixes, compiler internals are a bit out of my area of expertise.
>
> > > gcc-bpf is a niche. If gcc devs want it to become a real
> > > alternative to clang they have to always aim for feature parity
> > > instead of inventing their own ways of doing things.
> >
> > What's ultimately going to help the most in regards to helping gcc-bpf reach
> > feature parity with clang is getting it minimally usable in the real
> > world, because
> > that's how you're going to get more people testing+fixing bugs so that all these
> > differences/incompatibilities can be worked though/fixed.
>
> Can gcc-bpf compile all of selftests/bpf ?
Don't pretty much all of those use?:
#include <bpf/bpf_helpers.h>
Which doesn't really work without adding kernel_helper support to libbpf at the
moment when building with gcc-bpf.
> How many of compiled programs will pass the verifier ?
Not really sure, still been working through toolchain/build issues...kinda
tricky to do proper testing when those are all using clang specific headers.
Would be handy to get integration testing running against them with gcc-bpf
so that we can at least get a baseline in terms of what's working and catch
regressions when fixing compiler/toolchain issues, right now I think gcc-bpf is
mostly only using an internal test suite.
>
> > If nobody can compile a real world BPF program with gcc-bpf it's likely going to
> > lag further behind.
>
> selftest/bpf is a first milestone that gcc-bpf has to pass before talking about
> 'real world' bpf progs.
A test suite designed to exercise lots of edge cases isn't exactly a great first
milestone for something like this, something like the 3 small systemd BPF
programs on the other hand would be a good start IMO, since they are
widely used real world programs and relatively simple. They aren't going to
exercise all potential edge cases but they are a good starting point when it
comes to having say something for testing real world toolchain integrations
against(which is in really rough shape at the moment).
I mean even getting some normal-ish progs buildable without downstream
library patches would be a big improvement as one can then iterate a lot easier.
I mean, we're dealing with multiple issues here, some of which are more
toolchain/integration issues and others are compiler issues. If we can get
a little more integration testing going it's going to be easier to flush out the
remaining compiler issues. Kinda tricky to fix one without fixing the other.
On Tue, Jul 12, 2022 at 7:45 PM Alexei Starovoitov
<[email protected]> wrote:
>
> On Tue, Jul 12, 2022 at 6:29 PM James Hilliard
> <[email protected]> wrote:
> >
> > On Tue, Jul 12, 2022 at 7:18 PM Alexei Starovoitov
> > <[email protected]> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 07:10:27PM -0600, James Hilliard wrote:
> > > > On Tue, Jul 12, 2022 at 10:48 AM Alexei Starovoitov
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > >
> > > > > > > CC Quentin as well
> > > > > > >
> > > > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
> > > > > > > <[email protected]> wrote:
> > > > > > >>
> > > > > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <[email protected]> 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.
> > > >
> > > > Sounds to me like clang is relying on an unreliable hack that may
> > > > be difficult to implement in GCC, so let's see what's the best option
> > > > moving forwards in terms of a migration path for both GCC and clang.
> > >
> > > The following is a valid C code:
> > > static long (*foo) (void) = (void *) 1234;
> > > foo();
> > >
> > > and GCC has to generate correct assembly assuming it runs at -O1 or higher.
> >
> > Providing -O1 or higher with gcc-bpf does not seem to work at the moment.
>
> Let's fix gcc first.
FYI this should now be fixed in master:
https://github.com/gcc-mirror/gcc/commit/6d1f144b3e6e3761375bea657718f58fb720fb44
>
> > > There is no indirect call insn defined in BPF ISA yet,
> > > so the -O0 behavior is undefined.
> >
> > Well GCC at least seems to be able to compile BPF programs with -O0 using
> > kernel_helper. I assume -O0 is probably just targeting the minimum BPF ISA
> > optimization level or something like that which avoids indirect calls.
>
> There are other reasons why -O0 compiled progs will
> fail in the verifier.
>
> > >
> > > > Or we can just feature detect kernel_helper and leave the (void *)1 style
> > > > fallback in place until we drop support for clang variants that don't support
> > > > kernel_helper. This would provide GCC compatibility and a better migration
> > > > path for clang as well as clang will then automatically use the new variant
> > > > whenever support for kernel_helper is introduced.
> > >
> > > Support for valid C code will not be dropped from clang.
> >
> > That wasn't what I was suggesting, I was suggesting adding support for
> > kernel_helper to clang, and then in the future libbpf(not clang) can
> > drop support
> > for the (void *)1 style in the future if desired(or can just keep the
> > fallback). By
> > feature detecting kernel_helper and providing a fallback we get a nice clean
> > migration path.
>
> Makes sense. That deprecation step is far away though.
> Assuming that kernel_helper attr is actually necessary
> we have to add its support to clang as well.
> We have to keep compilers in sync.
> gcc-bpf is a niche. If gcc devs want it to become a real
> alternative to clang they have to always aim for feature parity
> instead of inventing their own ways of doing things.