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