Re: [PATCH] Don't fold in save_expr (PR sanitizer/80536, PR sanitizer/80386)

[Marek Polacek <polacek at redhat dot com>]

On Fri, May 12, 2017 at 09:32:48AM +0200, Richard Biener wrote:
> On Thu, May 11, 2017 at 3:49 PM, Marek Polacek <polacek@redhat.com> wrote:
> > 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.

I've come to believe this was purely to discover more invariants.

> > 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?
> 
> Interesting - I tried this once (removing fold () from save_expr) but
> it failed miserably
> (don't remember the details).  Maybe the cp/ part fixes it up.

I think that must've been before C++ added all that constexpr bits.  The
cp_fold part is just an optimization that triggers very rarely.

> Did you include Ada in testing?

Not before, so I've tested this now with Ada included -- looks fine still.

> Otherwise the tree.c change is ok.

Thanks.  Jason/Joseph, any comments?

> (yay, one less to go in the attempt to remove fold ())

Do we have any low hanging fruit here?  Suppose not...

	Marek