[Bug middle-end/95903] New: gcc 10: wrong code with -fwrapv

[Bug middle-end/95903] New: gcc 10: wrong code with -fwrapv

[markus at oberhumer dot com]

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

            Bug ID: 95903
           Summary: gcc 10: wrong code with -fwrapv
           Product: gcc
           Version: 10.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: markus at oberhumer dot com
  Target Milestone: ---

Link at Compiler Explorer: https://godbolt.org/z/VgNLcX


The following 2 functions should generate identical code when using "-fwrapv",
but
the second function is missing the sign extension after the add.


// compile with: gcc-10 -m64 -O2 -fwrapv

char get_byte_use_add(const char* ptr, int off)
{
    off += 1;
    return ptr[off];
}

char get_byte_use_plus(const char* ptr, int off)
{
    return ptr[off + 1]; // gcc: BUG
}

[Bug middle-end/95903] gcc 10: wrong code with -fwrapv

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org
   Last reconfirmed|                            |2020-06-26
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
We've been doing that since forever (e.g. 3.2 behaves the same), but for
-fwrapv it is indeed not a valid optimization.

[Bug middle-end/95903] gcc 10: wrong code with -fwrapv

[markus at oberhumer dot com]

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

--- Comment #2 from Markus F.X.J. Oberhumer <markus at oberhumer dot com> ---
Somewhat related: the same code compiled with arm64-gcc -mabi=ilp32 -frwapv
does miscompile *both* functions.

See https://gcc.godbolt.org/z/uxDAtx

Should I open a new issue for this?

[Bug middle-end/95903] gcc 10: wrong code with -fwrapv

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For ilp32 I'm not convinced it is a bug, the address space needs to be 32-bit
only and for this to cause problems, you need a 4GB+ element array with the
pointer pointing into the middle of it such that both ptr[INT_MAX] and
ptr[INT_MIN] are both valid.  I don't think that is possible for ilp32.

[Bug middle-end/95903] gcc 10: wrong code with -fwrapv

[markus at oberhumer dot com]

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

--- Comment #4 from Markus F.X.J. Oberhumer <markus at oberhumer dot com> ---
Yes, ilp32 might be a corner case.

Still, clang-10 does create the expected code.

See https://gcc.godbolt.org/z/aEokbX for

  clang-10 -target arm64_32-ios -O2 -fomit-frame-pointer -fwrapv

and 

  clang-10 -target arm64_32-ios -O2 -fomit-frame-pointer -fno-wrapv

[Bug middle-end/95903] gcc 10: wrong code with -fwrapv

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 48788
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48788&action=edit
gcc11-pr95903.patch

Untested fix.  I don't really care what clang generates or what you find
expected on ilp32 targets, what matters is if what GCC generates is fast, and
unless somebody finds a wrong-code issue with that, it doesn't need to be
changed.
For the lp64 case, I can imagine a testcase, though it will be pretty rare in
practice and we need to slow down -fwrapv generated code because of that (isn't
e.g. Linux kernel built with -fwrapv?), just don't want to add it into the
testsuite as that would require allocating at least 4GB of memory.

On the other side, on:
extern char arr[];

char
baz (int off)
{
  off += 1;
  return arr[off];
}

char
qux (int off)
{
  return arr[off + 1];
}
we do generate the addition and then sign extension, and we should not, because
we would get better code otherwise.  arr[INT_MIN] is in this case certainly
invalid.  But that is about different code.

[Bug middle-end/95903] gcc 10: wrong code with -fwrapv

[markus at oberhumer dot com]

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

--- Comment #6 from Markus F.X.J. Oberhumer <markus at oberhumer dot com> ---
Thanks for the quick fix!

And no need to be grumpy, I'm just trying to nail down those pesky edge
cases...

As for ILP32, here is another suspicious test case, now only using just a
little bit more than 2 GiB of memory: https://gcc.godbolt.org/z/mRf2kd

// arm64-linux-gcc -mabi=ilp32 -O2 -fwrapv -fwrapv-pointer

char get_byte(const char* ptr, int off)
{
    off += 2147483647;
    return ptr[off];
}

[Bug middle-end/95903] gcc 10: wrong code with -fwrapv

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I believe all arrays larger than half of the address space are outside of the
standard already, one can't perform e.g. address arithmetics on those because
it overflows the ptrdiff_t in which it is evaluated.
It has been discussed in other PRs.
I don't know about aarch64 ilp32, but for several arches with 64-bit hw
pointers either the hw has some addressing mode that ignores the upper bits
(e.g. zero or sign extends addresses during access), or often the AS is even
limited to just half of the size, so that both sign and zero extension behave
the same.
If you believe there is a problem for aarch64, please file a separate PR and
the people familiar with that target needs to discuss it, certainly it is
unrelated to this, because at GIMPLE the sources just contain pointers and
pointers are 32-bit for these, so there is no extension at that level.  It is
just the backend driven extensions depending on POINTERS_EXTEND_UNSIGNED (not
defined at all, the usual case when ptr_mode == Pmode, 1 for zero extension, 0
for sign extension and -1 for special instructions to perform it).

[Bug middle-end/95903] gcc 10: wrong code with -fwrapv

[markus at oberhumer dot com]

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

--- Comment #8 from Markus F.X.J. Oberhumer <markus at oberhumer dot com> ---
Got it, thanks!

[Bug middle-end/95903] gcc 10: wrong code with -fwrapv

