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

On 12/07/2022 05:40, Andrii Nakryiko wrote:

quoted

CC Quentin as well

On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
[off-list ref] wrote:

quoted

On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song [off-list ref] wrote:

quoted

On 7/6/22 10:28 AM, James Hilliard wrote:

quoted

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