Re: [PATCH v2 bpf-next 10/11] selftests/bpf: pass all BPF .o's through BPF static linker

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

On Wed, Mar 17, 2021 at 2:00 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Mar 17, 2021 at 1:47 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Mar 16, 2021 at 10:34 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Sat, Mar 13, 2021 at 11:35:36AM -0800, Andrii Nakryiko wrote:
> > > >
> > > > -$(TRUNNER_BPF_SKELS): $(TRUNNER_OUTPUT)/%.skel.h:                    \
> > > > -                   $(TRUNNER_OUTPUT)/%.o                             \
> > > > -                   $(BPFTOOL)                                        \
> > > > -                   | $(TRUNNER_OUTPUT)
> > > > +$(TRUNNER_BPF_SKELS): %.skel.h: %.o $(BPFTOOL) | $(TRUNNER_OUTPUT)
> > > >       $$(call msg,GEN-SKEL,$(TRUNNER_BINARY),$$@)
> > > > -     $(Q)$$(BPFTOOL) gen skeleton $$< > $$@
> > > > +     $(Q)$$(BPFTOOL) gen object $$(<:.o=.bpfo) $$<
> > > > +     $(Q)$$(BPFTOOL) gen skeleton $$(<:.o=.bpfo) > $$@
> > >
> > > Do we really need this .bpfo extension?
> >
> > I thought it would be a better way to avoid user's confusion with .o's
> > as produced by compiler and .bpfo as a "final" linked BPF object,
> > produced by static linker. Technically, there is no requirement, of
> > course. If you think it will be less confusing to stick to .o, that's
> > fine.
> >
> > > bpftool in the previous patch doesn't really care about the extension.
> >
> > the only thing that cares is the logic to derive object name when
> > generating skeleton (we strip .o and/or .bpfo). No loader should ever
> > care about extension, it could be my_obj.whocares and it should be
> > fine.
> >
> > > It's still a valid object file with the same ELF format.
> >
> > Yes, with some extra niceties like fixed up BTF, stripped out DWARF,
> > etc. Maybe in the future there will be more "normalization" done as
> > compared to what Clang produces.
> >
> > > I think if we keep the same .o extension for linked .o-s it will be easier.
> > > Otherwise all loaders would need to support both .o and .bpfo,
> > > but the later is no different than the former in terms of contents of the file
> > > and ways to parse it.
> >
> > So no loaders should care right now. But as I said, I can drop .bpfo as well.
> >
> > >
> > > For testing of the linker this linked .o can be a temp file or better yet a unix pipe ?
> > > bpftool gen object - one.o second.o|bpftool gen skeleton -
> >
> > So I tried to briefly add support for that to `gen skeleton` and `gen
> > object` by using /proc/self/fd/{0,1} and that works for `gen object`,
> > but only if stdout is redirected to a real file. When piping output to
> > another process, libelf fails to write to such a special file for some
> > reason. `gen skeleton` is also failing to read from a piped stdin
> > because of use of mmap(). So there would need to be more work done to
> > support piping like that.
> >
> > But in any case I'd like to have those intermediate object file
> > results lying on disk for further inspection, if anything isn't right,
> > so I'll use temp file regardless.
>
> May keep those temp .o files with .linked.o suffix?

sure, no problem

> Also have you considered doing:
> clang -target bpf prog.c -o prog.o
> bpftool gen obj obj1.o prog.o
> bpftool gen obj obj2.o obj1.o
> diff obj1.o obj2.o
> They should be the same, right?

yeah, I can add check for that