Re: [PATCH v2] libbpf: add GCC support for bpf_tail_call_static

[Andrii Nakryiko]

On Mon, Aug 29, 2022 at 2:05 PM James Hilliard
<james.hilliard1@xxxxxxxxx> 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.
>
> We need to use GCC assembly syntax when the compiler does not define
> __clang__ as LLVM inline assembly is not fully compatible with GCC.
>
> Signed-off-by: James Hilliard <james.hilliard1@xxxxxxxxx>
> ---
> Changes v1 -> v2:
>   - drop __BPF__ check as GCC now defines __bpf__
> ---
>  tools/lib/bpf/bpf_helpers.h | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index 7349b16b8e2f..867b734839dd 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,12 +148,19 @@ 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"
> +       asm volatile(
> +#ifdef __clang__
> +                    "r1 = %[ctx]\n\t"
>                      "r2 = %[map]\n\t"
>                      "r3 = %[slot]\n\t"
> +#else
> +                    "mov %%r1,%[ctx]\n\t"
> +                    "mov %%r2,%[map]\n\t"
> +                    "mov %%r3,%[slot]\n\t"
> +#endif

Hey James,

I don't think it's a good idea to have a completely different BPF asm
syntax in GCC-BPF vs what Clang is supporting. Note that Clang syntax
is also what BPF users see in BPF verifier log and in llvm-objdump
output, so that's what BPF users are familiar with.

This will cause constant and unavoidable maintenance burden both for
libraries like libbpf and end users and their BPF apps as well.

Given you are trying to make GCC-BPF part of the BPF ecosystem, please
think about how to help the ecosystem, move it forward and unify it,
not how to branch out and have Clang vs GCC differences everywhere.
There is a lot of embedded BPF asm in production applications, having
to write something as trivial as `r1 = X` in GCC or Clang-specific
ways is a huge burden.

As such, we've reverted your patch ([0]). Please add de facto BPF asm
syntax support to GCC-BPF and this change won't be necessary.

  [0] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=665f5d3577ef43e929d59cf39683037887c351bf

>                      "call 12"
>                      :: [ctx]"r"(ctx), [map]"r"(map), [slot]"i"(slot)
>                      : "r0", "r1", "r2", "r3", "r4", "r5");
> --
> 2.34.1
>