bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang
@ 2021-04-13 15:34 Yonghong Song
  2021-04-13 15:34 ` [PATCH bpf-next v3 1/5] selftests: set CC to clang in lib.mk if LLVM is set Yonghong Song
                   ` (6 more replies)
  0 siblings, 7 replies; 18+ messages in thread
From: Yonghong Song @ 2021-04-13 15:34 UTC (permalink / raw)
  To: bpf
  Cc: Andrii Nakryiko, Arnaldo Carvalho de Melo, kernel-team,
	Nick Desaulniers, Sedat Dilek

To build kernel with clang, people typically use
  make -j60 LLVM=1 LLVM_IAS=1
LLVM_IAS=1 is not required for non-LTO build but
is required for LTO build. In my environment,
I am always having LLVM_IAS=1 regardless of
whether LTO is enabled or not.

After kernel is build with clang, the following command
can be used to build selftests with clang:
  make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1

I am using latest bpf-next kernel code base and
latest clang built from source from
  https://github.com/llvm/llvm-project.git
Using earlier version of llvm may have compilation errors, see
  tools/testing/selftests/bpf
due to continuous development in llvm bpf features and selftests
to use these features.

To run bpf selftest properly, you need have certain necessary
kernel configs like at:
  bpf-next:tools/testing/selftests/bpf/config
(not that this is not a complete .config file and some other configs
 might still be needed.)

Currently, using the above command, some compilations
still use gcc and there are also compilation errors and warnings.
This patch set intends to fix these issues.
Patch #1 and #2 fixed the issue so clang/clang++ is
used instead of gcc/g++. Patch #3 fixed a compilation
failure. Patch #4 and #5 fixed various compiler warnings.

Changelog:
  v2 -> v3:
    . more test environment description in cover letter. (Sedat)
    . use a different fix, but similar to other use in selftests/bpf
      Makefile, to exclude header files from CXX compilation command
      line. (Andrii)
    . fix codes instead of adding -Wno-format-security. (Andrii)
  v1 -> v2:
    . add -Wno-unused-command-line-argument and -Wno-format-security
      for clang only as (1). gcc does not exhibit those
      warnings, and (2). -Wno-unused-command-line-argument is
      only supported by clang. (Sedat)

Yonghong Song (5):
  selftests: set CC to clang in lib.mk if LLVM is set
  tools: allow proper CC/CXX/... override with LLVM=1 in
    Makefile.include
  selftests/bpf: fix test_cpp compilation failure with clang
  selftests/bpf: silence clang compilation warnings
  bpftool: fix a clang compilation warning

 tools/bpf/bpftool/net.c                              |  2 +-
 tools/scripts/Makefile.include                       | 12 ++++++++++--
 tools/testing/selftests/bpf/Makefile                 |  7 ++++++-
 tools/testing/selftests/bpf/prog_tests/fexit_sleep.c |  4 ++--
 .../selftests/bpf/prog_tests/ns_current_pid_tgid.c   |  4 ++--
 tools/testing/selftests/lib.mk                       |  4 ++++
 6 files changed, 25 insertions(+), 8 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH bpf-next v3 1/5] selftests: set CC to clang in lib.mk if LLVM is set
  2021-04-13 15:34 [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang Yonghong Song
@ 2021-04-13 15:34 ` Yonghong Song
  2021-04-13 22:04   ` Andrii Nakryiko
  2021-04-13 15:34 ` [PATCH bpf-next v3 2/5] tools: allow proper CC/CXX/... override with LLVM=1 in Makefile.include Yonghong Song
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Yonghong Song @ 2021-04-13 15:34 UTC (permalink / raw)
  To: bpf
  Cc: Andrii Nakryiko, Arnaldo Carvalho de Melo, kernel-team,
	Nick Desaulniers, Sedat Dilek

selftests/bpf/Makefile includes lib.mk. With the following command
  make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
  make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1 V=1
some files are still compiled with gcc. This patch
fixed lib.mk issue which sets CC to gcc in all cases.

Cc: Sedat Dilek <sedat.dilek@gmail.com>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/testing/selftests/lib.mk | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index a5ce26d548e4..9a41d8bb9ff1 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -1,6 +1,10 @@
 # This mimics the top-level Makefile. We do it explicitly here so that this
 # Makefile can operate with or without the kbuild infrastructure.
+ifneq ($(LLVM),)
+CC := clang
+else
 CC := $(CROSS_COMPILE)gcc
+endif
 
 ifeq (0,$(MAKELEVEL))
     ifeq ($(OUTPUT),)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH bpf-next v3 2/5] tools: allow proper CC/CXX/... override with LLVM=1 in Makefile.include
  2021-04-13 15:34 [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang Yonghong Song
  2021-04-13 15:34 ` [PATCH bpf-next v3 1/5] selftests: set CC to clang in lib.mk if LLVM is set Yonghong Song
@ 2021-04-13 15:34 ` Yonghong Song
  2021-04-13 15:34 ` [PATCH bpf-next v3 3/5] selftests/bpf: fix test_cpp compilation failure with clang Yonghong Song
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 18+ messages in thread
From: Yonghong Song @ 2021-04-13 15:34 UTC (permalink / raw)
  To: bpf
  Cc: Andrii Nakryiko, Arnaldo Carvalho de Melo, kernel-team,
	Nick Desaulniers, Sedat Dilek

selftests/bpf/Makefile includes tools/scripts/Makefile.include.
With the following command
  make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
  make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1 V=1
some files are still compiled with gcc. This patch
fixed the case if CC/AR/LD/CXX/STRIP is allowed to be
overridden, it will be written to clang/llvm-ar/..., instead of
gcc binaries. The definition of CC_NO_CLANG is also relocated
to the place after the above CC is defined.

Cc: Sedat Dilek <sedat.dilek@gmail.com>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/scripts/Makefile.include | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include
index a402f32a145c..91130648d8e6 100644
--- a/tools/scripts/Makefile.include
+++ b/tools/scripts/Makefile.include
@@ -39,8 +39,6 @@ EXTRA_WARNINGS += -Wundef
 EXTRA_WARNINGS += -Wwrite-strings
 EXTRA_WARNINGS += -Wformat
 
-CC_NO_CLANG := $(shell $(CC) -dM -E -x c /dev/null | grep -Fq "__clang__"; echo $$?)
-
 # Makefiles suck: This macro sets a default value of $(2) for the
 # variable named by $(1), unless the variable has been set by
 # environment or command line. This is necessary for CC and AR
@@ -52,12 +50,22 @@ define allow-override
     $(eval $(1) = $(2)))
 endef
 
+ifneq ($(LLVM),)
+$(call allow-override,CC,clang)
+$(call allow-override,AR,llvm-ar)
+$(call allow-override,LD,ld.lld)
+$(call allow-override,CXX,clang++)
+$(call allow-override,STRIP,llvm-strip)
+else
 # Allow setting various cross-compile vars or setting CROSS_COMPILE as a prefix.
 $(call allow-override,CC,$(CROSS_COMPILE)gcc)
 $(call allow-override,AR,$(CROSS_COMPILE)ar)
 $(call allow-override,LD,$(CROSS_COMPILE)ld)
 $(call allow-override,CXX,$(CROSS_COMPILE)g++)
 $(call allow-override,STRIP,$(CROSS_COMPILE)strip)
