Re: [PATCH v5 bpf-next 9/9] selftests/bpf: Add test for resolve_btfids

[Jiri Olsa <jolsa@redhat.com>]

On Tue, Jul 07, 2020 at 10:49:22AM -0700, Andrii Nakryiko wrote:

SNIP

> > > >  # Get Clang's default includes on this system, as opposed to those seen by
> > > >  # '-target bpf'. This fixes "missing" files on some architectures/distros,
> > > >  # such as asm/byteorder.h, asm/socket.h, asm/sockios.h, sys/cdefs.h etc.
> > > > @@ -333,7 +343,8 @@ $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o:                   \
> > > >                       $(TRUNNER_BPF_SKELS)                              \
> > > >                       $$(BPFOBJ) | $(TRUNNER_OUTPUT)
> > > >         $$(call msg,TEST-OBJ,$(TRUNNER_BINARY),$$@)
> > > > -       cd $$(@D) && $$(CC) -I. $$(CFLAGS) -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F)
> > > > +       cd $$(@D) && $$(CC) -I. $$(CFLAGS) $(TRUNNER_EXTRA_CFLAGS)      \
> > > > +       -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F)
> > > >
> > > >  $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o:                          \
> > > >                        %.c                                              \
> > > > @@ -355,6 +366,7 @@ $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)                   \
> > > >                              | $(TRUNNER_BINARY)-extras
> > > >         $$(call msg,BINARY,,$$@)
> > > >         $$(CC) $$(CFLAGS) $$(filter %.a %.o,$$^) $$(LDLIBS) -o $$@
> > > > +       $(TRUNNER_BINARY_EXTRA_CMD)
> > >
> > > no need to make this generic, just write out resolve_btfids here explicitly
> >
> > currently resolve_btfids fails if there's no .BTF.ids section found,
> > but we can make it silently pass i nthis case and then we can invoke
> > it for all the binaries
> 
> ah, I see. Yeah, either we can add an option to resolve_btfids to not
> error when .BTF_ids is missing (probably best), or we can check
> whether the test has .BTF_ids section, and if it does - run
> resolve_btfids on it. Just ignoring errors always is more error-prone,
> because we won't know if it's a real problem we are ignoring, or
> missing .BTF_ids.

ok, sounds good

> > > > +static int resolve_symbols(void)
> > > > +{
> > > > +       const char *path = VMLINUX_BTF;
> > >
> > >
> > > This build-time parameter passing to find the original VMLINUX_BTF
> > > really sucks, IMO.
> > >
> > > Why not use the btf_dump tests approach and have our own small
> > > "vmlinux BTF", which resolve_btfids would use to resolve these IDs?
> > > See how btf_dump_xxx.c files define BTFs that are used in tests. You
> > > can do something similar here, and use a well-known BPF object file as
> > > a source of BTF, both here in a test and in Makefile for --btf param
> > > to resolve_btfids?
> >
> > well VMLINUX_BTF is there and those types are used are not going
> > away any time soon ;-) but yea, we can do that.. we do this also
> > for bpftrace, it's nicer
> 
> 
> "VMLINUX_BTF is there" is not really true in a lot of more complicated
> setups, which is why I'd like to avoid that assumption. E.g., for
> libbpf Travis CI, we build self-tests in one VM, but run the binary in
> a different VM. So either vmlinux itself or the path to it might
> change.

ok

> 
> Also, having full control over **small** BTF allows to create various
> test situations that might be harder to pinpoint in real vmlinux BTF,
> e.g., same-named entities with different KINDS (typedef vs struct,
> etc). Then if that fails, debugging this on a small BTF is much-much
> easier than on a real thing. Real vmlinux BTF is being tested each
> time you build a kernel and run selftests inside VM either way, so I
> don't think we lose anything in terms of coverage.

agreed, will add that

thanks,
jirka