[PATCH v2] libbpf: add GCC support for bpf_tail_call_static

[PATCH v2] libbpf: add GCC support for bpf_tail_call_static

[James Hilliard]

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@gmail.com>
---
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
 		     "call 12"
 		     :: [ctx]"r"(ctx), [map]"r"(map), [slot]"i"(slot)
 		     : "r0", "r1", "r2", "r3", "r4", "r5");
-- 
2.34.1

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Mon, 29 Aug 2022 15:05:46 -0600 you 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.
> 
> [...]

Here is the summary with links:
  - [v2] libbpf: add GCC support for bpf_tail_call_static
    https://git.kernel.org/bpf/bpf-next/c/14e5ce79943a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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@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.
>
> 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@gmail.com>
> ---
> 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
>

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

[James Hilliard]

On Fri, Sep 9, 2022 at 12:05 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Aug 29, 2022 at 2:05 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.
> >
> > 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@gmail.com>
> > ---
> > 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.

Is the difference a BPF specific assembly format deviation or a generic
deviation in assembler template syntax between GCC/llvm?
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate

>
> 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
> >

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

[Andrii Nakryiko]

On Fri, Sep 9, 2022 at 11:23 AM James Hilliard
<james.hilliard1@gmail.com> wrote:
>
> On Fri, Sep 9, 2022 at 12:05 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Aug 29, 2022 at 2:05 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.
> > >
> > > 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@gmail.com>
> > > ---
> > > 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.
>
> Is the difference a BPF specific assembly format deviation or a generic
> deviation in assembler template syntax between GCC/llvm?
> https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate
>

Sorry, I don't understand the question. I'm talking about the above
snippet with "r1 = %[ctx]" vs "mov %%r1,%[ctx]". Seems like the rest
stayed the same. So this would be a "BPF specific assembly format"
case, right? I don't know what else could be different with GCC-BPF
assembly.

> >
> > 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
> > >

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

[James Hilliard]

On Fri, Sep 9, 2022 at 12:56 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Sep 9, 2022 at 11:23 AM James Hilliard
> <james.hilliard1@gmail.com> wrote:
> >
> > On Fri, Sep 9, 2022 at 12:05 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Mon, Aug 29, 2022 at 2:05 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.
> > > >
> > > > 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@gmail.com>
> > > > ---
> > > > 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.
> >
> > Is the difference a BPF specific assembly format deviation or a generic
> > deviation in assembler template syntax between GCC/llvm?
> > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate
> >
>
> Sorry, I don't understand the question. I'm talking about the above
> snippet with "r1 = %[ctx]" vs "mov %%r1,%[ctx]". Seems like the rest
> stayed the same. So this would be a "BPF specific assembly format"
> case, right? I don't know what else could be different with GCC-BPF
> assembly.

I mean it's unclear if it's a generic assembly template format difference
that applies to all targets or one that's BPF target specific.

Anyways for now I sent a new patch so that bpf_tail_call_static is defined
on non-clang compilers so that it will work when GCC-BPF supports the
existing asm format.
https://lore.kernel.org/bpf/20220909224544.3702931-1-james.hilliard1@gmail.com/

>
> > >
> > > 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
> > > >

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

[Jose E. Marchesi]

> On Fri, Sep 9, 2022 at 12:56 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Fri, Sep 9, 2022 at 11:23 AM James Hilliard
>> <james.hilliard1@gmail.com> wrote:
>> >
>> > On Fri, Sep 9, 2022 at 12:05 PM Andrii Nakryiko
>> > <andrii.nakryiko@gmail.com> wrote:
>> > >
>> > > On Mon, Aug 29, 2022 at 2:05 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.
>> > > >
>> > > > 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@gmail.com>
>> > > > ---
>> > > > 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.
>> >
>> > Is the difference a BPF specific assembly format deviation or a generic
>> > deviation in assembler template syntax between GCC/llvm?
>> > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate
>> >
>>
>> Sorry, I don't understand the question. I'm talking about the above
>> snippet with "r1 = %[ctx]" vs "mov %%r1,%[ctx]". Seems like the rest
>> stayed the same. So this would be a "BPF specific assembly format"
>> case, right? I don't know what else could be different with GCC-BPF
>> assembly.
>
> I mean it's unclear if it's a generic assembly template format difference
> that applies to all targets or one that's BPF target specific.

