[Bug c/66090] New: Wrong loop code generation with -O2 on ARM

[Bug c/66090] New: Wrong loop code generation with -O2 on ARM

[christian.prochaska@genode-labs.com]

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

            Bug ID: 66090
           Summary: Wrong loop code generation with -O2 on ARM
           Product: gcc
           Version: 4.9.2
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: christian.prochaska@genode-labs.com
  Target Milestone: ---

test.c:

void func()
{
    unsigned int i;

    unsigned int *ptr = (unsigned int*)0xfffff000;

    for (i = 0; i < 1024; i++)
        *(ptr++) = 0;
}

Compiled with GCC 4.9.2 from the Xubuntu 15.04 repository:

arm-linux-gnueabihf-gcc-4.9 -marm -c -O2 test.c

Generated code:

00000000 <func>:
func():
   0:   e3a03a0f        mov     r3, #61440      ; 0xf000
   4:   e3a02000        mov     r2, #0
   8:   e34f3fff        movt    r3, #65535      ; 0xffff
   c:   e4832004        str     r2, [r3], #4
  10:   eafffffd        b       c <func+0xc>

There is no check of the loop exit condition.

When compiling with -O1, it is there:

00000000 <func>:
func():
   0:   e3a03a0f        mov     r3, #61440      ; 0xf000
   4:   e34f3fff        movt    r3, #65535      ; 0xffff
   8:   e3a02000        mov     r2, #0
   c:   e4832004        str     r2, [r3], #4
  10:   e3530000        cmp     r3, #0
  14:   1afffffc        bne     c <func+0xc>
  18:   e12fff1e        bx      lr

The problem does not occur with GCC 4.8.4.

[Bug c/66090] Wrong loop code generation with -O2 on ARM

[kugan at gcc dot gnu.org]

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

kugan at gcc dot gnu.org changed:

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

--- Comment #1 from kugan at gcc dot gnu.org ---
trunk also has the same issue and ivopt seems to be wrong here:

in test.c.135t.cunroll:

  # ivtmp_2 = PHI <ivtmp_1(4), 1024(2)>
  ptr_5 = ptr_12 + 4;
  *ptr_12 = 0;
  i_7 = i_11 + 1;
  ivtmp_1 = ivtmp_2 - 1;
  if (ivtmp_1 != 0)


In test.c.138t.ivopts:

  # ptr_12 = PHI <ptr_5(4), 4294963200B(2)>
  ptr_5 = ptr_12 + 4;
  MEM[base: ptr_5, offset: 4294967292B] = 0;
  if (ptr_5 != 0B)

In vrp, ptr_5 is ~[0B, 0B] and hence the condition is eliminated but the
problem was introduced in ivopt.

[Bug c/66090] Wrong loop code generation with -O2 on ARM

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Since the pointer is wrapping to null, you need to use
-fno-delete-null-pointer-checks to disable removing of checks of null pointers
or rather it enables allowing pointers to wrap to become null.
In C, once a pointer is non-null, it can't become null by adding a value to it.
 So GCC enables an optimization around that case (and
fno-delete-null-pointer-checks disables that optimization).

So no GCC bug, just wrongly assuming pointers can't become null pointers if
they were not null pointers.

[Bug c/66090] Wrong loop code generation with -O2 on ARM

[rearnsha at gcc dot gnu.org]

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

Richard Earnshaw <rearnsha at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
                 CC|                            |rguenth at gcc dot gnu.org
         Resolution|INVALID                     |---

--- Comment #3 from Richard Earnshaw <rearnsha at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #2)
> Since the pointer is wrapping to null, you need to use
> -fno-delete-null-pointer-checks to disable removing of checks of null
> pointers or rather it enables allowing pointers to wrap to become null.
> In C, once a pointer is non-null, it can't become null by adding a value to
> it.  So GCC enables an optimization around that case (and
> fno-delete-null-pointer-checks disables that optimization).
> 
> So no GCC bug, just wrongly assuming pointers can't become null pointers if
> they were not null pointers.

I'm inclined to disagree.  

Firstly, the code does not check for a null pointer, it checks a loop iterator
variable.

Secondly, although the pointer value wraps to zero, that value is never
dereferenced; the last memory access is at 0xfffffffc.

So I think the compiler is incorrectly removing a valid termination check.

Richi, I'd like your opinion on this one.

[Bug c/66090] Wrong loop code generation with -O2 on ARM

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |arm, i?86-*-*

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
Reproducible on i?86 as well, btw.

[Bug c/66090] Wrong loop code generation with -O2 on ARM

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
The testcase computes ptr + 1024 which evaluates to zero (even without IVOPTs
coming into play).  This invokes undefined behavior as even if you consider
ptr + 1024 being "one after the last array element and not dereferenced"
(the NULL pointer is special in C, if you want to lift this restriction you
need to provide -fno-delete-null-pointer-checks).

Joseph may be able to provide more C standard legalise wording here.  But it's
certainly how the middle-end works.

[Bug c/66090] Wrong loop code generation with -O2 on ARM

[joseph at codesourcery dot com]

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

--- Comment #6 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
I think this comes under tracking pointer provenance (DR#260) and saying 
that certain arithmetic on pointers derived by casts from integers has 
undefined behavior.

[Bug c/66090] Wrong loop code generation with -O2 on ARM

[wad at infinet dot ru]

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

Andrew <wad at infinet dot ru> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |wad at infinet dot ru

--- Comment #7 from Andrew <wad at infinet dot ru> ---

IMHO

"So no GCC bug, just wrongly assuming pointers can't become null pointers if
they were not null pointers."

Nevertheless, that is no reason to generate an INFINITY LOOP, which will be
cleaned all the memory, including the NULL address.

[Bug c/66090] Wrong loop code generation with -O2 on ARM

[ktkachov at gcc dot gnu.org]

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

ktkachov at gcc dot gnu.org changed:

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

--- Comment #8 from ktkachov at gcc dot gnu.org ---
(In reply to Andrew from comment #7)

> Nevertheless, that is no reason to generate an INFINITY LOOP, which will be
> cleaned all the memory, including the NULL address.

If the program has undefined behaviour, then the compiler makes no guarantees
at all, it's allowed to do pretty much anything at any part of the program

[Bug c/66090] Wrong loop code generation with -O2 on ARM

[pinskia at gcc dot gnu.org]

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

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew from comment #7)
> IMHO
> 
> "So no GCC bug, just wrongly assuming pointers can't become null pointers if
> they were not null pointers."
> 
> Nevertheless, that is no reason to generate an INFINITY LOOP, which will be
> cleaned all the memory, including the NULL address.

But undefined behavior can do anything.  And this is just undefined behavior.

[Bug c/66090] Wrong loop code generation with -O2 on ARM

[wad at infinet dot ru]

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

--- Comment #10 from Andrew <wad at infinet dot ru> ---
(In reply to Christian Prochaska from comment #0)
> test.c:
> 
> void func()
> {
>     unsigned int i;
> 
>     unsigned int *ptr = (unsigned int*)0xfffff000;
> 
>     for (i = 0; i < 1024; i++)
>         *(ptr++) = 0;
> }

There is no attempts to check pointer for NULL.
But, this is hidden attempt to substitute one test (i < 1024) to another (ptr
!= NULL), which will be the result of unexpected "always true".

And by the way:

void *ptr = malloc(1024);

if (ptr == NULL) /* always false ? Never panic()? */
  panic();

[Bug c/66090] Wrong loop code generation with -O2 on ARM

[mpolacek at gcc dot gnu.org]

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

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 #11 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Seems like the conclusion is that this is just UB in the code.  Closing thus.