On Sat, 12 Jun 2021 at 00:33, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> 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
