miscompilation of unsigned comparison on aarch64

Bug #1267761 reported by Matthias Klose on 2014-01-10
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Linaro GCC
Fix Released
Undecided
Michael Collison
gcc
Fix Released
Medium
gcc-4.8 (Ubuntu)
High
Unassigned
Trusty
High
Unassigned

Hi,

This slightly strangely written program (it's distilled down from frame_offset_overflow in the gcc source itself) should print "bigger" if the first argument is bigger than 10 (or negative, but let's ignore that please):

#include <stdlib.h>
#include <stdio.h>

int a[2] = { 10, 20 };

int
is_bigger (long offset, int index)
{
  unsigned long size = -offset;

  if (size > a[index])
    {
      printf("bigger\n");
      return 1;
    }

  return 0;
}

int
main (int argc, char** argv)
{
  long v;
  v = atol(argv[1]);
  is_bigger(-v, 0);
  return 0;
}

When compiled at -O1 or above (and with inlining disabled at -O2 and above), though, it bungles the 0 case:

(t-doko)mwhudson@arm64:~$ gcc-4.9 -O3 test.c -o test -fno-inline -Wall
(t-doko)mwhudson@arm64:~$ ./test 1
(t-doko)mwhudson@arm64:~$ ./test 11
bigger
(t-doko)mwhudson@arm64:~$ ./test 0
bigger
(t-doko)mwhudson@arm64:~$ gcc-4.9 -O0 test.c -o test -Wall
(t-doko)mwhudson@arm64:~$ ./test 1
(t-doko)mwhudson@arm64:~$ ./test 11
bigger
(t-doko)mwhudson@arm64:~$ ./test 0
(t-doko)mwhudson@arm64:~$

What's going on? Here's the disassembly of is_bigger (at O3):

