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

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 10/11] selftests/bpf: pass all BPF .o's through BPF static linker
Date: Wed, 17 Mar 2021 13:47:11 -0700	[thread overview]
Message-ID: <CAEf4BzZ2TxWqVG=VqFw18--dvC=M5ONRUAde2EB_0yBaYkHb2Q@mail.gmail.com> (raw)
In-Reply-To: <20210317053437.r5zsoksdqrxtt22r@ast-mbp>

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.