| Message ID | 20230112004024.1934601-1-irogers@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [v1] tools/resolve_btfids: Install subcmd headers | expand |
On Wed, Jan 11, 2023 at 04:40:24PM -0800, Ian Rogers wrote: > Previously tools/lib/subcmd was added to the include path, switch to > installing the headers and then including from that directory. This > avoids dependencies on headers internal to tools/lib/subcmd. Add the > missing subcmd directory to the affected #include. > > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/bpf/resolve_btfids/Makefile | 19 ++++++++++++++----- > tools/bpf/resolve_btfids/main.c | 2 +- > 2 files changed, 15 insertions(+), 6 deletions(-) this depends on changes that went to Arnaldo's tree right? I can't apply this on bpf-next/master > > diff --git a/tools/bpf/resolve_btfids/Makefile b/tools/bpf/resolve_btfids/Makefile > index 19a3112e271a..de7d29cf43d6 100644 > --- a/tools/bpf/resolve_btfids/Makefile > +++ b/tools/bpf/resolve_btfids/Makefile > @@ -35,21 +35,29 @@ SUBCMD_SRC := $(srctree)/tools/lib/subcmd/ > BPFOBJ := $(OUTPUT)/libbpf/libbpf.a > LIBBPF_OUT := $(abspath $(dir $(BPFOBJ)))/ > SUBCMDOBJ := $(OUTPUT)/libsubcmd/libsubcmd.a > +SUBCMD_OUT := $(abspath $(dir $(SUBCMDOBJ)))/ > > LIBBPF_DESTDIR := $(LIBBPF_OUT) > LIBBPF_INCLUDE := $(LIBBPF_DESTDIR)include > > +SUBCMD_DESTDIR := $(SUBCMD_OUT) > +SUBCMD_INCLUDE := $(SUBCMD_DESTDIR)include > + > BINARY := $(OUTPUT)/resolve_btfids > BINARY_IN := $(BINARY)-in.o > > all: $(BINARY) > > +prepare: $(SUBCMDOBJ) do we need special target for that? we already have BPFOBJ dependency placed in the BINARY_IN as prereq why not place both as BINARY_IN prereq, or is there some other reason for new 'prepare' target? jirka > + > $(OUTPUT) $(OUTPUT)/libsubcmd $(LIBBPF_OUT): > $(call msg,MKDIR,,$@) > $(Q)mkdir -p $(@) > > $(SUBCMDOBJ): fixdep FORCE | $(OUTPUT)/libsubcmd > - $(Q)$(MAKE) -C $(SUBCMD_SRC) OUTPUT=$(abspath $(dir $@))/ $(abspath $@) > + $(Q)$(MAKE) -C $(SUBCMD_SRC) OUTPUT=$(SUBCMD_OUT) \ > + DESTDIR=$(SUBCMD_DESTDIR) prefix= \ > + $(abspath $@) install_headers > > $(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(LIBBPF_OUT) > $(Q)$(MAKE) $(submake_extras) -C $(LIBBPF_SRC) OUTPUT=$(LIBBPF_OUT) \ > @@ -60,14 +68,14 @@ CFLAGS += -g \ > -I$(srctree)/tools/include \ > -I$(srctree)/tools/include/uapi \ > -I$(LIBBPF_INCLUDE) \ > - -I$(SUBCMD_SRC) > + -I$(SUBCMD_INCLUDE) > > LIBS = -lelf -lz > > export srctree OUTPUT CFLAGS Q > include $(srctree)/tools/build/Makefile.include > > -$(BINARY_IN): $(BPFOBJ) fixdep FORCE | $(OUTPUT) > +$(BINARY_IN): $(BPFOBJ) fixdep FORCE prepare | $(OUTPUT) > $(Q)$(MAKE) $(build)=resolve_btfids > > $(BINARY): $(BPFOBJ) $(SUBCMDOBJ) $(BINARY_IN) > @@ -79,7 +87,8 @@ clean_objects := $(wildcard $(OUTPUT)/*.o \ > $(OUTPUT)/.*.o.d \ > $(LIBBPF_OUT) \ > $(LIBBPF_DESTDIR) \ > - $(OUTPUT)/libsubcmd \ > + $(SUBCMD_OUT) \ > + $(SUBCMD_DESTDIR) \ > $(OUTPUT)/resolve_btfids) > > ifneq ($(clean_objects),) > @@ -96,4 +105,4 @@ tags: > > FORCE: > > -.PHONY: all FORCE clean tags > +.PHONY: all FORCE clean tags prepare > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c > index 80cd7843c677..77058174082d 100644 > --- a/tools/bpf/resolve_btfids/main.c > +++ b/tools/bpf/resolve_btfids/main.c > @@ -75,7 +75,7 @@ > #include <linux/err.h> > #include <bpf/btf.h> > #include <bpf/libbpf.h> > -#include <parse-options.h> > +#include <subcmd/parse-options.h> > > #define BTF_IDS_SECTION ".BTF_ids" > #define BTF_ID "__BTF_ID__" > -- > 2.39.0.314.g84b9a713c41-goog >
On Mon, Jan 16, 2023 at 3:22 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Wed, Jan 11, 2023 at 04:40:24PM -0800, Ian Rogers wrote: > > Previously tools/lib/subcmd was added to the include path, switch to > > installing the headers and then including from that directory. This > > avoids dependencies on headers internal to tools/lib/subcmd. Add the > > missing subcmd directory to the affected #include. > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/bpf/resolve_btfids/Makefile | 19 ++++++++++++++----- > > tools/bpf/resolve_btfids/main.c | 2 +- > > 2 files changed, 15 insertions(+), 6 deletions(-) > > this depends on changes that went to Arnaldo's tree right? > I can't apply this on bpf-next/master Hmm.. sorry for that. I did the work on the master branch of git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > > > > diff --git a/tools/bpf/resolve_btfids/Makefile b/tools/bpf/resolve_btfids/Makefile > > index 19a3112e271a..de7d29cf43d6 100644 > > --- a/tools/bpf/resolve_btfids/Makefile > > +++ b/tools/bpf/resolve_btfids/Makefile > > @@ -35,21 +35,29 @@ SUBCMD_SRC := $(srctree)/tools/lib/subcmd/ > > BPFOBJ := $(OUTPUT)/libbpf/libbpf.a > > LIBBPF_OUT := $(abspath $(dir $(BPFOBJ)))/ > > SUBCMDOBJ := $(OUTPUT)/libsubcmd/libsubcmd.a > > +SUBCMD_OUT := $(abspath $(dir $(SUBCMDOBJ)))/ > > > > LIBBPF_DESTDIR := $(LIBBPF_OUT) > > LIBBPF_INCLUDE := $(LIBBPF_DESTDIR)include > > > > +SUBCMD_DESTDIR := $(SUBCMD_OUT) > > +SUBCMD_INCLUDE := $(SUBCMD_DESTDIR)include > > + > > BINARY := $(OUTPUT)/resolve_btfids > > BINARY_IN := $(BINARY)-in.o > > > > all: $(BINARY) > > > > +prepare: $(SUBCMDOBJ) > > do we need special target for that? we already have BPFOBJ dependency > placed in the BINARY_IN as prereq BPFOBJ is $(OUTPUT)/libbpf/libbpf.a which is needed at link time. The prepare step is one we have elsewhere and it creates things like the header files necessary to compile the C code, so we need it earlier is the answer. > why not place both as BINARY_IN prereq, or is there some other reason > for new 'prepare' target? I was trying to follow the convention elsewhere in the tree of having a prepare target that does things like get the necessary header files ready. Having prepare is useful because if an additional dependency is added, then it just needs to be added to prepare. It could be tedious to list all the dependencies for every C file, although Makefile.build handles most of that. It isn't clear to me why $(BPFOBJ) is a dependency of $(BINARY_IN) as it is already a dependency of $(BINARY). Thanks, Ian > jirka > > > + > > $(OUTPUT) $(OUTPUT)/libsubcmd $(LIBBPF_OUT): > > $(call msg,MKDIR,,$@) > > $(Q)mkdir -p $(@) > > > > $(SUBCMDOBJ): fixdep FORCE | $(OUTPUT)/libsubcmd > > - $(Q)$(MAKE) -C $(SUBCMD_SRC) OUTPUT=$(abspath $(dir $@))/ $(abspath $@) > > + $(Q)$(MAKE) -C $(SUBCMD_SRC) OUTPUT=$(SUBCMD_OUT) \ > > + DESTDIR=$(SUBCMD_DESTDIR) prefix= \ > > + $(abspath $@) install_headers > > > > $(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(LIBBPF_OUT) > > $(Q)$(MAKE) $(submake_extras) -C $(LIBBPF_SRC) OUTPUT=$(LIBBPF_OUT) \ > > @@ -60,14 +68,14 @@ CFLAGS += -g \ > > -I$(srctree)/tools/include \ > > -I$(srctree)/tools/include/uapi \ > > -I$(LIBBPF_INCLUDE) \ > > - -I$(SUBCMD_SRC) > > + -I$(SUBCMD_INCLUDE) > > > > LIBS = -lelf -lz > > > > export srctree OUTPUT CFLAGS Q > > include $(srctree)/tools/build/Makefile.include > > > > -$(BINARY_IN): $(BPFOBJ) fixdep FORCE | $(OUTPUT) > > +$(BINARY_IN): $(BPFOBJ) fixdep FORCE prepare | $(OUTPUT) > > $(Q)$(MAKE) $(build)=resolve_btfids > > > > $(BINARY): $(BPFOBJ) $(SUBCMDOBJ) $(BINARY_IN) > > @@ -79,7 +87,8 @@ clean_objects := $(wildcard $(OUTPUT)/*.o \ > > $(OUTPUT)/.*.o.d \ > > $(LIBBPF_OUT) \ > > $(LIBBPF_DESTDIR) \ > > - $(OUTPUT)/libsubcmd \ > > + $(SUBCMD_OUT) \ > > + $(SUBCMD_DESTDIR) \ > > $(OUTPUT)/resolve_btfids) > > > > ifneq ($(clean_objects),) > > @@ -96,4 +105,4 @@ tags: > > > > FORCE: > > > > -.PHONY: all FORCE clean tags > > +.PHONY: all FORCE clean tags prepare > > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c > > index 80cd7843c677..77058174082d 100644 > > --- a/tools/bpf/resolve_btfids/main.c > > +++ b/tools/bpf/resolve_btfids/main.c > > @@ -75,7 +75,7 @@ > > #include <linux/err.h> > > #include <bpf/btf.h> > > #include <bpf/libbpf.h> > > -#include <parse-options.h> > > +#include <subcmd/parse-options.h> > > > > #define BTF_IDS_SECTION ".BTF_ids" > > #define BTF_ID "__BTF_ID__" > > -- > > 2.39.0.314.g84b9a713c41-goog > >
On Mon, Jan 16, 2023 at 09:20:39AM -0800, Ian Rogers wrote: > On Mon, Jan 16, 2023 at 3:22 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > On Wed, Jan 11, 2023 at 04:40:24PM -0800, Ian Rogers wrote: > > > Previously tools/lib/subcmd was added to the include path, switch to > > > installing the headers and then including from that directory. This > > > avoids dependencies on headers internal to tools/lib/subcmd. Add the > > > missing subcmd directory to the affected #include. > > > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > --- > > > tools/bpf/resolve_btfids/Makefile | 19 ++++++++++++++----- > > > tools/bpf/resolve_btfids/main.c | 2 +- > > > 2 files changed, 15 insertions(+), 6 deletions(-) > > > > this depends on changes that went to Arnaldo's tree right? > > I can't apply this on bpf-next/master > > Hmm.. sorry for that. I did the work on the master branch of > git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > > > > > > > diff --git a/tools/bpf/resolve_btfids/Makefile b/tools/bpf/resolve_btfids/Makefile > > > index 19a3112e271a..de7d29cf43d6 100644 > > > --- a/tools/bpf/resolve_btfids/Makefile > > > +++ b/tools/bpf/resolve_btfids/Makefile > > > @@ -35,21 +35,29 @@ SUBCMD_SRC := $(srctree)/tools/lib/subcmd/ > > > BPFOBJ := $(OUTPUT)/libbpf/libbpf.a > > > LIBBPF_OUT := $(abspath $(dir $(BPFOBJ)))/ > > > SUBCMDOBJ := $(OUTPUT)/libsubcmd/libsubcmd.a > > > +SUBCMD_OUT := $(abspath $(dir $(SUBCMDOBJ)))/ > > > > > > LIBBPF_DESTDIR := $(LIBBPF_OUT) > > > LIBBPF_INCLUDE := $(LIBBPF_DESTDIR)include > > > > > > +SUBCMD_DESTDIR := $(SUBCMD_OUT) > > > +SUBCMD_INCLUDE := $(SUBCMD_DESTDIR)include > > > + > > > BINARY := $(OUTPUT)/resolve_btfids > > > BINARY_IN := $(BINARY)-in.o > > > > > > all: $(BINARY) > > > > > > +prepare: $(SUBCMDOBJ) > > > > do we need special target for that? we already have BPFOBJ dependency > > placed in the BINARY_IN as prereq > > BPFOBJ is $(OUTPUT)/libbpf/libbpf.a which is needed at link time. The > prepare step is one we have elsewhere and it creates things like the > header files necessary to compile the C code, so we need it earlier is > the answer. > > > why not place both as BINARY_IN prereq, or is there some other reason > > for new 'prepare' target? > > I was trying to follow the convention elsewhere in the tree of having > a prepare target that does things like get the necessary header files > ready. Having prepare is useful because if an additional dependency is > added, then it just needs to be added to prepare. It could be tedious > to list all the dependencies for every C file, although Makefile.build ok, could we maybe add the BPFOBJ in prepare target as well? > handles most of that. It isn't clear to me why $(BPFOBJ) is a > dependency of $(BINARY_IN) as it is already a dependency of $(BINARY). I think that if you specify OUTPUT then we need the libbpf headers to be created before we go to compile resolve_btfids objects thanks, jirka
On Mon, Jan 16, 2023 at 10:20 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Mon, Jan 16, 2023 at 09:20:39AM -0800, Ian Rogers wrote: > > On Mon, Jan 16, 2023 at 3:22 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > > > > > On Wed, Jan 11, 2023 at 04:40:24PM -0800, Ian Rogers wrote: > > > > Previously tools/lib/subcmd was added to the include path, switch to > > > > installing the headers and then including from that directory. This > > > > avoids dependencies on headers internal to tools/lib/subcmd. Add the > > > > missing subcmd directory to the affected #include. > > > > > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > > --- > > > > tools/bpf/resolve_btfids/Makefile | 19 ++++++++++++++----- > > > > tools/bpf/resolve_btfids/main.c | 2 +- > > > > 2 files changed, 15 insertions(+), 6 deletions(-) > > > > > > this depends on changes that went to Arnaldo's tree right? > > > I can't apply this on bpf-next/master > > > > Hmm.. sorry for that. I did the work on the master branch of > > git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > > > > > > > > > > diff --git a/tools/bpf/resolve_btfids/Makefile b/tools/bpf/resolve_btfids/Makefile > > > > index 19a3112e271a..de7d29cf43d6 100644 > > > > --- a/tools/bpf/resolve_btfids/Makefile > > > > +++ b/tools/bpf/resolve_btfids/Makefile > > > > @@ -35,21 +35,29 @@ SUBCMD_SRC := $(srctree)/tools/lib/subcmd/ > > > > BPFOBJ := $(OUTPUT)/libbpf/libbpf.a > > > > LIBBPF_OUT := $(abspath $(dir $(BPFOBJ)))/ > > > > SUBCMDOBJ := $(OUTPUT)/libsubcmd/libsubcmd.a > > > > +SUBCMD_OUT := $(abspath $(dir $(SUBCMDOBJ)))/ > > > > > > > > LIBBPF_DESTDIR := $(LIBBPF_OUT) > > > > LIBBPF_INCLUDE := $(LIBBPF_DESTDIR)include > > > > > > > > +SUBCMD_DESTDIR := $(SUBCMD_OUT) > > > > +SUBCMD_INCLUDE := $(SUBCMD_DESTDIR)include > > > > + > > > > BINARY := $(OUTPUT)/resolve_btfids > > > > BINARY_IN := $(BINARY)-in.o > > > > > > > > all: $(BINARY) > > > > > > > > +prepare: $(SUBCMDOBJ) > > > > > > do we need special target for that? we already have BPFOBJ dependency > > > placed in the BINARY_IN as prereq > > > > BPFOBJ is $(OUTPUT)/libbpf/libbpf.a which is needed at link time. The > > prepare step is one we have elsewhere and it creates things like the > > header files necessary to compile the C code, so we need it earlier is > > the answer. > > > > > why not place both as BINARY_IN prereq, or is there some other reason > > > for new 'prepare' target? > > > > I was trying to follow the convention elsewhere in the tree of having > > a prepare target that does things like get the necessary header files > > ready. Having prepare is useful because if an additional dependency is > > added, then it just needs to be added to prepare. It could be tedious > > to list all the dependencies for every C file, although Makefile.build > > ok, could we maybe add the BPFOBJ in prepare target as well? Agreed, added to v2. > > handles most of that. It isn't clear to me why $(BPFOBJ) is a > > dependency of $(BINARY_IN) as it is already a dependency of $(BINARY). > > I think that if you specify OUTPUT then we need the libbpf headers > to be created before we go to compile resolve_btfids objects > > thanks, > jirka Yep. I also noticed that this code is doing the "CC=$(HOSTCC)" thing. The problem with that is Makefile.include (line 2) will set things like CFLAGS based on CC and then you change it. I sent out some objtool build cleanup for this: https://lore.kernel.org/lkml/20230105090155.357604-4-irogers@google.com/ I'll add a patch for this in v2. Thanks, Ian
diff --git a/tools/bpf/resolve_btfids/Makefile b/tools/bpf/resolve_btfids/Makefile index 19a3112e271a..de7d29cf43d6 100644 --- a/tools/bpf/resolve_btfids/Makefile +++ b/tools/bpf/resolve_btfids/Makefile @@ -35,21 +35,29 @@ SUBCMD_SRC := $(srctree)/tools/lib/subcmd/ BPFOBJ := $(OUTPUT)/libbpf/libbpf.a LIBBPF_OUT := $(abspath $(dir $(BPFOBJ)))/ SUBCMDOBJ := $(OUTPUT)/libsubcmd/libsubcmd.a +SUBCMD_OUT := $(abspath $(dir $(SUBCMDOBJ)))/ LIBBPF_DESTDIR := $(LIBBPF_OUT) LIBBPF_INCLUDE := $(LIBBPF_DESTDIR)include +SUBCMD_DESTDIR := $(SUBCMD_OUT) +SUBCMD_INCLUDE := $(SUBCMD_DESTDIR)include + BINARY := $(OUTPUT)/resolve_btfids BINARY_IN := $(BINARY)-in.o all: $(BINARY) +prepare: $(SUBCMDOBJ) + $(OUTPUT) $(OUTPUT)/libsubcmd $(LIBBPF_OUT): $(call msg,MKDIR,,$@) $(Q)mkdir -p $(@) $(SUBCMDOBJ): fixdep FORCE | $(OUTPUT)/libsubcmd - $(Q)$(MAKE) -C $(SUBCMD_SRC) OUTPUT=$(abspath $(dir $@))/ $(abspath $@) + $(Q)$(MAKE) -C $(SUBCMD_SRC) OUTPUT=$(SUBCMD_OUT) \ + DESTDIR=$(SUBCMD_DESTDIR) prefix= \ + $(abspath $@) install_headers $(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(LIBBPF_OUT) $(Q)$(MAKE) $(submake_extras) -C $(LIBBPF_SRC) OUTPUT=$(LIBBPF_OUT) \ @@ -60,14 +68,14 @@ CFLAGS += -g \ -I$(srctree)/tools/include \ -I$(srctree)/tools/include/uapi \ -I$(LIBBPF_INCLUDE) \ - -I$(SUBCMD_SRC) + -I$(SUBCMD_INCLUDE) LIBS = -lelf -lz export srctree OUTPUT CFLAGS Q include $(srctree)/tools/build/Makefile.include -$(BINARY_IN): $(BPFOBJ) fixdep FORCE | $(OUTPUT) +$(BINARY_IN): $(BPFOBJ) fixdep FORCE prepare | $(OUTPUT) $(Q)$(MAKE) $(build)=resolve_btfids $(BINARY): $(BPFOBJ) $(SUBCMDOBJ) $(BINARY_IN) @@ -79,7 +87,8 @@ clean_objects := $(wildcard $(OUTPUT)/*.o \ $(OUTPUT)/.*.o.d \ $(LIBBPF_OUT) \ $(LIBBPF_DESTDIR) \ - $(OUTPUT)/libsubcmd \ + $(SUBCMD_OUT) \ + $(SUBCMD_DESTDIR) \ $(OUTPUT)/resolve_btfids) ifneq ($(clean_objects),) @@ -96,4 +105,4 @@ tags: FORCE: -.PHONY: all FORCE clean tags +.PHONY: all FORCE clean tags prepare diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c index 80cd7843c677..77058174082d 100644 --- a/tools/bpf/resolve_btfids/main.c +++ b/tools/bpf/resolve_btfids/main.c @@ -75,7 +75,7 @@ #include <linux/err.h> #include <bpf/btf.h> #include <bpf/libbpf.h> -#include <parse-options.h> +#include <subcmd/parse-options.h> #define BTF_IDS_SECTION ".BTF_ids" #define BTF_ID "__BTF_ID__"
Previously tools/lib/subcmd was added to the include path, switch to installing the headers and then including from that directory. This avoids dependencies on headers internal to tools/lib/subcmd. Add the missing subcmd directory to the affected #include. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/bpf/resolve_btfids/Makefile | 19 ++++++++++++++----- tools/bpf/resolve_btfids/main.c | 2 +- 2 files changed, 15 insertions(+), 6 deletions(-)