[Bug sanitizer/80536] [6/7/8 Regression] UBSAN: compile time segfault

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80536

--- Comment #13 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #12)
> (In reply to Marek Polacek from comment #11)
> > (In reply to Jakub Jelinek from comment #5)
> > > To expand on that, I think we want to drop that call from there and instead
> > > be able to simplify somehow a SAVE_EXPR if after c_fully_fold or cp_fold it
> > > becomes simple enough not to require any saving.
> > 
> > Hmm, I'm not sure what you mean.  save_expr has
> > 
> > 3351   if (tree_invariant_p_1 (inner))
> > 3352     return expr;
> 
> Sure, it has and also has skip_simple_arithmetic.  But without the fold
> there is a chance (though small, as fold isn't recursive) that it previously
> would turn something non-invariant/simple arithmetics into invariant/simple
> arith and we wouldn't create the SAVE_EXPR, but now do.  Besides increased
> memory footprint that wouldn't be bad, the problem is that I don't see any
> of the recursive folders being able to undo that, so we end up with them
> until gimplification.

This is true, but it happens very rarely.  It can happen e.g. when the fold()
call in save_expr() folds away the first operand of a COMPOUND_EXPR, and the
second operand is e.g.
(long unsigned int) ((sizetype) SAVE_EXPR <n> * 4)
then skip_simple_arithmetic can pull out "SAVE_EXPR <n>" out of it, which is 
tree_invariant_p_1.  

> Thus, it would be nice if e.g. cp_fold, or fold, or c_fully_fold_internal
> was able to fold a SAVE_EXPR where:
>   inner = skip_simple_arithmetic (TREE_OPERAND (save_expr, 0));
>   if (TREE_CODE (inner) == ERROR_MARK)
>     return inner;
> 
>   if (tree_invariant_p_1 (inner))
>     return TREE_OPERAND (save_expr, 0);

But even if I add this to fold or c_fully_fold, we don't have any guarantees
that any of these will be called before gimplification, right?  So most likely
we'd end up with the new SAVE_EXPR in the gimplifier, which, as you point out,
is not that bad.

> The problem on the C FE side (that would be nice to fix) is that it has its
> own c_save_expr that wants the operand to be c_fully_folded already when
> creating the SAVE_EXPR, it would be better if we could post-pone that and
> perhaps use some flag on the SAVE_EXPR to indicate whether we've
> c_fully_folded the operand already or not and only fully fold it once (C++
> FE does that through its hash maps) the first time something calls
> c_fully_fold on the SAVE_EXPR.
> So maybe you should start just with the C++ FE for now, or do it in fold too.

But c_fully_fold nor cp_fold step into SAVE_EXPRs, they just return them
unmodified.  What can happen though is that c_save_expr gets something that
c_fully_fold folds into an invariant/simple arith, in which case we don't wrap
it in SAVE_EXPR, so the same expression might be folded multiple times, right? 
And that could be solved by adding a hash map to c_fully_fold.

So shouldn't we first apply just this?

Comments very appreciated.

--- 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,33 @@ 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 (SAVE_EXPR, TREE_TYPE (expr), expr);
+  SET_EXPR_LOCATION (expr, EXPR_LOCATION (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