| Message ID | 20220706172814.169274-1-james.hilliard1@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2] bpf/scripts: Generate GCC compatible helpers | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
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 <james.hilliard1@gmail.com> > --- > 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 <yhs@fb.com> 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 <james.hilliard1@gmail.com> > > --- > > 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 <james.hilliard1@gmail.com> wrote: > > On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> 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 <james.hilliard1@gmail.com> > > > --- > > > 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 > <james.hilliard1@gmail.com> wrote: >> >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> 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 > <james.hilliard1@gmail.com> wrote: >> >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> 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@gmail.com> >> > > --- >> > > 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 >> <james.hilliard1@gmail.com> wrote: >>> >>> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> 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 <jose.marchesi@oracle.com> wrote: > > > > CC Quentin as well > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard > > <james.hilliard1@gmail.com> wrote: > >> > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> 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 <quentin@isovalent.com> 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 > > <james.hilliard1@gmail.com> wrote: > >> > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> 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/20220712232556.248863-1-james.hilliard1@gmail.com/ > > 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 <alexei.starovoitov@gmail.com> wrote: > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi > <jose.marchesi@oracle.com> wrote: > > > > > > > CC Quentin as well > > > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard > > > <james.hilliard1@gmail.com> wrote: > > >> > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> 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 07:10:27PM -0600, James Hilliard wrote: > On Tue, Jul 12, 2022 at 10:48 AM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi > > <jose.marchesi@oracle.com> wrote: > > > > > > > > > > CC Quentin as well > > > > > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard > > > > <james.hilliard1@gmail.com> wrote: > > > >> > > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> 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:18 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> 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 > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi > > > <jose.marchesi@oracle.com> wrote: > > > > > > > > > > > > > CC Quentin as well > > > > > > > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard > > > > > <james.hilliard1@gmail.com> wrote: > > > > >> > > > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> 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 <james.hilliard1@gmail.com> wrote: > > On Tue, Jul 12, 2022 at 7:18 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> 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 > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi > > > > <jose.marchesi@oracle.com> wrote: > > > > > > > > > > > > > > > > CC Quentin as well > > > > > > > > > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard > > > > > > <james.hilliard1@gmail.com> wrote: > > > > > >> > > > > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> 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 7:45 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Jul 12, 2022 at 6:29 PM James Hilliard > <james.hilliard1@gmail.com> wrote: > > > > On Tue, Jul 12, 2022 at 7:18 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> 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 > > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi > > > > > <jose.marchesi@oracle.com> wrote: > > > > > > > > > > > > > > > > > > > CC Quentin as well > > > > > > > > > > > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard > > > > > > > <james.hilliard1@gmail.com> wrote: > > > > > > >> > > > > > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> 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 > <alexei.starovoitov@gmail.com> wrote: > > > > On Tue, Jul 12, 2022 at 6:29 PM James Hilliard > > <james.hilliard1@gmail.com> wrote: > > > > > > On Tue, Jul 12, 2022 at 7:18 PM Alexei Starovoitov > > > <alexei.starovoitov@gmail.com> 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 > > > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > > > > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi > > > > > > <jose.marchesi@oracle.com> wrote: > > > > > > > > > > > > > > > > > > > > > > CC Quentin as well > > > > > > > > > > > > > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard > > > > > > > > <james.hilliard1@gmail.com> wrote: > > > > > > > >> > > > > > > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> 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 <alexei.starovoitov@gmail.com> 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 > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Tue, Jul 12, 2022 at 6:29 PM James Hilliard > > > <james.hilliard1@gmail.com> wrote: > > > > > > > > On Tue, Jul 12, 2022 at 7:18 PM Alexei Starovoitov > > > > <alexei.starovoitov@gmail.com> 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 > > > > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > > > > > > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi > > > > > > > <jose.marchesi@oracle.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > CC Quentin as well > > > > > > > > > > > > > > > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard > > > > > > > > > <james.hilliard1@gmail.com> wrote: > > > > > > > > >> > > > > > > > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> 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 <alexei.starovoitov@gmail.com> wrote: > > On Tue, Jul 12, 2022 at 6:29 PM James Hilliard > <james.hilliard1@gmail.com> wrote: > > > > On Tue, Jul 12, 2022 at 7:18 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> 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 > > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi > > > > > <jose.marchesi@oracle.com> wrote: > > > > > > > > > > > > > > > > > > > CC Quentin as well > > > > > > > > > > > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard > > > > > > > <james.hilliard1@gmail.com> wrote: > > > > > > >> > > > > > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> 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.
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('') ###############################################################################
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 <james.hilliard1@gmail.com> --- Changes v1 -> v2: - more details in commit log --- scripts/bpf_doc.py | 43 ++++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 17 deletions(-)