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