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