0000000000400608 <is_bigger>:
  400608: b0000082 adrp x2, 411000 <_GLOBAL_OFFSET_TABLE_+0x28>
  40060c: 91010042 add x2, x2, #0x40
  400610: a9bf7bfd stp x29, x30, [sp,#-16]!
  400614: 52800003 mov w3, #0x0 // #0
  400618: 910003fd mov x29, sp
  40061c: b8a1d841 ldrsw x1, [x2,w1,sxtw #2]
  400620: ab00003f cmn x1, x0
  400624: 540000a2 b.cs 400638 <is_bigger+0x30>
  400628: 90000000 adrp x0, 400000 <_init-0x3f8>
  40062c: 911b6000 add x0, x0, #0x6d8
  400630: 97ffff90 bl 400470 <puts@plt>
  400634: 52800023 mov w3, #0x1 // #1
  400638: 2a0303e0 mov w0, w3
  40063c: a8c17bfd ldp x29, x30, [sp],#16
  400640: d65f03c0 ret

Basically it seems that the condition "-offset > val" is being compiled as "val + offset does not overflow", which is not valid for offset == 0.

In , Pinskia (pinskia) wrote :

(insn 14 13 15 2 (set (reg:CC_SWP 66 cc)
        (compare:CC_SWP (neg:DI (reg:DI 0 x0 [ offset ]))
            (reg:DI 1 x1 [orig:85 D.3895 ] [85]))) t7.c:11 114 {*compare_negdi}
     (expr_list:REG_DEAD (reg:DI 1 x1 [orig:85 D.3895 ] [85])
        (expr_list:REG_DEAD (reg:DI 0 x0 [ offset ])
            (nil))))

--- CUT ---
Here is a testcase that fails at -O1 and above without any arguments.

int a[2] = { 10, 20 };

int
is_bigger (long, int) __attribute__((noinline,noclone));

int
is_bigger (long offset, int index)
{
  unsigned long size = -offset;

  if (size > a[index])
   return 1;

  return 0;
}

int
main (int argc, char** argv)
{
  long v;
  if (is_bigger(0, 0))
    __builtin_abort ();
  if (!is_bigger(1, 0))
    __builtin_abort ();
  if (is_bigger(-10, 0))
    __builtin_abort ();
  if (!is_bigger(10, 0))
    __builtin_abort ();
  return 0;
}

Matthias Klose (doko) wrote :

Michael, is this seen with the gcc-4.8 linaro as well?

Yes, I found it there first
On 10 Jan 2014 22:40, "Matthias Klose" <email address hidden> wrote:

> Michael, is this seen with the gcc-4.8 linaro as well?
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1267761
>
> Title:
> miscompilation of unsigned comparison on aarch64
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/gcc/+bug/1267761/+subscriptions
>

Matthias Klose (doko) on 2014-01-10
Changed in gcc-4.8 (Ubuntu Trusty):
importance: Undecided → High
milestone: none → ubuntu-14.01
status: New → Confirmed
Changed in gcc:
importance: Unknown → Medium
status: Unknown → Confirmed

Author: rearnsha
Date: Fri Jan 10 15:21:21 2014
New Revision: 206530

URL: http://gcc.gnu.org/viewcvs?rev=206530&root=gcc&view=rev
Log:
PR target/59744
Fix ChangeLog typos in previous commit (r206529).

Modified:
    trunk/gcc/ChangeLog

Fixed on trunk

Changed in gcc:
status: Confirmed → Fix Released

Hi, thanks for the super fast fix. Could it be backported to 4.8? git cherry-pick gives a conflict in aarch64.md which is probably easy to fix if you know how this code works:

(define_insn "*compare_neg<mode>"
<<<<<<< HEAD
  [(set (reg:CC CC_REGNUM)
 (compare:CC
  (match_operand:GPI 0 "register_operand" "r")
  (neg:GPI (match_operand:GPI 1 "register_operand" "r"))))]
||||||| parent of 46b590a... PR target/9744
  [(set (reg:CC_SWP CC_REGNUM)
 (compare:CC_SWP
  (neg:GPI (match_operand:GPI 0 "register_operand" "r"))
  (match_operand:GPI 1 "register_operand" "r")))]
=======
  [(set (reg:CC_Z CC_REGNUM)
 (compare:CC_Z
  (neg:GPI (match_operand:GPI 0 "register_operand" "r"))
  (match_operand:GPI 1 "register_operand" "r")))]
>>>>>>> 46b590a... PR target/9744
  ""
  "cmn\\t%<w>0, %<w>1"
  [(set_attr "v8type" "alus")
   (set_attr "mode" "<MODE>")]
)

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package gcc-4.8 - 4.8.2-13ubuntu1

---------------
gcc-4.8 (4.8.2-13ubuntu1) trusty; urgency=medium

  * Merge with Debian; remaining changes:
    - Build from the upstream source.

gcc-4.8 (4.8.2-13) unstable; urgency=medium

  * Update to SVN 20140112 (r206564) from the gcc-4_8-branch.
    - Fix miscompilation due to wrong RTL-optimization (see
      PR rtl-optimization/59137). Closes: #716635.

  [ Aurelien Jarno ]
  * Reenable the testsuite on mips.

  [ Matthias Klose ]
  * Add libgcc, libstdc++, libgfortran symbols files for ppc64el.
  * Update libitm symbols for non-default multilibs.
  * Add libgcc, libstdc++, libgfortran symbols files for arm64.
  * Fix PR target/59588 (AArch64), backport proposed patch. LP: #1263576.
  * Fix PR target/59744 (AArch64), taken from the trunk. LP: #1267761.
 -- Matthias Klose <email address hidden> Sun, 12 Jan 2014 14:16:19 +0100

Changed in gcc-4.8 (Ubuntu Trusty):
status: Confirmed → Fix Released

I don't expect that pattern to match on 4.8 (the pattern is not canonical form), which is why this wasn't seen before.

If you can find a test-case that triggers on that branch, I'll do a back-port; otherwise, there's no point.

I couldn't reproduce the failure using 4.8.

I saw the problem with Linaro GCC 4.8, but haven't tried the vanilla 4.8 branch. If the committed test case doesn't fail, I'll believe that it is not a problem. Sorry for the noise.

Changed in gcc-linaro:
assignee: nobody → Michael Collison (michael-collison)
In , Yroux (yroux) wrote :

Author: yroux
Date: Mon Aug 11 22:08:03 2014
New Revision: 213842

URL: https://gcc.gnu.org/viewcvs?rev=213842&root=gcc&view=rev
Log:
gcc/
2014-08-11 Michael Collison <email address hidden>

 Backport from trunk r206529, r206530
 2014-01-10 Richard Earnshaw <email address hidden>

 PR target/59744
 * aarch64-modes.def (CC_Zmode): New flags mode.
 * aarch64.c (aarch64_select_cc_mode): Only allow NEG when the condition
 represents an equality.
 (aarch64_get_condition_code): Handle CC_Zmode.
 * aarch64.md (compare_neg<mode>): Restrict to equality operations.

gcc/testsuite/
2014-08-11 Michael Collison <email address hidden>

 Backport from trunk r206529
 2014-01-10 Richard Earnshaw <email address hidden>

 PR target/59744
 * gcc.target/aarch64/cmn-neg.c: Use equality comparisons.
 * gcc.target/aarch64/cmn-neg2.c: New test.

Added:
    branches/linaro/gcc-4_8-branch/gcc/testsuite/gcc.target/aarch64/cmn-neg2.c
Modified:
    branches/linaro/gcc-4_8-branch/gcc/ChangeLog.linaro
    branches/linaro/gcc-4_8-branch/gcc/config/aarch64/aarch64-modes.def
    branches/linaro/gcc-4_8-branch/gcc/config/aarch64/aarch64.c
    branches/linaro/gcc-4_8-branch/gcc/config/aarch64/aarch64.md
    branches/linaro/gcc-4_8-branch/gcc/testsuite/ChangeLog.linaro
    branches/linaro/gcc-4_8-branch/gcc/testsuite/gcc.target/aarch64/cmn-neg.c

Changed in gcc-linaro:
status: New → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.