It is certainly BPF specific.

I think that when I first wrote the BPF GNU toolchain port the assembly
format used by LLVM was different than it is now: I certainly didn't
invent the syntax the GNU assembler uses for BPF.

It seems LLVM settled with that C-like syntax for assembly instead,
which is very unconventional.

> Anyways for now I sent a new patch so that bpf_tail_call_static is defined
> on non-clang compilers so that it will work when GCC-BPF supports the
> existing asm format.
> https://lore.kernel.org/bpf/20220909224544.3702931-1-james.hilliard1@gmail.com/

I don't think this patch makes much sense: these guards are designed to
avoid compilers that do not support the inline assembly (as presumably
happen with clang < 8) to choke on the header file.  That's also the
case of GCC BPF at the moment.

With this patch, people won't be currentty able to use bpf_helpers.h
with GCC BPF even if they don't use bpf_tail_call_static.

>> > >
>> > > 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
>> > > >

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

[James Hilliard]

On Sat, Sep 10, 2022 at 12:53 AM Jose E. Marchesi
<jose.marchesi@oracle.com> wrote:
>
>
> > On Fri, Sep 9, 2022 at 12:56 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >>
> >> On Fri, Sep 9, 2022 at 11:23 AM James Hilliard
> >> <james.hilliard1@gmail.com> wrote:
> >> >
> >> > On Fri, Sep 9, 2022 at 12:05 PM Andrii Nakryiko
> >> > <andrii.nakryiko@gmail.com> wrote:
> >> > >
> >> > > On Mon, Aug 29, 2022 at 2:05 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.
> >> > > >
> >> > > > 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@gmail.com>
> >> > > > ---
> >> > > > 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.
> >> >
> >> > Is the difference a BPF specific assembly format deviation or a generic
> >> > deviation in assembler template syntax between GCC/llvm?
> >> > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate
> >> >
> >>
> >> Sorry, I don't understand the question. I'm talking about the above
> >> snippet with "r1 = %[ctx]" vs "mov %%r1,%[ctx]". Seems like the rest
> >> stayed the same. So this would be a "BPF specific assembly format"
> >> case, right? I don't know what else could be different with GCC-BPF
> >> assembly.
> >
> > I mean it's unclear if it's a generic assembly template format difference
> > that applies to all targets or one that's BPF target specific.
>
> It is certainly BPF specific.
>
> I think that when I first wrote the BPF GNU toolchain port the assembly
> format used by LLVM was different than it is now: I certainly didn't
> invent the syntax the GNU assembler uses for BPF.
>
> It seems LLVM settled with that C-like syntax for assembly instead,
> which is very unconventional.
>
> > Anyways for now I sent a new patch so that bpf_tail_call_static is defined
> > on non-clang compilers so that it will work when GCC-BPF supports the
> > existing asm format.
> > https://lore.kernel.org/bpf/20220909224544.3702931-1-james.hilliard1@gmail.com/
>
> I don't think this patch makes much sense: these guards are designed to
> avoid compilers that do not support the inline assembly (as presumably
> happen with clang < 8) to choke on the header file.  That's also the
> case of GCC BPF at the moment.
>
> With this patch, people won't be currentty able to use bpf_helpers.h
> with GCC BPF even if they don't use bpf_tail_call_static.

That doesn't seem to be an issue here, from what I can tell it's not
a failure in the compiler but a failure in the assembler, so having
bpf_tail_call_static unguarded in GCC doesn't break anything
that isn't already broken.

>
> >> > >
> >> > > 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
> >> > > >

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

[Jose E. Marchesi]