+endif
+
+CC_NO_CLANG := $(shell $(CC) -dM -E -x c /dev/null | grep -Fq "__clang__"; echo $$?)
 
 ifneq ($(LLVM),)
 HOSTAR  ?= llvm-ar
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH bpf-next v3 3/5] selftests/bpf: fix test_cpp compilation failure with clang
  2021-04-13 15:34 [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang Yonghong Song
  2021-04-13 15:34 ` [PATCH bpf-next v3 1/5] selftests: set CC to clang in lib.mk if LLVM is set Yonghong Song
  2021-04-13 15:34 ` [PATCH bpf-next v3 2/5] tools: allow proper CC/CXX/... override with LLVM=1 in Makefile.include Yonghong Song
@ 2021-04-13 15:34 ` Yonghong Song
  2021-04-13 22:05   ` Andrii Nakryiko
  2021-04-13 15:34 ` [PATCH bpf-next v3 4/5] selftests/bpf: silence clang compilation warnings Yonghong Song
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Yonghong Song @ 2021-04-13 15:34 UTC (permalink / raw)
  To: bpf
  Cc: Andrii Nakryiko, Arnaldo Carvalho de Melo, kernel-team,
	Nick Desaulniers, Sedat Dilek

With clang compiler:
  make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
  make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1
the test_cpp build failed due to the failure:
  warning: treating 'c-header' input as 'c++-header' when in C++ mode, this behavior is deprecated [-Wdeprecated]
  clang-13: error: cannot specify -o when generating multiple output files

test_cpp compilation flag looks like:
  clang++ -g -Og -rdynamic -Wall -I<...> ... \
  -Dbpf_prog_load=bpf_prog_test_load -Dbpf_load_program=bpf_test_load_program \
  test_cpp.cpp <...>/test_core_extern.skel.h <...>/libbpf.a <...>/test_stub.o \
  -lcap -lelf -lz -lrt -lpthread -o <...>/test_cpp

The clang++ compiler complains the header file in the command line and
also failed the compilation due to this.
Let us remove the header file from the command line which is not intended
any way, and this fixed the compilation problem.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/testing/selftests/bpf/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 6448c626498f..dcc2dc1f2a86 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -481,7 +481,7 @@ $(OUTPUT)/test_verifier: test_verifier.c verifier/tests.h $(BPFOBJ) | $(OUTPUT)
 # Make sure we are able to include and link libbpf against c++.
 $(OUTPUT)/test_cpp: test_cpp.cpp $(OUTPUT)/test_core_extern.skel.h $(BPFOBJ)
 	$(call msg,CXX,,$@)
-	$(Q)$(CXX) $(CFLAGS) $^ $(LDLIBS) -o $@
+	$(Q)$(CXX) $(CFLAGS) $(filter %.a %.o %.cpp,$^) $(LDLIBS) -o $@
 
 # Benchmark runner
 $(OUTPUT)/bench_%.o: benchs/bench_%.c bench.h
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH bpf-next v3 4/5] selftests/bpf: silence clang compilation warnings
  2021-04-13 15:34 [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang Yonghong Song
                   ` (2 preceding siblings ...)
  2021-04-13 15:34 ` [PATCH bpf-next v3 3/5] selftests/bpf: fix test_cpp compilation failure with clang Yonghong Song
@ 2021-04-13 15:34 ` Yonghong Song
  2021-04-13 22:07   ` Andrii Nakryiko
  2021-04-13 15:34 ` [PATCH bpf-next v3 5/5] bpftool: fix a clang compilation warning Yonghong Song
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Yonghong Song @ 2021-04-13 15:34 UTC (permalink / raw)
  To: bpf
  Cc: Andrii Nakryiko, Arnaldo Carvalho de Melo, kernel-team,
	Nick Desaulniers, Sedat Dilek

With clang compiler:
  make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
  make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1
Some linker flags are not used/effective for some binaries and
we have warnings like:
  warning: -lelf: 'linker' input unused [-Wunused-command-line-argument]

We also have warnings like:
  .../selftests/bpf/prog_tests/ns_current_pid_tgid.c:74:57: note: treat the string as an argument to avoid this
        if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid", strerror(errno)))
                                                               ^
                                                               "%s",
  .../selftests/bpf/test_progs.h:129:35: note: expanded from macro 'CHECK'
        _CHECK(condition, tag, duration, format)
                                         ^
  .../selftests/bpf/test_progs.h:108:21: note: expanded from macro '_CHECK'
                fprintf(stdout, ##format);                              \
                                  ^
The first warning can be silenced with clang option -Wno-unused-command-line-argument.
For the second warning, source codes are modified as suggested by the compiler
to silence the warning. Since gcc does not support the option
-Wno-unused-command-line-argument and the warning only happens with clang
compiler, the option -Wno-unused-command-line-argument is enabled only when
clang compiler is used.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/testing/selftests/bpf/Makefile                         | 5 +++++
 tools/testing/selftests/bpf/prog_tests/fexit_sleep.c         | 4 ++--
 tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c | 4 ++--
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index dcc2dc1f2a86..c45ae13b88a0 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -28,6 +28,11 @@ CFLAGS += -g -Og -rdynamic -Wall $(GENFLAGS) $(SAN_CFLAGS)		\
 	  -Dbpf_load_program=bpf_test_load_program
 LDLIBS += -lcap -lelf -lz -lrt -lpthread
 
+# Silence some warnings when compiled with clang
+ifneq ($(LLVM),)
+CFLAGS += -Wno-unused-command-line-argument
+endif
+
 # Order correspond to 'make run_tests' order
 TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
 	test_verifier_log test_dev_cgroup \
diff --git a/tools/testing/selftests/bpf/prog_tests/fexit_sleep.c b/tools/testing/selftests/bpf/prog_tests/fexit_sleep.c
index 6c4d42a2386f..ccc7e8a34ab6 100644
--- a/tools/testing/selftests/bpf/prog_tests/fexit_sleep.c
+++ b/tools/testing/selftests/bpf/prog_tests/fexit_sleep.c
@@ -39,7 +39,7 @@ void test_fexit_sleep(void)
 		goto cleanup;
 
 	cpid = clone(do_sleep, child_stack + STACK_SIZE, CLONE_FILES | SIGCHLD, fexit_skel);
-	if (CHECK(cpid == -1, "clone", strerror(errno)))
+	if (CHECK(cpid == -1, "clone", "%s\n", strerror(errno)))
 		goto cleanup;
 
 	/* wait until first sys_nanosleep ends and second sys_nanosleep starts */
@@ -65,7 +65,7 @@ void test_fexit_sleep(void)
 	/* kill the thread to unwind sys_nanosleep stack through the trampoline */
 	kill(cpid, 9);
 
-	if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid", strerror(errno)))
+	if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid", "%s\n", strerror(errno)))
 		goto cleanup;
 	if (CHECK(WEXITSTATUS(wstatus) != 0, "exitstatus", "failed"))
 		goto cleanup;
diff --git a/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c b/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
index 31a3114906e2..2535788e135f 100644
--- a/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
+++ b/tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c
@@ -68,10 +68,10 @@ static void test_ns_current_pid_tgid_new_ns(void)
 	cpid = clone(test_current_pid_tgid, child_stack + STACK_SIZE,
 		     CLONE_NEWPID | SIGCHLD, NULL);
 
-	if (CHECK(cpid == -1, "clone", strerror(errno)))
+	if (CHECK(cpid == -1, "clone", "%s\n", strerror(errno)))
 		return;
 
-	if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid", strerror(errno)))
+	if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid", "%s\n", strerror(errno)))
 		return;
 
 	if (CHECK(WEXITSTATUS(wstatus) != 0, "newns_pidtgid", "failed"))
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH bpf-next v3 5/5] bpftool: fix a clang compilation warning
  2021-04-13 15:34 [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang Yonghong Song
                   ` (3 preceding siblings ...)
  2021-04-13 15:34 ` [PATCH bpf-next v3 4/5] selftests/bpf: silence clang compilation warnings Yonghong Song
@ 2021-04-13 15:34 ` Yonghong Song
  2021-04-13 19:36 ` [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang Martin KaFai Lau
  2021-04-16  0:21 ` Alexei Starovoitov
  6 siblings, 0 replies; 18+ messages in thread
From: Yonghong Song @ 2021-04-13 15:34 UTC (permalink / raw)
  To: bpf
  Cc: Andrii Nakryiko, Arnaldo Carvalho de Melo, kernel-team,
	Nick Desaulniers, Sedat Dilek

With clang compiler:
  make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
  # build selftests/bpf or bpftool
  make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1
  make -j60 -C tools/bpf/bpftool LLVM=1 LLVM_IAS=1
the following compilation warning showed up,
  net.c:160:37: warning: comparison of integers of different signs: '__u32' (aka 'unsigned int') and 'int' [-Wsign-compare]
                for (nh = (struct nlmsghdr *)buf; NLMSG_OK(nh, len);
                                                  ^~~~~~~~~~~~~~~~~
  .../tools/include/uapi/linux/netlink.h:99:24: note: expanded from macro 'NLMSG_OK'
                           (nlh)->nlmsg_len <= (len))
                           ~~~~~~~~~~~~~~~~ ^   ~~~

In this particular case, "len" is defined as "int" and (nlh)->nlmsg_len is "unsigned int".
The macro NLMSG_OK is defined as below in uapi/linux/netlink.h.
  #define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
                             (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
                             (nlh)->nlmsg_len <= (len))

The clang compiler complains the comparision "(nlh)->nlmsg_len <= (len))",
but in bpftool/net.c, it is already ensured that "len > 0" must be true.
So theoretically the compiler could deduce that comparison of
"(nlh)->nlmsg_len" and "len" is okay, but this really depends on compiler
internals. Let us add an explicit type conversion (from "int" to "unsigned int")
for "len" in NLMSG_OK to silence this warning right now.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/bpf/bpftool/net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
index ff3aa0cf3997..f836d115d7d6 100644
--- a/tools/bpf/bpftool/net.c
+++ b/tools/bpf/bpftool/net.c
@@ -157,7 +157,7 @@ static int netlink_recv(int sock, __u32 nl_pid, __u32 seq,
 		if (len == 0)
 			break;
 
-		for (nh = (struct nlmsghdr *)buf; NLMSG_OK(nh, len);
+		for (nh = (struct nlmsghdr *)buf; NLMSG_OK(nh, (unsigned int)len);
 		     nh = NLMSG_NEXT(nh, len)) {
 			if (nh->nlmsg_pid != nl_pid) {
 				ret = -LIBBPF_ERRNO__WRNGPID;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang
  2021-04-13 15:34 [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang Yonghong Song
                   ` (4 preceding siblings ...)
  2021-04-13 15:34 ` [PATCH bpf-next v3 5/5] bpftool: fix a clang compilation warning Yonghong Song
@ 2021-04-13 19:36 ` Martin KaFai Lau
  2021-04-16  0:21 ` Alexei Starovoitov
  6 siblings, 0 replies; 18+ messages in thread
From: Martin KaFai Lau @ 2021-04-13 19:36 UTC (permalink / raw)
  To: Yonghong Song
  Cc: bpf, Andrii Nakryiko, Arnaldo Carvalho de Melo, kernel-team,
	Nick Desaulniers, Sedat Dilek

On Tue, Apr 13, 2021 at 08:34:08AM -0700, Yonghong Song wrote:
> To build kernel with clang, people typically use
>   make -j60 LLVM=1 LLVM_IAS=1
> LLVM_IAS=1 is not required for non-LTO build but
> is required for LTO build. In my environment,
> I am always having LLVM_IAS=1 regardless of
> whether LTO is enabled or not.
> 
> After kernel is build with clang, the following command
> can be used to build selftests with clang:
>   make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1
> 
> I am using latest bpf-next kernel code base and
> latest clang built from source from
>   https://github.com/llvm/llvm-project.git
> Using earlier version of llvm may have compilation errors, see
>   tools/testing/selftests/bpf
> due to continuous development in llvm bpf features and selftests
> to use these features.
> 
> To run bpf selftest properly, you need have certain necessary
> kernel configs like at:
>   bpf-next:tools/testing/selftests/bpf/config
> (not that this is not a complete .config file and some other configs
>  might still be needed.)
> 
> Currently, using the above command, some compilations
> still use gcc and there are also compilation errors and warnings.
> This patch set intends to fix these issues.
> Patch #1 and #2 fixed the issue so clang/clang++ is
> used instead of gcc/g++. Patch #3 fixed a compilation
> failure. Patch #4 and #5 fixed various compiler warnings.
Acked-by: Martin KaFai Lau <kafai@fb.com>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH bpf-next v3 1/5] selftests: set CC to clang in lib.mk if LLVM is set
  2021-04-13 15:34 ` [PATCH bpf-next v3 1/5] selftests: set CC to clang in lib.mk if LLVM is set Yonghong Song
@ 2021-04-13 22:04   ` Andrii Nakryiko
  2021-04-13 22:13     ` Nick Desaulniers
  0 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2021-04-13 22:04 UTC (permalink / raw)
  To: Yonghong Song
  Cc: bpf, Andrii Nakryiko, Arnaldo Carvalho de Melo, Kernel Team,
	Nick Desaulniers, Sedat Dilek

On Tue, Apr 13, 2021 at 8:34 AM Yonghong Song <yhs@fb.com> wrote:
>
> selftests/bpf/Makefile includes lib.mk. With the following command
>   make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
>   make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1 V=1
> some files are still compiled with gcc. This patch
> fixed lib.mk issue which sets CC to gcc in all cases.
>
> Cc: Sedat Dilek <sedat.dilek@gmail.com>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  tools/testing/selftests/lib.mk | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> index a5ce26d548e4..9a41d8bb9ff1 100644
> --- a/tools/testing/selftests/lib.mk
> +++ b/tools/testing/selftests/lib.mk
> @@ -1,6 +1,10 @@
>  # This mimics the top-level Makefile. We do it explicitly here so that this
>  # Makefile can operate with or without the kbuild infrastructure.
> +ifneq ($(LLVM),)
> +CC := clang

Does this mean that cross-compilation with Clang doesn't work at all
or is achieved in some other way?


> +else
>  CC := $(CROSS_COMPILE)gcc
> +endif
>
>  ifeq (0,$(MAKELEVEL))
>      ifeq ($(OUTPUT),)
> --
> 2.30.2
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH bpf-next v3 3/5] selftests/bpf: fix test_cpp compilation failure with clang
  2021-04-13 15:34 ` [PATCH bpf-next v3 3/5] selftests/bpf: fix test_cpp compilation failure with clang Yonghong Song
@ 2021-04-13 22:05   ` Andrii Nakryiko
  0 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2021-04-13 22:05 UTC (permalink / raw)
  To: Yonghong Song
  Cc: bpf, Andrii Nakryiko, Arnaldo Carvalho de Melo, Kernel Team,
	Nick Desaulniers, Sedat Dilek

On Tue, Apr 13, 2021 at 8:34 AM Yonghong Song <yhs@fb.com> wrote:
>
> With clang compiler:
>   make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
>   make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1
> the test_cpp build failed due to the failure:
>   warning: treating 'c-header' input as 'c++-header' when in C++ mode, this behavior is deprecated [-Wdeprecated]
>   clang-13: error: cannot specify -o when generating multiple output files
>
> test_cpp compilation flag looks like:
>   clang++ -g -Og -rdynamic -Wall -I<...> ... \
>   -Dbpf_prog_load=bpf_prog_test_load -Dbpf_load_program=bpf_test_load_program \
>   test_cpp.cpp <...>/test_core_extern.skel.h <...>/libbpf.a <...>/test_stub.o \
>   -lcap -lelf -lz -lrt -lpthread -o <...>/test_cpp
>
> The clang++ compiler complains the header file in the command line and
> also failed the compilation due to this.
> Let us remove the header file from the command line which is not intended
> any way, and this fixed the compilation problem.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  tools/testing/selftests/bpf/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 6448c626498f..dcc2dc1f2a86 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -481,7 +481,7 @@ $(OUTPUT)/test_verifier: test_verifier.c verifier/tests.h $(BPFOBJ) | $(OUTPUT)
>  # Make sure we are able to include and link libbpf against c++.
>  $(OUTPUT)/test_cpp: test_cpp.cpp $(OUTPUT)/test_core_extern.skel.h $(BPFOBJ)
>         $(call msg,CXX,,$@)
> -       $(Q)$(CXX) $(CFLAGS) $^ $(LDLIBS) -o $@
> +       $(Q)$(CXX) $(CFLAGS) $(filter %.a %.o %.cpp,$^) $(LDLIBS) -o $@
>
>  # Benchmark runner
>  $(OUTPUT)/bench_%.o: benchs/bench_%.c bench.h
> --
> 2.30.2
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH bpf-next v3 4/5] selftests/bpf: silence clang compilation warnings
  2021-04-13 15:34 ` [PATCH bpf-next v3 4/5] selftests/bpf: silence clang compilation warnings Yonghong Song
@ 2021-04-13 22:07   ` Andrii Nakryiko
  2021-04-13 22:17     ` Nick Desaulniers
  0 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2021-04-13 22:07 UTC (permalink / raw)
  To: Yonghong Song
  Cc: bpf, Andrii Nakryiko, Arnaldo Carvalho de Melo, Kernel Team,
	Nick Desaulniers, Sedat Dilek

On Tue, Apr 13, 2021 at 8:34 AM Yonghong Song <yhs@fb.com> wrote:
>
> With clang compiler:
>   make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
>   make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1
> Some linker flags are not used/effective for some binaries and
> we have warnings like:
>   warning: -lelf: 'linker' input unused [-Wunused-command-line-argument]
>
> We also have warnings like:
>   .../selftests/bpf/prog_tests/ns_current_pid_tgid.c:74:57: note: treat the string as an argument to avoid this
>         if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid", strerror(errno)))
>                                                                ^
>                                                                "%s",
>   .../selftests/bpf/test_progs.h:129:35: note: expanded from macro 'CHECK'
>         _CHECK(condition, tag, duration, format)
>                                          ^
>   .../selftests/bpf/test_progs.h:108:21: note: expanded from macro '_CHECK'
>                 fprintf(stdout, ##format);                              \
>                                   ^
> The first warning can be silenced with clang option -Wno-unused-command-line-argument.
> For the second warning, source codes are modified as suggested by the compiler
> to silence the warning. Since gcc does not support the option
> -Wno-unused-command-line-argument and the warning only happens with clang
> compiler, the option -Wno-unused-command-line-argument is enabled only when
> clang compiler is used.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---

LGTM, please see the question below.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  tools/testing/selftests/bpf/Makefile                         | 5 +++++
>  tools/testing/selftests/bpf/prog_tests/fexit_sleep.c         | 4 ++--
>  tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c | 4 ++--
>  3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index dcc2dc1f2a86..c45ae13b88a0 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -28,6 +28,11 @@ CFLAGS += -g -Og -rdynamic -Wall $(GENFLAGS) $(SAN_CFLAGS)           \
>           -Dbpf_load_program=bpf_test_load_program
>  LDLIBS += -lcap -lelf -lz -lrt -lpthread
>
> +# Silence some warnings when compiled with clang
> +ifneq ($(LLVM),)

This won't handle the case where someone does `make CC=clang`, right?
Do we care at all?


> +CFLAGS += -Wno-unused-command-line-argument
> +endif
> +

[...]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH bpf-next v3 1/5] selftests: set CC to clang in lib.mk if LLVM is set
  2021-04-13 22:04   ` Andrii Nakryiko
@ 2021-04-13 22:13     ` Nick Desaulniers
  2021-04-13 22:27       ` Yonghong Song
  0 siblings, 1 reply; 18+ messages in thread
From: Nick Desaulniers @ 2021-04-13 22:13 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Yonghong Song, bpf, Andrii Nakryiko, Arnaldo Carvalho de Melo,
	Kernel Team, Sedat Dilek

On Tue, Apr 13, 2021 at 3:05 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Apr 13, 2021 at 8:34 AM Yonghong Song <yhs@fb.com> wrote:
> >
> > selftests/bpf/Makefile includes lib.mk. With the following command
> >   make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
> >   make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1 V=1
> > some files are still compiled with gcc. This patch
> > fixed lib.mk issue which sets CC to gcc in all cases.
> >
> > Cc: Sedat Dilek <sedat.dilek@gmail.com>
> > Signed-off-by: Yonghong Song <yhs@fb.com>
> > ---
> >  tools/testing/selftests/lib.mk | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> > index a5ce26d548e4..9a41d8bb9ff1 100644
> > --- a/tools/testing/selftests/lib.mk
> > +++ b/tools/testing/selftests/lib.mk
> > @@ -1,6 +1,10 @@
> >  # This mimics the top-level Makefile. We do it explicitly here so that this
> >  # Makefile can operate with or without the kbuild infrastructure.
> > +ifneq ($(LLVM),)
> > +CC := clang
>
> Does this mean that cross-compilation with Clang doesn't work at all
> or is achieved in some other way?

Right, this probably doesn't support cross compilation w/ Clang.
Rather than invoke `$(CROSS_COMPILE) clang`, you'd do `clang
--target=$(CROSS_COMPILE)`.  Even then, cross linking executables is
hairy.  But at least this should enable native compilation, which is a
start.

>
>
> > +else
> >  CC := $(CROSS_COMPILE)gcc
> > +endif
> >
> >  ifeq (0,$(MAKELEVEL))
> >      ifeq ($(OUTPUT),)
> > --
> > 2.30.2
> >



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH bpf-next v3 4/5] selftests/bpf: silence clang compilation warnings
  2021-04-13 22:07   ` Andrii Nakryiko
@ 2021-04-13 22:17     ` Nick Desaulniers
  2021-04-13 22:37       ` Yonghong Song
  0 siblings, 1 reply; 18+ messages in thread
From: Nick Desaulniers @ 2021-04-13 22:17 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Yonghong Song, bpf, Andrii Nakryiko, Arnaldo Carvalho de Melo,
	Kernel Team, Sedat Dilek

On Tue, Apr 13, 2021 at 3:08 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Apr 13, 2021 at 8:34 AM Yonghong Song <yhs@fb.com> wrote:
> >
> > With clang compiler:
> >   make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
> >   make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1
> > Some linker flags are not used/effective for some binaries and
> > we have warnings like:
> >   warning: -lelf: 'linker' input unused [-Wunused-command-line-argument]
> >
> > We also have warnings like:
> >   .../selftests/bpf/prog_tests/ns_current_pid_tgid.c:74:57: note: treat the string as an argument to avoid this
> >         if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid", strerror(errno)))
> >                                                                ^
> >                                                                "%s",
> >   .../selftests/bpf/test_progs.h:129:35: note: expanded from macro 'CHECK'
> >         _CHECK(condition, tag, duration, format)
> >                                          ^
> >   .../selftests/bpf/test_progs.h:108:21: note: expanded from macro '_CHECK'
> >                 fprintf(stdout, ##format);                              \
> >                                   ^
> > The first warning can be silenced with clang option -Wno-unused-command-line-argument.
> > For the second warning, source codes are modified as suggested by the compiler
> > to silence the warning. Since gcc does not support the option
> > -Wno-unused-command-line-argument and the warning only happens with clang
> > compiler, the option -Wno-unused-command-line-argument is enabled only when
> > clang compiler is used.
> >
> > Signed-off-by: Yonghong Song <yhs@fb.com>
> > ---
>
> LGTM, please see the question below.
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
> >  tools/testing/selftests/bpf/Makefile                         | 5 +++++
> >  tools/testing/selftests/bpf/prog_tests/fexit_sleep.c         | 4 ++--
> >  tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c | 4 ++--
> >  3 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index dcc2dc1f2a86..c45ae13b88a0 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -28,6 +28,11 @@ CFLAGS += -g -Og -rdynamic -Wall $(GENFLAGS) $(SAN_CFLAGS)           \
> >           -Dbpf_load_program=bpf_test_load_program
> >  LDLIBS += -lcap -lelf -lz -lrt -lpthread
> >
> > +# Silence some warnings when compiled with clang
> > +ifneq ($(LLVM),)
>
> This won't handle the case where someone does `make CC=clang`, right?
> Do we care at all?

Right; it would be better to check CC=clang and have LLVM=1 enable
CC=clang from a higher level Makefile (since tools/build/Makefile
seems to override the top level $CC). See the top level Makefile
L448-456.  Then it should work for CC=clang or LLVM=1.

>
>
> > +CFLAGS += -Wno-unused-command-line-argument
> > +endif
> > +
>
> [...]



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH bpf-next v3 1/5] selftests: set CC to clang in lib.mk if LLVM is set
  2021-04-13 22:13     ` Nick Desaulniers
@ 2021-04-13 22:27       ` Yonghong Song
  2021-04-13 22:34         ` Nick Desaulniers
  2021-04-13 23:22         ` Andrii Nakryiko
  0 siblings, 2 replies; 18+ messages in thread
From: Yonghong Song @ 2021-04-13 22:27 UTC (permalink / raw)
  To: Nick Desaulniers, Andrii Nakryiko
  Cc: bpf, Andrii Nakryiko, Arnaldo Carvalho de Melo, Kernel Team, Sedat Dilek



On 4/13/21 3:13 PM, Nick Desaulniers wrote:
> On Tue, Apr 13, 2021 at 3:05 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Tue, Apr 13, 2021 at 8:34 AM Yonghong Song <yhs@fb.com> wrote:
>>>
>>> selftests/bpf/Makefile includes lib.mk. With the following command
>>>    make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
>>>    make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1 V=1
>>> some files are still compiled with gcc. This patch
>>> fixed lib.mk issue which sets CC to gcc in all cases.
>>>
>>> Cc: Sedat Dilek <sedat.dilek@gmail.com>
>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>> ---
>>>   tools/testing/selftests/lib.mk | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
>>> index a5ce26d548e4..9a41d8bb9ff1 100644
>>> --- a/tools/testing/selftests/lib.mk
>>> +++ b/tools/testing/selftests/lib.mk
>>> @@ -1,6 +1,10 @@
>>>   # This mimics the top-level Makefile. We do it explicitly here so that this
>>>   # Makefile can operate with or without the kbuild infrastructure.
>>> +ifneq ($(LLVM),)
>>> +CC := clang
>>
>> Does this mean that cross-compilation with Clang doesn't work at all
>> or is achieved in some other way?
> 
> Right, this probably doesn't support cross compilation w/ Clang.
> Rather than invoke `$(CROSS_COMPILE) clang`, you'd do `clang
> --target=$(CROSS_COMPILE)`.  Even then, cross linking executables is
> hairy.  But at least this should enable native compilation, which is a
> start.

See https://clang.llvm.org/docs/CrossCompilation.html.
As Nick said, clang prefers --target=$(CROSS_COMPILE) to
indicate cross compilation. User can pass additional
flags (CFLAGS) for cross compilation for the time being.
This is the same as main kernel Makefile.

ifneq ($(LLVM),)
CC              = clang
LD              = ld.lld
AR              = llvm-ar
NM              = llvm-nm
OBJCOPY         = llvm-objcopy
OBJDUMP         = llvm-objdump
READELF         = llvm-readelf
STRIP           = llvm-strip
else
CC              = $(CROSS_COMPILE)gcc
LD              = $(CROSS_COMPILE)ld
AR              = $(CROSS_COMPILE)ar
NM              = $(CROSS_COMPILE)nm
OBJCOPY         = $(CROSS_COMPILE)objcopy
OBJDUMP         = $(CROSS_COMPILE)objdump
READELF         = $(CROSS_COMPILE)readelf
STRIP           = $(CROSS_COMPILE)strip
endif

>>
>>
>>> +else
>>>   CC := $(CROSS_COMPILE)gcc
>>> +endif
>>>
>>>   ifeq (0,$(MAKELEVEL))
>>>       ifeq ($(OUTPUT),)
>>> --
>>> 2.30.2
>>>
> 
> 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH bpf-next v3 1/5] selftests: set CC to clang in lib.mk if LLVM is set
  2021-04-13 22:27       ` Yonghong Song
@ 2021-04-13 22:34         ` Nick Desaulniers
  2021-04-13 23:22         ` Andrii Nakryiko
  1 sibling, 0 replies; 18+ messages in thread
From: Nick Desaulniers @ 2021-04-13 22:34 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Andrii Nakryiko, bpf, Andrii Nakryiko, Arnaldo Carvalho de Melo,
	Kernel Team, Sedat Dilek, Masahiro Yamada

On Tue, Apr 13, 2021 at 3:27 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 4/13/21 3:13 PM, Nick Desaulniers wrote:
> > On Tue, Apr 13, 2021 at 3:05 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >>
> >> On Tue, Apr 13, 2021 at 8:34 AM Yonghong Song <yhs@fb.com> wrote:
> >>>
> >>> selftests/bpf/Makefile includes lib.mk. With the following command
> >>>    make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
> >>>    make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1 V=1
> >>> some files are still compiled with gcc. This patch
> >>> fixed lib.mk issue which sets CC to gcc in all cases.
> >>>
> >>> Cc: Sedat Dilek <sedat.dilek@gmail.com>
> >>> Signed-off-by: Yonghong Song <yhs@fb.com>
> >>> ---
> >>>   tools/testing/selftests/lib.mk | 4 ++++
> >>>   1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> >>> index a5ce26d548e4..9a41d8bb9ff1 100644
> >>> --- a/tools/testing/selftests/lib.mk
> >>> +++ b/tools/testing/selftests/lib.mk
> >>> @@ -1,6 +1,10 @@
> >>>   # This mimics the top-level Makefile. We do it explicitly here so that this
> >>>   # Makefile can operate with or without the kbuild infrastructure.
> >>> +ifneq ($(LLVM),)
> >>> +CC := clang
> >>
> >> Does this mean that cross-compilation with Clang doesn't work at all
> >> or is achieved in some other way?
> >
> > Right, this probably doesn't support cross compilation w/ Clang.
> > Rather than invoke `$(CROSS_COMPILE) clang`, you'd do `clang
> > --target=$(CROSS_COMPILE)`.  Even then, cross linking executables is
> > hairy.  But at least this should enable native compilation, which is a
> > start.
>
> See https://clang.llvm.org/docs/CrossCompilation.html.
> As Nick said, clang prefers --target=$(CROSS_COMPILE) to
> indicate cross compilation. User can pass additional
> flags (CFLAGS) for cross compilation for the time being.
> This is the same as main kernel Makefile.
>
> ifneq ($(LLVM),)
> CC              = clang
> LD              = ld.lld
> AR              = llvm-ar
> NM              = llvm-nm
> OBJCOPY         = llvm-objcopy
> OBJDUMP         = llvm-objdump
> READELF         = llvm-readelf
> STRIP           = llvm-strip
> else
> CC              = $(CROSS_COMPILE)gcc
> LD              = $(CROSS_COMPILE)ld
> AR              = $(CROSS_COMPILE)ar
> NM              = $(CROSS_COMPILE)nm
> OBJCOPY         = $(CROSS_COMPILE)objcopy
> OBJDUMP         = $(CROSS_COMPILE)objdump
> READELF         = $(CROSS_COMPILE)readelf
> STRIP           = $(CROSS_COMPILE)strip
> endif

Right, then later in the top level Makefile to achieve cross compilation support
 569 ifneq ($(CROSS_COMPILE),)
 570 CLANG_FLAGS += --target=$(notdir $(CROSS_COMPILE:%-=%))
...

The top level Makefile has this all wired up correctly; I wonder if
tools/ could make use of the existing exports, rather than redoing all
of that work?

>
> >>
> >>
> >>> +else
> >>>   CC := $(CROSS_COMPILE)gcc
> >>> +endif
> >>>
> >>>   ifeq (0,$(MAKELEVEL))
> >>>       ifeq ($(OUTPUT),)
> >>> --
> >>> 2.30.2
> >>>
> >
> >
> >



-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH bpf-next v3 4/5] selftests/bpf: silence clang compilation warnings
  2021-04-13 22:17     ` Nick Desaulniers
@ 2021-04-13 22:37       ` Yonghong Song
  0 siblings, 0 replies; 18+ messages in thread
From: Yonghong Song @ 2021-04-13 22:37 UTC (permalink / raw)
  To: Nick Desaulniers, Andrii Nakryiko
  Cc: bpf, Andrii Nakryiko, Arnaldo Carvalho de Melo, Kernel Team, Sedat Dilek



On 4/13/21 3:17 PM, Nick Desaulniers wrote:
> On Tue, Apr 13, 2021 at 3:08 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>>
>> On Tue, Apr 13, 2021 at 8:34 AM Yonghong Song <yhs@fb.com> wrote:
>>>
>>> With clang compiler:
>>>    make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
>>>    make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1
>>> Some linker flags are not used/effective for some binaries and
>>> we have warnings like:
>>>    warning: -lelf: 'linker' input unused [-Wunused-command-line-argument]
>>>
>>> We also have warnings like:
>>>    .../selftests/bpf/prog_tests/ns_current_pid_tgid.c:74:57: note: treat the string as an argument to avoid this
>>>          if (CHECK(waitpid(cpid, &wstatus, 0) == -1, "waitpid", strerror(errno)))
>>>                                                                 ^
>>>                                                                 "%s",
>>>    .../selftests/bpf/test_progs.h:129:35: note: expanded from macro 'CHECK'
>>>          _CHECK(condition, tag, duration, format)
>>>                                           ^
>>>    .../selftests/bpf/test_progs.h:108:21: note: expanded from macro '_CHECK'
>>>                  fprintf(stdout, ##format);                              \
>>>                                    ^
>>> The first warning can be silenced with clang option -Wno-unused-command-line-argument.
>>> For the second warning, source codes are modified as suggested by the compiler
>>> to silence the warning. Since gcc does not support the option
>>> -Wno-unused-command-line-argument and the warning only happens with clang
>>> compiler, the option -Wno-unused-command-line-argument is enabled only when
>>> clang compiler is used.
>>>
>>> Signed-off-by: Yonghong Song <yhs@fb.com>
>>> ---
>>
>> LGTM, please see the question below.
>>
>> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>>
>>>   tools/testing/selftests/bpf/Makefile                         | 5 +++++
>>>   tools/testing/selftests/bpf/prog_tests/fexit_sleep.c         | 4 ++--
>>>   tools/testing/selftests/bpf/prog_tests/ns_current_pid_tgid.c | 4 ++--
>>>   3 files changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>>> index dcc2dc1f2a86..c45ae13b88a0 100644
>>> --- a/tools/testing/selftests/bpf/Makefile
>>> +++ b/tools/testing/selftests/bpf/Makefile
>>> @@ -28,6 +28,11 @@ CFLAGS += -g -Og -rdynamic -Wall $(GENFLAGS) $(SAN_CFLAGS)           \
>>>            -Dbpf_load_program=bpf_test_load_program
>>>   LDLIBS += -lcap -lelf -lz -lrt -lpthread
>>>
>>> +# Silence some warnings when compiled with clang
>>> +ifneq ($(LLVM),)
>>
>> This won't handle the case where someone does `make CC=clang`, right?
>> Do we care at all?
> 
> Right; it would be better to check CC=clang and have LLVM=1 enable
> CC=clang from a higher level Makefile (since tools/build/Makefile
> seems to override the top level $CC). See the top level Makefile
> L448-456.  Then it should work for CC=clang or LLVM=1.

I would consider LLVM=1 is recommended and should be supported
first. Support for CC=clang CXX=clang++ etc. can be done later
if people find cases where LLVM=1 does not fit their need.

> 
>>
>>
>>> +CFLAGS += -Wno-unused-command-line-argument
>>> +endif
>>> +
>>
>> [...]
> 
> 
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH bpf-next v3 1/5] selftests: set CC to clang in lib.mk if LLVM is set
  2021-04-13 22:27       ` Yonghong Song
  2021-04-13 22:34         ` Nick Desaulniers
@ 2021-04-13 23:22         ` Andrii Nakryiko
  1 sibling, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2021-04-13 23:22 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Nick Desaulniers, bpf, Andrii Nakryiko, Arnaldo Carvalho de Melo,
	Kernel Team, Sedat Dilek

On Tue, Apr 13, 2021 at 3:27 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 4/13/21 3:13 PM, Nick Desaulniers wrote:
> > On Tue, Apr 13, 2021 at 3:05 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >>
> >> On Tue, Apr 13, 2021 at 8:34 AM Yonghong Song <yhs@fb.com> wrote:
> >>>
> >>> selftests/bpf/Makefile includes lib.mk. With the following command
> >>>    make -j60 LLVM=1 LLVM_IAS=1  <=== compile kernel
> >>>    make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1 V=1
> >>> some files are still compiled with gcc. This patch
> >>> fixed lib.mk issue which sets CC to gcc in all cases.
> >>>
> >>> Cc: Sedat Dilek <sedat.dilek@gmail.com>
> >>> Signed-off-by: Yonghong Song <yhs@fb.com>
> >>> ---
> >>>   tools/testing/selftests/lib.mk | 4 ++++
> >>>   1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
> >>> index a5ce26d548e4..9a41d8bb9ff1 100644
> >>> --- a/tools/testing/selftests/lib.mk
> >>> +++ b/tools/testing/selftests/lib.mk
> >>> @@ -1,6 +1,10 @@
> >>>   # This mimics the top-level Makefile. We do it explicitly here so that this
> >>>   # Makefile can operate with or without the kbuild infrastructure.
> >>> +ifneq ($(LLVM),)
> >>> +CC := clang
> >>
> >> Does this mean that cross-compilation with Clang doesn't work at all
> >> or is achieved in some other way?
> >
> > Right, this probably doesn't support cross compilation w/ Clang.
> > Rather than invoke `$(CROSS_COMPILE) clang`, you'd do `clang
> > --target=$(CROSS_COMPILE)`.  Even then, cross linking executables is
> > hairy.  But at least this should enable native compilation, which is a
> > start.
>
> See https://clang.llvm.org/docs/CrossCompilation.html.
> As Nick said, clang prefers --target=$(CROSS_COMPILE) to
> indicate cross compilation. User can pass additional
> flags (CFLAGS) for cross compilation for the time being.
> This is the same as main kernel Makefile.
>
> ifneq ($(LLVM),)
> CC              = clang
> LD              = ld.lld
> AR              = llvm-ar
> NM              = llvm-nm
> OBJCOPY         = llvm-objcopy
> OBJDUMP         = llvm-objdump
> READELF         = llvm-readelf
> STRIP           = llvm-strip
> else
> CC              = $(CROSS_COMPILE)gcc
> LD              = $(CROSS_COMPILE)ld
> AR              = $(CROSS_COMPILE)ar
> NM              = $(CROSS_COMPILE)nm
> OBJCOPY         = $(CROSS_COMPILE)objcopy
> OBJDUMP         = $(CROSS_COMPILE)objdump
> READELF         = $(CROSS_COMPILE)readelf
> STRIP           = $(CROSS_COMPILE)strip
> endif
>

Ok, sounds good. We can improve cross-compilation separately.

Acked-by: Andrii Nakryiko <andrii@kernel.org>


> >>
> >>
> >>> +else
> >>>   CC := $(CROSS_COMPILE)gcc
> >>> +endif
> >>>
> >>>   ifeq (0,$(MAKELEVEL))
> >>>       ifeq ($(OUTPUT),)
> >>> --
> >>> 2.30.2
> >>>
> >
> >
> >

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang
  2021-04-13 15:34 [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang Yonghong Song
                   ` (5 preceding siblings ...)
  2021-04-13 19:36 ` [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang Martin KaFai Lau
@ 2021-04-16  0:21 ` Alexei Starovoitov
  2021-04-16  3:59   ` Yonghong Song
  6 siblings, 1 reply; 18+ messages in thread
From: Alexei Starovoitov @ 2021-04-16  0:21 UTC (permalink / raw)
  To: Yonghong Song, bpf
  Cc: Andrii Nakryiko, Arnaldo Carvalho de Melo, kernel-team,
	Nick Desaulniers, Sedat Dilek

On 4/13/21 8:34 AM, Yonghong Song wrote:
> To build kernel with clang, people typically use
>    make -j60 LLVM=1 LLVM_IAS=1
> LLVM_IAS=1 is not required for non-LTO build but
> is required for LTO build. In my environment,
> I am always having LLVM_IAS=1 regardless of
> whether LTO is enabled or not.
> 
> After kernel is build with clang, the following command
> can be used to build selftests with clang:
>    make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1
> 
> I am using latest bpf-next kernel code base and
> latest clang built from source from
>    https://github.com/llvm/llvm-project.git
> Using earlier version of llvm may have compilation errors, see
>    tools/testing/selftests/bpf
> due to continuous development in llvm bpf features and selftests
> to use these features.
> 
> To run bpf selftest properly, you need have certain necessary
> kernel configs like at:
>    bpf-next:tools/testing/selftests/bpf/config
> (not that this is not a complete .config file and some other configs
>   might still be needed.)
> 
> Currently, using the above command, some compilations
> still use gcc and there are also compilation errors and warnings.
> This patch set intends to fix these issues.
> Patch #1 and #2 fixed the issue so clang/clang++ is
> used instead of gcc/g++. Patch #3 fixed a compilation
> failure. Patch #4 and #5 fixed various compiler warnings.
> 
> Changelog:
>    v2 -> v3:
>      . more test environment description in cover letter. (Sedat)
>      . use a different fix, but similar to other use in selftests/bpf
>        Makefile, to exclude header files from CXX compilation command
>        line. (Andrii)
>      . fix codes instead of adding -Wno-format-security. (Andrii)

I struggled to tweak my llvm setup, but at the end it compiled and
selftests/bpf/test_progs passed compiled by clang,
so I've applied to bpf-next.

The things I've seen:
1.
include <iostream> not found due to my setup quirks.
2.
diff selftests/bpf/tools/build/libbpf/libbpf_global_syms.tmp	
diff selftests/bpf/tools/build/libbpf/libbpf_versioned_syms.tmp	
  btf__set_pointer_size
  btf__str_by_offset
  btf__type_by_id
+LIBBPF_0.0.1
+LIBBPF_0.0.2
and this was happening with packaged llvm builds,
but my own llvm build was fine, so I didn't debug further.

3.
clang-12: error: unsupported option '-mrecord-mcount' for target 
'x86_64-unknown-linux-gnu'
due to kernel not built with clang.

I suspect followups will be needed to make it bulletproof.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang
  2021-04-16  0:21 ` Alexei Starovoitov
@ 2021-04-16  3:59   ` Yonghong Song
  0 siblings, 0 replies; 18+ messages in thread
From: Yonghong Song @ 2021-04-16  3:59 UTC (permalink / raw)
  To: Alexei Starovoitov, bpf
  Cc: Andrii Nakryiko, Arnaldo Carvalho de Melo, kernel-team,
	Nick Desaulniers, Sedat Dilek



On 4/15/21 5:21 PM, Alexei Starovoitov wrote:
> On 4/13/21 8:34 AM, Yonghong Song wrote:
>> To build kernel with clang, people typically use
>>    make -j60 LLVM=1 LLVM_IAS=1
>> LLVM_IAS=1 is not required for non-LTO build but
>> is required for LTO build. In my environment,
>> I am always having LLVM_IAS=1 regardless of
>> whether LTO is enabled or not.
>>
>> After kernel is build with clang, the following command
>> can be used to build selftests with clang:
>>    make -j60 -C tools/testing/selftests/bpf LLVM=1 LLVM_IAS=1
>>
>> I am using latest bpf-next kernel code base and
>> latest clang built from source from
>>    https://github.com/llvm/llvm-project.git
>> Using earlier version of llvm may have compilation errors, see
>>    tools/testing/selftests/bpf
>> due to continuous development in llvm bpf features and selftests
>> to use these features.
>>
>> To run bpf selftest properly, you need have certain necessary
>> kernel configs like at:
>>    bpf-next:tools/testing/selftests/bpf/config
>> (not that this is not a complete .config file and some other configs
>>   might still be needed.)
>>
>> Currently, using the above command, some compilations
>> still use gcc and there are also compilation errors and warnings.
>> This patch set intends to fix these issues.
>> Patch #1 and #2 fixed the issue so clang/clang++ is
>> used instead of gcc/g++. Patch #3 fixed a compilation
>> failure. Patch #4 and #5 fixed various compiler warnings.
>>
>> Changelog:
>>    v2 -> v3:
>>      . more test environment description in cover letter. (Sedat)
>>      . use a different fix, but similar to other use in selftests/bpf
>>        Makefile, to exclude header files from CXX compilation command
>>        line. (Andrii)
>>      . fix codes instead of adding -Wno-format-security. (Andrii)
> 
> I struggled to tweak my llvm setup, but at the end it compiled and
> selftests/bpf/test_progs passed compiled by clang,
> so I've applied to bpf-next.
> 
> The things I've seen:
> 1.
> include <iostream> not found due to my setup quirks.

This header file is included by test_cpp.cpp. Probably
some clang stdc++ setup issue.

> 2.
> diff selftests/bpf/tools/build/libbpf/libbpf_global_syms.tmp
> diff selftests/bpf/tools/build/libbpf/libbpf_versioned_syms.tmp
> btf__set_pointer_size
> btf__str_by_offset
> btf__type_by_id
> +LIBBPF_0.0.1
> +LIBBPF_0.0.2
> and this was happening with packaged llvm builds,
> but my own llvm build was fine, so I didn't debug further.

I am also use latest llvm as I can easily verify all selftests passed.

> 
> 3.
> clang-12: error: unsupported option '-mrecord-mcount' for target 
> 'x86_64-unknown-linux-gnu'
> due to kernel not built with clang.

Top level Makefile has
Makefile:  CC_FLAGS_FTRACE      += -mrecord-mcount
tools Makefile has some dependency on top level Makefile.

> 
> I suspect followups will be needed to make it bulletproof.

Definitely, Nick has libz issues, cross compile is not supported,
tools build depends on kernel build, both or neither with LLVM=1, ...
clang build got some traction recently and hopefully more people
can start to contribute to make the build process more robust.

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2021-04-16  3:59 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13 15:34 [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang Yonghong Song
2021-04-13 15:34 ` [PATCH bpf-next v3 1/5] selftests: set CC to clang in lib.mk if LLVM is set Yonghong Song
2021-04-13 22:04   ` Andrii Nakryiko
2021-04-13 22:13     ` Nick Desaulniers
2021-04-13 22:27       ` Yonghong Song
2021-04-13 22:34         ` Nick Desaulniers
2021-04-13 23:22         ` Andrii Nakryiko
2021-04-13 15:34 ` [PATCH bpf-next v3 2/5] tools: allow proper CC/CXX/... override with LLVM=1 in Makefile.include Yonghong Song
2021-04-13 15:34 ` [PATCH bpf-next v3 3/5] selftests/bpf: fix test_cpp compilation failure with clang Yonghong Song
2021-04-13 22:05   ` Andrii Nakryiko
2021-04-13 15:34 ` [PATCH bpf-next v3 4/5] selftests/bpf: silence clang compilation warnings Yonghong Song
2021-04-13 22:07   ` Andrii Nakryiko
2021-04-13 22:17     ` Nick Desaulniers
2021-04-13 22:37       ` Yonghong Song
2021-04-13 15:34 ` [PATCH bpf-next v3 5/5] bpftool: fix a clang compilation warning Yonghong Song
2021-04-13 19:36 ` [PATCH bpf-next v3 0/5] bpf: tools: support build selftests/bpf with clang Martin KaFai Lau
2021-04-16  0:21 ` Alexei Starovoitov
2021-04-16  3:59   ` Yonghong Song

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).