[Bug tree-optimization/101912] New: -Wmaybe-uninitialized false alarm in tzdb localtime.c

[Bug tree-optimization/101912] New: -Wmaybe-uninitialized false alarm in tzdb localtime.c

[eggert at cs dot ucla.edu]

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

            Bug ID: 101912
           Summary: -Wmaybe-uninitialized false alarm in tzdb localtime.c
           Product: gcc
           Version: 11.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: eggert at cs dot ucla.edu
  Target Milestone: ---

Created attachment 51303
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51303&action=edit
-Wuninitialized false alarm

This is a simplified version of a problem I found while compiling tzdb's
localtime.c with gcc (GCC) 11.2.1 20210728 (Red Hat 11.2.1-1) on x86-64.
Compile the attached program w.i with:

gcc -Wmaybe-uninitialized -O2 -S w.i

The output is:

w.i: In function ‘tzloadbody’:
w.i:17:27: warning: ‘prevcorr’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
   16 |                        : (corr == prevcorr
      |                          ~~~~~~~~~~~~~~~~~
   17 |                           || corr == prevcorr - 1)))))
      |                           ^~~~~~~~~~~~~~~~~~~~~~~~


This is a false alarm, since the underlined code is executed only when leapcnt
is nonzero, and leapcnt is set to a nonzero value only after prevcorr is
initialized.

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[msebor at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
      Known to fail|                            |10.3.0, 11.2.0, 12.0,
                   |                            |6.5.0, 7.5.0, 8.5.0, 9.3.0
           Keywords|                            |diagnostic
             Status|UNCONFIRMED                 |NEW
                 CC|                            |msebor at gcc dot gnu.org
   Last reconfirmed|                            |2021-08-16

--- Comment #1 from Martin Sebor <msebor at gcc dot gnu.org> ---
Confirmed, not a regression.  The warning pass considers only one condition: n
> 0.  It doesn't consider the conjunction of that condition with those implied
by not using the other unintialized PHI operand and that the use is unreachable
otherwise (i.e., that !(corr == 1 && leapcnt != 0 && prevcorr - 1 > 1 && n !=
leapcnt) cannot be true at the point of the use).  The logic seem too
complicated to me to figure it out but I could be missing something.

int tzloadbody ()
{
  int corr;
  int leapcnt;
  int prevcorr;
  int n;
  _Bool _1;
  _Bool _2;
  _Bool _5;
  unsigned int _6;
  int _8;
  unsigned int _9;
  _Bool _21;
  _Bool _27;

  <bb 2> [local count: 95397018]:
  # .MEM_12 = VDEF <.MEM_11(D)>
  n_13 = getint ();
  if (n_13 > 0)
    goto <bb 7>; [96.34%]                               >>> if n > 0...
  else
    goto <bb 9>; [3.66%]

  <bb 7> [local count: 91905487]:                       <<<
...prevcorr_14(D)(7) uninitialized here

  <bb 3> [local count: 1034442874]:                     <<< if n > 0 && !(corr
== 1 && leapcnt != 0 && prevcorr - 1 > 1 && n != leapcnt)
  # prevcorr_18 = PHI <corr_16(8), prevcorr_14(D)(7)>   <<< 
  # leapcnt_23 = PHI <leapcnt_17(8), 0(7)>
  # .MEM_20 = PHI <.MEM_15(8), .MEM_12(7)>
  # .MEM_15 = VDEF <.MEM_20>
  corr_16 = getint ();
  if (corr_16 <= 0)
    goto <bb 10>; [3.66%]
  else
    goto <bb 4>; [96.34%]

  <bb 4> [local count: 996582264]:
  _1 = corr_16 == 1;
  _2 = leapcnt_23 != 0;
  _9 = (unsigned int) prevcorr_18;                       <<<
-Wmaybe-uninitialized
  _6 = _9 + 4294967295;
  _5 = _6 > 1;
  _21 = _1 & _2;
  _27 = _5 & _21;
  if (_27 != 0)
    goto <bb 11>; [21.78%]
  else
    goto <bb 5>; [78.22%]                               >>> corr == 1 &&
leapcnt != 0 && prevcorr - 1 > 1

  <bb 11> [local count: 217055616]:
  goto <bb 6>; [100.00%]

  <bb 5> [local count: 978344809]:
  leapcnt_17 = leapcnt_23 + 1;
  if (n_13 != leapcnt_17)
    goto <bb 8>; [96.34%]                               >>> corr == 1 &&
leapcnt != 0 && prevcorr - 1 > 1 && n != leapcnt
  else
    goto <bb 12>; [3.66%]

  <bb 12> [local count: 35807421]:
  goto <bb 6>; [100.00%]

  <bb 8> [local count: 942537388]:
  goto <bb 3>; [100.00%]                                >>> corr == 1 &&
leapcnt != 0 && prevcorr - 1 > 1 && n != leapcnt

  <bb 9> [local count: 3491531]:

  <bb 6> [local count: 95397019]:
  # _8 = PHI <0(9), n_13(12), -1(11), -1(10)>
  # .MEM_10 = PHI <.MEM_12(9), .MEM_15(12), .MEM_15(11), .MEM_15(10)>
  # VUSE <.MEM_10>
  return _8;

}

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[aldyh at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |aldyh at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org

--- Comment #2 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
ISTM that the warning is correct, given the IL the uninit pass gets.

On the 7->3 entry to the loop prevcorr_23 can be undefined, and it is read in
BB4 with no guard whatsoever:

  <bb 7> [local count: 85931698]:

  <bb 3> [local count: 1034442873]:
  # prevcorr_23 = PHI <corr_14(8), prevcorr_12(D)(7)>
  # leapcnt_24 = PHI <leapcnt_15(8), 0(7)>
  corr_14 = getint ();
  if (corr_14 <= 0)
    goto <bb 10>; [3.66%]
  else
    goto <bb 4>; [96.34%]

  <bb 10> [local count: 37860610]:
  goto <bb 6>; [100.00%]

  <bb 4> [local count: 996582262]:
  _1 = corr_14 == 1;
  _2 = leapcnt_24 != 0;
  _7 = (unsigned int) prevcorr_23;
  _5 = _7 + 4294967295;
  _4 = _5 > 1;
  _22 = _1 & _2;
  _18 = _4 & _22;
  if (_18 != 0)
    goto <bb 11>; [1.21%]
  else
    goto <bb 5>; [98.79%]

However, in the original source we shouldn't read from prevcorr if leapcnt ==
0, which is the case on entry to the loop:

              && !(leapcnt == 0
                   || (prevcorr < corr
                       ? corr == prevcorr + 1
                       : (corr == prevcorr
                          || corr == prevcorr - 1)))))

