On Mon, Jun 14, 2021 at 2:21 AM Lorenz Bauer <lmb@xxxxxxxxxxxxxx> wrote:
>
> 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.
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
