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