Bug 81088 - UBSAN: false positive as a result of reassosiation
Summary: UBSAN: false positive as a result of reassosiation
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 8.0
: P3 normal
Target Milestone: 7.3
Assignee: Richard Biener
URL:
Keywords: wrong-code
Depends on:
Blocks: yarpgen
  Show dependency treegraph
 
Reported: 2017-06-13 21:30 UTC by Dmitry Babokin
Modified: 2021-11-01 23:07 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work: 7.2.1, 8.0
Known to fail: 7.1.0, 7.2.0
Last reconfirmed: 2017-06-14 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Babokin 2017-06-13 21:30:55 UTC
gcc rev249171.

> cat f.cpp
short s = 2;
short y = 1;
int i;
int main() {
  i = -(s + int(~unsigned(0 / y))) + 0x7fffffff;
  return 0;
}

> g++ -fsanitize=undefined -O0 f.cpp -o out
> ./out
f.cpp:5:36: runtime error: signed integer overflow: -2 + -2147483648 cannot be represented in type 'int'
Comment 1 Richard Biener 2017-06-14 08:23:26 UTC
We associate to

(1 - (int) s) + 2147483647

without -fsanitize=undefined which is fine.  But with -fsanitize=undefined
we end up with:

;; Function main (null)
;; enabled by -tree-original


{
  i = if (SAVE_EXPR <(int) y>;, SAVE_EXPR <(int) y> == 0 || (SAVE_EXPR <(int) y>;, 0);)
    {
      __builtin___ubsan_handle_divrem_overflow (&*.Lubsan_data0, 0, (unsigned long) SAVE_EXPR <(int) y>);
    }
  else
    {
      <<< Unknown tree: void_cst >>>
    }, SAVE_EXPR <(int) y>;, -(int) s + -2147483648(OVF);;
  return 0;
}


this goes wrong somewhere when fully folding

-((int) s + (int) ~(if (SAVE_EXPR <(int) y>;, SAVE_EXPR <(int) y> == 0 || (SAVE_EXPR <(int) y>;, 0);)
  {
    __builtin___ubsan_handle_divrem_overflow (&*.Lubsan_data0, 0, (unsigned long) SAVE_EXPR <(int) y>);
  }
else
  {
    <<< Unknown tree: void_cst >>>
  }, (unsigned int) (0 / SAVE_EXPR <(int) y>);)) + 2147483647
Comment 2 Richard Biener 2017-06-14 08:27:57 UTC
Testcase that goes wrong without -fsanitize=undefined:

short s = 2;
short y = 1;
int i;
int main() {
    i = -(s + (int)(~( y == 0 ? 1 : 0 , (unsigned)(0/y)))) + 0x7fffffff;
    return 0;
}

folded to:

;; Function main (null)
;; enabled by -tree-original


{
  i = y == 0;, -2147483648(OVF) - (int) s;;
  return 0;
}
Comment 3 Richard Biener 2017-06-14 08:55:34 UTC
And the issue is that

associate:
...
          /* Only do something if we found more than two objects.  Otherwise,
             nothing has changed and we risk infinite recursion.  */
          if (ok
              && (2 < ((var0 != 0) + (var1 != 0)
                       + (con0 != 0) + (con1 != 0)
                       + (lit0 != 0) + (lit1 != 0)
                       + (minus_lit0 != 0) + (minus_lit1 != 0))))
            {
              bool any_overflows = false;
              if (lit0) any_overflows |= TREE_OVERFLOW (lit0);
              if (lit1) any_overflows |= TREE_OVERFLOW (lit1);
...
              /* Don't introduce overflows through reassociation.  */
              if (!any_overflows
                  && ((lit0 && TREE_OVERFLOW_P (lit0))
                      || (minus_lit0 && TREE_OVERFLOW_P (minus_lit0))))
                return NULL_TREE;

fails to see the overflow when associating

  (1(OVF) - (int) s) + 2147483647

the 1(OVF) is from (int)0xffffffffU which is implementation defined but
IIRC the FEs require the TREE_OVERFLOW flag to be set.

IMHO this any_overflows code is simply bogus.  To avoid dropping useful
association split_tree should drop pre-existing TREE_OVERFLOW.
Comment 4 Richard Biener 2017-06-14 11:40:51 UTC
Author: rguenth
Date: Wed Jun 14 11:40:20 2017
New Revision: 249192

URL: https://gcc.gnu.org/viewcvs?rev=249192&root=gcc&view=rev
Log:
2017-06-14  Richard Biener  <rguenther@suse.de>

	PR middle-end/81088
	* fold-const.c (split_tree): Drop TREE_OVERFLOW flag from
	literal constants.
	(fold_binary_loc): When associating do not treat pre-existing
	TREE_OVERFLOW on literal constants as a reason to allow
	TREE_OVERFLOW on associated literal constants.

	* c-c++-common/ubsan/pr81088.c: New testcase.

Added:
    trunk/gcc/testsuite/c-c++-common/ubsan/pr81088.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/fold-const.c
    trunk/gcc/testsuite/ChangeLog
Comment 5 Richard Biener 2017-06-14 11:44:03 UTC
Fixed on trunk sofar.
Comment 6 Richard Biener 2017-08-28 12:50:27 UTC
Author: rguenth
Date: Mon Aug 28 12:49:55 2017
New Revision: 251381

URL: https://gcc.gnu.org/viewcvs?rev=251381&root=gcc&view=rev
Log:
2017-08-28  Richard Biener  <rguenther@suse.de>

	Backport from mainline
	2017-06-14  Richard Biener  <rguenther@suse.de>

	PR middle-end/81088
	* fold-const.c (split_tree): Drop TREE_OVERFLOW flag from
	literal constants.
	(fold_binary_loc): When associating do not treat pre-existing
	TREE_OVERFLOW on literal constants as a reason to allow
	TREE_OVERFLOW on associated literal constants.

	* c-c++-common/ubsan/pr81088.c: New testcase.

	2017-06-13  Richard Biener  <rguenther@suse.de>

	PR middle-end/81065
	* fold-const.c (extract_muldiv_1): Remove bogus distribution
	case of C * (x * C2 + C3).
	(fold_addr_of_array_ref_difference): Properly fold index difference.

	* c-c++-common/ubsan/pr81065.c: New testcase.

	2017-06-08  Marek Polacek  <polacek@redhat.com>

	PR sanitize/80932
	* c-c++-common/ubsan/pr80932.c: Test with ints, not with long ints.

	2017-06-07  Marek Polacek  <polacek@redhat.com>

	PR sanitizer/80932
	* fold-const.c (extract_muldiv_1) <case MINUS_EXPR>: Add
	TYPE_OVERFLOW_WRAPS check. 

	* c-c++-common/ubsan/pr80932.c: New test.

Added:
    branches/gcc-7-branch/gcc/testsuite/c-c++-common/ubsan/pr80932.c
    branches/gcc-7-branch/gcc/testsuite/c-c++-common/ubsan/pr81065.c
    branches/gcc-7-branch/gcc/testsuite/c-c++-common/ubsan/pr81088.c
Modified:
    branches/gcc-7-branch/gcc/ChangeLog
    branches/gcc-7-branch/gcc/fold-const.c
    branches/gcc-7-branch/gcc/testsuite/ChangeLog
Comment 7 Richard Biener 2017-08-28 12:51:12 UTC
Fixed for GCC 7.3+.