[PATCH, middle-end]: Fix PR 43057, [LTO] fold check: original tree changed by fold

[Uros Bizjak]

On Sun, Nov 21, 2010 at 10:13 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Sun, Nov 21, 2010 at 09:48:54PM +0100, Richard Guenther wrote:
>> > Actually, --enable-checking=all build broke in fold-const.c, line
>> > 13361 (trying to build libstdc++/src/debug.cc) in the same call to
>> > pedantic_non_lvalue_loc. This points to the problem in the called
>> > function itself.
>> >
>> > Attached patch unshares the expression in the pedantic_non_lvalue_loc
>> > itself. We shouldn't change the original operand by setting the
>> > location.
>> >
>> > 2010-11-21  Uros Bizjak  <ubizjak@gmail.com>
>> >
>> >        PR middle-end/43057
>> >        * fold-const.c (pedantic_non_lvalue_loc): Unshare x before
>> >        setting location.
>> >
>> > OK for mainline and 4.5?
>>
>> Ok.
>
> I don't think this is desirable.
> 1) unshare_expr won't unshare SAVE_EXPR/TARGET_EXPR/BIND_EXPR
> 2) you unshare even when there is no reason for it (!CAN_HAVE_LOCATION_P
> (x) or when it already has the desired locus)
> 3) you don't need to do a deep unshare just to set location
>
> So, I think it would be much better to do something like:
> if (CAN_HAVE_LOCATION_P (x)
>    && EXPR_LOCATION (x) != loc
>    && TREE_CODE (x) != SAVE_EXPR
>    && TREE_CODE (x) != TARGET_EXPR
>    && TREE_CODE (x) != BIND_EXPR)
>  {
>    x = copy_node (x);
>    SET_EXPR_LOCATION (x, loc);
>  }
>
> Also, as I said in the PR, this isn't the only place in fold-const.c that
> needs fixing for --enable-checking=fold, there are more than 10 similar
> spots.  And buildN_loc should be introduced and used where possible.

Your proposed approach also works (and fixes bootstrap with
--enable-checking=all, too). If there are no objections from Richi, I
propose to commit this change to mainline (since it enables bootstrap
with --enable-checking=fold) and fix all fold fails from the testsuite
separately.

Thanks,
Uros.