[Bug c/59933] for loop goes wild with assert() enabled
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59933 Marek Polacek mpolacek at gcc dot gnu.org changed: What|Removed |Added Status|UNCONFIRMED |RESOLVED CC||mpolacek at gcc dot gnu.org Resolution|--- |INVALID --- Comment #4 from Marek Polacek mpolacek at gcc dot gnu.org --- There's no good reproducer, but if -fno-aggressive-loop-optimizations helps, most likely the code invokes undefined behavior. Tentatively closing as invalid (PR59982 was closed too).
[Bug c/59933] for loop goes wild with assert() enabled
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59933 --- Comment #5 from Mark Warner warnerme at ptd dot net --- sizeof(NSQ_del_dec_struct) / sizeof(opus_int32) is guaranteed to produced a even number with a remainder of 0. Note the __attribute__ ((__aligned__ (8))) to make it a multiple of 8 in size.
[Bug c/59933] for loop goes wild with assert() enabled
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59933 --- Comment #6 from Mark Warner warnerme at ptd dot net --- If it is invalid, why does -Wall not trigger anything ?
[Bug c/59933] for loop goes wild with assert() enabled
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59933 --- Comment #7 from Marek Polacek mpolacek at gcc dot gnu.org --- (int)(sizeof(NSQ_del_dec_struct) / sizeof(opus_int32) seems to be 1168/4 = 292, but sLPC_Q14 has only 112 elements.
[Bug c/59933] for loop goes wild with assert() enabled
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59933 Jakub Jelinek jakub at gcc dot gnu.org changed: What|Removed |Added CC||jakub at gcc dot gnu.org --- Comment #8 from Jakub Jelinek jakub at gcc dot gnu.org --- The code is not invalid C, just triggers undefined behavior, so it is not invalid at compile time, just at runtime if you ever hit this. GCC optimizes based on the assumption that undefined behavior doesn't happen in a correct program. While we have -Waggressive-loop-optimizations warning, it (intentionally) warns solely about the case where the loop has single exit and constant loop iteration count, which is not the case here, the number of iterations is i = 292 ? 0 : 292 - i. The loop will trigger undefined behavior whenever i is 292, if it is bigger, then there is no bug.
[Bug c/59933] for loop goes wild with assert() enabled
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59933 --- Comment #9 from Ian Hamilton ian at g0tcd dot com --- Yes, that's all proper and correct. The invalid C code induces undefined behaviour. I don't think anyone is disputing that. However, to be pragmatic for a moment, the experience of thousands of developers out there, working with legacy code, and trying to update their toolset to include gcc 4.8 is that code which compiled without warnings and worked with the old gcc compiler now still compiles without warnings, but fails at runtime with the 4.8 series compiler. Sometimes, the runtime failures are occasional and difficult to track down if (for example) it lies on an error handling path. This makes it even harder for these developers to figure out what's going on. If the compiler could provide a warning when it encounters this sort of invalid code, that would be a good thing, as it would highlight the old latent bugs and give developers the opportunity to fix them. However, it doesn't, so the developers working on legacy code really have no alternative to either using the -fno-aggressive-loop-optimizations switch to stabilse their legacy code (even assuming they understand what's happening), or sticking with the old version of the compiler. So I think the request to the gcc developers is to find a way of providing a compiler warning when the loop optimiser encounters problem code, to give developers a fighting chance of debugging their legacy code.
[Bug c/59933] for loop goes wild with assert() enabled
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59933 --- Comment #10 from Jakub Jelinek jakub at gcc dot gnu.org --- We have -fsanitize=undefined which can catch some issues, though the array bounds instrumentation (nor __builtin_object_size based instrumentation) has not been added yet for GCC 4.9, will be hopefully there in the next release. As for warnings, even the current -Waggressive-loop-optimizationsh warning (for the const number of iterations, single loop exit easy case where you know that if the loop is reachable, if there is undefined behavior in some loop iteration, you will trigger it) still has occassional false positives (various PRs about that, usually the issue is that while it is true there is such a loop, the loop is actually dead), further warnings would have huge false positive rate, to the extent that it would be rarely useful.
[Bug c/59933] for loop goes wild with assert() enabled
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59933 --- Comment #11 from Mark Warner warnerme at ptd dot net --- I'm confused .. what about.. for (k = i; k (int)(sizeof(NSQ_del_dec_struct) / sizeof(opus_int32)); ++k) ... is illegal or invalid ? Why does it only fail if -DDEBUG is defined ? I mean, this code worked fine for months .. and now
[Bug c/59933] for loop goes wild with assert() enabled
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59933 --- Comment #12 from Jakub Jelinek jakub at gcc dot gnu.org --- (In reply to Mark Warner from comment #11) I'm confused .. what about.. for (k = i; k (int)(sizeof(NSQ_del_dec_struct) / sizeof(opus_int32)); ++k) ... is illegal or invalid ? Why does it only fail if -DDEBUG is defined ? I mean, this code worked fine for months .. and now The undefined behavior is if i is smaller than 292, you access out of bound array members (well, in C already the address arithmetics is undefined behavior when you go further than one past the last element of the array). The sLPC_Q14 has only 112 entries, so say if i is 0, then when k is 112, you invoke undefined behavior, because you can't read or write sLPC_Q14[112].
[Bug c/59933] for loop goes wild with assert() enabled
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59933 --- Comment #13 from Ian Hamilton ian at g0tcd dot com --- (In reply to Mark Warner from comment #11) I'm confused .. what about.. for (k = i; k (int)(sizeof(NSQ_del_dec_struct) / sizeof(opus_int32)); ++k) ... is illegal or invalid ? Why does it only fail if -DDEBUG is defined ? I mean, this code worked fine for months .. and now What this code seems to be doing is copying a section at the end of the sLPC_Q14 array at the beginning of the NSQ_del_dec_struct structure, plus all the other structure members that follow the array (RandState[32], Q_Q10[32], etc.). It is doing this by deliberately running the sLPC_Q14 array index beyond the end of the array, i.e. it is RELYING on undefined behaviour. I'd have said your work-around solution is actually the better code.
[Bug c/59933] for loop goes wild with assert() enabled
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59933 Ian Hamilton ian at g0tcd dot com changed: What|Removed |Added CC||ian at g0tcd dot com --- Comment #2 from Ian Hamilton ian at g0tcd dot com --- I found a similar bug in 4.8.1. See bug report http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59982 You could try adding the -fno-aggressive-loop-optimizations flag and see if that fixes your problem with assert() enabled.
[Bug c/59933] for loop goes wild with assert() enabled
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59933 --- Comment #3 from Ian Hamilton ian at g0tcd dot com --- Just a thought. Does ((int)(sizeof(NSQ_del_dec_struct) / sizeof(opus_int32)) correctly give you the size of the sLPC_Q14 array? From playing with my test case, it seems that if the optimiser spots that k will go beyond the end of your array, it doesn't bother checking it each time round the loop, hence an infinte loop. Seems that gcc 4.8's aggressive loop optimiser also gives up aggressively when presented with invalid code.
[Bug c/59933] for loop goes wild with assert() enabled
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59933 --- Comment #1 from Mark Warner warnerme at ptd dot net --- Created attachment 31945 -- http://gcc.gnu.org/bugzilla/attachment.cgi?id=31945action=edit C source of subroutines which contain problem for loop This is a file from OPUS. As sent it can't be run, but the problem was bad enough that I thought looking at the generated code might be enough. All, except system, include files are inlined.