> On Sat, Sep 10, 2022 at 12:53 AM Jose E. Marchesi
> <jose.marchesi@oracle.com> wrote:
>>
>>
>> > On Fri, Sep 9, 2022 at 12:56 PM Andrii Nakryiko
>> > <andrii.nakryiko@gmail.com> wrote:
>> >>
>> >> On Fri, Sep 9, 2022 at 11:23 AM James Hilliard
>> >> <james.hilliard1@gmail.com> wrote:
>> >> >
>> >> > On Fri, Sep 9, 2022 at 12:05 PM Andrii Nakryiko
>> >> > <andrii.nakryiko@gmail.com> wrote:
>> >> > >
>> >> > > On Mon, Aug 29, 2022 at 2:05 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.
>> >> > > >
>> >> > > > 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@gmail.com>
>> >> > > > ---
>> >> > > > 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.
>> >> >
>> >> > Is the difference a BPF specific assembly format deviation or a generic
>> >> > deviation in assembler template syntax between GCC/llvm?
>> >> > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate
>> >> >
>> >>
>> >> Sorry, I don't understand the question. I'm talking about the above
>> >> snippet with "r1 = %[ctx]" vs "mov %%r1,%[ctx]". Seems like the rest
>> >> stayed the same. So this would be a "BPF specific assembly format"
>> >> case, right? I don't know what else could be different with GCC-BPF
>> >> assembly.
>> >
>> > I mean it's unclear if it's a generic assembly template format difference
>> > that applies to all targets or one that's BPF target specific.
>>
>> It is certainly BPF specific.
>>
>> I think that when I first wrote the BPF GNU toolchain port the assembly
>> format used by LLVM was different than it is now: I certainly didn't
>> invent the syntax the GNU assembler uses for BPF.
>>
>> It seems LLVM settled with that C-like syntax for assembly instead,
>> which is very unconventional.
>>
>> > Anyways for now I sent a new patch so that bpf_tail_call_static is defined
>> > on non-clang compilers so that it will work when GCC-BPF supports the
>> > existing asm format.
>> > https://lore.kernel.org/bpf/20220909224544.3702931-1-james.hilliard1@gmail.com/
>>
>> I don't think this patch makes much sense: these guards are designed to
>> avoid compilers that do not support the inline assembly (as presumably
>> happen with clang < 8) to choke on the header file.  That's also the
>> case of GCC BPF at the moment.
>>
>> With this patch, people won't be currentty able to use bpf_helpers.h
>> with GCC BPF even if they don't use bpf_tail_call_static.
>
> That doesn't seem to be an issue here, from what I can tell it's not
> a failure in the compiler but a failure in the assembler, so having
> bpf_tail_call_static unguarded in GCC doesn't break anything
> that isn't already broken.

IMO it makes it worse, because:

1) With your patch the error message will happen at assembly time
   (invalid syntax in the intermediate .s file mixed with valid syntax)
   pointing to a location in a temporary .S file.  With the guards, you
   get an error at compile time instead, pointing to the undefined
   function in the C source.  IMO it is much easier to figure out what
   is wrong in the second case than in the first.

2) If/when we support the C-like assembly syntax in GCC, you will want
   to guard the function anyway with a GCC_MAJOR > 12 (or whatever) very
   much like it is done with clang >= 8.

>> >> > >
>> >> > > 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
>> >> > > >

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

[James Hilliard]

