mbox series

[v5,bpf-next,00/15] samples: bpf: improve/fix cross-compilation

Message ID 20191011002808.28206-1-ivan.khoronzhuk@linaro.org
Headers show
Series samples: bpf: improve/fix cross-compilation | expand

Message

Ivan Khoronzhuk Oct. 11, 2019, 12:27 a.m. UTC
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

Comments

Sergei Shtylyov Oct. 11, 2019, 8:49 a.m. UTC | #1
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
Sergei Shtylyov Oct. 11, 2019, 11:16 a.m. UTC | #2
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
Ilias Apalodimas Oct. 11, 2019, 12:07 p.m. UTC | #3
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>
Sergei Shtylyov Oct. 13, 2019, 5:06 p.m. UTC | #4
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
Stanislav Fomichev Nov. 21, 2019, 9:42 p.m. UTC | #5
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

>
Andrii Nakryiko Nov. 22, 2019, 12:11 a.m. UTC | #6
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)
[...]
Stanislav Fomichev Nov. 22, 2019, 4:32 p.m. UTC | #7
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++.
Ivan Khoronzhuk Nov. 22, 2019, 11:47 p.m. UTC | #8
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