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

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

On Mon, Nov 22, 2010 at 9:34 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> 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.

It works for me.

Richard.

> Thanks,
> Uros.
>