[Bug c++/105534] New: -Wmaybe-uninitialized shouldn't suppress -Wuninitialized warnings

[Bug c++/105534] New: -Wmaybe-uninitialized shouldn't suppress -Wuninitialized warnings

[redbeard0531 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105534

            Bug ID: 105534
           Summary: -Wmaybe-uninitialized shouldn't suppress
                    -Wuninitialized warnings
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: redbeard0531 at gmail dot com
  Target Milestone: ---

The following function emits a -Wuninitialized warning on ++count with -Wall
https://godbolt.org/z/KfaMEETY1:

int test(bool cond) {
    int count;
    ++count;
    return count;
}

Making the increment be conditional changes it to a -Wmaybe-uninitialized
warning, which is suppressed with -Wno-maybe-uninitialized.
https://godbolt.org/z/qarMrqW7E

int test(bool cond) {
    int count;
    if (cond) ++count;
    return count;
}

This makes no sense. count is never initialized on any path through the
function, and it is returned on all paths.

We use -Wall with -Wno-maybe-uninitialized on our codebase because we were
getting too many false-positives with -Wmaybe-initialized, in particular from
third-party headers that we didn't want to modify. At the time we decided to do
that, we didn't realize that we would also be missing out on clearly
uninitialized cases like this.

[Bug c++/105534] -Wmaybe-uninitialized cases shouldn't suppress -Wuninitialized warnings

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105534

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|unknown                     |12.1.0
   Last reconfirmed|                            |2022-05-09
     Ever confirmed|0                           |1
           Keywords|                            |diagnostic
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed.  I think we are not considering that

  # count_1 = PHI <count_3(D)(5), count_4(3)>

is actually the same variable plus that count_4 can be considered an
uninitialized use even though it is (conditional)

  count_4 = count_3(D) + 1;

instead what we are keying on is the uses of count_3(D) only, of which
one is in the PHI and one in the add.  Note that after emitting one
diagnostic we're silencing the other because we mark 'count' as being
already diagnosed.

If you modify the testcase to

int test(_Bool cond) {
    int count, count2;
    if (cond) return ++count;
    return count2;
}

you get

t2.c: In function 'test':
t2.c:3:22: warning: 'count' may be used uninitialized [-Wmaybe-uninitialized]
    3 |     if (cond) return ++count;
      |                      ^~~~~~~
t2.c:2:9: note: 'count' was declared here
    2 |     int count, count2;
      |         ^~~~~
t2.c:4:12: warning: 'count2' may be used uninitialized [-Wmaybe-uninitialized]
    4 |     return count2;
      |            ^~~~~~
t2.c:2:16: note: 'count2' was declared here
    2 |     int count, count2;
      |                ^~~~~~

and the IL is very similar:

  <bb 3> [local count: 365072224]:
  count_5 = count_4(D) + 1;

  # _1 = PHI <count_5(3), count2_3(D)(5)>

you would probably agree to the "may be used" kind used here in which case
it's exactly noticing that the same variable is used on all paths into the
PHI node.

If you'd say this is also a case of an unconditional uninitialized use
(it is!) then it might be even easier to implement this since we don't
need to prove the variables returned are the same ones, just that they
have uninitialized uses.

[Bug c++/105534] -Wmaybe-uninitialized cases shouldn't suppress -Wuninitialized warnings

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105534

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Note there's

  _2 = value_1(D) * x_2;

where _2 might be effectively uninitialized iff x_2 is not zero.  When x_2
is zero then _2 has a well-defined value.  So to start thinking about this
I'd do a RPO walk recording a lattice of { UNDEF, MAY_UNDEF, DEF } where
the above would be "MAY_UNDEF".  PHIs would then simply merge.

That would be enough to better distinguish may from must in case the
must diagnostic is OK for the two different variables being returned.
Otherwise that needs to be taken into account as well, maybe by tracking
which variables a value is from.  Consider

void test (int x)
{
   int value1, value2;
   return (value1 * x) + value2;
}

where we maybe use value1 uninitialized and definitely value2.

Note this all leaves open which point to diagnose - the expression closest
to the (possibly conditional and possible multiple) computations with
value1 and value2 or the expression where the first (or last?) unconditional
use appears in?

[Bug c++/105534] -Wmaybe-uninitialized cases shouldn't suppress -Wuninitialized warnings

[redbeard0531 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105534

--- Comment #3 from Mathias Stearn <redbeard0531 at gmail dot com> ---
One slightly confusing aspect is that the wording of the flag implies that the
variable may or may not be uninitialzied (because in -Wmaybe-uninitialized
maybe binds directly to uninitialized), while phrasing of the warning message
is about the usage being conditional: "may be used uninitialized". And of
course the documentation (at least the man page) uses a different phrasing:

> For an object with automatic or allocated storage duration, if there exists a path from the function entry to a use of the object that is initialized, but there exist some other paths for which the object is not initialized, the compiler emits a warning if it cannot prove the uninitialized paths are not executed at run time.

For both the initial example with count, and your example with count2, I'd say
that the "there exists a path from the function entry to a use of the object
that is initialized" bit is clearly not satisfied, so if we assume the
documentation is correct, then those cases both lack a "maybe" and the
variables are clearly uninitialized.

This would also match my intuition for -Winitialized which is that it
definitively errors if all paths from declaration to any usage result in the
variable being uninitialized.

PS - This test case is a reduced example from an actual bug that luckily was
found by coverity before release: https://jira.mongodb.org/browse/SERVER-66306.
I dug in to make a repro because I was expecting that we would have gotten a
compiler error on that code before it was even committed. I'm also exploring
whether we can stop passing -Wno-maybe-uninitialized, but it looks like we
still get false positives in third-party headers, so it doesn't seem likely.

[Bug c++/105534] -Wmaybe-uninitialized cases shouldn't suppress -Wuninitialized warnings

[redbeard0531 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105534

--- Comment #4 from Mathias Stearn <redbeard0531 at gmail dot com> ---
(In reply to Richard Biener from comment #2)
> Note there's
> 
>   _2 = value_1(D) * x_2;
> 
> where _2 might be effectively uninitialized iff x_2 is not zero.  When x_2
> is zero then _2 has a well-defined value.

Not according to the C++ standard. http://eel.is/c++draft/basic.indet seems
pretty clear that unless x_2 is std::byte (which doesn't support
multiplication) or an "unsigned ordinary character type", then that is UB. 

FWIW I still think I'd expect the warning on "unsigned char x, y; y = x * 0;",
but I would definitiely expect the warning for int.