On Sat, Sep 10, 2022 at 2:43 AM Jose E. Marchesi
<jose.marchesi@oracle.com> wrote:
>
>
> > On Sat, Sep 10, 2022 at 12:53 AM Jose E. Marchesi
> > <jose.marchesi@oracle.com> wrote:
> >>
> >>
> >> > On Fri, Sep 9, 2022 at 12:56 PM Andrii Nakryiko
> >> > <andrii.nakryiko@gmail.com> wrote:
> >> >>
> >> >> On Fri, Sep 9, 2022 at 11:23 AM James Hilliard
> >> >> <james.hilliard1@gmail.com> wrote:
> >> >> >
> >> >> > On Fri, Sep 9, 2022 at 12:05 PM Andrii Nakryiko
> >> >> > <andrii.nakryiko@gmail.com> wrote:
> >> >> > >
> >> >> > > On Mon, Aug 29, 2022 at 2:05 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.
> >> >> > > >
> >> >> > > > 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@gmail.com>
> >> >> > > > ---
> >> >> > > > 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.
> >> >> >
> >> >> > Is the difference a BPF specific assembly format deviation or a generic
> >> >> > deviation in assembler template syntax between GCC/llvm?
> >> >> > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate
> >> >> >
> >> >>
> >> >> Sorry, I don't understand the question. I'm talking about the above
> >> >> snippet with "r1 = %[ctx]" vs "mov %%r1,%[ctx]". Seems like the rest
> >> >> stayed the same. So this would be a "BPF specific assembly format"
> >> >> case, right? I don't know what else could be different with GCC-BPF
> >> >> assembly.
> >> >
> >> > I mean it's unclear if it's a generic assembly template format difference
> >> > that applies to all targets or one that's BPF target specific.
> >>
> >> It is certainly BPF specific.
> >>
> >> I think that when I first wrote the BPF GNU toolchain port the assembly
> >> format used by LLVM was different than it is now: I certainly didn't
> >> invent the syntax the GNU assembler uses for BPF.
> >>
> >> It seems LLVM settled with that C-like syntax for assembly instead,
> >> which is very unconventional.
> >>
> >> > Anyways for now I sent a new patch so that bpf_tail_call_static is defined
> >> > on non-clang compilers so that it will work when GCC-BPF supports the
> >> > existing asm format.
> >> > https://lore.kernel.org/bpf/20220909224544.3702931-1-james.hilliard1@gmail.com/
> >>
> >> I don't think this patch makes much sense: these guards are designed to
> >> avoid compilers that do not support the inline assembly (as presumably
> >> happen with clang < 8) to choke on the header file.  That's also the
> >> case of GCC BPF at the moment.
> >>
> >> With this patch, people won't be currentty able to use bpf_helpers.h
> >> with GCC BPF even if they don't use bpf_tail_call_static.
> >
> > That doesn't seem to be an issue here, from what I can tell it's not
> > a failure in the compiler but a failure in the assembler, so having
> > bpf_tail_call_static unguarded in GCC doesn't break anything
> > that isn't already broken.
>
> IMO it makes it worse, because:
>
> 1) With your patch the error message will happen at assembly time
>    (invalid syntax in the intermediate .s file mixed with valid syntax)
>    pointing to a location in a temporary .S file.  With the guards, you
>    get an error at compile time instead, pointing to the undefined
>    function in the C source.  IMO it is much easier to figure out what
>    is wrong in the second case than in the first.

A compile time error may be somewhat misleading as one would
suspect the issue is with a missing include or something along
those lines, even though the issue is with the inline assembly.

>
> 2) If/when we support the C-like assembly syntax in GCC, you will want
>    to guard the function anyway with a GCC_MAJOR > 12 (or whatever) very
>    much like it is done with clang >= 8.

Wouldn't we need to check the GAS version instead of the GCC
version? AFAIU GAS version isn't known at compile time so a version
check may not make sense here for GCC.

>
> >> >> > >
> >> >> > > 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
> >> >> > > >

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

[Alexei Starovoitov]

On Sat, Sep 10, 2022 at 1:43 AM Jose E. Marchesi
<jose.marchesi@oracle.com> wrote:
>
> 2) If/when we support the C-like assembly syntax in GCC,

Thank you for considering supporting the standard BPF assembly
syntax in GCC.
I agree that C-like asm looks unusual.
The main reason to pick that style was the ease of understanding
and to avoid gnu vs intel asm order confusion.
We didn't want to deal with question whether 'mov r1, r2'
means r1->r2 or r2->r1. The C style asm r1=r2 is unambiguous.