[Bug middle-end/80548] -Wmaybe-uninitialized false positive when an assignment is added

[Bug middle-end/80548] -Wmaybe-uninitialized false positive when an assignment is added

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |24639
   Last reconfirmed|2017-04-28 00:00:00         |2021-4-13
                 CC|                            |msebor at gcc dot gnu.org
      Known to fail|                            |11.0

--- Comment #2 from Martin Sebor <msebor at gcc dot gnu.org> ---
Reconfirmed with GCC 11 and the IL below annotated with my comments.  I don't
see anything in the IL to rule out the uninitialized use:

void f (int b)
{
  int y;
  int x;
  int _1;

  <bb 2> [local count: 59055799]:
  if (b_5(D) != 0)
    goto <bb 3>; [33.00%]
  else
    goto <bb 11>; [67.00%]              >>> b == 0

  <bb 3> [local count: 19488414]:
  # .MEM_9 = VDEF <.MEM_8(D)>
  x_10 = g ();
  # .MEM_11 = VDEF <.MEM_9>
  y_12 = g ();
  goto <bb 6>; [100.00%]                >>> b != 0

  <bb 4> [local count: 1014686025]:
  if (b_5(D) != 0)
    goto <bb 5>; [5.50%]
  else
    goto <bb 10>; [94.50%]

  <bb 10> [local count: 958878294]:
  goto <bb 8>; [100.00%]

  <bb 5> [local count: 55807731]:       <<< x_17 used when g() != 0 AND b != 0
  # .MEM_14 = VDEF <.MEM_13>
  h (x_17, y_2);                        <<< warning for x_17
  # .MEM_15 = VDEF <.MEM_14>
  y_16 = g ();
  goto <bb 7>; [100.00%]

  <bb 11> [local count: 39567385]:      <<< b == 0

  <bb 6> [local count: 59055799]:       <<< x_6(D) uninitialized when b != 0 OR
b == 0
  # x_17 = PHI <x_10(3), x_6(D)(11)>    <<< x_17 <-- x_6(D)(11)
  # y_18 = PHI <y_12(3), y_7(D)(11)>
  # .MEM_19 = PHI <.MEM_11(3), .MEM_8(D)(11)>

  <bb 7> [local count: 114863530]:
  # y_2 = PHI <y_18(6), y_16(5)>
  # .MEM_20 = PHI <.MEM_19(6), .MEM_15(5)>

  <bb 8> [local count: 1073741824]:
  # .MEM_4 = PHI <.MEM_20(7), .MEM_13(10)>
  # .MEM_13 = VDEF <.MEM_4>
  _1 = g ();
  if (_1 != 0)
    goto <bb 4>; [94.50%]               >>> g() != 0
  else
    goto <bb 9>; [5.50%]

  <bb 9> [local count: 59055800]:
  # VUSE <.MEM_13>
  return;


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=24639
[Bug 24639] [meta-bug] bug to track all Wuninitialized issues

[Bug tree-optimization/80548] -Wmaybe-uninitialized false positive when an assignment is added

[aldyh at gcc dot gnu.org]

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

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |aldyh at gcc dot gnu.org,
                   |                            |amacleod at redhat dot com,
                   |                            |law at gcc dot gnu.org

--- Comment #3 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
I think we should start thinking of any Wuninitialized warnings in terms of
missed threads, at least initially.  Jeff has mentioned this in the past, an
I'm slowly being converted.  I mean, not that there aren't problems in the
uninit code, but we should at least check that there are no missing threads.

The problematic read from x is in BB5:

  <bb 5> [local count: 55807731]:
  h (x_17, y_2);
  y_16 = g ();
  goto <bb 7>; [100.00%]

On paths starting on 2->3 x_17 is defined, whereas on paths starting on 2->6
x_17 is undefined:

  <bb 6> [local count: 59055799]:
  # x_17 = PHI <x_10(3), x_6(D)(2)>

Looking at the last threader (threadfull2) before the uninit pass we see boths
paths are registered for threading:

  [3] Registering jump thread: (3, 6) incoming edge;  (6, 7) normal (7, 8)
normal (8, 4) normal (4, 5) nocopy; 

  [4] Registering jump thread: (2, 6) incoming edge;  (6, 7) normal (7, 8)
normal (8, 4) normal (4, 10) nocopy; 

However, there were no threaded paths in the entire compilation:

$ grep thread a.c.338t.statistics 
$

So sometime between registering the path for threading and the lowlevel BB
copier, we dropped the threads.

The culprit is duplicate_thread_path in the generic bits:

      /* We do not handle subloops, i.e. all the blocks must belong to the
         same loop.  */
      if (region[i]->loop_father != loop)
        return false;

This is an area I'm unfamiliar with, but it seems we should be able to thread
this when loop optimizations are done.

And indeed, if we allow this when loop_done, no warning is emitted.

[Bug tree-optimization/80548] -Wmaybe-uninitialized false positive when an assignment is added

[aldyh at gcc dot gnu.org]

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

--- Comment #4 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
Created attachment 51908
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51908&action=edit
untested patch

Like this.  It fixes the problem at least for -O2.

[Bug tree-optimization/80548] -Wmaybe-uninitialized false positive when an assignment is added

[aldyh at gcc dot gnu.org]

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

--- Comment #5 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
(In reply to Aldy Hernandez from comment #4)
> Created attachment 51908 [details]
> untested patch
> 
> Like this.  It fixes the problem at least for -O2.

For -O1 y'all are on your own because there are no threaders smart enough to
pick up the pattern.

We'd need a threader at -O1 that runs after loop_done to pick it up:

$ ./cc1 a.c -O1 -Wall  -fenable-tree-threadfull2 -quiet
cc1: note: enable pass tree-threadfull2 for functions in the range of [0,
4294967295]
$ 

Voila, no warning.

DOM3 runs after loop_done, so that would be candidate to handle this since it
threads at -O1.  However, this won't happen in this release.  It would require
a converting DOM threading to a hybrid approach (and see if the custom copier
can handle cross-loop threading), or convert it entirely to the backward
threader (would require ranger handling floats).

[Bug tree-optimization/80548] -Wmaybe-uninitialized false positive when an assignment is added

[aldyh at gcc dot gnu.org]

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

--- Comment #6 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
(In reply to Aldy Hernandez from comment #4)
> Created attachment 51908 [details]
> untested patch
> 
> Like this.  It fixes the problem at least for -O2.

Richi responded that the current BB copier won't handle this:
https://gcc.gnu.org/pipermail/gcc-patches/2021-December/585884.html

[Bug tree-optimization/80548] -Wmaybe-uninitialized false positive when an assignment is added

[law at gcc dot gnu.org]

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

Jeffrey A. Law <law at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #7 from Jeffrey A. Law <law at gcc dot gnu.org> ---
It looks like this got fixed on the trunk.   Not sure if it's from Aldy or
Richi's work.  Just happy it's fixed :-)

[Bug tree-optimization/80548] -Wmaybe-uninitialized false positive when an assignment is added

[vincent-gcc at vinc17 dot net]

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

--- Comment #8 from Vincent Lefèvre <vincent-gcc at vinc17 dot net> ---
Indeed, compared to GCC 12.2.0, the trunk no longer warns on the simple
testcase I provided. However, I cannot see any change of the warnings on my
original file (to myself: tmd/binary32/hrcases.c), except concerning the order
of the warnings (on this file, I get 2 spurious -Wmaybe-uninitialized warnings,
and they are now reversed). I'll try to provide another simple testcase.

I'm wondering whether this bug is really fixed or it just happens to have
disappeared on the testcase just because of a side effect of some other change
in GCC and a small change addition to the testcase would make it reappear.

[Bug tree-optimization/80548] -Wmaybe-uninitialized false positive when an assignment is added

[law at gcc dot gnu.org]

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

--- Comment #9 from Jeffrey A. Law <law at gcc dot gnu.org> ---
These warnings are certainly sensitive to all kinds of things, so it's possible
it's just gone latent.  The only way to be sure would be to bisect all the work
between gcc-12 and the trunk and pour over the dumps with a fine tooth comb. I
would hazard a guess it was Aldy's backwards threader work, particularly around
not bailing out too early for subpaths based on comments in the BZ, but one
would have to bisect to be 100% sure.

What I would recommend would be opening a new bug for the new cases.  While
these bugs often appear to be the same, it's actually fairly rare in practice
that they have the same underlying cause.  Each has to be analysed
individually.

[Bug tree-optimization/80548] -Wmaybe-uninitialized false positive when an assignment is added

[vincent-gcc at vinc17 dot net]

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

--- Comment #10 from Vincent Lefèvre <vincent-gcc at vinc17 dot net> ---
(In reply to Jeffrey A. Law from comment #9)
> These warnings are certainly sensitive to all kinds of things, so it's
> possible it's just gone latent.  The only way to be sure would be to bisect
> all the work between gcc-12 and the trunk and pour over the dumps with a
> fine tooth comb. I would hazard a guess it was Aldy's backwards threader
> work, particularly around not bailing out too early for subpaths based on
> comments in the BZ, but one would have to bisect to be 100% sure.

The commit that made the warning disappear is actually the one fixing PR106754.

commit 0a4a2667dc115ca73b552fcabf8570620dfbe55f
Author: Richard Biener <rguenther@suse.de>
Date:   2022-09-06 13:46:00 +0200

    tree-optimization/106754 - fix compute_control_dep_chain defect

    The following handles the situation of a loop exit along the
    control path to the PHI def or from there to the use in a different
    way, aoviding premature abort of the walks as noticed in the two
    cases where the exit is outermost (gcc.dg/uninit-pred-11.c) or
    wrapped in a condition that is on the path (gcc.dg/uninit-pred-12.c).
    Instead of handling such exits during recursion we now pick them
    up in the parent when walking post-dominators.  That requires an
    additional post-dominator walk at the outermost level which is
    facilitated by splitting out the walk to a helper function and
    the existing wrapper added earlier.

    The patch also removes the bogus early exit from
    uninit_analysis::init_use_preds, fixing a simplified version
    of the PR106155 testcase.

[Bug tree-optimization/80548] -Wmaybe-uninitialized false positive when an assignment is added

[law at gcc dot gnu.org]

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

--- Comment #11 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Thanks!  So the change in question improves the decisions in the predicate
analysis code, which can be best thought of as a filter for the false positives
that are still in the IL.

As I said in my previous comment, the best way forward is to get those two new
instances filed as distinct bugs in BZ.

[Bug tree-optimization/80548] -Wmaybe-uninitialized false positive when an assignment is added

[vincent-gcc at vinc17 dot net]

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

--- Comment #12 from Vincent Lefèvre <vincent-gcc at vinc17 dot net> ---
(In reply to Jeffrey A. Law from comment #11)
> As I said in my previous comment, the best way forward is to get those two
> new instances filed as distinct bugs in BZ.

See PR107838 and PR107839.