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

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@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 14:00:33 -0700	[thread overview]
Message-ID: <CAADnVQ+1w8rqGNax6PysTh5SwpdKr8dd3TmCEvwDZ2+=ouTRkA@mail.gmail.com> (raw)
In-Reply-To: <CAEf4BzZ2TxWqVG=VqFw18--dvC=M5ONRUAde2EB_0yBaYkHb2Q@mail.gmail.com>

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?
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?