[PATCH bpf] lib: bpf: tracing: fail compilation if target arch is missing

[PATCH bpf] lib: bpf: tracing: fail compilation if target arch is missing

[Lorenz Bauer]

bpf2go is the Go equivalent of libbpf skeleton. The convention is that
the compiled BPF is checked into the repository to facilitate distributing
BPF as part of Go packages. To make this portable, bpf2go by default
generates both bpfel and bpfeb variants of the C.

Using bpf_tracing.h is inherently non-portable since the fields of
struct pt_regs differ between platforms, so CO-RE can't help us here.
The only way of working around this is to compile for each target
platform independently. bpf2go can't do this by default since there
are too many platforms.

Define the various PT_... macros when no target can be determined and
turn them into compilation failures. This works because bpf2go always
compiles for bpf targets, so the compiler fallback doesn't kick in.
Conditionally define __bpf_missing_target so that we can inject a
more appropriate error message at build time. The user can then
choose which platform to target explicitly.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
---
 tools/lib/bpf/bpf_tracing.h | 46 +++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)

diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
index c0f3a26aa582..438174adb3f8 100644
--- a/tools/lib/bpf/bpf_tracing.h
+++ b/tools/lib/bpf/bpf_tracing.h
@@ -25,26 +25,35 @@
 	#define bpf_target_sparc
 	#define bpf_target_defined
 #else
-	#undef bpf_target_defined
-#endif
 
 /* Fall back to what the compiler says */
-#ifndef bpf_target_defined
 #if defined(__x86_64__)
 	#define bpf_target_x86
+	#define bpf_target_defined
 #elif defined(__s390__)
 	#define bpf_target_s390
+	#define bpf_target_defined
 #elif defined(__arm__)
 	#define bpf_target_arm
+	#define bpf_target_defined
 #elif defined(__aarch64__)
 	#define bpf_target_arm64
+	#define bpf_target_defined
 #elif defined(__mips__)
 	#define bpf_target_mips
+	#define bpf_target_defined
 #elif defined(__powerpc__)
 	#define bpf_target_powerpc
+	#define bpf_target_defined
 #elif defined(__sparc__)
 	#define bpf_target_sparc
+	#define bpf_target_defined
+#endif /* no compiler target */
+
 #endif
+
+#ifndef __bpf_target_missing
+#define __bpf_target_missing "GCC error \"Must specify a target arch via __TARGET_ARCH_xxx\""
 #endif
 
 #if defined(bpf_target_x86)
@@ -287,7 +296,7 @@ struct pt_regs;
 #elif defined(bpf_target_sparc)
 #define BPF_KPROBE_READ_RET_IP(ip, ctx)		({ (ip) = PT_REGS_RET(ctx); })
 #define BPF_KRETPROBE_READ_RET_IP		BPF_KPROBE_READ_RET_IP
-#else
+#elif defined(bpf_target_defined)
 #define BPF_KPROBE_READ_RET_IP(ip, ctx)					    \
 	({ bpf_probe_read_kernel(&(ip), sizeof(ip), (void *)PT_REGS_RET(ctx)); })
 #define BPF_KRETPROBE_READ_RET_IP(ip, ctx)				    \
@@ -295,6 +304,35 @@ struct pt_regs;
 			  (void *)(PT_REGS_FP(ctx) + sizeof(ip))); })
 #endif
 
+#if !defined(bpf_target_defined)
+
+#define PT_REGS_PARM1(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_PARM2(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_PARM3(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_PARM4(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_PARM5(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_RET(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_FP(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_RC(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_SP(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_IP(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+
+#define PT_REGS_PARM1_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_PARM2_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_PARM3_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_PARM4_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_PARM5_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_RET_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_FP_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_RC_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_SP_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define PT_REGS_IP_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
+
+#define BPF_KPROBE_READ_RET_IP(ip, ctx) ({ _Pragma(__bpf_target_missing); 0ull; })
+#define BPF_KRETPROBE_READ_RET_IP(ip, ctx) ({ _Pragma(__bpf_target_missing); 0ull; })
+
+#endif /* !defined(bpf_target_defined) */
+
 #ifndef ___bpf_concat
 #define ___bpf_concat(a, b) a ## b
 #endif
-- 
2.30.2

Re: [PATCH bpf] lib: bpf: tracing: fail compilation if target arch is missing

[Andrii Nakryiko]

