I had an interesting time coming to grips with these two PRs. But it
essentially comes down to the fold call in save_expr. With that, we can call
fold() on an expression whose operands weren't folded yet, but that is what
code in fold_binary_loc assumes. Since fold() is not recursive, we could end
up there with expressions such as
i * ((unsigned long) -0 + 1) * 2
causing various sorts of problems: turning valid code into invalid, infinite
recursion, etc.
It's not clear why save_expr folds. I did some archeology but all I could
find was r67189 (that's 2003) which had:
- tree t = expr;
+ tree t = fold (expr);
tree inner;
- /* Don't fold a COMPONENT_EXPR: if the operand was a CONSTRUCTOR (the
- only time it will fold), it can cause problems with PLACEHOLDER_EXPRs
- in Ada. Moreover, it isn't at all clear why we fold here at all. */
- if (TREE_CODE (t) != COMPONENT_REF)
- t = fold (t);
so it wasn't clear even 14 years ago. But now we know the call is harmful.
Now, fold() doesn't recurse, but can it happen that it folds something anyway?
And maybe turn the expression into an invariant so that we don't need to wrap
it in a SAVE_EXPR? Turns out it can, but in the C FE, which either uses
c_save_expr that c_fully_folds, or calls save_expr when in_late_binary_op (so
the operands have already been folded), it's very rare, and can only be
triggered
by code such as
void
f (int i)
{
int (*a)[i];
int x[sizeof (*a)];
}
so I'm not very worried about that. For C++, Jakub suggested to add SAVE_EXPR
handling to cp_fold.
Future work: get rid of c_save_expr, only use save_expr, and teach c_fully_fold
how to fold SAVE_EXPR (once). Although there's this c_wrap_maybe_const
business...
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2017-05-11 Marek Polacek <pola...@redhat.com>
PR sanitizer/80536
PR sanitizer/80386
* cp-gimplify.c (cp_fold): Handle SAVE_EXPR.
* tree.c (save_expr): Don't fold the expression.
* c-c++-common/ubsan/pr80536.c: New test.
* g++.dg/ubsan/pr80386.C: New test.
diff --git gcc/cp/cp-gimplify.c gcc/cp/cp-gimplify.c
index de62414..38a8ed4 100644
--- gcc/cp/cp-gimplify.c
+++ gcc/cp/cp-gimplify.c
@@ -2428,6 +2428,15 @@ cp_fold (tree x)
x = fold (x);
break;
+ case SAVE_EXPR:
+ /* A SAVE_EXPR might contain e.g. (0 * i) + (0 * j), which, after
+ folding, evaluates to an invariant. In that case no need to wrap
+ this folded tree with a SAVE_EXPR. */
+ r = cp_fold (TREE_OPERAND (x, 0));
+ if (tree_invariant_p (r))
+ x = r;
+ break;
+
default:
return org_x;
}
diff --git gcc/testsuite/c-c++-common/ubsan/pr80536.c
gcc/testsuite/c-c++-common/ubsan/pr80536.c
index e69de29..23913ad 100644
--- gcc/testsuite/c-c++-common/ubsan/pr80536.c
+++ gcc/testsuite/c-c++-common/ubsan/pr80536.c
@@ -0,0 +1,9 @@
+/* PR sanitizer/80536 */
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=undefined" } */
+
+int
+foo (int i)
+{
+ return ((i * (unsigned long long) (-0 + 1UL)) * 2) % 1;
+}
diff --git gcc/testsuite/g++.dg/ubsan/pr80386.C
gcc/testsuite/g++.dg/ubsan/pr80386.C
index e69de29..60122da 100644
--- gcc/testsuite/g++.dg/ubsan/pr80386.C
+++ gcc/testsuite/g++.dg/ubsan/pr80386.C
@@ -0,0 +1,13 @@
+// PR sanitizer/80386
+// { dg-do run }
+// { dg-options "-fsanitize=undefined -fno-sanitize-recover" }
+
+static unsigned long long int i = 13996271126042720493ULL;
+
+int
+main ()
+{
+ int r = (((2921 + 0) - short(i)) + 0x7fffffff) >> 0;
+ asm volatile ("" : "+g" (r));
+ return 0;
+}
diff --git gcc/tree.c gcc/tree.c
index b76b521..7506725 100644
--- gcc/tree.c
+++ gcc/tree.c
@@ -3337,7 +3337,6 @@ tree_invariant_p (tree t)
tree
save_expr (tree expr)
{
- tree t = fold (expr);
tree inner;
/* If the tree evaluates to a constant, then we don't want to hide that
@@ -3345,33 +3344,32 @@ save_expr (tree expr)
However, a read-only object that has side effects cannot be bypassed.
Since it is no problem to reevaluate literals, we just return the
literal node. */
- inner = skip_simple_arithmetic (t);
+ inner = skip_simple_arithmetic (expr);
if (TREE_CODE (inner) == ERROR_MARK)
return inner;
if (tree_invariant_p_1 (inner))
- return t;
+ return expr;
/* If INNER contains a PLACEHOLDER_EXPR, we must evaluate it each time, since
it means that the size or offset of some field of an object depends on
the value within another field.
- Note that it must not be the case that T contains both a PLACEHOLDER_EXPR
+ Note that it must not be the case that EXPR contains both a
PLACEHOLDER_EXPR
and some variable since it would then need to be both evaluated once and
evaluated more than once. Front-ends must assure this case cannot
happen by surrounding any such subexpressions in their own SAVE_EXPR
and forcing evaluation at the proper time. */
if (contains_placeholder_p (inner))
- return t;
+ return expr;
- t = build1 (SAVE_EXPR, TREE_TYPE (expr), t);
- SET_EXPR_LOCATION (t, EXPR_LOCATION (expr));
+ expr = build1_loc (EXPR_LOCATION (expr), SAVE_EXPR, TREE_TYPE (expr), expr);
/* This expression might be placed ahead of a jump to ensure that the
value was computed on both sides of the jump. So make sure it isn't
eliminated as dead. */
- TREE_SIDE_EFFECTS (t) = 1;
- return t;
+ TREE_SIDE_EFFECTS (expr) = 1;
+ return expr;
}
/* Look inside EXPR into any simple arithmetic operations. Return the
Marek
