| Message ID | 20210313193537.1548766-11-andrii@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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? bpftool in the previous patch doesn't really care about the extension. It's still a valid object file with the same ELF format. 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. 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 -
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.
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?
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
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore index 4866f6a21901..811da0ea3ecd 100644 --- a/tools/testing/selftests/bpf/.gitignore +++ b/tools/testing/selftests/bpf/.gitignore @@ -36,4 +36,5 @@ test_cpp /runqslower /bench *.ko +*.bpfo xdpxceiver diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index dbca39f45382..7ed5690be237 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -355,12 +355,10 @@ $(TRUNNER_BPF_OBJS): $(TRUNNER_OUTPUT)/%.o: \ $$(call $(TRUNNER_BPF_BUILD_RULE),$$<,$$@, \ $(TRUNNER_BPF_CFLAGS)) -$(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) > $$@ endif # ensure we set up tests.h header generation rule just once
Pass all individual BPF object files (progs/*.o) through `bpftool gen object` command to validate that BPF static linker doesn't corrupt them. Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- tools/testing/selftests/bpf/.gitignore | 1 + tools/testing/selftests/bpf/Makefile | 8 +++----- 2 files changed, 4 insertions(+), 5 deletions(-)