[PATCH bpf-next 0/4] Generate NULL in vmlinux.h

Generate NULL definition as part of vmlinux.h. This is a pretty common and
unfortunate annoyance that most users of vmlinux.h have to deal with. Patch #2
drops such custom NULL definition in one of the selftests. Patches #3 and #4
make bpftool and selftests compilations stricter by treating warnings as
errors.

Andrii Nakryiko (4):
  bpftool: generate NULL definition in vmlinux.h
  selftests/bpf: drop custom NULL #define in skb_pkt_end selftest
  selftests/bpf: treat compilation warnings as errors
  bpftool: treat compilation warnings as errors

 tools/bpf/bpftool/Makefile                      | 3 ++-
 tools/bpf/bpftool/btf.c                         | 2 ++
 tools/bpf/bpftool/jit_disasm.c                  | 3 +++
 tools/testing/selftests/bpf/Makefile            | 4 ++--
 tools/testing/selftests/bpf/progs/skb_pkt_end.c | 1 -
 5 files changed, 9 insertions(+), 4 deletions(-)

-- 
2.30.2

Given that vmlinux.h is not compatible with headers like stdint.h, NULL poses
an annoying problem: it is defined as #define, so is not captured in BTF, so
is not emitted into vmlinux.h. This leads to users either sticking to explicit
0, or defining their own NULL (as progs/skb_pkt_end.c does).

It's pretty trivial for bpftool to generate NULL definition, though, so let's
just do that. This might cause compilation warning for existing BPF
applications:

progs/skb_pkt_end.c:7:9: warning: 'NULL' macro redefined [-Wmacro-redefined]
  progs/skb_pkt_end.c:7:9: error: 'NULL' macro redefined [-Werror,-Wmacro-redefined]
  #define NULL 0
          ^
  /tmp/linux/tools/testing/selftests/bpf/tools/include/vmlinux.h:4:9: note: previous definition is here
  #define NULL ((void *)0)
	  ^

It is trivial to fix, though, so long-term benefits outweight temporary
inconveniences.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/bpf/bpftool/btf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 62953bbf68b4..ff6a76632873 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c

@@ -405,6 +405,8 @@ static int dump_btf_c(const struct btf *btf,
 	printf("#ifndef __VMLINUX_H__\n");
 	printf("#define __VMLINUX_H__\n");
 	printf("\n");
+	printf("#define NULL ((void *)0)\n");
+	printf("\n");
 	printf("#ifndef BPF_NO_PRESERVE_ACCESS_INDEX\n");
 	printf("#pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)\n");
 	printf("#endif\n\n");

-- 
2.30.2

Make selftests/bpf compilation more strict by treating warnings as errors. We
are generally pretty good at keeping compilation clean, but it's easy to miss
minor warnings. So let's not trust people and just employ stricter compiler
enforcement.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index d0db2b673c6f..7e18f3e66e61 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile

@@ -21,7 +21,7 @@ endif
 
 BPF_GCC		?= $(shell command -v bpf-gcc;)
 SAN_CFLAGS	?=
-CFLAGS += -g -Og -rdynamic -Wall $(GENFLAGS) $(SAN_CFLAGS)		\
+CFLAGS += -g -Og -rdynamic -Wall -Werror $(GENFLAGS) $(SAN_CFLAGS)	\
 	  -I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR)		\
 	  -I$(TOOLSINCDIR) -I$(APIDIR) -I$(OUTPUT)			\
 	  -Dbpf_prog_load=bpf_prog_test_load				\

@@ -270,7 +270,7 @@ IS_LITTLE_ENDIAN = $(shell $(CC) -dM -E - </dev/null | \
 MENDIAN=$(if $(IS_LITTLE_ENDIAN),-mlittle-endian,-mbig-endian)
 
 CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG))
-BPF_CFLAGS = -g -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) 			\
+BPF_CFLAGS = -g -Werror -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) 		\
 	     -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR)			\
 	     -I$(abspath $(OUTPUT)/../usr/include)
 

