Message ID | 20191011002808.28206-12-ivan.khoronzhuk@linaro.org |
---|---|
State | Accepted |
Delegated to: | BPF Maintainers |
Headers | show |
Series | samples: bpf: improve/fix cross-compilation | expand |
On 10/11, Ivan Khoronzhuk wrote: > No need to use C++ for test_libbpf target when libbpf is on C and it > can be tested with C, after this change the CXXFLAGS in makefiles can > be avoided, at least in bpf samples, when sysroot is used, passing > same C/LDFLAGS as for lib. > > Add "return 0" in test_libbpf to avoid warn, but also remove spaces at > start of the lines to keep same style and avoid warns while apply. Hey, just spotted this patch, not sure how it slipped through. The c++ test was there to make sure libbpf can be included and linked against c++ code (i.e. libbpf headers don't have some c++ keywords/etc). Any particular reason you were not happy with it? Can we revert it back to c++ and fix your use-case instead? Alternatively, we can just remove this test if we don't really care about c++. > Acked-by: Andrii Nakryiko <andriin@fb.com> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> > --- > tools/lib/bpf/Makefile | 18 +++++------------- > .../lib/bpf/{test_libbpf.cpp => test_libbpf.c} | 14 ++++++++------ > 2 files changed, 13 insertions(+), 19 deletions(-) > rename tools/lib/bpf/{test_libbpf.cpp => test_libbpf.c} (61%) > > diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile > index 1270955e4845..46280b5ad48d 100644 > --- a/tools/lib/bpf/Makefile > +++ b/tools/lib/bpf/Makefile > @@ -52,7 +52,7 @@ ifndef VERBOSE > endif > > FEATURE_USER = .libbpf > -FEATURE_TESTS = libelf libelf-mmap bpf reallocarray cxx > +FEATURE_TESTS = libelf libelf-mmap bpf reallocarray > FEATURE_DISPLAY = libelf bpf > > INCLUDES = -I. -I$(srctree)/tools/include -I$(srctree)/tools/arch/$(ARCH)/include/uapi -I$(srctree)/tools/include/uapi > @@ -142,15 +142,7 @@ GLOBAL_SYM_COUNT = $(shell readelf -s --wide $(BPF_IN) | \ > VERSIONED_SYM_COUNT = $(shell readelf -s --wide $(OUTPUT)libbpf.so | \ > grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | sort -u | wc -l) > > -CMD_TARGETS = $(LIB_TARGET) $(PC_FILE) > - > -CXX_TEST_TARGET = $(OUTPUT)test_libbpf > - > -ifeq ($(feature-cxx), 1) > - CMD_TARGETS += $(CXX_TEST_TARGET) > -endif > - > -TARGETS = $(CMD_TARGETS) > +CMD_TARGETS = $(LIB_TARGET) $(PC_FILE) $(OUTPUT)test_libbpf > > all: fixdep > $(Q)$(MAKE) all_cmd > @@ -190,8 +182,8 @@ $(OUTPUT)libbpf.so.$(LIBBPF_VERSION): $(BPF_IN) > $(OUTPUT)libbpf.a: $(BPF_IN) > $(QUIET_LINK)$(RM) $@; $(AR) rcs $@ $^ > > -$(OUTPUT)test_libbpf: test_libbpf.cpp $(OUTPUT)libbpf.a > - $(QUIET_LINK)$(CXX) $(INCLUDES) $^ -lelf -o $@ > +$(OUTPUT)test_libbpf: test_libbpf.c $(OUTPUT)libbpf.a > + $(QUIET_LINK)$(CC) $(INCLUDES) $^ -lelf -o $@ > > $(OUTPUT)libbpf.pc: > $(QUIET_GEN)sed -e "s|@PREFIX@|$(prefix)|" \ > @@ -266,7 +258,7 @@ config-clean: > $(Q)$(MAKE) -C $(srctree)/tools/build/feature/ clean >/dev/null > > clean: > - $(call QUIET_CLEAN, libbpf) $(RM) $(TARGETS) $(CXX_TEST_TARGET) \ > + $(call QUIET_CLEAN, libbpf) $(RM) $(CMD_TARGETS) \ > *.o *~ *.a *.so *.so.$(LIBBPF_MAJOR_VERSION) .*.d .*.cmd \ > *.pc LIBBPF-CFLAGS bpf_helper_defs.h > $(call QUIET_CLEAN, core-gen) $(RM) $(OUTPUT)FEATURE-DUMP.libbpf > diff --git a/tools/lib/bpf/test_libbpf.cpp b/tools/lib/bpf/test_libbpf.c > similarity index 61% > rename from tools/lib/bpf/test_libbpf.cpp > rename to tools/lib/bpf/test_libbpf.c > index fc134873bb6d..f0eb2727b766 100644 > --- a/tools/lib/bpf/test_libbpf.cpp > +++ b/tools/lib/bpf/test_libbpf.c > @@ -7,12 +7,14 @@ > > int main(int argc, char *argv[]) > { > - /* libbpf.h */ > - libbpf_set_print(NULL); > + /* libbpf.h */ > + libbpf_set_print(NULL); > > - /* bpf.h */ > - bpf_prog_get_fd_by_id(0); > + /* bpf.h */ > + bpf_prog_get_fd_by_id(0); > > - /* btf.h */ > - btf__new(NULL, 0); > + /* btf.h */ > + btf__new(NULL, 0); > + > + return 0; > } > -- > 2.17.1 >
On Thu, Nov 21, 2019 at 1:42 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > On 10/11, Ivan Khoronzhuk wrote: > > No need to use C++ for test_libbpf target when libbpf is on C and it > > can be tested with C, after this change the CXXFLAGS in makefiles can > > be avoided, at least in bpf samples, when sysroot is used, passing > > same C/LDFLAGS as for lib. > > > > Add "return 0" in test_libbpf to avoid warn, but also remove spaces at > > start of the lines to keep same style and avoid warns while apply. > Hey, just spotted this patch, not sure how it slipped through. > The c++ test was there to make sure libbpf can be included and > linked against c++ code (i.e. libbpf headers don't have some c++ > keywords/etc). > > Any particular reason you were not happy with it? Can we revert it > back to c++ and fix your use-case instead? Alternatively, we can just > remove this test if we don't really care about c++. > No one seemed to know why we have C++ pieces in pure C library and its Makefile, so we decide to "fix" this. :) But I do understand your concern. Would it be possible to instead do this as a proper selftests test? Do you mind taking a look at that? Thanks! (please trim irrelevant parts) [...]
On 11/21, Andrii Nakryiko wrote: > On Thu, Nov 21, 2019 at 1:42 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > On 10/11, Ivan Khoronzhuk wrote: > > > No need to use C++ for test_libbpf target when libbpf is on C and it > > > can be tested with C, after this change the CXXFLAGS in makefiles can > > > be avoided, at least in bpf samples, when sysroot is used, passing > > > same C/LDFLAGS as for lib. > > > Add "return 0" in test_libbpf to avoid warn, but also remove spaces at > > > start of the lines to keep same style and avoid warns while apply. > > Hey, just spotted this patch, not sure how it slipped through. > > The c++ test was there to make sure libbpf can be included and > > linked against c++ code (i.e. libbpf headers don't have some c++ > > keywords/etc). > > > > Any particular reason you were not happy with it? Can we revert it > > back to c++ and fix your use-case instead? Alternatively, we can just > > remove this test if we don't really care about c++. > > > > No one seemed to know why we have C++ pieces in pure C library and its > Makefile, so we decide to "fix" this. :) It's surprising, the commit 8c4905b995c6 clearly states the reason for adding it. Looks like it deserved a real comment in the Makefile :-) > But I do understand your concern. Would it be possible to instead do > this as a proper selftests test? Do you mind taking a look at that? Ack, will move this test_libbpf.c into selftests and convert back to c++.
On Fri, Nov 22, 2019 at 08:32:11AM -0800, Stanislav Fomichev wrote: >On 11/21, Andrii Nakryiko wrote: >> On Thu, Nov 21, 2019 at 1:42 PM Stanislav Fomichev <sdf@fomichev.me> wrote: >> > >> > On 10/11, Ivan Khoronzhuk wrote: >> > > No need to use C++ for test_libbpf target when libbpf is on C and it >> > > can be tested with C, after this change the CXXFLAGS in makefiles can >> > > be avoided, at least in bpf samples, when sysroot is used, passing >> > > same C/LDFLAGS as for lib. >> > > Add "return 0" in test_libbpf to avoid warn, but also remove spaces at >> > > start of the lines to keep same style and avoid warns while apply. >> > Hey, just spotted this patch, not sure how it slipped through. >> > The c++ test was there to make sure libbpf can be included and >> > linked against c++ code (i.e. libbpf headers don't have some c++ >> > keywords/etc). >> > >> > Any particular reason you were not happy with it? Can we revert it >> > back to c++ and fix your use-case instead? Alternatively, we can just >> > remove this test if we don't really care about c++. >> > >> >> No one seemed to know why we have C++ pieces in pure C library and its >> Makefile, so we decide to "fix" this. :) >It's surprising, the commit 8c4905b995c6 clearly states the reason >for adding it. Looks like it deserved a real comment in the Makefile :-) I dislike changing things like this, but I was asked while review and it seemed logical enough. The comment could prevent us from doing this. > >> But I do understand your concern. Would it be possible to instead do >> this as a proper selftests test? Do you mind taking a look at that? >Ack, will move this test_libbpf.c into selftests and convert back to >c++.
On 11/23, Ivan Khoronzhuk wrote: > On Fri, Nov 22, 2019 at 08:32:11AM -0800, Stanislav Fomichev wrote: > > On 11/21, Andrii Nakryiko wrote: > > > On Thu, Nov 21, 2019 at 1:42 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > > > > > On 10/11, Ivan Khoronzhuk wrote: > > > > > No need to use C++ for test_libbpf target when libbpf is on C and it > > > > > can be tested with C, after this change the CXXFLAGS in makefiles can > > > > > be avoided, at least in bpf samples, when sysroot is used, passing > > > > > same C/LDFLAGS as for lib. > > > > > Add "return 0" in test_libbpf to avoid warn, but also remove spaces at > > > > > start of the lines to keep same style and avoid warns while apply. > > > > Hey, just spotted this patch, not sure how it slipped through. > > > > The c++ test was there to make sure libbpf can be included and > > > > linked against c++ code (i.e. libbpf headers don't have some c++ > > > > keywords/etc). > > > > > > > > Any particular reason you were not happy with it? Can we revert it > > > > back to c++ and fix your use-case instead? Alternatively, we can just > > > > remove this test if we don't really care about c++. > > > > > > > > > > No one seemed to know why we have C++ pieces in pure C library and its > > > Makefile, so we decide to "fix" this. :) > > It's surprising, the commit 8c4905b995c6 clearly states the reason > > for adding it. Looks like it deserved a real comment in the Makefile :-) > > I dislike changing things like this, but I was asked while review and > it seemed logical enough. The comment could prevent us from doing this. No worries, I'll add it back with a comment :-) > > > But I do understand your concern. Would it be possible to instead do > > > this as a proper selftests test? Do you mind taking a look at that? > > Ack, will move this test_libbpf.c into selftests and convert back to > > c++. > > -- > Regards, > Ivan Khoronzhuk
diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile index 1270955e4845..46280b5ad48d 100644 --- a/tools/lib/bpf/Makefile +++ b/tools/lib/bpf/Makefile @@ -52,7 +52,7 @@ ifndef VERBOSE endif FEATURE_USER = .libbpf -FEATURE_TESTS = libelf libelf-mmap bpf reallocarray cxx +FEATURE_TESTS = libelf libelf-mmap bpf reallocarray FEATURE_DISPLAY = libelf bpf INCLUDES = -I. -I$(srctree)/tools/include -I$(srctree)/tools/arch/$(ARCH)/include/uapi -I$(srctree)/tools/include/uapi @@ -142,15 +142,7 @@ GLOBAL_SYM_COUNT = $(shell readelf -s --wide $(BPF_IN) | \ VERSIONED_SYM_COUNT = $(shell readelf -s --wide $(OUTPUT)libbpf.so | \ grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | sort -u | wc -l) -CMD_TARGETS = $(LIB_TARGET) $(PC_FILE) - -CXX_TEST_TARGET = $(OUTPUT)test_libbpf - -ifeq ($(feature-cxx), 1) - CMD_TARGETS += $(CXX_TEST_TARGET) -endif - -TARGETS = $(CMD_TARGETS) +CMD_TARGETS = $(LIB_TARGET) $(PC_FILE) $(OUTPUT)test_libbpf all: fixdep $(Q)$(MAKE) all_cmd @@ -190,8 +182,8 @@ $(OUTPUT)libbpf.so.$(LIBBPF_VERSION): $(BPF_IN) $(OUTPUT)libbpf.a: $(BPF_IN) $(QUIET_LINK)$(RM) $@; $(AR) rcs $@ $^ -$(OUTPUT)test_libbpf: test_libbpf.cpp $(OUTPUT)libbpf.a - $(QUIET_LINK)$(CXX) $(INCLUDES) $^ -lelf -o $@ +$(OUTPUT)test_libbpf: test_libbpf.c $(OUTPUT)libbpf.a + $(QUIET_LINK)$(CC) $(INCLUDES) $^ -lelf -o $@ $(OUTPUT)libbpf.pc: $(QUIET_GEN)sed -e "s|@PREFIX@|$(prefix)|" \ @@ -266,7 +258,7 @@ config-clean: $(Q)$(MAKE) -C $(srctree)/tools/build/feature/ clean >/dev/null clean: - $(call QUIET_CLEAN, libbpf) $(RM) $(TARGETS) $(CXX_TEST_TARGET) \ + $(call QUIET_CLEAN, libbpf) $(RM) $(CMD_TARGETS) \ *.o *~ *.a *.so *.so.$(LIBBPF_MAJOR_VERSION) .*.d .*.cmd \ *.pc LIBBPF-CFLAGS bpf_helper_defs.h $(call QUIET_CLEAN, core-gen) $(RM) $(OUTPUT)FEATURE-DUMP.libbpf diff --git a/tools/lib/bpf/test_libbpf.cpp b/tools/lib/bpf/test_libbpf.c similarity index 61% rename from tools/lib/bpf/test_libbpf.cpp rename to tools/lib/bpf/test_libbpf.c index fc134873bb6d..f0eb2727b766 100644 --- a/tools/lib/bpf/test_libbpf.cpp +++ b/tools/lib/bpf/test_libbpf.c @@ -7,12 +7,14 @@ int main(int argc, char *argv[]) { - /* libbpf.h */ - libbpf_set_print(NULL); + /* libbpf.h */ + libbpf_set_print(NULL); - /* bpf.h */ - bpf_prog_get_fd_by_id(0); + /* bpf.h */ + bpf_prog_get_fd_by_id(0); - /* btf.h */ - btf__new(NULL, 0); + /* btf.h */ + btf__new(NULL, 0); + + return 0; }