It looks like we lost the leapcnt gate protecting the prevcorr_23 read.

Unless I'm missing some subtle language thinggie, we loose the gate in the
ifcombine pass.  Before it, the read from prevcorr was protected by:

  <bb 4> [local count: 996582262]:
  _1 = corr_14 == 1;
  _2 = leapcnt_5 != 0;
  _3 = _1 & _2;
  if (_3 != 0)
    goto <bb 5>; [50.00%]
  else
    goto <bb 7>; [50.00%]

and after ifcombine we have:

  <bb 4> [local count: 996582262]:
  _1 = corr_14 == 1;
  _2 = leapcnt_5 != 0;
  _3 = _1 & _2;
  _19 = prevcorr_4 != 1;
  _16 = _3 & _19;
  _21 = prevcorr_4 != 2;
  _18 = _16 & _21;
  if (_18 != 0)
    goto <bb 7>; [1.21%]
  else
    goto <bb 5>; [98.79%]

Unfortunately, adding -fdisable-tree-ifcombine doesn't make the warning go
away, but at least the IL by the uninit pass seems correct.

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[aldyh at gcc dot gnu.org]

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

--- Comment #3 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---

>               && !(leapcnt == 0
>                    || (prevcorr < corr
>                        ? corr == prevcorr + 1
>                        : (corr == prevcorr
>                           || corr == prevcorr - 1)))))
> 

I guess the question is whether language rules allow us to read prevcorr when
leapcnt== 0?

Like we're doing here:

> and after ifcombine we have:
> 
>   <bb 4> [local count: 996582262]:
>   _1 = corr_14 == 1;
>   _2 = leapcnt_5 != 0;
>   _3 = _1 & _2;
>   _19 = prevcorr_4 != 1;
>   _16 = _3 & _19;
>   _21 = prevcorr_4 != 2;
>   _18 = _16 & _21;
>   if (_18 != 0)
>     goto <bb 7>; [1.21%]
>   else
>     goto <bb 5>; [98.79%]

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[eggert at cs dot ucla.edu]

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

--- Comment #4 from eggert at cs dot ucla.edu ---
(In reply to Aldy Hernandez from comment #3)
> >               && !(leapcnt == 0
> >                    || (prevcorr < corr
> >                        ? corr == prevcorr + 1
> >                        : (corr == prevcorr
> >                           || corr == prevcorr - 1)))))
> > 
> 
> I guess the question is whether language rules allow us to read prevcorr
> when leapcnt== 0?

The C language rules do not allow that. When leapcnt is zero, behavior must be
as if the prevcorr expression is not evaluated.

Although the compiler can generate machine code that evaluates prevcorr at the
machine level (so long as the observable behavior is the same, which is the
case as prevcorr is not volatile and no untoward behavior can result from
evaluating the prevcorr expression), it's incorrect if the compiler warns the
programmer that prevcorr is used uninitialized.

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[rguenther at suse dot de]

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