On Thu, Jun 10, 2021 at 9:10 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> bpf2go is the Go equivalent of libbpf skeleton. The convention is that
> the compiled BPF is checked into the repository to facilitate distributing
> BPF as part of Go packages. To make this portable, bpf2go by default
> generates both bpfel and bpfeb variants of the C.
>
> Using bpf_tracing.h is inherently non-portable since the fields of
> struct pt_regs differ between platforms, so CO-RE can't help us here.
> The only way of working around this is to compile for each target
> platform independently. bpf2go can't do this by default since there
> are too many platforms.
>
> Define the various PT_... macros when no target can be determined and
> turn them into compilation failures. This works because bpf2go always
> compiles for bpf targets, so the compiler fallback doesn't kick in.
> Conditionally define __bpf_missing_target so that we can inject a
> more appropriate error message at build time. The user can then
> choose which platform to target explicitly.
>
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> ---
>  tools/lib/bpf/bpf_tracing.h | 46 +++++++++++++++++++++++++++++++++----
>  1 file changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index c0f3a26aa582..438174adb3f8 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -25,26 +25,35 @@
>         #define bpf_target_sparc
>         #define bpf_target_defined
>  #else
> -       #undef bpf_target_defined
> -#endif
>
>  /* Fall back to what the compiler says */
> -#ifndef bpf_target_defined

Hm... doesn't this auto-guessing based on host architecture defeats
your goal? You don't want bpf_tracing.h to guess the right set of
PT_REGS macros, no?

I thought you'll do something like

#ifndef bpf_target_guess
#define bpf_target_guess 1
#endif

#if !defined(bpf_target_defined) && bpf_target_guess

/* then try to use host architecture */

But I guess I'm missing something...

>  #if defined(__x86_64__)
>         #define bpf_target_x86
> +       #define bpf_target_defined
>  #elif defined(__s390__)
>         #define bpf_target_s390
> +       #define bpf_target_defined

btw, instead of having this zoo of bpf_target_<arch> and also
bpf_traget_defined, how about simplifying it to a single variable that
would contain the actual architecture:

#define BPF_TARGET_ARCH "s390"

And then do

#if BPF_TARGET_ARCH == "s390"
#elif BPF_TARGET_ARCH == "arm"
...
#else /* unknown bpf_target_arch or not defined */
_Pragma(...)
#endif

WDYT? We can eventually move away from weird-looking __TARGET_ARCH_x86
to just -DBPF_TARGET_ARCH=x86. We'll need to support __TARGET_ARCH_xxx
for backwards compatibility, but at least new use cases can be cleaner
and more meaningful.

>  #elif defined(__arm__)
>         #define bpf_target_arm
> +       #define bpf_target_defined
>  #elif defined(__aarch64__)
>         #define bpf_target_arm64
> +       #define bpf_target_defined
>  #elif defined(__mips__)
>         #define bpf_target_mips
> +       #define bpf_target_defined
>  #elif defined(__powerpc__)
>         #define bpf_target_powerpc
> +       #define bpf_target_defined
>  #elif defined(__sparc__)
>         #define bpf_target_sparc
> +       #define bpf_target_defined
> +#endif /* no compiler target */
> +
>  #endif
> +
> +#ifndef __bpf_target_missing
> +#define __bpf_target_missing "GCC error \"Must specify a target arch via __TARGET_ARCH_xxx\""

If you goal is to customize the error message, why not parameterize
the error message part only, not the "GCC error \"\"" part?

>  #endif
>
>  #if defined(bpf_target_x86)
> @@ -287,7 +296,7 @@ struct pt_regs;
>  #elif defined(bpf_target_sparc)
>  #define BPF_KPROBE_READ_RET_IP(ip, ctx)                ({ (ip) = PT_REGS_RET(ctx); })
>  #define BPF_KRETPROBE_READ_RET_IP              BPF_KPROBE_READ_RET_IP
> -#else
> +#elif defined(bpf_target_defined)
>  #define BPF_KPROBE_READ_RET_IP(ip, ctx)                                            \
>         ({ bpf_probe_read_kernel(&(ip), sizeof(ip), (void *)PT_REGS_RET(ctx)); })
>  #define BPF_KRETPROBE_READ_RET_IP(ip, ctx)                                 \
> @@ -295,6 +304,35 @@ struct pt_regs;
>                           (void *)(PT_REGS_FP(ctx) + sizeof(ip))); })
>  #endif
>
> +#if !defined(bpf_target_defined)
> +
> +#define PT_REGS_PARM1(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_PARM2(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_PARM3(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_PARM4(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_PARM5(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_RET(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_FP(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_RC(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_SP(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_IP(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +
> +#define PT_REGS_PARM1_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_PARM2_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_PARM3_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_PARM4_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_PARM5_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_RET_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_FP_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_RC_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_SP_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define PT_REGS_IP_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })

