[PATCH v2 bpf-next 0/4] Build BPF selftests and its libbpf, bpftool in debug mode

[PATCH v2 bpf-next 0/4] Build BPF selftests and its libbpf, bpftool in debug mode

[Andrii Nakryiko]

Build BPF selftests and libbpf and bpftool, that are used as part of
selftests, in debug mode (specifically, -Og). This makes it much simpler and
nicer to do development and/or bug fixing. See patch #4 for some unscientific
measurements.

This patch set fixes new maybe-unitialized warnings produced in -Og build
mode. Patch #1 fixes the blocker which was causing some XDP selftests failures
due to non-zero padding in bpf_xdp_set_link_opts, which only happened in debug
mode.

Andrii Nakryiko (4):
  libbpf: add explicit padding to bpf_xdp_set_link_opts
  bpftool: fix maybe-uninitialized warnings
  selftests/bpf: fix maybe-uninitialized warning in xdpxceiver test
  selftests/bpf: build everything in debug mode

 tools/bpf/bpftool/btf.c                  | 3 +++
 tools/bpf/bpftool/main.c                 | 3 +--
 tools/bpf/bpftool/map.c                  | 2 +-
 tools/lib/bpf/libbpf.h                   | 1 +
 tools/testing/selftests/bpf/Makefile     | 7 +++++--
 tools/testing/selftests/bpf/xdpxceiver.c | 4 ++--
 6 files changed, 13 insertions(+), 7 deletions(-)

-- 
2.24.1

[PATCH v2 bpf-next 1/4] libbpf: add explicit padding to bpf_xdp_set_link_opts

[Andrii Nakryiko]

Adding such anonymous padding fixes the issue with uninitialized portions of
bpf_xdp_set_link_opts when using LIBBPF_DECLARE_OPTS macro with inline field
initialization:

DECLARE_LIBBPF_OPTS(bpf_xdp_set_link_opts, opts, .old_fd = -1);

When such code is compiled in debug mode, compiler is generating code that
leaves padding bytes uninitialized, which triggers error inside libbpf APIs
that do strict zero initialization checks for OPTS structs.

Adding anonymous padding field fixes the issue.

Fixes: bd5ca3ef93cd ("libbpf: Add function to set link XDP fd while specifying old program")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/lib/bpf/libbpf.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 3c35eb401931..3d690d4e785c 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -507,6 +507,7 @@ struct xdp_link_info {
 struct bpf_xdp_set_link_opts {
 	size_t sz;
 	int old_fd;
+	size_t :0;
 };
 #define bpf_xdp_set_link_opts__last_field old_fd
 
-- 
2.24.1

[PATCH v2 bpf-next 2/4] bpftool: fix maybe-uninitialized warnings

[Andrii Nakryiko]

Somehow when bpftool is compiled in -Og mode, compiler produces new warnings
about possibly uninitialized variables. Fix all the reported problems.

Fixes: 2119f2189df1 ("bpftool: add C output format option to btf dump subcommand")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
---
 tools/bpf/bpftool/btf.c  | 3 +++
 tools/bpf/bpftool/main.c | 3 +--
 tools/bpf/bpftool/map.c  | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 985610c3f193..62953bbf68b4 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -546,6 +546,7 @@ static int do_dump(int argc, char **argv)
 			NEXT_ARG();
 			if (argc < 1) {
 				p_err("expecting value for 'format' option\n");
+				err = -EINVAL;
 				goto done;
 			}
 			if (strcmp(*argv, "c") == 0) {
@@ -555,11 +556,13 @@ static int do_dump(int argc, char **argv)
 			} else {
 				p_err("unrecognized format specifier: '%s', possible values: raw, c",
 				      *argv);
+				err = -EINVAL;
 				goto done;
 			}
 			NEXT_ARG();
 		} else {
 			p_err("unrecognized option: '%s'", *argv);
+			err = -EINVAL;
 			goto done;
 		}
 	}
diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
index b86f450e6fce..d9afb730136a 100644
--- a/tools/bpf/bpftool/main.c
+++ b/tools/bpf/bpftool/main.c
@@ -276,7 +276,7 @@ static int do_batch(int argc, char **argv)
 	int n_argc;
 	FILE *fp;
 	char *cp;
-	int err;
+	int err = 0;
 	int i;
 
 	if (argc < 2) {
@@ -370,7 +370,6 @@ static int do_batch(int argc, char **argv)
 	} else {
 		if (!json_output)
 			printf("processed %d commands\n", lines);
-		err = 0;
 	}
 err_close:
 	if (fp != stdin)
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index b400364ee054..09ae0381205b 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -100,7 +100,7 @@ static int do_dump_btf(const struct btf_dumper *d,
 		       void *value)
 {
 	__u32 value_id;
-	int ret;
+	int ret = 0;
 
 	/* start of key-value pair */
 	jsonw_start_object(d->jw);
-- 
2.24.1

[PATCH v2 bpf-next 3/4] selftests/bpf: fix maybe-uninitialized warning in xdpxceiver test

[Andrii Nakryiko]

xsk_ring_prod__reserve() doesn't necessarily set idx in some conditions, so
from static analysis point of view compiler is right about the problems like:

In file included from xdpxceiver.c:92:
xdpxceiver.c: In function ‘xsk_populate_fill_ring’:
/data/users/andriin/linux/tools/testing/selftests/bpf/tools/include/bpf/xsk.h:119:20: warning: ‘idx’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  return &addrs[idx & fill->mask];
                ~~~~^~~~~~~~~~~~
xdpxceiver.c:300:6: note: ‘idx’ was declared here
  u32 idx;
      ^~~
xdpxceiver.c: In function ‘tx_only’:
xdpxceiver.c:596:30: warning: ‘idx’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   struct xdp_desc *tx_desc = xsk_ring_prod__tx_desc(&xsk->tx, idx + i);
                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Fix two warnings reported by compiler by pre-initializing variable.

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

diff --git a/tools/testing/selftests/bpf/xdpxceiver.c b/tools/testing/selftests/bpf/xdpxceiver.c
index 8b0f7fdd9003..1e21a3172687 100644
--- a/tools/testing/selftests/bpf/xdpxceiver.c
+++ b/tools/testing/selftests/bpf/xdpxceiver.c
@@ -297,7 +297,7 @@ static void xsk_configure_umem(struct ifobject *data, void *buffer, u64 size)
 static void xsk_populate_fill_ring(struct xsk_umem_info *umem)
 {
 	int ret, i;
-	u32 idx;
+	u32 idx = 0;
 
 	ret = xsk_ring_prod__reserve(&umem->fq, XSK_RING_PROD__DEFAULT_NUM_DESCS, &idx);
 	if (ret != XSK_RING_PROD__DEFAULT_NUM_DESCS)
@@ -584,7 +584,7 @@ static void rx_pkt(struct xsk_socket_info *xsk, struct pollfd *fds)
 
 static void tx_only(struct xsk_socket_info *xsk, u32 *frameptr, int batch_size)
 {
-	u32 idx;
+	u32 idx = 0;
 	unsigned int i;
 	bool tx_invalid_test = stat_test_type == STAT_TEST_TX_INVALID;
 	u32 len = tx_invalid_test ? XSK_UMEM__DEFAULT_FRAME_SIZE + 1 : PKT_SIZE;
-- 
2.24.1

[PATCH v2 bpf-next 4/4] selftests/bpf: build everything in debug mode

[Andrii Nakryiko]

Build selftests, bpftool, and libbpf in debug mode with DWARF data to
facilitate easier debugging.

In terms of impact on building and running selftests. Build is actually faster
now:

BEFORE: make -j60  380.21s user 37.87s system 1466% cpu 28.503 total
AFTER:  make -j60  345.47s user 37.37s system 1599% cpu 23.939 total

test_progs runtime seems to be the same:

BEFORE:
real    1m5.139s
user    0m1.600s
sys     0m43.977s

AFTER:
real    1m3.799s
user    0m1.721s
sys     0m42.420s

Huge difference is being able to debug issues throughout test_progs, bpftool,
and libbpf without constantly updating 3 Makefiles by hand (including GDB
seeing the source code without any extra incantations).

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

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index c3999587bc23..d0db2b673c6f 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 -rdynamic -Wall -O2 $(GENFLAGS) $(SAN_CFLAGS)		\
+CFLAGS += -g -Og -rdynamic -Wall $(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				\
@@ -201,6 +201,7 @@ $(DEFAULT_BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile)    \
 		    $(HOST_BPFOBJ) | $(HOST_BUILD_DIR)/bpftool
 	$(Q)$(MAKE) $(submake_extras)  -C $(BPFTOOLDIR)			       \
 		    CC=$(HOSTCC) LD=$(HOSTLD)				       \
+		    EXTRA_CFLAGS='-g -Og'				       \
 		    OUTPUT=$(HOST_BUILD_DIR)/bpftool/			       \
 		    prefix= DESTDIR=$(HOST_SCRATCH_DIR)/ install
 
@@ -218,6 +219,7 @@ $(BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile)		       \
 	   ../../../include/uapi/linux/bpf.h                                   \
 	   | $(INCLUDE_DIR) $(BUILD_DIR)/libbpf
 	$(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) OUTPUT=$(BUILD_DIR)/libbpf/ \
+		    EXTRA_CFLAGS='-g -Og'					       \
 		    DESTDIR=$(SCRATCH_DIR) prefix= all install_headers
 
 ifneq ($(BPFOBJ),$(HOST_BPFOBJ))
@@ -225,7 +227,8 @@ $(HOST_BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile)                \
 	   ../../../include/uapi/linux/bpf.h                                   \
 	   | $(INCLUDE_DIR) $(HOST_BUILD_DIR)/libbpf
 	$(Q)$(MAKE) $(submake_extras) -C $(BPFDIR)                             \
-		OUTPUT=$(HOST_BUILD_DIR)/libbpf/ CC=$(HOSTCC) LD=$(HOSTLD)     \
+		    EXTRA_CFLAGS='-g -Og'					       \
+		    OUTPUT=$(HOST_BUILD_DIR)/libbpf/ CC=$(HOSTCC) LD=$(HOSTLD) \
 		    DESTDIR=$(HOST_SCRATCH_DIR)/ prefix= all install_headers
 endif
 
-- 
2.24.1

Re: [PATCH v2 bpf-next 4/4] selftests/bpf: build everything in debug mode

[Andrii Nakryiko]

On Sat, Mar 13, 2021 at 1:09 PM Andrii Nakryiko <andrii@kernel.org> wrote:
>
> Build selftests, bpftool, and libbpf in debug mode with DWARF data to
> facilitate easier debugging.
>
> In terms of impact on building and running selftests. Build is actually faster
> now:
>
> BEFORE: make -j60  380.21s user 37.87s system 1466% cpu 28.503 total
> AFTER:  make -j60  345.47s user 37.37s system 1599% cpu 23.939 total
>
> test_progs runtime seems to be the same:
>
> BEFORE:
> real    1m5.139s
> user    0m1.600s
> sys     0m43.977s
>
> AFTER:
> real    1m3.799s
> user    0m1.721s
> sys     0m42.420s
>
> Huge difference is being able to debug issues throughout test_progs, bpftool,
> and libbpf without constantly updating 3 Makefiles by hand (including GDB
> seeing the source code without any extra incantations).
>
> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> ---
>  tools/testing/selftests/bpf/Makefile | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index c3999587bc23..d0db2b673c6f 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 -rdynamic -Wall -O2 $(GENFLAGS) $(SAN_CFLAGS)             \
> +CFLAGS += -g -Og -rdynamic -Wall $(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                            \
> @@ -201,6 +201,7 @@ $(DEFAULT_BPFTOOL): $(wildcard $(BPFTOOLDIR)/*.[ch] $(BPFTOOLDIR)/Makefile)    \
>                     $(HOST_BPFOBJ) | $(HOST_BUILD_DIR)/bpftool
>         $(Q)$(MAKE) $(submake_extras)  -C $(BPFTOOLDIR)                        \
>                     CC=$(HOSTCC) LD=$(HOSTLD)                                  \
> +                   EXTRA_CFLAGS='-g -Og'                                      \

I was asked about '-Og' flag and the minimum GCC version that supports
it. It seems it was added in GCC 4.8 ([0]), so given the kernel's
minimum version is GCC 4.9, we shouldn't need take any extra
precautions to handle older compilers.

  [0] https://gcc.gnu.org/gcc-4.8/changes.html

>                     OUTPUT=$(HOST_BUILD_DIR)/bpftool/                          \
>                     prefix= DESTDIR=$(HOST_SCRATCH_DIR)/ install
>
> @@ -218,6 +219,7 @@ $(BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile)                 \
>            ../../../include/uapi/linux/bpf.h                                   \
>            | $(INCLUDE_DIR) $(BUILD_DIR)/libbpf
>         $(Q)$(MAKE) $(submake_extras) -C $(BPFDIR) OUTPUT=$(BUILD_DIR)/libbpf/ \
> +                   EXTRA_CFLAGS='-g -Og'                                              \
>                     DESTDIR=$(SCRATCH_DIR) prefix= all install_headers
>
>  ifneq ($(BPFOBJ),$(HOST_BPFOBJ))
> @@ -225,7 +227,8 @@ $(HOST_BPFOBJ): $(wildcard $(BPFDIR)/*.[ch] $(BPFDIR)/Makefile)                \
>            ../../../include/uapi/linux/bpf.h                                   \
>            | $(INCLUDE_DIR) $(HOST_BUILD_DIR)/libbpf
>         $(Q)$(MAKE) $(submake_extras) -C $(BPFDIR)                             \
> -               OUTPUT=$(HOST_BUILD_DIR)/libbpf/ CC=$(HOSTCC) LD=$(HOSTLD)     \
> +                   EXTRA_CFLAGS='-g -Og'                                              \
> +                   OUTPUT=$(HOST_BUILD_DIR)/libbpf/ CC=$(HOSTCC) LD=$(HOSTLD) \
>                     DESTDIR=$(HOST_SCRATCH_DIR)/ prefix= all install_headers
>  endif
>
> --
> 2.24.1
>

Re: [PATCH v2 bpf-next 0/4] Build BPF selftests and its libbpf, bpftool in debug mode

[Alexei Starovoitov]

On 3/13/21 1:09 PM, Andrii Nakryiko wrote:
> Build BPF selftests and libbpf and bpftool, that are used as part of
> selftests, in debug mode (specifically, -Og). This makes it much simpler and
> nicer to do development and/or bug fixing. See patch #4 for some unscientific
> measurements.
> 
> This patch set fixes new maybe-unitialized warnings produced in -Og build
> mode. Patch #1 fixes the blocker which was causing some XDP selftests failures
> due to non-zero padding in bpf_xdp_set_link_opts, which only happened in debug
> mode.

It was applied. gitbot doesn't seem to auto-reply anymore. hmm.

Re: [PATCH v2 bpf-next 0/4] Build BPF selftests and its libbpf, bpftool in debug mode

[Andrii Nakryiko]

On Tue, Mar 16, 2021 at 2:02 PM Alexei Starovoitov <ast@fb.com> wrote:
>
> On 3/13/21 1:09 PM, Andrii Nakryiko wrote:
> > Build BPF selftests and libbpf and bpftool, that are used as part of
> > selftests, in debug mode (specifically, -Og). This makes it much simpler and
> > nicer to do development and/or bug fixing. See patch #4 for some unscientific
> > measurements.
> >
> > This patch set fixes new maybe-unitialized warnings produced in -Og build
> > mode. Patch #1 fixes the blocker which was causing some XDP selftests failures
> > due to non-zero padding in bpf_xdp_set_link_opts, which only happened in debug
> > mode.
>
> It was applied. gitbot doesn't seem to auto-reply anymore. hmm.

Thanks! Yeah, it seems to be unreliable lately. I think it sent some
notifications yesterday, but not all.