Re: [PATCH bpf-next 1/3] bpftool: add extra CO-RE mode to btf dump command

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Dec 19, 2019 at 9:06 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Dec 18, 2019 at 11:06:56PM -0800, Andrii Nakryiko wrote:
> > +     if (core_mode) {
> > +             printf("#if defined(__has_attribute) && __has_attribute(preserve_access_index)\n");
> > +             printf("#define __CLANG_BPF_CORE_SUPPORTED\n");
> > +             printf("#pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)\n");
> > +             printf("#endif\n\n");
>
> I think it's dangerous to automatically opt-out when clang is not new enough.
> bpf prog will compile fine, but it will be missing co-re relocations.
> How about doing something like:
>   printf("#ifdef NEEDS_CO_RE\n");
>   printf("#pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)\n");
>   printf("#endif\n\n");
> and emit it always when 'format c'.
> Then on the program side it will look:
> #define NEEDS_CO_RE
> #include "vmlinux.h"
> If clang is too old there will be a compile time error which is a good thing.
> Future features will have different NEEDS_ macros.

Wouldn't it be cleaner to separate vanilla C types dump vs
CO-RE-specific one? I'd prefer to have them separate and not require
every application to specify this #define NEEDS_CO_RE macro.
Furthermore, later we probably are going to add some additional
auto-generated types, definitions, etc, so plain C types dump and
CO-RE-specific one will deviate quite a bit. So it feels cleaner to
separate them now instead of polluting `format c` with irrelevant
noise.

I can unconditionally assume preserve_access_index availability,
though, because Clang 10 release is going to have all those features
needed for BPF CO-RE. I can also add nicer compiler error, if this
feature is not detected. Ok?

BTW, the reason I added this opt-out is because if you use
bpf_core_read() and BPF_CORE_READ() macros, you don't really need
those structs marked as relocatable. But again, I think it's fine to
just assume it has to be supported by compiler.