nit: why ull suffix?

> +
> +#define BPF_KPROBE_READ_RET_IP(ip, ctx) ({ _Pragma(__bpf_target_missing); 0ull; })
> +#define BPF_KRETPROBE_READ_RET_IP(ip, ctx) ({ _Pragma(__bpf_target_missing); 0ull; })
> +
> +#endif /* !defined(bpf_target_defined) */
> +
>  #ifndef ___bpf_concat
>  #define ___bpf_concat(a, b) a ## b
>  #endif
> --
> 2.30.2
>

Re: [PATCH bpf] lib: bpf: tracing: fail compilation if target arch is missing

[Lorenz Bauer]

On Sat, 12 Jun 2021 at 00:33, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> Hm... doesn't this auto-guessing based on host architecture defeats
> your goal? You don't want bpf_tracing.h to guess the right set of
> PT_REGS macros, no?
>
> I thought you'll do something like
>
> #ifndef bpf_target_guess
> #define bpf_target_guess 1
> #endif
>
> #if !defined(bpf_target_defined) && bpf_target_guess
>
> /* then try to use host architecture */
>
> But I guess I'm missing something...

I understood that you didn't want new defines :D I'll rework the patch.

>
> >  #if defined(__x86_64__)
> >         #define bpf_target_x86
> > +       #define bpf_target_defined
> >  #elif defined(__s390__)
> >         #define bpf_target_s390
> > +       #define bpf_target_defined
>
> btw, instead of having this zoo of bpf_target_<arch> and also
> bpf_traget_defined, how about simplifying it to a single variable that
> would contain the actual architecture:
>
> #define BPF_TARGET_ARCH "s390"
>
> And then do
>
> #if BPF_TARGET_ARCH == "s390"
> #elif BPF_TARGET_ARCH == "arm"
> ...
> #else /* unknown bpf_target_arch or not defined */
> _Pragma(...)
> #endif
>
> WDYT? We can eventually move away from weird-looking __TARGET_ARCH_x86
> to just -DBPF_TARGET_ARCH=x86. We'll need to support __TARGET_ARCH_xxx
> for backwards compatibility, but at least new use cases can be cleaner
> and more meaningful.

Yeah that would be nice. I think the preprocessor doesn't understand
strings. So we'd have to use special integers (or more macros) or a
char. That doesn't seem nicer.

> > +#ifndef __bpf_target_missing
> > +#define __bpf_target_missing "GCC error \"Must specify a target arch via __TARGET_ARCH_xxx\""
>
> If you goal is to customize the error message, why not parameterize
> the error message part only, not the "GCC error \"\"" part?

Because _Pragma is annoying: it doesn't accept strings that get
concatenated via the preprocessor: _Pragma("GCC error \"" OTHER_DEFINE
"\"") gives me trouble. It's possible to avoid this via another macro
expansion though. Up to you.

>
> >  #endif
> >
> >  #if defined(bpf_target_x86)
> > @@ -287,7 +296,7 @@ struct pt_regs;
> >  #elif defined(bpf_target_sparc)
> >  #define BPF_KPROBE_READ_RET_IP(ip, ctx)                ({ (ip) = PT_REGS_RET(ctx); })
> >  #define BPF_KRETPROBE_READ_RET_IP              BPF_KPROBE_READ_RET_IP
> > -#else
> > +#elif defined(bpf_target_defined)
> >  #define BPF_KPROBE_READ_RET_IP(ip, ctx)                                            \
> >         ({ bpf_probe_read_kernel(&(ip), sizeof(ip), (void *)PT_REGS_RET(ctx)); })
> >  #define BPF_KRETPROBE_READ_RET_IP(ip, ctx)                                 \
> > @@ -295,6 +304,35 @@ struct pt_regs;
> >                           (void *)(PT_REGS_FP(ctx) + sizeof(ip))); })
> >  #endif
> >
> > +#if !defined(bpf_target_defined)
> > +
> > +#define PT_REGS_PARM1(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_PARM2(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_PARM3(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_PARM4(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_PARM5(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_RET(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_FP(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_RC(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_SP(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_IP(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +
> > +#define PT_REGS_PARM1_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_PARM2_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_PARM3_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_PARM4_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_PARM5_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_RET_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_FP_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_RC_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_SP_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > +#define PT_REGS_IP_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
>
> nit: why ull suffix?

