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?
> 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.
Thanks for your feedback!
Quentin