[markus at oberhumer dot com]

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

--- Comment #9 from Markus F.X.J. Oberhumer <markus at oberhumer dot com> ---
[ Just for reference, created bug 95908 ]

[Bug middle-end/95903] gcc 10: wrong code with -fwrapv

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:37995960984ea2222346dd9d168d332cd6f7adf0

commit r11-1685-g37995960984ea2222346dd9d168d332cd6f7adf0
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Jun 27 12:38:23 2020 +0200

    c-family: Use TYPE_OVERFLOW_UNDEFINED instead of !TYPE_UNSIGNED in
pointer_sum [PR95903]

    For lp64 targets and int off ... ptr[off + 1]
    is lowered in pointer_sum to *(ptr + ((sizetype) off + (sizetype) 1)).
    That is fine when signed integer wrapping is undefined (and is not done
    already if off has unsigned type), but changes behavior for -fwrapv, where
    overflow is well defined.  Runtime test could be:
    int
    main ()
    {
      char *p = __builtin_malloc (0x100000000UL);
      if (!p) return 0;
      char *q = p + 0x80000000UL;
      int o = __INT_MAX__;
      q[o + 1] = 1;
      if (q[-__INT_MAX__ - 1] != 1) __builtin_abort ();
      return 0;
    }
    with -fwrapv or so, not included in the testsuite because it requires 4GB
    allocation (with some other test it would be enough to have something
    slightly above 2GB, but still...).

    2020-06-27  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/95903
    gcc/c-family/
            * c-common.c (pointer_int_sum): Use TYPE_OVERFLOW_UNDEFINED instead
of
            !TYPE_UNSIGNED check to see if we can apply distributive law and
handle
            smaller precision intop operands separately.
    gcc/testsuite/
            * c-c++-common/pr95903.c: New test.

[Bug middle-end/95903] gcc 10: wrong code with -fwrapv

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:e29959c5fcd6c37b426c2939d7ba0c199b40d1ac

commit r10-8384-ge29959c5fcd6c37b426c2939d7ba0c199b40d1ac
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Jun 27 12:38:23 2020 +0200

    c-family: Use TYPE_OVERFLOW_UNDEFINED instead of !TYPE_UNSIGNED in
pointer_sum [PR95903]

    For lp64 targets and int off ... ptr[off + 1]
    is lowered in pointer_sum to *(ptr + ((sizetype) off + (sizetype) 1)).
    That is fine when signed integer wrapping is undefined (and is not done
    already if off has unsigned type), but changes behavior for -fwrapv, where
    overflow is well defined.  Runtime test could be:
    int
    main ()
    {
      char *p = __builtin_malloc (0x100000000UL);
      if (!p) return 0;
      char *q = p + 0x80000000UL;
      int o = __INT_MAX__;
      q[o + 1] = 1;
      if (q[-__INT_MAX__ - 1] != 1) __builtin_abort ();
      return 0;
    }
    with -fwrapv or so, not included in the testsuite because it requires 4GB
    allocation (with some other test it would be enough to have something
    slightly above 2GB, but still...).

    2020-06-27  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/95903
    gcc/c-family/
            * c-common.c (pointer_int_sum): Use TYPE_OVERFLOW_UNDEFINED instead
of
            !TYPE_UNSIGNED check to see if we can apply distributive law and
handle
            smaller precision intop operands separately.
    gcc/testsuite/
            * c-c++-common/pr95903.c: New test.

    (cherry picked from commit 37995960984ea2222346dd9d168d332cd6f7adf0)

[Bug middle-end/95903] gcc 10: wrong code with -fwrapv

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:a1eb6e41832816b671d3b945014f7e8255167470

commit r9-8902-ga1eb6e41832816b671d3b945014f7e8255167470
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Jun 27 12:38:23 2020 +0200

    c-family: Use TYPE_OVERFLOW_UNDEFINED instead of !TYPE_UNSIGNED in
pointer_sum [PR95903]

    For lp64 targets and int off ... ptr[off + 1]
    is lowered in pointer_sum to *(ptr + ((sizetype) off + (sizetype) 1)).
    That is fine when signed integer wrapping is undefined (and is not done
    already if off has unsigned type), but changes behavior for -fwrapv, where
    overflow is well defined.  Runtime test could be:
    int
    main ()
    {
      char *p = __builtin_malloc (0x100000000UL);
      if (!p) return 0;
      char *q = p + 0x80000000UL;
      int o = __INT_MAX__;
      q[o + 1] = 1;
      if (q[-__INT_MAX__ - 1] != 1) __builtin_abort ();
      return 0;
    }
    with -fwrapv or so, not included in the testsuite because it requires 4GB
    allocation (with some other test it would be enough to have something
    slightly above 2GB, but still...).

    2020-06-27  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/95903
    gcc/c-family/
            * c-common.c (pointer_int_sum): Use TYPE_OVERFLOW_UNDEFINED instead
of
            !TYPE_UNSIGNED check to see if we can apply distributive law and
handle
            smaller precision intop operands separately.
    gcc/testsuite/
            * c-c++-common/pr95903.c: New test.

    (cherry picked from commit 37995960984ea2222346dd9d168d332cd6f7adf0)

[Bug middle-end/95903] gcc 10: wrong code with -fwrapv

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For 8.5 fixed in r8-10507-g111d3ebb356dd9f08bb0f9d0ad1044dd67a8c16c and for
9.4+ by the above commit.