Message ID | 20191011002808.28206-1-ivan.khoronzhuk@linaro.org |
---|---|
Headers | show |
Series | samples: bpf: improve/fix cross-compilation | expand |
More grammar nitpicking... On 11.10.2019 3:28, Ivan Khoronzhuk wrote: > While compiling natively, the host's cflags and ldflags are equal to > ones used from HOSTCFLAGS and HOSTLDFLAGS. When cross compiling it > should have own, used for target arch. While verification, for arm, While verifying. > arm64 and x86_64 the following flags were used always: > > -Wall -O2 > -fomit-frame-pointer > -Wmissing-prototypes > -Wstrict-prototypes > > So, add them as they were verified and used before adding > Makefile.target and lets omit "-fomit-frame-pointer" as were proposed > while review, as no sense in such optimization for samples. > > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> [...] MBR, Sergei
On 10/11/2019 12:57 PM, Ivan Khoronzhuk wrote: >>> While compiling natively, the host's cflags and ldflags are equal to >>> ones used from HOSTCFLAGS and HOSTLDFLAGS. When cross compiling it >>> should have own, used for target arch. While verification, for arm, >> >> While verifying. > While verification stage. While *in* verification stage, "while" doesn't combine with nouns w/o a preposition. >>> arm64 and x86_64 the following flags were used always: >>> >>> -Wall -O2 >>> -fomit-frame-pointer >>> -Wmissing-prototypes >>> -Wstrict-prototypes >>> >>> So, add them as they were verified and used before adding >>> Makefile.target and lets omit "-fomit-frame-pointer" as were proposed >>> while review, as no sense in such optimization for samples. >>> >>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >> [...] MBR, Sergei
On Fri, Oct 11, 2019 at 03:27:53AM +0300, Ivan Khoronzhuk wrote: > This series contains mainly fixes/improvements for cross-compilation > but not only, tested for arm, arm64, and intended for any arch. > Also verified on native build (not cross compilation) for x86_64 > and arm, arm64. > > Initial RFC link: > https://lkml.org/lkml/2019/8/29/1665 > > Prev. version: > https://lkml.org/lkml/2019/10/9/1045 > > Besides the patches given here, the RFC also contains couple patches > related to llvm clang > arm: include: asm: swab: mask rev16 instruction for clang > arm: include: asm: unified: mask .syntax unified for clang > They are necessarily to verify arm 32 build. > > Also, couple more fixes were added but are not merged in bpf-next yet, > they can be needed for verification/configuration steps, if not in > your tree the fixes can be taken here: > https://www.spinics.net/lists/netdev/msg601716.html > https://www.spinics.net/lists/netdev/msg601714.html > https://www.spinics.net/lists/linux-kbuild/msg23468.html > > Now, to build samples, SAMPLE_BPF should be enabled in config. > > The change touches not only cross-compilation and can have impact on > other archs and build environments, so might be good idea to verify > it in order to add appropriate changes, some warn options could be > tuned also. > > All is tested on x86-64 with clang installed (has to be built containing > targets for arm, arm64..., see llc --version, usually it's present already) > > Instructions to test native on x86_64 > ================================================= > Native build on x86_64 is done in usual way and shouldn't have difference > except HOSTCC is now printed as CC wile building the samples. > > Instructions to test cross compilation on arm64 > ================================================= > #Toolchain used for test: > gcc version 8.3.0 > (GNU Toolchain for the A-profile Architecture 8.3-2019.03 (arm-rel-8.36)) > > # Get some arm64 FS, containing at least libelf > I've used sdk for TI am65x got here: > http://downloads.ti.com/processor-sdk-linux/esd/AM65X/latest/exports/\ > ti-processor-sdk-linux-am65xx-evm-06.00.00.07-Linux-x86-Install.bin > > # Install this binary to some dir, say "sdk". > # Configure kernel (use defconfig as no matter), but clean everything > # before. > make ARCH=arm64 -C tools/ clean > make ARCH=arm64 -C samples/bpf clean > make ARCH=arm64 clean > make ARCH=arm64 defconfig > > # Enable SAMPLE_BPF and it's dependencies in config > > # The kernel version used in sdk doesn't correspond to checked one, > # but for this verification only headers need to be syched, > # so install them (can be enabled in config): > make ARCH=arm64 headers_install > > # or on SDK if need keep them in sync (not necessarily to verify): > > make ARCH=arm64 INSTALL_HDR_PATH=/../sdk/\ > ti-processor-sdk-linux-am65xx-evm-06.00.00.07/linux-devkit/sysroots/\ > aarch64-linux/usr headers_install > > # Build samples > make samples/bpf/ ARCH=arm64 CROSS_COMPILE="aarch64-linux-gnu-"\ > SYSROOT="/../sdk/ti-processor-sdk-linux-am65xx-evm-06.00.00.07/\ > linux-devkit/sysroots/aarch64-linux" > > Instructions to test cross compilation on arm > ================================================= > #Toolchains used for test: > arm-linux-gnueabihf-gcc (Linaro GCC 7.2-2017.11) 7.2.1 20171011 > or > arm-linux-gnueabihf-gcc > (GNU Toolchain for the A-profile Architecture 8.3-2019.03 \ > (arm-rel-8.36)) 8.3.0 > > # Get some FS, I've used sdk for TI am52xx got here: > http://downloads.ti.com/processor-sdk-linux/esd/AM57X/05_03_00_07/exports/\ > ti-processor-sdk-linux-am57xx-evm-05.03.00.07-Linux-x86-Install.bin > > # Install this binary to some dir, say "sdk". > # Configure kernel, but clean everything before. > make ARCH=arm -C tools/ clean > make ARCH=arm -C samples/bpf clean > make ARCH=arm clean > make ARCH=arm omap2plus_defconfig > > # The kernel version used in sdk doesn't correspond to checked one, but > # headers only should be synched, > # so install them (can be enabled in config): > > make ARCH=arm headers_install > > # or on SDK if need keep them in sync (not necessarily): > > make ARCH=arm INSTALL_HDR_PATH=/../sdk/\ > ti-processor-sdk-linux-am57xx-evm-05.03.00.07/linux-devkit/sysroots/\ > armv7ahf-neon-linux-gnueabi/usr headers_install > > # Build samples > make samples/bpf/ ARCH=arm CROSS_COMPILE="arm-linux-gnueabihf-"\ > SYSROOT="/../sdk/ti-processor-sdk-linux-am57xx-evm-05.03\ > .00.07/linux-devkit/sysroots/armv7ahf-neon-linux-gnueabi" > > > Based on bpf-next/master > > v5..v4: > - any changes, only missed SOBs are added > > v4..v3: > - renamed CLANG_EXTRA_CFLAGS on BPF_EXTRA_CFLAGS > - used filter for ARCH_ARM_SELECTOR > - omit "-fomit-frame-pointer" and use same flags for native and "cross" > - used sample/bpf prefixes > - use C instead of C++ compiler for test_libbpf target > > v3..v2: > - renamed makefile.progs to makeifle.target, as more appropriate > - left only __LINUX_ARM_ARCH__ for D options for arm > - for host build - left options from KBUILD_HOST for compatibility reasons > - split patch adding c/cxx/ld flags to libbpf by modules > - moved readme change to separate patch > - added patch setting options for cross-compile > - fixed issue with option error for syscall_nrs.S, > avoiding overlap for ccflags-y. > > v2..v1: > - restructured patches order > - split "samples: bpf: Makefile: base progs build on Makefile.progs" > to make change more readable. It added couple nice extra patches. > - removed redundant patch: > "samples: bpf: Makefile: remove target for native build" > - added fix: > "samples: bpf: makefile: fix cookie_uid_helper_example obj build" > - limited -D option filter only for arm > - improved comments > - added couple instructions to verify cross compilation for arm and > arm64 arches based on TI am57xx and am65xx sdks. > - corrected include a little order > > Ivan Khoronzhuk (15): > samples/bpf: fix HDR_PROBE "echo" > samples/bpf: fix cookie_uid_helper_example obj build > samples/bpf: use --target from cross-compile > samples/bpf: use own EXTRA_CFLAGS for clang commands > samples/bpf: use __LINUX_ARM_ARCH__ selector for arm > samples/bpf: drop unnecessarily inclusion for bpf_load > samples/bpf: add makefile.target for separate CC target build > samples/bpf: base target programs rules on Makefile.target > samples/bpf: use own flags but not HOSTCFLAGS > samples/bpf: use target CC environment for HDR_PROBE > libbpf: don't use cxx to test_libpf target > libbpf: add C/LDFLAGS to libbpf.so and test_libpf targets > samples/bpf: provide C/LDFLAGS to libbpf > samples/bpf: add sysroot support > samples/bpf: add preparation steps and sysroot info to readme > > samples/bpf/Makefile | 164 ++++++++++-------- > samples/bpf/Makefile.target | 75 ++++++++ > samples/bpf/README.rst | 41 ++++- > tools/lib/bpf/Makefile | 23 +-- > .../bpf/{test_libbpf.cpp => test_libbpf.c} | 14 +- > 5 files changed, 218 insertions(+), 99 deletions(-) > create mode 100644 samples/bpf/Makefile.target > rename tools/lib/bpf/{test_libbpf.cpp => test_libbpf.c} (61%) > > -- > 2.17.1 > For native compilation on x86_64 and aarch64 Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
On 13.10.2019 0:26, Ivan Khoronzhuk wrote: >>>>> While compiling natively, the host's cflags and ldflags are equal to >>>>> ones used from HOSTCFLAGS and HOSTLDFLAGS. When cross compiling it >>>>> should have own, used for target arch. While verification, for arm, >>>> >>>> While verifying. >>> While verification stage. >> >> While *in* verification stage, "while" doesn't combine with nouns w/o >> a preposition. > > > Sergei, better add me in cc list when msg is to me I can miss it. Hm, the earlier mails were addressed to you but no the last one -- not sure what happened there, sorry. > Regarding the language lesson, thanks, I will keep it in mind next > time, but the issue is not rude, if it's an issue at all, so I better > leave it as is, as not reasons to correct it w/o code changes and > everyone is able to understand it. Up to you. and the maintainer(s)... >>>>> arm64 and x86_64 the following flags were used always: >>>>> >>>>> -Wall -O2 >>>>> -fomit-frame-pointer >>>>> -Wmissing-prototypes >>>>> -Wstrict-prototypes >>>>> >>>>> So, add them as they were verified and used before adding >>>>> Makefile.target and lets omit "-fomit-frame-pointer" as were proposed >>>>> while review, as no sense in such optimization for samples. >>>>> >>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> >>>> [...] MBR, Sergei
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++. -- Regards, Ivan Khoronzhuk