On 9/13/21 4:18 PM, Michael Matz wrote:
> Hello,
>
> On Mon, 13 Sep 2021, Jeff Law via Gcc-patches wrote:
>
>>> So it looks like there's some undefined behavior going on, even before
>>> my patch.  I'd like to get some feedback, because this is usually the
>>> type of problems I see in the presence of a smarter threader... things
>>> get shuffled around, problematic code gets isolated, and warning
>>> passes have an easier time (or sometimes harder time) diagnosing
>>> things.
>> The original issue was PRE hanging, so I'd lean towards keeping the test
as-is
>> and instead twiddling any warning flags we can to make the diagnostics go
>> away.
>
> Or use this changed test avoiding the issues that I see with -W -Wall on
> this testcase.  I've verified that it still hangs before r194358 and is
> fixed by that revision.
>
> Generally I think, our testsuite, even for ICEs or these kinds of hangs,
> should make an effort to try to write conforming code; if at all possible.
> Here it is possible.
>
> (I don't know if the new threader causes additional warnings, of course,
> but at least the problems with sequence points and uninitialized use of
> 'j' aren't necessary to reproduce the bug)
>
>
> Ciao,
> Michael.
>
> /* { dg-do compile } */
> /* { dg-additional-options "-fno-split-loops" } */
>
> typedef unsigned short uint16_t;
>
> uint16_t a, b;
>
> int *j_global;
> uint16_t f(void)
> {
>    int c, **p;
>    short d = 2, e = 4;
>
>    for (;; b++)
>      {
>        int *j = j_global, k = 0;
>
>        for (; *j; j++)
>       {
>         for(; c; c++)
>           for(; k < 1; k++)
>             {
>               short *f = &d;
>
>               if(b)
>                 return *f;
>             }
>       }
>
>        if(!c)
>       d *= e;
>
>        a = d;
>        if ((a ? b = 0 : (**p ? : 1) != (d != 1 ? 1 : (b = 0))) != ((k ? a
: 0)
>         < (a * (c = k))))
>       **p = 0;
>      }
> }
>

Thanks for getting rid of the noise here.

I've simplified the above to show what's going on in the warning on
nds32-elf:

int george, *global;
int stuff(), readme();

int
f (void)
{
   int store;

   for (;;)
     {
       int k = 0;

       while (global)
        {
          for (; store; ++store)
            {
              for (; k < 1; k++)
                {
                  if (readme())
                    return 0;
                }
            }
        }

       store = k;
       if (george)
        stuff();
     }
}

The -Waggressive-loop-optimizations pass is complaining because of an
undefined iteration in the for(;store;++store) loop.  But this looks
like it's getting confused by threader having isolated an undefined path.

At the warning, the IL looks like this on entry:

   <bb 2> [local count: 55807730]:
   goto <bb 4>; [100.00%]

   <bb 4> [local count: 57254340]:
   # store_4 = PHI <k_42(3), store_14(D)(2)>
   global.0_25 = global;
   if (global.0_25 != 0B)
     goto <bb 12>; [94.50%]
   else
     goto <bb 13>; [5.50%]

...
...

  <bb 12> [local count: 54105352]:
   if (store_4 != 0)
     goto <bb 7>; [99.64%]
   else
     goto <bb 10>; [0.36%]

If global.0_25 was true on entry, the read from store_4 would be
undefined.  Presumably the warning pass is assuming this path always
gets executed.

This looks like a latent bug.  For that matter, the above snippet warns
with -fdisable-tree-thread2, even on x86-64 (and before my
patch).

Aldy

Reply via email to