[Bug c/59933] New: for loop goes wild with assert() enabled

[Bug c/59933] New: for loop goes wild with assert() enabled

[warnerme at ptd dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59933

            Bug ID: 59933
           Summary: for loop goes wild with assert() enabled
           Product: gcc
           Version: 4.8.2
            Status: UNCONFIRMED
          Severity: major
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: warnerme at ptd dot net

I have this odd case where a for loop goes wild.
But it only fails when assert() is enabled.

gcc  -Iinc -g -O2 -DDEBUG -fstack-protector-all -W -Wstrict-prototypes -Wall
-Wextra -Wcast-align -Wnested-externs -Wshadow -c -o build/NSQ_del_dec.o
c/NSQ_del_dec.c

If -DNDEBUG is used, the code works fine, although there is a slight difference
between CYGWIN32 (4.8.2-2) and CYGWIN64 (4.8.2-1).  This was not a problem with
gcc 4.7.

        if (RDmin_Q10 < RDmax_Q10)
        {
#if 1
            /* THIS IS THE CODE THAT FAILS */
            for (k = i; k < (int)(sizeof(NSQ_del_dec_struct) /
sizeof(opus_int32)); ++k)
            {
                psDelDec[RDmax_ind].sLPC_Q14[k] =
psDelDec[RDmin_ind].sLPC_Q14[k];
            }
#else
            /* THIS IS THE WORK-AROUND */
            int n = (sizeof(NSQ_del_dec_struct) / sizeof(opus_int32)) - i;
            opus_int32 *src = &psDelDec[RDmin_ind].sLPC_Q14[i];
            opus_int32 *dst = &psDelDec[RDmax_ind].sLPC_Q14[i];
            while (n-- > 0)
                *dst++ = *src++;
#endif
            psSampleState[RDmax_ind][0] = psSampleState[RDmin_ind][1];
        }

I've tried lots of combinations of code to get the work around, and this even
fails when I insert printf-s, but the most common with this exact code is that
it does sizeof(NSQ_del_dec_struct) number of loops leaving out the /
sizeof(opus_int32).  I have had some test test where the loop didn't stop till
it destroyed enough stack to crash it.

Sorry, that the file is a bit big but trying to whittle it down usually made
the problem go away.

[Bug c/59933] for loop goes wild with assert() enabled

[warnerme at ptd dot net]

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=31945&action=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.

[Bug c/59933] for loop goes wild with assert() enabled

[ian at g0tcd dot com]

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

[ian at g0tcd dot com]

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

[mpolacek at gcc dot gnu.org]

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

[warnerme at ptd dot net]

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

[warnerme at ptd dot net]

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

[mpolacek at gcc dot gnu.org]

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

[jakub at gcc dot gnu.org]

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

[ian at g0tcd dot com]

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

[jakub at gcc dot gnu.org]

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

[jakub at gcc dot gnu.org]

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

[ian at g0tcd dot com]

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.