-- 
2.30.2

Now that bpftool generates NULL definition as part of vmlinux.h, drop custom
NULL definition in skb_pkt_end.c.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/testing/selftests/bpf/progs/skb_pkt_end.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/progs/skb_pkt_end.c b/tools/testing/selftests/bpf/progs/skb_pkt_end.c
index cf6823f42e80..7f2eaa2f89f8 100644
--- a/tools/testing/selftests/bpf/progs/skb_pkt_end.c
+++ b/tools/testing/selftests/bpf/progs/skb_pkt_end.c

@@ -4,7 +4,6 @@
 #include <bpf/bpf_core_read.h>
 #include <bpf/bpf_helpers.h>
 
-#define NULL 0
 #define INLINE __always_inline
 
 #define skb_shorter(skb, len) ((void *)(long)(skb)->data + (len) > (void *)(long)skb->data_end)

-- 
2.30.2

Make bpftool compilation stricter and treat all compilation warnigs as errors.

Depending on libbfd version on the system, jit_disasm.c might trigger the
following compilation warning-turned-error:

jit_disasm.c: In function ‘disasm_print_insn’:
jit_disasm.c:121:29: error: assignment discards ‘const’ qualifier from pointer
target type [-Werror=discarded-qualifiers]
   info.disassembler_options = disassembler_options;
                                ^

This was fixed in libbfd, but older versions of the library are still widely
used. So disable -Wdiscarded-qualifiers for that particular line of code.

Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/bpf/bpftool/Makefile     | 3 ++-
 tools/bpf/bpftool/jit_disasm.c | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index b3073ae84018..59de954faaf5 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile

@@ -56,7 +56,8 @@ prefix ?= /usr/local
 bash_compdir ?= /usr/share/bash-completion/completions
 
 CFLAGS += -O2
-CFLAGS += -W -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers
+CFLAGS += -W -Wall -Wextra -Werror
+CFLAGS += -Wno-unused-parameter -Wno-missing-field-initializers
 CFLAGS += $(filter-out -Wswitch-enum -Wnested-externs,$(EXTRA_WARNINGS))
 CFLAGS += -DPACKAGE='"bpftool"' -D__EXPORTED_HEADERS__ \
 	-I$(if $(OUTPUT),$(OUTPUT),.) \

diff --git a/tools/bpf/bpftool/jit_disasm.c b/tools/bpf/bpftool/jit_disasm.c
index e7e7eee9f172..48bc7f7a542f 100644
--- a/tools/bpf/bpftool/jit_disasm.c
+++ b/tools/bpf/bpftool/jit_disasm.c

@@ -118,7 +118,10 @@ void disasm_print_insn(unsigned char *image, ssize_t len, int opcodes,
 	info.arch = bfd_get_arch(bfdf);
 	info.mach = bfd_get_mach(bfdf);
 	if (disassembler_options)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdiscarded-qualifiers"
 		info.disassembler_options = disassembler_options;
+#pragma GCC diagnostic push
 	info.buffer = image;
 	info.buffer_length = len;
 

-- 
2.30.2

On Tue, Mar 16, 2021 at 8:03 PM Andrii Nakryiko [off-list ref] wrote:

Given that vmlinux.h is not compatible with headers like stdint.h, NULL poses
an annoying problem: it is defined as #define, so is not captured in BTF, so
is not emitted into vmlinux.h. This leads to users either sticking to explicit
0, or defining their own NULL (as progs/skb_pkt_end.c does).

It's pretty trivial for bpftool to generate NULL definition, though, so let's
just do that. This might cause compilation warning for existing BPF
applications:

progs/skb_pkt_end.c:7:9: warning: 'NULL' macro redefined [-Wmacro-redefined]
  progs/skb_pkt_end.c:7:9: error: 'NULL' macro redefined [-Werror,-Wmacro-redefined]

oops, this shouldn't have been copy/pasted. This is how the line above
looks like if -Werror is specified in Makefile.