| Message ID | 20201209205301.2586678-1-adelg@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] selftests/bpf: Drop the need for LLVM's llc | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 42 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On 12/9/20 12:53 PM, Andrew Delgadillo wrote: > LLC is meant for compiler development and debugging. Consequently, it > exposes many low level options about its backend. To avoid future bugs > introduced by using the raw LLC tool, use clang directly so that all > appropriate options are passed to the back end. Agree that this indeed make build system simpler. > > Additionally, the native clang build rule was not being use in the > selftests Makefile, so remove it. This is true too. Otherwise, native clang build will require both clang and llc runs. The patch looks good and I have a few comments and hopefully you can accommodate. > > Signed-off-by: Andrew Delgadillo <adelg@google.com> > --- > tools/testing/selftests/bpf/Makefile | 20 ++++---------------- > 1 file changed, 4 insertions(+), 16 deletions(-) > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > index 944ae17a39ed..74870d365b62 100644 > --- a/tools/testing/selftests/bpf/Makefile > +++ b/tools/testing/selftests/bpf/Makefile > @@ -19,7 +19,6 @@ ifneq ($(wildcard $(GENHDR)),) > endif > > CLANG ?= clang > -LLC ?= llc > LLVM_OBJCOPY ?= llvm-objcopy > BPF_GCC ?= $(shell command -v bpf-gcc;) > SAN_CFLAGS ?= > @@ -256,24 +255,13 @@ $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h > # $3 - CFLAGS > # $4 - LDFLAGS > define CLANG_BPF_BUILD_RULE > - $(call msg,CLNG-LLC,$(TRUNNER_BINARY),$2) > - $(Q)($(CLANG) $3 -O2 -target bpf -emit-llvm \ > - -c $1 -o - || echo "BPF obj compilation failed") | \ > - $(LLC) -mattr=dwarfris -march=bpf -mcpu=v3 $4 -filetype=obj -o $2 > + $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2) > + $(Q)$(CLANG) $3 -O2 -target bpf -c $1 -o $2 -Xclang -target-feature -Xclang +dwarfris -mcpu=v3 $4 Yes, we still use +dwarfris here. The original llvm patch which introduded +dwarfris is: https://github.com/llvm/llvm-project/commit/03e1c8b8f9cc7b898217b7789d3887a903443c93 it is to workaround an elfutils/libdw issue as it does not support bpf backend so pahole cannot display debuginfo structures properly. Subsequently, the elfutils/libdw bpf support is added at https://sourceware.org/git/?p=elfutils.git;a=commitdiff;h=c1990d36cfe37a30bcc49422c37a6767fd190559 Any recent pahole should already build with the above fix. I tested with pahole 1.16 it works fine for binaries built without +dwarfris. Also BTF now can be used to dump structures. So could you also accommodate the change to remove +dwarfris option? > endef > # Similar to CLANG_BPF_BUILD_RULE, but with disabled alu32 > define CLANG_NOALU32_BPF_BUILD_RULE > - $(call msg,CLNG-LLC,$(TRUNNER_BINARY),$2) > - $(Q)($(CLANG) $3 -O2 -target bpf -emit-llvm \ > - -c $1 -o - || echo "BPF obj compilation failed") | \ > - $(LLC) -march=bpf -mcpu=v2 $4 -filetype=obj -o $2 > -endef > -# Similar to CLANG_BPF_BUILD_RULE, but using native Clang and bpf LLC > -define CLANG_NATIVE_BPF_BUILD_RULE > $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2) > - $(Q)($(CLANG) $3 -O2 -emit-llvm \ > - -c $1 -o - || echo "BPF obj compilation failed") | \ > - $(LLC) -march=bpf -mcpu=v3 $4 -filetype=obj -o $2 > + $(Q)$(CLANG) $3 -O2 -target bpf -c $1 -o $2 -mcpu=v2 $4 > endef > # Build BPF object using GCC > define GCC_BPF_BUILD_RULE > @@ -402,7 +390,7 @@ TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \ > $(wildcard progs/btf_dump_test_case_*.c) > TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE > TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) > -TRUNNER_BPF_LDFLAGS := -mattr=+alu32 > +TRUNNER_BPF_LDFLAGS := -Xclang -target-feature -Xclang +alu32 The +alu32 is only used for non-alu32 case where -mcpu=v3 actually implies alu32. So let us remove TRUNNER_BPF_LDFLAGS flag from Makefile too. > $(eval $(call DEFINE_TEST_RUNNER,test_progs)) > > # Define test_progs-no_alu32 test runner. >
On Wed, Dec 9, 2020 at 6:16 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 12/9/20 12:53 PM, Andrew Delgadillo wrote: > > LLC is meant for compiler development and debugging. Consequently, it > > exposes many low level options about its backend. To avoid future bugs > > introduced by using the raw LLC tool, use clang directly so that all > > appropriate options are passed to the back end. > > Agree that this indeed make build system simpler. > > > > > Additionally, the native clang build rule was not being use in the > > selftests Makefile, so remove it. > > This is true too. Otherwise, native clang build will require both > clang and llc runs. > > The patch looks good and I have a few comments and hopefully > you can accommodate. > > > > > Signed-off-by: Andrew Delgadillo <adelg@google.com> > > --- > > tools/testing/selftests/bpf/Makefile | 20 ++++---------------- > > 1 file changed, 4 insertions(+), 16 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > > index 944ae17a39ed..74870d365b62 100644 > > --- a/tools/testing/selftests/bpf/Makefile > > +++ b/tools/testing/selftests/bpf/Makefile > > @@ -19,7 +19,6 @@ ifneq ($(wildcard $(GENHDR)),) > > endif > > > > CLANG ?= clang > > -LLC ?= llc > > LLVM_OBJCOPY ?= llvm-objcopy > > BPF_GCC ?= $(shell command -v bpf-gcc;) > > SAN_CFLAGS ?= > > @@ -256,24 +255,13 @@ $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h > > # $3 - CFLAGS > > # $4 - LDFLAGS > > define CLANG_BPF_BUILD_RULE > > - $(call msg,CLNG-LLC,$(TRUNNER_BINARY),$2) > > - $(Q)($(CLANG) $3 -O2 -target bpf -emit-llvm \ > > - -c $1 -o - || echo "BPF obj compilation failed") | \ > > - $(LLC) -mattr=dwarfris -march=bpf -mcpu=v3 $4 -filetype=obj -o $2 > > + $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2) > > + $(Q)$(CLANG) $3 -O2 -target bpf -c $1 -o $2 -Xclang -target-feature -Xclang +dwarfris -mcpu=v3 $4 > > Yes, we still use +dwarfris here. > The original llvm patch which introduded +dwarfris is: > > https://github.com/llvm/llvm-project/commit/03e1c8b8f9cc7b898217b7789d3887a903443c93 > it is to workaround an elfutils/libdw issue as it does not support bpf > backend so pahole cannot display debuginfo structures properly. > Subsequently, the elfutils/libdw bpf support is added at > > https://sourceware.org/git/?p=elfutils.git;a=commitdiff;h=c1990d36cfe37a30bcc49422c37a6767fd190559 > > Any recent pahole should already build with the above fix. > I tested with pahole 1.16 it works fine for binaries built without > +dwarfris. Also BTF now can be used to dump structures. > > So could you also accommodate the change to remove +dwarfris option? > > > > endef > > # Similar to CLANG_BPF_BUILD_RULE, but with disabled alu32 > > define CLANG_NOALU32_BPF_BUILD_RULE > > - $(call msg,CLNG-LLC,$(TRUNNER_BINARY),$2) > > - $(Q)($(CLANG) $3 -O2 -target bpf -emit-llvm \ > > - -c $1 -o - || echo "BPF obj compilation failed") | \ > > - $(LLC) -march=bpf -mcpu=v2 $4 -filetype=obj -o $2 > > -endef > > -# Similar to CLANG_BPF_BUILD_RULE, but using native Clang and bpf LLC > > -define CLANG_NATIVE_BPF_BUILD_RULE > > $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2) > > - $(Q)($(CLANG) $3 -O2 -emit-llvm \ > > - -c $1 -o - || echo "BPF obj compilation failed") | \ > > - $(LLC) -march=bpf -mcpu=v3 $4 -filetype=obj -o $2 > > + $(Q)$(CLANG) $3 -O2 -target bpf -c $1 -o $2 -mcpu=v2 $4 > > endef > > # Build BPF object using GCC > > define GCC_BPF_BUILD_RULE > > @@ -402,7 +390,7 @@ TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \ > > $(wildcard progs/btf_dump_test_case_*.c) > > TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE > > TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) > > -TRUNNER_BPF_LDFLAGS := -mattr=+alu32 > > +TRUNNER_BPF_LDFLAGS := -Xclang -target-feature -Xclang +alu32 > > The +alu32 is only used for non-alu32 case where -mcpu=v3 actually > implies alu32. So let us remove TRUNNER_BPF_LDFLAGS flag from Makefile too. > > > $(eval $(call DEFINE_TEST_RUNNER,test_progs)) > > > > # Define test_progs-no_alu32 test runner. > > Thank you for reviewing. I will make those changes and send a v2 of the patch.
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index 944ae17a39ed..74870d365b62 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -19,7 +19,6 @@ ifneq ($(wildcard $(GENHDR)),) endif CLANG ?= clang -LLC ?= llc LLVM_OBJCOPY ?= llvm-objcopy BPF_GCC ?= $(shell command -v bpf-gcc;) SAN_CFLAGS ?= @@ -256,24 +255,13 @@ $(OUTPUT)/flow_dissector_load.o: flow_dissector_load.h # $3 - CFLAGS # $4 - LDFLAGS define CLANG_BPF_BUILD_RULE - $(call msg,CLNG-LLC,$(TRUNNER_BINARY),$2) - $(Q)($(CLANG) $3 -O2 -target bpf -emit-llvm \ - -c $1 -o - || echo "BPF obj compilation failed") | \ - $(LLC) -mattr=dwarfris -march=bpf -mcpu=v3 $4 -filetype=obj -o $2 + $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2) + $(Q)$(CLANG) $3 -O2 -target bpf -c $1 -o $2 -Xclang -target-feature -Xclang +dwarfris -mcpu=v3 $4 endef # Similar to CLANG_BPF_BUILD_RULE, but with disabled alu32 define CLANG_NOALU32_BPF_BUILD_RULE - $(call msg,CLNG-LLC,$(TRUNNER_BINARY),$2) - $(Q)($(CLANG) $3 -O2 -target bpf -emit-llvm \ - -c $1 -o - || echo "BPF obj compilation failed") | \ - $(LLC) -march=bpf -mcpu=v2 $4 -filetype=obj -o $2 -endef -# Similar to CLANG_BPF_BUILD_RULE, but using native Clang and bpf LLC -define CLANG_NATIVE_BPF_BUILD_RULE $(call msg,CLNG-BPF,$(TRUNNER_BINARY),$2) - $(Q)($(CLANG) $3 -O2 -emit-llvm \ - -c $1 -o - || echo "BPF obj compilation failed") | \ - $(LLC) -march=bpf -mcpu=v3 $4 -filetype=obj -o $2 + $(Q)$(CLANG) $3 -O2 -target bpf -c $1 -o $2 -mcpu=v2 $4 endef # Build BPF object using GCC define GCC_BPF_BUILD_RULE @@ -402,7 +390,7 @@ TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \ $(wildcard progs/btf_dump_test_case_*.c) TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -TRUNNER_BPF_LDFLAGS := -mattr=+alu32 +TRUNNER_BPF_LDFLAGS := -Xclang -target-feature -Xclang +alu32 $(eval $(call DEFINE_TEST_RUNNER,test_progs)) # Define test_progs-no_alu32 test runner.
LLC is meant for compiler development and debugging. Consequently, it exposes many low level options about its backend. To avoid future bugs introduced by using the raw LLC tool, use clang directly so that all appropriate options are passed to the back end. Additionally, the native clang build rule was not being use in the selftests Makefile, so remove it. Signed-off-by: Andrew Delgadillo <adelg@google.com> --- tools/testing/selftests/bpf/Makefile | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-)