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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux