| Message ID | 20200928090805.23343-3-lmb@cloudflare.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Mon, Sep 28, 2020 at 10:08:03AM +0100, Lorenz Bauer wrote: > We compare socket cookies to ensure that insertion into a sockmap worked. > Pull this out into a helper function for use in other tests. > > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> > --- > .../selftests/bpf/prog_tests/sockmap_basic.c | 50 +++++++++++++------ > 1 file changed, 36 insertions(+), 14 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c > index 4b7a527e7e82..67d3301bdf81 100644 > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c > @@ -50,6 +50,37 @@ static int connected_socket_v4(void) > return -1; > } > > +static void compare_cookies(struct bpf_map *src, struct bpf_map *dst) > +{ > + __u32 i, max_entries = bpf_map__max_entries(src); > + int err, duration, src_fd, dst_fd; This should have a compiler warning. "duration" is not initialized.
On Mon, Sep 28, 2020 at 10:59 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > +static void compare_cookies(struct bpf_map *src, struct bpf_map *dst) > > +{ > > + __u32 i, max_entries = bpf_map__max_entries(src); > > + int err, duration, src_fd, dst_fd; > This should have a compiler warning. "duration" is not initialized. There was a warning. I noticed it while applying and fixed it up. Lorenz, please upgrade your compiler. This is not the first time such warning has been missed.
On Tue, 29 Sep 2020 at 16:48, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: ... > There was a warning. I noticed it while applying and fixed it up. > Lorenz, please upgrade your compiler. This is not the first time such > warning has been missed. I tried reproducing this on latest bpf-next (b0efc216f577997) with gcc 9.3.0 by removing the initialization of duration: make: Entering directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf' TEST-OBJ [test_progs] sockmap_basic.test.o TEST-HDR [test_progs] tests.h EXT-OBJ [test_progs] test_progs.o EXT-OBJ [test_progs] cgroup_helpers.o EXT-OBJ [test_progs] trace_helpers.o EXT-OBJ [test_progs] network_helpers.o EXT-OBJ [test_progs] testing_helpers.o BINARY test_progs make: Leaving directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf' So, gcc doesn't issue a warning. Jakub did the following little experiment: jkbs@toad ~/tmp $ cat warning.c #include <stdio.h> int main(void) { int duration; fprintf(stdout, "%d", duration); return 0; } jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c warning.c: In function ‘main’: warning.c:7:2: warning: ‘duration’ is used uninitialized in this function [-Wuninitialized] 7 | fprintf(stdout, "%d", duration); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The simple case seems to work. However, adding the macro breaks things: jkbs@toad ~/tmp $ cat warning.c #include <stdio.h> #define _CHECK(duration) \ ({ \ fprintf(stdout, "%d", duration); \ }) #define CHECK() _CHECK(duration) int main(void) { int duration; CHECK(); return 0; } jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c jkbs@toad ~/tmp $ Maybe this is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501 ? The problem is still there on gcc 10. Compiling test_progs with clang does issue a warning FWIW, but it seems like other things break when doing that. -- Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
On Wed, Sep 30, 2020 at 10:28:33AM +0100, Lorenz Bauer wrote: > On Tue, 29 Sep 2020 at 16:48, Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > ... > > > There was a warning. I noticed it while applying and fixed it up. > > Lorenz, please upgrade your compiler. This is not the first time such > > warning has been missed. > > I tried reproducing this on latest bpf-next (b0efc216f577997) with gcc > 9.3.0 by removing the initialization of duration: > > make: Entering directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf' > TEST-OBJ [test_progs] sockmap_basic.test.o > TEST-HDR [test_progs] tests.h > EXT-OBJ [test_progs] test_progs.o > EXT-OBJ [test_progs] cgroup_helpers.o > EXT-OBJ [test_progs] trace_helpers.o > EXT-OBJ [test_progs] network_helpers.o > EXT-OBJ [test_progs] testing_helpers.o > BINARY test_progs > make: Leaving directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf' > > So, gcc doesn't issue a warning. Jakub did the following little experiment: > > jkbs@toad ~/tmp $ cat warning.c > #include <stdio.h> > > int main(void) > { > int duration; > > fprintf(stdout, "%d", duration); > > return 0; > } > jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c > warning.c: In function ‘main’: > warning.c:7:2: warning: ‘duration’ is used uninitialized in this > function [-Wuninitialized] > 7 | fprintf(stdout, "%d", duration); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > The simple case seems to work. However, adding the macro breaks things: > > jkbs@toad ~/tmp $ cat warning.c > #include <stdio.h> > > #define _CHECK(duration) \ > ({ \ > fprintf(stdout, "%d", duration); \ > }) > #define CHECK() _CHECK(duration) > > int main(void) > { > int duration; > > CHECK(); > > return 0; > } > jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c > jkbs@toad ~/tmp $ That's very interesting. Thanks for the pointers. I'm using gcc version 9.1.1 20190605 (Red Hat 9.1.1-2) and I saw this warning while compiling selftests, but I don't see it with above warning.c example. clang warns correctly in both cases. > Maybe this is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501 ? The > problem is still there on gcc 10. Compiling test_progs with clang does > issue a warning FWIW, but it seems like other things break when doing > that. That gcc bug has been opened since transition to ssa. That was a huge transition for gcc. But I think the bug number is not correct. It points to a different issue. I've checked -fdump-tree-uninit-all dump with and without macro. They're identical. The tree-ssa-uninit pass suppose to warn, but it doesn't. I wish I had more time to dig into it. A bit of debugging in gcc/tree-ssa-uninit.c can probably uncover the root cause.
On Thu, Oct 1, 2020 at 12:25 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Sep 30, 2020 at 10:28:33AM +0100, Lorenz Bauer wrote: > > On Tue, 29 Sep 2020 at 16:48, Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > ... > > > > > There was a warning. I noticed it while applying and fixed it up. > > > Lorenz, please upgrade your compiler. This is not the first time such > > > warning has been missed. > > > > I tried reproducing this on latest bpf-next (b0efc216f577997) with gcc > > 9.3.0 by removing the initialization of duration: > > > > make: Entering directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf' > > TEST-OBJ [test_progs] sockmap_basic.test.o > > TEST-HDR [test_progs] tests.h > > EXT-OBJ [test_progs] test_progs.o > > EXT-OBJ [test_progs] cgroup_helpers.o > > EXT-OBJ [test_progs] trace_helpers.o > > EXT-OBJ [test_progs] network_helpers.o > > EXT-OBJ [test_progs] testing_helpers.o > > BINARY test_progs > > make: Leaving directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf' > > > > So, gcc doesn't issue a warning. Jakub did the following little experiment: > > > > jkbs@toad ~/tmp $ cat warning.c > > #include <stdio.h> > > > > int main(void) > > { > > int duration; > > > > fprintf(stdout, "%d", duration); > > > > return 0; > > } > > jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c > > warning.c: In function ‘main’: > > warning.c:7:2: warning: ‘duration’ is used uninitialized in this > > function [-Wuninitialized] > > 7 | fprintf(stdout, "%d", duration); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > The simple case seems to work. However, adding the macro breaks things: > > > > jkbs@toad ~/tmp $ cat warning.c > > #include <stdio.h> > > > > #define _CHECK(duration) \ > > ({ \ > > fprintf(stdout, "%d", duration); \ > > }) > > #define CHECK() _CHECK(duration) > > > > int main(void) > > { > > int duration; > > > > CHECK(); > > > > return 0; > > } > > jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c > > jkbs@toad ~/tmp $ > > That's very interesting. Thanks for the pointers. > I'm using gcc version 9.1.1 20190605 (Red Hat 9.1.1-2) > and I saw this warning while compiling selftests, > but I don't see it with above warning.c example. > clang warns correctly in both cases. I think this might be the same problem I fixed for libbpf with [0]. Turns out, GCC explicitly calls out (somewhere in their docs) that uninitialized variable warnings work only when compiled in optimized mode, because some internal data structures used to detect this are only maintained in optimized mode build. Laurenz, can you try compiling your example with -O2? [0] https://patchwork.ozlabs.org/project/netdev/patch/20200929220604.833631-2-andriin@fb.com/ > > > Maybe this is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501 ? The > > problem is still there on gcc 10. Compiling test_progs with clang does > > issue a warning FWIW, but it seems like other things break when doing > > that. > > That gcc bug has been opened since transition to ssa. That was a huge > transition for gcc. But I think the bug number is not correct. It points to a > different issue. I've checked -fdump-tree-uninit-all dump with and without > macro. They're identical. The tree-ssa-uninit pass suppose to warn, but it > doesn't. I wish I had more time to dig into it. A bit of debugging in > gcc/tree-ssa-uninit.c can probably uncover the root cause.
On Thu, Oct 1, 2020 at 10:09 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Oct 1, 2020 at 12:25 AM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Wed, Sep 30, 2020 at 10:28:33AM +0100, Lorenz Bauer wrote: > > > On Tue, 29 Sep 2020 at 16:48, Alexei Starovoitov > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > ... > > > > > > > There was a warning. I noticed it while applying and fixed it up. > > > > Lorenz, please upgrade your compiler. This is not the first time such > > > > warning has been missed. > > > > > > I tried reproducing this on latest bpf-next (b0efc216f577997) with gcc > > > 9.3.0 by removing the initialization of duration: > > > > > > make: Entering directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf' > > > TEST-OBJ [test_progs] sockmap_basic.test.o > > > TEST-HDR [test_progs] tests.h > > > EXT-OBJ [test_progs] test_progs.o > > > EXT-OBJ [test_progs] cgroup_helpers.o > > > EXT-OBJ [test_progs] trace_helpers.o > > > EXT-OBJ [test_progs] network_helpers.o > > > EXT-OBJ [test_progs] testing_helpers.o > > > BINARY test_progs > > > make: Leaving directory '/home/lorenz/dev/bpf-next/tools/testing/selftests/bpf' > > > > > > So, gcc doesn't issue a warning. Jakub did the following little experiment: > > > > > > jkbs@toad ~/tmp $ cat warning.c > > > #include <stdio.h> > > > > > > int main(void) > > > { > > > int duration; > > > > > > fprintf(stdout, "%d", duration); > > > > > > return 0; > > > } > > > jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c > > > warning.c: In function ‘main’: > > > warning.c:7:2: warning: ‘duration’ is used uninitialized in this > > > function [-Wuninitialized] > > > 7 | fprintf(stdout, "%d", duration); > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > The simple case seems to work. However, adding the macro breaks things: > > > > > > jkbs@toad ~/tmp $ cat warning.c > > > #include <stdio.h> > > > > > > #define _CHECK(duration) \ > > > ({ \ > > > fprintf(stdout, "%d", duration); \ > > > }) > > > #define CHECK() _CHECK(duration) > > > > > > int main(void) > > > { > > > int duration; > > > > > > CHECK(); > > > > > > return 0; > > > } > > > jkbs@toad ~/tmp $ gcc -Wall -o /dev/null warning.c > > > jkbs@toad ~/tmp $ > > > > That's very interesting. Thanks for the pointers. > > I'm using gcc version 9.1.1 20190605 (Red Hat 9.1.1-2) > > and I saw this warning while compiling selftests, > > but I don't see it with above warning.c example. > > clang warns correctly in both cases. > > I think this might be the same problem I fixed for libbpf with [0]. > Turns out, GCC explicitly calls out (somewhere in their docs) that > uninitialized variable warnings work only when compiled in optimized > mode, because some internal data structures used to detect this are > only maintained in optimized mode build. > > Laurenz, can you try compiling your example with -O2? All of my experiments I did with -O2. > [0] https://patchwork.ozlabs.org/project/netdev/patch/20200929220604.833631-2-andriin@fb.com/ > > > > > > Maybe this is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501 ? The > > > problem is still there on gcc 10. Compiling test_progs with clang does > > > issue a warning FWIW, but it seems like other things break when doing > > > that. > > > > That gcc bug has been opened since transition to ssa. That was a huge > > transition for gcc. But I think the bug number is not correct. It points to a > > different issue. I've checked -fdump-tree-uninit-all dump with and without > > macro. They're identical. The tree-ssa-uninit pass suppose to warn, but it > > doesn't. I wish I had more time to dig into it. A bit of debugging in > > gcc/tree-ssa-uninit.c can probably uncover the root cause.
On Thu, 1 Oct 2020 at 18:11, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > > > I think this might be the same problem I fixed for libbpf with [0]. > > Turns out, GCC explicitly calls out (somewhere in their docs) that > > uninitialized variable warnings work only when compiled in optimized > > mode, because some internal data structures used to detect this are > > only maintained in optimized mode build. > > > > Laurenz, can you try compiling your example with -O2? > > All of my experiments I did with -O2. If anybody wants to play with this more: https://godbolt.org/z/77P6P9 Seems like red hat GCC has some special sauce that fixes this behaviour?
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c index 4b7a527e7e82..67d3301bdf81 100644 --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c @@ -50,6 +50,37 @@ static int connected_socket_v4(void) return -1; } +static void compare_cookies(struct bpf_map *src, struct bpf_map *dst) +{ + __u32 i, max_entries = bpf_map__max_entries(src); + int err, duration, src_fd, dst_fd; + + src_fd = bpf_map__fd(src); + dst_fd = bpf_map__fd(dst); + + for (i = 0; i < max_entries; i++) { + __u64 src_cookie, dst_cookie; + + err = bpf_map_lookup_elem(src_fd, &i, &src_cookie); + if (err && errno == ENOENT) { + err = bpf_map_lookup_elem(dst_fd, &i, &dst_cookie); + CHECK(!err, "map_lookup_elem(dst)", "element %u not deleted\n", i); + CHECK(err && errno != ENOENT, "map_lookup_elem(dst)", "%s\n", + strerror(errno)); + continue; + } + if (CHECK(err, "lookup_elem(src)", "%s\n", strerror(errno))) + continue; + + err = bpf_map_lookup_elem(dst_fd, &i, &dst_cookie); + if (CHECK(err, "lookup_elem(dst)", "%s\n", strerror(errno))) + continue; + + CHECK(dst_cookie != src_cookie, "cookie mismatch", + "%llu != %llu (pos %u)\n", dst_cookie, src_cookie, i); + } +} + /* Create a map, populate it with one socket, and free the map. */ static void test_sockmap_create_update_free(enum bpf_map_type map_type) { @@ -109,9 +140,9 @@ static void test_skmsg_helpers(enum bpf_map_type map_type) static void test_sockmap_update(enum bpf_map_type map_type) { struct bpf_prog_test_run_attr tattr; - int err, prog, src, dst, duration = 0; + int err, prog, src, duration = 0; struct test_sockmap_update *skel; - __u64 src_cookie, dst_cookie; + struct bpf_map *dst_map; const __u32 zero = 0; char dummy[14] = {0}; __s64 sk; @@ -127,18 +158,14 @@ static void test_sockmap_update(enum bpf_map_type map_type) prog = bpf_program__fd(skel->progs.copy_sock_map); src = bpf_map__fd(skel->maps.src); if (map_type == BPF_MAP_TYPE_SOCKMAP) - dst = bpf_map__fd(skel->maps.dst_sock_map); + dst_map = skel->maps.dst_sock_map; else - dst = bpf_map__fd(skel->maps.dst_sock_hash); + dst_map = skel->maps.dst_sock_hash; err = bpf_map_update_elem(src, &zero, &sk, BPF_NOEXIST); if (CHECK(err, "update_elem(src)", "errno=%u\n", errno)) goto out; - err = bpf_map_lookup_elem(src, &zero, &src_cookie); - if (CHECK(err, "lookup_elem(src, cookie)", "errno=%u\n", errno)) - goto out; - tattr = (struct bpf_prog_test_run_attr){ .prog_fd = prog, .repeat = 1, @@ -151,12 +178,7 @@ static void test_sockmap_update(enum bpf_map_type map_type) "errno=%u retval=%u\n", errno, tattr.retval)) goto out; - err = bpf_map_lookup_elem(dst, &zero, &dst_cookie); - if (CHECK(err, "lookup_elem(dst, cookie)", "errno=%u\n", errno)) - goto out; - - CHECK(dst_cookie != src_cookie, "cookie mismatch", "%llu != %llu\n", - dst_cookie, src_cookie); + compare_cookies(skel->maps.src, dst_map); out: test_sockmap_update__destroy(skel);
We compare socket cookies to ensure that insertion into a sockmap worked. Pull this out into a helper function for use in other tests. Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> --- .../selftests/bpf/prog_tests/sockmap_basic.c | 50 +++++++++++++------ 1 file changed, 36 insertions(+), 14 deletions(-)