On Wed, May 17, 2023 at 8:02 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote:
>
> 2023-05-16 14:30 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> > On Fri, May 12, 2023 at 3:34 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote:
> >>
> >> From: Alexander Lobakin <alobakin@xxxxx>
> >>
> >> Fix the following error when building bpftool:
> >>
> >> CLANG profiler.bpf.o
> >> CLANG pid_iter.bpf.o
> >> skeleton/profiler.bpf.c:18:21: error: invalid application of 'sizeof' to an incomplete type 'struct bpf_perf_event_value'
> >> __uint(value_size, sizeof(struct bpf_perf_event_value));
> >> ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helpers.h:13:39: note: expanded from macro '__uint'
> >> tools/bpf/bpftool/bootstrap/libbpf/include/bpf/bpf_helper_defs.h:7:8: note: forward declaration of 'struct bpf_perf_event_value'
> >> struct bpf_perf_event_value;
> >> ^
> >>
> >> struct bpf_perf_event_value is being used in the kernel only when
> >> CONFIG_BPF_EVENTS is enabled, so it misses a BTF entry then.
> >> Define struct bpf_perf_event_value___local with the
> >> `preserve_access_index` attribute inside the pid_iter BPF prog to
> >> allow compiling on any configs. It is a full mirror of a UAPI
> >> structure, so is compatible both with and w/o CO-RE.
> >> bpf_perf_event_read_value() requires a pointer of the original type,
> >> so a cast is needed.
> >>
> >> Fixes: 47c09d6a9f67 ("bpftool: Introduce "prog profile" command")
> >> Suggested-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> >> Signed-off-by: Alexander Lobakin <alobakin@xxxxx>
> >> Signed-off-by: Quentin Monnet <quentin@xxxxxxxxxxxxx>
> >> ---
> >
> > What's the point of using vmlinux.h at all if we redefine every single
> > type? bpf_perf_event_value is part of BPF UAPI, so if we included
> > linux/bpf.h header we'd get it.
>
> I gave a quick try at the UAPI header before posting this patch, but it
> was an Ubuntu box and I got the "asm/types.h not found" error. If I
> remember correctly, one way to fix this is to have the gcc-multilib,
> which I'd rather avoid to add as a dependency; or adding the correct
> include path for x86_64 at least, which I haven't tried for bpftool yet.
>
> >
> > This feels a bit split-brained. We either drop vmlinux.h completely
> > and use UAPI headers + CO-RE-relocatable definitions of internal
> > types, or we make sure that vmlinux.h does work (e.g., by pre-checking
> > in a very small version of it). Both using vmlinux.h and not relying
> > on it having necessary types seems like the worst of both worlds?...
>
> Yeah I do feel like I'm missing something in this set and the approach
> is not optimal. What do you mean exactly by "pre-checking in a very
> small version of it"? Checking for the availability of a few types and
> exit early from the functions if they're missing, because we're assuming
> we won't have support for the feature?
So I was thinking that we'll keep relying on vmlinux BTF and proper
Kconfig for bpftool build inside kernel repo, no changes there. But
then we can use `bpftool gen min_core_btf` on resulting .bpf.o files
to dump only types that are actually used/referenced by bpftool's BPF
object files, and then we can generate vmlinux.h from that minimized
BTF.
I haven't tried it, and I'm sure there will be some hiccups along the
way, but that was the idea.
>
> > Quentin, can you see if you can come up with some simple way to use
> > vmlinux.h if building from inside kernel repo, but using a minimized
> > vmlinux.h generated using `bpftool gen min_core_btf` when building
> > from Github mirror? Sync script could also generate this minimal
> > vmlinux.h automatically, presumably?
>
> Sure, I can look into it. But not right now - I'd like to get the
> current issue, and the (unrelated) LLVM feature detection, sorted before
> starting on this.
Sounds good. In general, your patches look good to me, I was hoping we
can avoid unnecessary definitions of UAPI types, but if that doesn't
work out of the box, then I'm fine with it.
>
> Thanks for your feedback!
> Quentin
