Bug 82274 - __builtin_mul_overflow fails to detect overflow for int64_t when compiled with -m32
Summary: __builtin_mul_overflow fails to detect overflow for int64_t when compiled wit...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 5.4.0
: P3 normal
Target Milestone: 6.5
Assignee: Jakub Jelinek
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2017-09-21 00:01 UTC by Mats Palmgren
Modified: 2017-10-13 23:55 UTC (History)
3 users (show)

See Also:
Host:
Target: i?86-*-*
Build:
Known to work:
Known to fail: 5.1.0, 7.2.0
Last reconfirmed: 2017-09-21 00:00:00


Attachments
bug.cpp (275 bytes, text/x-csrc)
2017-09-21 00:02 UTC, Mats Palmgren
Details
gcc8-pr82274.patch (1.19 KB, patch)
2017-09-22 10:05 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mats Palmgren 2017-09-21 00:01:39 UTC
The result from this test should be:
# g++ -std=c++11 bug.cpp  && ./a.out
SUCCESS: overflow detected
Result is 0

but when compiled with -m32 it fails to detect the overflow:
# g++ -m32 -std=c++11 bug.cpp  && ./a.out
ERROR: failed to detect overflow!
Result is 0

However, adding -O makes it work again:
# g++ -O -m32 -std=c++11 bug.cpp  && ./a.out
SUCCESS: overflow detected
Result is 0

# g++ --version
g++ (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609

(same results with g++ (GCC) 7.2.0)
Comment 1 Mats Palmgren 2017-09-21 00:02:56 UTC
Created attachment 42214 [details]
bug.cpp
Comment 2 Richard Biener 2017-09-21 09:51:22 UTC
Confirmed.  Not a regression.
Comment 3 Jakub Jelinek 2017-09-21 10:25:10 UTC
Testcase that fails also on x86_64 -m64 (again, without optimizations only, with optimizations we fold it at compile time):

int
main ()
{
#ifdef __SIZEOF_INT128__
  __int128 m = -(((__int128) 1) << (__CHAR_BIT__ * __SIZEOF_INT128__ / 2));
  __int128 r;
#else
  long long m = -(1LL << (__CHAR_BIT__ * __SIZEOF_LONG_LONG__ / 2));
  long long r;
#endif
  if (!__builtin_mul_overflow (m, m, &r))
    __builtin_abort ();
  return 0;
}
Comment 4 Jakub Jelinek 2017-09-21 16:25:54 UTC
internal-fn.c implements here whatever we have written in libgcc2.c for __mulvti3 (for 64-bit arches) or __mulvdi3 (for 32-bit arches).
And it seems this is (hopefully the only) incorrectly handled special case.
That routine first handles the case where both operands are within range of sign-extended half-sized integers (never overflows), then when one of them is and the other is arbitrary (or vice versa, mul is commutative) - this one needs 2 multiplications and sometimes overflows, sometimes doesn't, then when both of them are within the range of max signed half type + 1 to max unsigned half type, then when one of them is in that range and other is in range of minus (max unsigned half type + 1) to (min signed half type - 1) and finally when both are of the latter kind (everything else is always overflow).
That last case is:
                  if (uu.s.high == (Wtype) -1 && vv.s.high == (Wtype) - 1)
                    {
                      DWunion ww = {.ll = (UDWtype) (UWtype) uu.s.low
                                    * (UDWtype) (UWtype) vv.s.low};

                      ww.s.high -= uu.s.low;
                      ww.s.high -= vv.s.low;
                      if (__builtin_expect (ww.s.high >= 0, 1))
                        return ww.ll;
                    }
If both uu.s.low == vv.s.low == 0, then we return 0 even when there is overflow.
Wonder if ww.s.high >= 0 && ww.ll wouldn't be sufficient.
Comment 5 Jakub Jelinek 2017-09-22 10:05:44 UTC
Created attachment 42224 [details]
gcc8-pr82274.patch

Untested fix.
Comment 6 Jakub Jelinek 2017-10-13 17:19:43 UTC
Author: jakub
Date: Fri Oct 13 17:19:12 2017
New Revision: 253734

URL: https://gcc.gnu.org/viewcvs?rev=253734&root=gcc&view=rev
Log:
	PR target/82274
	* internal-fn.c (expand_mul_overflow): If both operands have
	the same highpart of -1 or 0 and the topmost bit of lowpart
	is different, overflow is if res <= 0 rather than res < 0.

	* libgcc2.c (__mulvDI3): If both operands have
	the same highpart of -1 and the topmost bit of lowpart is 0,
	multiplication overflows even if both lowparts are 0.

	* gcc.dg/pr82274-1.c: New test.
	* gcc.dg/pr82274-2.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/pr82274-1.c
    trunk/gcc/testsuite/gcc.dg/pr82274-2.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/internal-fn.c
    trunk/gcc/testsuite/ChangeLog
    trunk/libgcc/ChangeLog
    trunk/libgcc/libgcc2.c
Comment 7 Jakub Jelinek 2017-10-13 17:27:00 UTC
Author: jakub
Date: Fri Oct 13 17:26:28 2017
New Revision: 253735

URL: https://gcc.gnu.org/viewcvs?rev=253735&root=gcc&view=rev
Log:
	PR target/82274
	* internal-fn.c (expand_mul_overflow): If both operands have
	the same highpart of -1 or 0 and the topmost bit of lowpart
	is different, overflow is if res <= 0 rather than res < 0.

	* libgcc2.c (__mulvDI3): If both operands have
	the same highpart of -1 and the topmost bit of lowpart is 0,
	multiplication overflows even if both lowparts are 0.

	* gcc.dg/pr82274-1.c: New test.
	* gcc.dg/pr82274-2.c: New test.

Added:
    branches/gcc-7-branch/gcc/testsuite/gcc.dg/pr82274-1.c
    branches/gcc-7-branch/gcc/testsuite/gcc.dg/pr82274-2.c
Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/internal-fn.c
    branches/gcc-7-branch/gcc/testsuite/ChangeLog
    branches/gcc-7-branch/libgcc/ChangeLog
    branches/gcc-7-branch/libgcc/libgcc2.c
Comment 8 Jakub Jelinek 2017-10-13 20:15:16 UTC
Author: jakub
Date: Fri Oct 13 20:14:34 2017
New Revision: 253743

URL: https://gcc.gnu.org/viewcvs?rev=253743&root=gcc&view=rev
Log:
	PR target/82274
	* internal-fn.c (expand_mul_overflow): If both operands have
	the same highpart of -1 or 0 and the topmost bit of lowpart
	is different, overflow is if res <= 0 rather than res < 0.

	* libgcc2.c (__mulvDI3): If both operands have
	the same highpart of -1 and the topmost bit of lowpart is 0,
	multiplication overflows even if both lowparts are 0.

	* gcc.dg/pr82274-1.c: New test.
	* gcc.dg/pr82274-2.c: New test.

Added:
    branches/gcc-6-branch/gcc/testsuite/gcc.dg/pr82274-1.c
    branches/gcc-6-branch/gcc/testsuite/gcc.dg/pr82274-2.c
Modified:
    branches/gcc-6-branch/gcc/ChangeLog
    branches/gcc-6-branch/gcc/internal-fn.c
    branches/gcc-6-branch/gcc/testsuite/ChangeLog
    branches/gcc-6-branch/libgcc/ChangeLog
    branches/gcc-6-branch/libgcc/libgcc2.c
Comment 9 Jakub Jelinek 2017-10-13 23:55:49 UTC
Should be fixed for 6.5+ and 7.3+/8+.