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

[Richard Biener <richard dot guenther at gmail dot com>]

On Fri, May 12, 2017 at 12:09 PM, Marek Polacek <polacek@redhat.com> wrote:
> 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...

Factoring out the quaternary cases to better handle

  fold (build4 (code, ...

that happens in quite a few places.  And then there's

        tree t = build3 (BIT_FIELD_REF, currop->type, genop0, op1, op2);
        REF_REVERSE_STORAGE_ORDER (t) = currop->reverse;
        return fold (t);

for the lack of [fold_]buildN getting the REF_REVERSE_STORAGE_ORDER flag ...
(I fully blame Eric for this ;)).

Ok, now starts to be non-low-hanging.

Richard.

>         Marek