Bug 81097 - UBSAN: false positive for not existing negation operator and a bogus message
Summary: UBSAN: false positive for not existing negation operator and a bogus message
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: sanitizer (show other bugs)
Version: 8.0
: P3 normal
Target Milestone: ---
Assignee: Richard Biener
URL:
Keywords:
Depends on:
Blocks: yarpgen
  Show dependency treegraph
 
Reported: 2017-06-14 20:20 UTC by Dmitry Babokin
Modified: 2021-11-01 23:07 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-06-15 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-14 20:20:49 UTC
gcc rev249202.

Test case doesn't contain negations, but ubsan claims that negation has a problem.

Error message also contains reference to '<unknown>' type. Though problems seem to be related.

> cat f.cpp
unsigned int a = 3309568;
unsigned int b = -1204857327;
short c = -10871;
short x;
int main() {
  x = (short(~a) | ~c) +  (short(~b) | ~c);
  return 0;
}

> g++ -fsanitize=undefined -O0 f.cpp -o out
> ./out
f.cpp:6:18: runtime error: negation of -32768 cannot be represented in type '<unknown>'; cast to an unsigned type to negate this value to itself
Comment 1 Marek Polacek 2017-06-15 09:51:13 UTC
COnfirmed.  I'll take a look.  Some folding running of the rails again.
Comment 2 Richard Biener 2017-06-20 08:50:35 UTC
The negation is emitted from

#1  0x0000000000d6fb2f in split_tree (loc=2147483652, 
    in=<bit_not_expr 0x7ffff69f9920>, type=<integer_type 0x7ffff69e5348>, 
    code=PLUS_EXPR, conp=0x7fffffffc758, litp=0x7fffffffc750, 
    minus_litp=0x7fffffffc748, negate_p=0)
    at /space/rguenther/src/svn/early-lto-debug/gcc/fold-const.c:858
858           var = negate_expr (TREE_OPERAND (in, 0));
(gdb) l
853                && code == PLUS_EXPR)
854         {
855           /* -X - 1 is folded to ~X, undo that here.  Do _not_ do this
856              when IN is constant.  */
857           *minus_litp = build_one_cst (TREE_TYPE (in));
858           var = negate_expr (TREE_OPERAND (in, 0));

when folding

  (unsigned short) ~((signed short) a & (signed short) c)
+ (unsigned short) ~((signed short) b & (signed short) c)

so first-hand the error is obvious - we do not convert to 'type' before
negating.

But!  I think that may be not enough to fix things (it fixes this testcase)
as we generally cannot fold ~x to -x - 1 for types with undefined overflow.
Comment 3 Marek Polacek 2017-06-20 09:31:37 UTC
Well, my fix was

--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -850,7 +850,8 @@ split_tree (location_t loc, tree in, tree type, enum tree_code code,
   else if (TREE_CONSTANT (in))
     *conp = in;
   else if (TREE_CODE (in) == BIT_NOT_EXPR
-      && code == PLUS_EXPR)
+      && code == PLUS_EXPR
+      && negate_expr_p (TREE_OPERAND (in, 0)))
     {
       /* -X - 1 is folded to ~X, undo that here.  Do _not_ do this
          when IN is constant.  */

but it has some fallout it seems.

Also, the comment says
-X - 1 is folded to ~X, undo that here.
but there was not -X - 1, the bit not came from
~x | ~y -> ~(x & y)
in match.pd.
Comment 4 rguenther@suse.de 2017-06-20 09:53:07 UTC
On Tue, 20 Jun 2017, mpolacek at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81097
> 
> --- Comment #3 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
> Well, my fix was
> 
> --- a/gcc/fold-const.c
> +++ b/gcc/fold-const.c
> @@ -850,7 +850,8 @@ split_tree (location_t loc, tree in, tree type, enum
> tree_code code,
>    else if (TREE_CONSTANT (in))
>      *conp = in;
>    else if (TREE_CODE (in) == BIT_NOT_EXPR
> -      && code == PLUS_EXPR)
> +      && code == PLUS_EXPR
> +      && negate_expr_p (TREE_OPERAND (in, 0)))
>      {
>        /* -X - 1 is folded to ~X, undo that here.  Do _not_ do this
>           when IN is constant.  */
> 
> but it has some fallout it seems.
> 
> Also, the comment says
> -X - 1 is folded to ~X, undo that here.
> but there was not -X - 1, the bit not came from
> ~x | ~y -> ~(x & y)
> in match.pd.

Yeah, it's not "undoing" but trying to cleverly combine ~x + 1 to x.
Possibly the fallout testcases you've seen or some archeology will tell.

I think it's really trying to "undo" this transform AFAIR (I remember
adding this code).

Using negate_expr_p is checking for "easily negatable", you could
use a TYPE_OVERFLOW_UNDEFINED check but that would have similar
fallout.  Or just force the negation to use an unsigned type
(my patch does this for the testcase), but I'm not even sure that
will work because if the original expr is int then ~x + ~x
becomes (int)(-(unsigned)x - (unsigned)x) - 2 and supposedly we can
craft x to be so that (int)(-(unsigned)x - (unsigned)x) - 2 will
overflow.  The associate case is somewhat defensive when associating
TYPE_OVERFLOW_UNDEFINED stuff but I'm not sure what it does is enough.

I'm currentlyu testing

Index: gcc/fold-const.c
===================================================================
--- gcc/fold-const.c    (revision 249397)
+++ gcc/fold-const.c    (working copy)
@@ -853,9 +853,9 @@ split_tree (location_t loc, tree in, tre
           && code == PLUS_EXPR)
     {
       /* -X - 1 is folded to ~X, undo that here.  Do _not_ do this
-         when IN is constant.  */
-      *minus_litp = build_one_cst (TREE_TYPE (in));
-      var = negate_expr (TREE_OPERAND (in, 0));
+         when IN is constant.  Convert to TYPE before negating.  */
+      *minus_litp = build_one_cst (type);
+      var = negate_expr (fold_convert_loc (loc, type, TREE_OPERAND (in, 
0)));
     }
   else
     var = in;

which is, as I said, not enough but another testcase is appreciated ;)
Comment 5 Richard Biener 2017-06-20 12:47:17 UTC
Author: rguenth
Date: Tue Jun 20 12:46:46 2017
New Revision: 249407

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

	PR middle-end/81097
	* fold-const.c (split_tree): Fold to type before negating.

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

Added:
    trunk/gcc/testsuite/c-c++-common/ubsan/pr81097.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/fold-const.c
    trunk/gcc/testsuite/ChangeLog
Comment 6 Richard Biener 2017-06-20 12:49:05 UTC
Fixed.  I believe latent issues remain, not 100% sure.