On Tue, Jul 12, 2022 at 7:45 PM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Tue, Jul 12, 2022 at 6:29 PM James Hilliard > <james.hilliard1@xxxxxxxxx> wrote: > > > > On Tue, Jul 12, 2022 at 7:18 PM Alexei Starovoitov > > <alexei.starovoitov@xxxxxxxxx> 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@xxxxxxxxx> wrote: > > > > > > > > > > On Tue, Jul 12, 2022 at 4:20 AM Jose E. Marchesi > > > > > <jose.marchesi@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > > CC Quentin as well > > > > > > > > > > > > > > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard > > > > > > > <james.hilliard1@xxxxxxxxx> wrote: > > > > > > >> > > > > > > >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@xxxxxx> wrote: > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > >> > On 7/6/22 10:28 AM, James Hilliard wrote: > > > > > > >> > > The current bpf_helper_defs.h helpers are llvm specific and don't work > > > > > > >> > > correctly with gcc. > > > > > > >> > > > > > > > > >> > > GCC appears to required kernel helper funcs to have the following > > > > > > >> > > attribute set: __attribute__((kernel_helper(NUM))) > > > > > > >> > > > > > > > > >> > > Generate gcc compatible headers based on the format in bpf-helpers.h. > > > > > > >> > > > > > > > > >> > > This adds conditional blocks for GCC while leaving clang codepaths > > > > > > >> > > unchanged, for example: > > > > > > >> > > #if __GNUC__ && !__clang__ > > > > > > >> > > void *bpf_map_lookup_elem(void *map, const void *key) > > > > > > >> > > __attribute__((kernel_helper(1))); > > > > > > >> > > #else > > > > > > >> > > static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1; > > > > > > >> > > #endif > > > > > > >> > > > > > > > >> > It does look like that gcc kernel_helper attribute is better than > > > > > > >> > '(void *) 1' style. The original clang uses '(void *) 1' style is > > > > > > >> > just for simplicity. > > > > > > >> > > > > > > >> Isn't the original style going to be needed for backwards compatibility with > > > > > > >> older clang versions for a while? > > > > > > > > > > > > > > I'm curious, is there any added benefit to having this special > > > > > > > kernel_helper attribute vs what we did in Clang for a long time? > > > > > > > Did GCC do it just to be different and require workarounds like this > > > > > > > or there was some technical benefit to this? > > > > > > > > > > > > We did it that way so we could make trouble and piss you off. > > > > > > > > > > > > Nah :) > > > > > > > > > > > > We did it that way because technically speaking the clang construction > > > > > > works relying on particular optimizations to happen to get correct > > > > > > compiled programs, which is not guaranteed to happen and _may_ break in > > > > > > the future. > > > > > > > > > > > > In fact, if you compile a call to such a function prototype with clang > > > > > > with -O0 the compiler will try to load the function's address in a > > > > > > register and then emit an invalid BPF instruction: > > > > > > > > > > > > 28: 8d 00 00 00 03 00 00 00 *unknown* > > > > > > > > > > > > On the other hand the kernel_helper attribute is bullet-proof: will work > > > > > > with any optimization level, with any version of the compiler, and in > > > > > > our opinion it is also more readable, more tidy and more correct. > > > > > > > > > > > > Note I'm not saying what you do in clang is not reasonable; it may be, > > > > > > obviously it works well enough for you in practice. Only that we have > > > > > > good reasons for doing it differently in GCC. > > > > > > > > > > Not questioning the validity of the reasons, but they created > > > > > the unnecessary difference between compilers. > > > > > > > > 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.