--- Comment #5 from rguenther at suse dot de <rguenther at suse dot de> ---
On Tue, 30 Nov 2021, eggert at cs dot ucla.edu wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101912
> 
> --- Comment #4 from eggert at cs dot ucla.edu ---
> (In reply to Aldy Hernandez from comment #3)
> > >               && !(leapcnt == 0
> > >                    || (prevcorr < corr
> > >                        ? corr == prevcorr + 1
> > >                        : (corr == prevcorr
> > >                           || corr == prevcorr - 1)))))
> > > 
> > 
> > I guess the question is whether language rules allow us to read prevcorr
> > when leapcnt== 0?
> 
> The C language rules do not allow that. When leapcnt is zero, behavior must be
> as if the prevcorr expression is not evaluated.
> 
> Although the compiler can generate machine code that evaluates prevcorr at the
> machine level (so long as the observable behavior is the same, which is the
> case as prevcorr is not volatile and no untoward behavior can result from
> evaluating the prevcorr expression), it's incorrect if the compiler warns the
> programmer that prevcorr is used uninitialized.

Correct.  I think we have quite some duplicates around this issue
of making short-circuiting conditionals not short-circuiting (we do
that even early during GENERIC folding).  "Proving" that all participating
sub-expressions are fully initialized is impossible so the only
reasonable way to "fix" the issue (the uninit warnings) might be
to mark the now unconditionally evaluated sub-expressions with
-Wno-uninitialized (aka no-warning or suppress uninit warnings).

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[aldyh at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |law at gcc dot gnu.org

--- Comment #6 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
(In reply to rguenther@suse.de from comment #5)
> On Tue, 30 Nov 2021, eggert at cs dot ucla.edu wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101912
> > 
> > --- Comment #4 from eggert at cs dot ucla.edu ---
> > (In reply to Aldy Hernandez from comment #3)
> > > >               && !(leapcnt == 0
> > > >                    || (prevcorr < corr
> > > >                        ? corr == prevcorr + 1
> > > >                        : (corr == prevcorr
> > > >                           || corr == prevcorr - 1)))))
> > > > 
> > > 
> > > I guess the question is whether language rules allow us to read prevcorr
> > > when leapcnt== 0?
> > 
> > The C language rules do not allow that. When leapcnt is zero, behavior must be
> > as if the prevcorr expression is not evaluated.
> > 
> > Although the compiler can generate machine code that evaluates prevcorr at the
> > machine level (so long as the observable behavior is the same, which is the
> > case as prevcorr is not volatile and no untoward behavior can result from
> > evaluating the prevcorr expression), it's incorrect if the compiler warns the
> > programmer that prevcorr is used uninitialized.
> 
> Correct.  I think we have quite some duplicates around this issue
> of making short-circuiting conditionals not short-circuiting (we do
> that even early during GENERIC folding).  "Proving" that all participating
> sub-expressions are fully initialized is impossible so the only
> reasonable way to "fix" the issue (the uninit warnings) might be
> to mark the now unconditionally evaluated sub-expressions with
> -Wno-uninitialized (aka no-warning or suppress uninit warnings).

If this isn't something we are going to fix then by all means, mark them all as
-Wno-uninitialized.  I'm for whatever reduces the false positive rates in this
area :).

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |needs-bisection

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Seems to be fixed on the trunk; that is there is no uninitialized warning on
the trunk.

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amacleod at redhat dot com,
                   |                            |marxin at gcc dot gnu.org
           Keywords|needs-bisection             |

--- Comment #8 from Martin Liška <marxin at gcc dot gnu.org> ---
Fixed on master with r13-1938-g87dd4c8c83768aaf.

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
We simply optimize this better now.  I'll add the testcase.

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:7afecddf1ecbb93ee44517ad6d3e9c1c0d68e722

commit r13-5064-g7afecddf1ecbb93ee44517ad6d3e9c1c0d68e722
Author: Richard Biener <rguenther@suse.de>
Date:   Mon Jan 9 11:00:43 2023 +0100

    tree-optimization/101912 - testcase for fixed uninit case

    We now properly optimize this testcase and no longer diagnose
    a bogus uninit use.

            PR tree-optimization/101912
            * gcc.dg/uninit-pr101912.c: New testcase.

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED
      Known to work|                            |13.0
   Target Milestone|---                         |13.0

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed for GCC 13.

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[sjames at gcc dot gnu.org]

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

Sam James <sjames at gcc dot gnu.org> changed:

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

--- Comment #12 from Sam James <sjames at gcc dot gnu.org> ---
Per 24af552876eff707f75d30d3f0f0e7a5d62dd857
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109462#c7) and the test being
XFAILed, I think this is now unfixed?

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[sjames at gcc dot gnu.org]

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

--- Comment #13 from Sam James <sjames at gcc dot gnu.org> ---
(In reply to Sam James from comment #12)
> Per 24af552876eff707f75d30d3f0f0e7a5d62dd857
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109462#c7) and the test being
> XFAILed, I think this is now unfixed?

r13-7150-g24af552876eff707f75d30d3f0f0e7a5d62dd857

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|13.0                        |13.2

--- Comment #14 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 13.1 is being released, retargeting bugs to GCC 13.2.

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |FIXED
      Known to fail|                            |13.1.0
      Known to work|13.0                        |14.0

--- Comment #15 from Richard Biener <rguenth at gcc dot gnu.org> ---
r14-2289-gb083203f053f16 removed the XFAIL again.

[Bug tree-optimization/101912] -Wmaybe-uninitialized false alarm in tzdb localtime.c

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|13.2                        |14.0