Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies

[Andrii Nakryiko]

On Thu, Oct 1, 2020 at 12:25 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxx/

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