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

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

[Andrii Nakryiko]

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.

v1->v2:
  - fix few typos and wrong copy/paste;
  - fix #pragma push -> pop.

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

[PATCH v2 bpf-next 1/4] bpftool: generate NULL definition in vmlinux.h

[Andrii Nakryiko]

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]
  #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

Re: [PATCH v2 bpf-next 1/4] bpftool: generate NULL definition in vmlinux.h

[Andrii Nakryiko]

On Tue, Mar 16, 2021 at 8:13 PM Andrii Nakryiko <andrii@kernel.org> 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]
>   #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");


On second thought, this could also be done in bpf_helpers.h, which is
pretty much always included in BPF programs. I think that's a bit more
maintainable and less magical to users, so I'll go with that in v3.

> +       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
>

Re: [PATCH v2 bpf-next 1/4] bpftool: generate NULL definition in vmlinux.h

[Alexei Starovoitov]

On Wed, Mar 17, 2021 at 10:38 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
> > 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");
>
>
> On second thought, this could also be done in bpf_helpers.h, which is
> pretty much always included in BPF programs. I think that's a bit more
> maintainable and less magical to users, so I'll go with that in v3.

yep. good idea.

[PATCH v2 bpf-next 2/4] selftests/bpf: drop custom NULL #define in skb_pkt_end selftest

[Andrii Nakryiko]

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

[PATCH v2 bpf-next 3/4] selftests/bpf: treat compilation warnings as errors

[Andrii Nakryiko]

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

[PATCH v2 bpf-next 4/4] bpftool: treat compilation warnings as errors

[Andrii Nakryiko]

Make bpftool compilation stricter and treat all compilation warnings 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..20083fbed5c2 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 pop
 	info.buffer = image;
 	info.buffer_length = len;
 
-- 
2.30.2