Without it we sometimes get an integer cast warning, something about
an int to void* cast I think?

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf] lib: bpf: tracing: fail compilation if target arch is missing

[Andrii Nakryiko]

On Mon, Jun 14, 2021 at 2:21 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Sat, 12 Jun 2021 at 00:33, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > Hm... doesn't this auto-guessing based on host architecture defeats
> > your goal? You don't want bpf_tracing.h to guess the right set of
> > PT_REGS macros, no?
> >
> > I thought you'll do something like
> >
> > #ifndef bpf_target_guess
> > #define bpf_target_guess 1
> > #endif
> >
> > #if !defined(bpf_target_defined) && bpf_target_guess
> >
> > /* then try to use host architecture */
> >
> > But I guess I'm missing something...
>
> I understood that you didn't want new defines :D I'll rework the patch.

It doesn't seem avoidable. But I'm surprised you are satisfied with
your patch, it doesn't seem to solve your problem, because you'll
never trigger those _Pragmas as you'll just fallback to using your
host architecture. Isn't that right? How did you test your patch?

>
> >
> > >  #if defined(__x86_64__)
> > >         #define bpf_target_x86
> > > +       #define bpf_target_defined
> > >  #elif defined(__s390__)
> > >         #define bpf_target_s390
> > > +       #define bpf_target_defined
> >
> > btw, instead of having this zoo of bpf_target_<arch> and also
> > bpf_traget_defined, how about simplifying it to a single variable that
> > would contain the actual architecture:
> >
> > #define BPF_TARGET_ARCH "s390"
> >
> > And then do
> >
> > #if BPF_TARGET_ARCH == "s390"
> > #elif BPF_TARGET_ARCH == "arm"
> > ...
> > #else /* unknown bpf_target_arch or not defined */
> > _Pragma(...)
> > #endif
> >
> > WDYT? We can eventually move away from weird-looking __TARGET_ARCH_x86
> > to just -DBPF_TARGET_ARCH=x86. We'll need to support __TARGET_ARCH_xxx
> > for backwards compatibility, but at least new use cases can be cleaner
> > and more meaningful.
>
> Yeah that would be nice. I think the preprocessor doesn't understand
> strings. So we'd have to use special integers (or more macros) or a
> char. That doesn't seem nicer.

Yeah, somehow reading some online docs I got the impression that
strings are supported, but you are right, it doesn't seem like they
are :( Never mind then.

>
> > > +#ifndef __bpf_target_missing
> > > +#define __bpf_target_missing "GCC error \"Must specify a target arch via __TARGET_ARCH_xxx\""
> >
> > If you goal is to customize the error message, why not parameterize
> > the error message part only, not the "GCC error \"\"" part?
>
> Because _Pragma is annoying: it doesn't accept strings that get
> concatenated via the preprocessor: _Pragma("GCC error \"" OTHER_DEFINE
> "\"") gives me trouble. It's possible to avoid this via another macro
> expansion though. Up to you.

argh... that's fine, let's leave it as is

>
> >
> > >  #endif
> > >
> > >  #if defined(bpf_target_x86)
> > > @@ -287,7 +296,7 @@ struct pt_regs;
> > >  #elif defined(bpf_target_sparc)
> > >  #define BPF_KPROBE_READ_RET_IP(ip, ctx)                ({ (ip) = PT_REGS_RET(ctx); })
> > >  #define BPF_KRETPROBE_READ_RET_IP              BPF_KPROBE_READ_RET_IP
> > > -#else
> > > +#elif defined(bpf_target_defined)
> > >  #define BPF_KPROBE_READ_RET_IP(ip, ctx)                                            \
> > >         ({ bpf_probe_read_kernel(&(ip), sizeof(ip), (void *)PT_REGS_RET(ctx)); })
> > >  #define BPF_KRETPROBE_READ_RET_IP(ip, ctx)                                 \
> > > @@ -295,6 +304,35 @@ struct pt_regs;
> > >                           (void *)(PT_REGS_FP(ctx) + sizeof(ip))); })
> > >  #endif
> > >
> > > +#if !defined(bpf_target_defined)
> > > +
> > > +#define PT_REGS_PARM1(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_PARM2(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_PARM3(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_PARM4(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_PARM5(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_RET(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_FP(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_RC(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_SP(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_IP(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +
> > > +#define PT_REGS_PARM1_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_PARM2_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_PARM3_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_PARM4_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_PARM5_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_RET_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_FP_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_RC_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_SP_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> > > +#define PT_REGS_IP_CORE(x) ({ _Pragma(__bpf_target_missing); 0ull; })
> >
> > nit: why ull suffix?
>
> Without it we sometimes get an integer cast warning, something about
> an int to void* cast I think?

