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