| Message ID | 20220909224544.3702931-1-james.hilliard1@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: define bpf_tail_call_static when __clang__ is not defined | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-16 | success | Logs for test_verifier on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-17 | success | Logs for test_verifier on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-10 | success | Logs for test_progs on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-11 | success | Logs for test_progs on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-13 | success | Logs for test_progs_no_alu32 on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-14 | success | Logs for test_progs_no_alu32 on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for test_maps on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-9 | success | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-12 | success | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_verifier on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
On Fri, Sep 9, 2022 at 3:46 PM James Hilliard <james.hilliard1@gmail.com> wrote: > > The bpf_tail_call_static function is currently not defined unless > using clang >= 8. > > To support bpf_tail_call_static on GCC we can check if __clang__ is > not defined to enable bpf_tail_call_static. > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > --- > tools/lib/bpf/bpf_helpers.h | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > index 7349b16b8e2f..30fc95e7cd76 100644 > --- a/tools/lib/bpf/bpf_helpers.h > +++ b/tools/lib/bpf/bpf_helpers.h > @@ -131,7 +131,7 @@ > /* > * Helper function to perform a tail call with a constant/immediate map slot. > */ > -#if __clang_major__ >= 8 && defined(__bpf__) > +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__) > static __always_inline void > bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > { > @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > __bpf_unreachable(); > > /* > - * Provide a hard guarantee that LLVM won't optimize setting r2 (map > - * pointer) and r3 (constant map index) from _different paths_ ending > + * Provide a hard guarantee that the compiler won't optimize setting r2 > + * (map pointer) and r3 (constant map index) from _different paths_ ending > * up at the _same_ call insn as otherwise we won't be able to use the > * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel > * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key > @@ -148,8 +148,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > * > * Note on clobber list: we need to stay in-line with BPF calling > * convention, so even if we don't end up using r0, r4, r5, we need > - * to mark them as clobber so that LLVM doesn't end up using them > - * before / after the call. > + * to mark them as clobber so that the compiler doesn't end up using > + * them before / after the call. > */ > asm volatile("r1 = %[ctx]\n\t" > "r2 = %[map]\n\t" will this compile as is on GCC-BPF? I'm trying to understand what's the point. Once GCC supports this ASM syntax we can add similar check to __clang_major__, instead of allowing it for all GCC versions? We must have done __clang_major__ check for a reason, old Clangs probably had some problems compiling this. Maybe Daniel remembers? > -- > 2.34.1 >
On 9/9/22 5:26 PM, Andrii Nakryiko wrote: > On Fri, Sep 9, 2022 at 3:46 PM James Hilliard <james.hilliard1@gmail.com> wrote: >> >> The bpf_tail_call_static function is currently not defined unless >> using clang >= 8. >> >> To support bpf_tail_call_static on GCC we can check if __clang__ is >> not defined to enable bpf_tail_call_static. >> >> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> >> --- >> tools/lib/bpf/bpf_helpers.h | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h >> index 7349b16b8e2f..30fc95e7cd76 100644 >> --- a/tools/lib/bpf/bpf_helpers.h >> +++ b/tools/lib/bpf/bpf_helpers.h >> @@ -131,7 +131,7 @@ >> /* >> * Helper function to perform a tail call with a constant/immediate map slot. >> */ >> -#if __clang_major__ >= 8 && defined(__bpf__) >> +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__) >> static __always_inline void >> bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) >> { >> @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) >> __bpf_unreachable(); >> >> /* >> - * Provide a hard guarantee that LLVM won't optimize setting r2 (map >> - * pointer) and r3 (constant map index) from _different paths_ ending >> + * Provide a hard guarantee that the compiler won't optimize setting r2 >> + * (map pointer) and r3 (constant map index) from _different paths_ ending >> * up at the _same_ call insn as otherwise we won't be able to use the >> * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel >> * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key >> @@ -148,8 +148,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) >> * >> * Note on clobber list: we need to stay in-line with BPF calling >> * convention, so even if we don't end up using r0, r4, r5, we need >> - * to mark them as clobber so that LLVM doesn't end up using them >> - * before / after the call. >> + * to mark them as clobber so that the compiler doesn't end up using >> + * them before / after the call. >> */ >> asm volatile("r1 = %[ctx]\n\t" >> "r2 = %[map]\n\t" > > will this compile as is on GCC-BPF? I'm trying to understand what's > the point. Once GCC supports this ASM syntax we can add similar check > to __clang_major__, instead of allowing it for all GCC versions? > > We must have done __clang_major__ check for a reason, old Clangs > probably had some problems compiling this. Maybe Daniel remembers? Yes, clang >= 8 is needed to ensure 'slot' is propagated as 'const' instead of variables. clang 6 added support for inline asm but clang 8 is needed for above 'const' propagation into asm code. > >> -- >> 2.34.1 >>
On Fri, Sep 9, 2022 at 6:26 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Fri, Sep 9, 2022 at 3:46 PM James Hilliard <james.hilliard1@gmail.com> wrote: > > > > The bpf_tail_call_static function is currently not defined unless > > using clang >= 8. > > > > To support bpf_tail_call_static on GCC we can check if __clang__ is > > not defined to enable bpf_tail_call_static. > > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > > --- > > tools/lib/bpf/bpf_helpers.h | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > > index 7349b16b8e2f..30fc95e7cd76 100644 > > --- a/tools/lib/bpf/bpf_helpers.h > > +++ b/tools/lib/bpf/bpf_helpers.h > > @@ -131,7 +131,7 @@ > > /* > > * Helper function to perform a tail call with a constant/immediate map slot. > > */ > > -#if __clang_major__ >= 8 && defined(__bpf__) > > +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__) > > static __always_inline void > > bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > > { > > @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > > __bpf_unreachable(); > > > > /* > > - * Provide a hard guarantee that LLVM won't optimize setting r2 (map > > - * pointer) and r3 (constant map index) from _different paths_ ending > > + * Provide a hard guarantee that the compiler won't optimize setting r2 > > + * (map pointer) and r3 (constant map index) from _different paths_ ending > > * up at the _same_ call insn as otherwise we won't be able to use the > > * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel > > * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key > > @@ -148,8 +148,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) > > * > > * Note on clobber list: we need to stay in-line with BPF calling > > * convention, so even if we don't end up using r0, r4, r5, we need > > - * to mark them as clobber so that LLVM doesn't end up using them > > - * before / after the call. > > + * to mark them as clobber so that the compiler doesn't end up using > > + * them before / after the call. > > */ > > asm volatile("r1 = %[ctx]\n\t" > > "r2 = %[map]\n\t" > > will this compile as is on GCC-BPF? I'm trying to understand what's > the point. Once GCC supports this ASM syntax we can add similar check > to __clang_major__, instead of allowing it for all GCC versions? So, it's a bit different since the ASM syntax issue is more of an issue with GAS, we would need a check along these lines I think for a proper migration path: https://lore.kernel.org/bpf/20220912063514.2824432-1-james.hilliard1@gmail.com/ We would set something like once GAS support for the llvm syntax is added: .ifdef .gasversion. < 24000 I don't think we can do a compiler version check here like with llvm since GCC release cycles are out of sync with GAS(llvm assembler is kept in sync with the compiler AFAIU). > > We must have done __clang_major__ check for a reason, old Clangs > probably had some problems compiling this. Maybe Daniel remembers? > > > -- > > 2.34.1 > >
diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h index 7349b16b8e2f..30fc95e7cd76 100644 --- a/tools/lib/bpf/bpf_helpers.h +++ b/tools/lib/bpf/bpf_helpers.h @@ -131,7 +131,7 @@ /* * Helper function to perform a tail call with a constant/immediate map slot. */ -#if __clang_major__ >= 8 && defined(__bpf__) +#if (!defined(__clang__) || __clang_major__ >= 8) && defined(__bpf__) static __always_inline void bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) { @@ -139,8 +139,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) __bpf_unreachable(); /* - * Provide a hard guarantee that LLVM won't optimize setting r2 (map - * pointer) and r3 (constant map index) from _different paths_ ending + * Provide a hard guarantee that the compiler won't optimize setting r2 + * (map pointer) and r3 (constant map index) from _different paths_ ending * up at the _same_ call insn as otherwise we won't be able to use the * jmpq/nopl retpoline-free patching by the x86-64 JIT in the kernel * given they mismatch. See also d2e4c1e6c294 ("bpf: Constant map key @@ -148,8 +148,8 @@ bpf_tail_call_static(void *ctx, const void *map, const __u32 slot) * * Note on clobber list: we need to stay in-line with BPF calling * convention, so even if we don't end up using r0, r4, r5, we need - * to mark them as clobber so that LLVM doesn't end up using them - * before / after the call. + * to mark them as clobber so that the compiler doesn't end up using + * them before / after the call. */ asm volatile("r1 = %[ctx]\n\t" "r2 = %[map]\n\t"