hmm.. ok

>
> --
> Lorenz Bauer  |  Systems Engineer
> 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
>
> www.cloudflare.com

Re: [PATCH bpf] lib: bpf: tracing: fail compilation if target arch is missing

[Lorenz Bauer]

On Tue, 15 Jun 2021 at 00:27, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> It doesn't seem avoidable. But I'm surprised you are satisfied with
> your patch, it doesn't seem to solve your problem, because you'll
> never trigger those _Pragmas as you'll just fallback to using your
> host architecture. Isn't that right? How did you test your patch?

I tested the patch by removing -D__TARGET_ARCH_$(SRCARCH) from
BPF_CFLAGS in the Makefile. The pragmas are triggered because the
testsuite compiles with -target bpf. This prevents the "host arch"
fallback from activating. bpf2go specifies -target bpf(el|eb) as well,
so any users will get the _Pragma if they use a new enough
bpf_tracing.h.

> >
> > Without it we sometimes get an integer cast warning, something about
> > an int to void* cast I think?
>
> hmm.. ok

This is the error I get:

progs/lsm.c:166:14: warning: cast to 'void *' from smaller integer
type 'int' [-Wint-to-void-pointer-cast]
        void *ptr = (void *)PT_REGS_PARM1(regs);
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

Re: [PATCH bpf] lib: bpf: tracing: fail compilation if target arch is missing

[Andrii Nakryiko]

On Tue, Jun 15, 2021 at 3:11 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> On Tue, 15 Jun 2021 at 00:27, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > It doesn't seem avoidable. But I'm surprised you are satisfied with
> > your patch, it doesn't seem to solve your problem, because you'll
> > never trigger those _Pragmas as you'll just fallback to using your
> > host architecture. Isn't that right? How did you test your patch?
>
> I tested the patch by removing -D__TARGET_ARCH_$(SRCARCH) from
> BPF_CFLAGS in the Makefile. The pragmas are triggered because the
> testsuite compiles with -target bpf. This prevents the "host arch"
> fallback from activating. bpf2go specifies -target bpf(el|eb) as well,
> so any users will get the _Pragma if they use a new enough
> bpf_tracing.h.

Oh, I didn't realize  -target bpf will prevent host architecture
fallback. In that case we don't need a new #define, cool.

>
> > >
> > > Without it we sometimes get an integer cast warning, something about
> > > an int to void* cast I think?
> >
> > hmm.. ok
>
> This is the error I get:
>
> progs/lsm.c:166:14: warning: cast to 'void *' from smaller integer
> type 'int' [-Wint-to-void-pointer-cast]
>         void *ptr = (void *)PT_REGS_PARM1(regs);
>                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~

oh, ok, but then those zeros probably best to mark as longs, not long
longs (even thought for BPF it's the same), as (unsigned) long is a
logical equivalent of a pointer, right?

>
> --
> Lorenz Bauer  |  Systems Engineer
> 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK
>
> www.cloudflare.com

Re: [PATCH bpf] lib: bpf: tracing: fail compilation if target arch is missing

[Lorenz Bauer]

On Tue, 15 Jun 2021 at 19:53, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> >
> > > >
> > > > Without it we sometimes get an integer cast warning, something about
> > > > an int to void* cast I think?
> > >
> > > hmm.. ok
> >
> > This is the error I get:
> >
> > progs/lsm.c:166:14: warning: cast to 'void *' from smaller integer
> > type 'int' [-Wint-to-void-pointer-cast]
> >         void *ptr = (void *)PT_REGS_PARM1(regs);
> >                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> oh, ok, but then those zeros probably best to mark as longs, not long
> longs (even thought for BPF it's the same), as (unsigned) long is a
> logical equivalent of a pointer, right?

Ack, that seems to work as well. I sent a v2, also renamed
__bpf_target_missing to __BPF_TARGET_MISSING.

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com