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

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

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 523 bytes --]

Hello!

Attached patch fixes PR 43057 by unsharing arg0 argument.

2010-11-20  Uros Bizjak  <ubizjak@gmail.com>

	PR middle-end/43057
	* fold-const.c (fold_ternary_loc) <COND_EXPR>: Unshare arg0 when
	converting A ? 1 : 0 to A.

Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu
{,-m32}. FWIW, "--enable-checking=all" bootstrap is still running and
it already compiled up to stage-2 gcc/dse.o. I will leave the
bootstrap overnight to see if it breaks (it probably won't).

OK for mainline and 4.5?

Uros.

[-- Attachment #2: p.diff.txt --]
[-- Type: text/plain, Size: 621 bytes --]

Index: fold-const.c
===================================================================
--- fold-const.c	(revision 166974)
+++ fold-const.c	(working copy)
@@ -13415,7 +13415,7 @@ fold_ternary_loc (location_t loc, enum t
 	     a COND, which will recurse.  In that case, the COND_EXPR
 	     is probably the best choice, so leave it alone.  */
 	  && type == TREE_TYPE (arg0))
-	return pedantic_non_lvalue_loc (loc, arg0);
+	return pedantic_non_lvalue_loc (loc, unshare_expr (arg0));
 
       /* Convert A ? 0 : 1 to !A.  This prefers the use of NOT_EXPR
 	 over COND_EXPR in cases such as floating point comparisons.  */

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

[Richard Guenther]

On Sat, Nov 20, 2010 at 8:03 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> Hello!
>
> Attached patch fixes PR 43057 by unsharing arg0 argument.
>
> 2010-11-20  Uros Bizjak  <ubizjak@gmail.com>
>
>        PR middle-end/43057
>        * fold-const.c (fold_ternary_loc) <COND_EXPR>: Unshare arg0 when
>        converting A ? 1 : 0 to A.
>
> Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu
> {,-m32}. FWIW, "--enable-checking=all" bootstrap is still running and
> it already compiled up to stage-2 gcc/dse.o. I will leave the
> bootstrap overnight to see if it breaks (it probably won't).
>
> OK for mainline and 4.5?

Ok.

Thanks,
Richard.

> Uros.
>

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

[Uros Bizjak]

On Sun, Nov 21, 2010 at 7:43 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Sat, Nov 20, 2010 at 8:03 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> Hello!
>>
>> Attached patch fixes PR 43057 by unsharing arg0 argument.
>>
>> 2010-11-20  Uros Bizjak  <ubizjak@gmail.com>
>>
>>        PR middle-end/43057
>>        * fold-const.c (fold_ternary_loc) <COND_EXPR>: Unshare arg0 when
>>        converting A ? 1 : 0 to A.
>>
>> Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu
>> {,-m32}. FWIW, "--enable-checking=all" bootstrap is still running and
>> it already compiled up to stage-2 gcc/dse.o. I will leave the
>> bootstrap overnight to see if it breaks (it probably won't).
>>
>> OK for mainline and 4.5?
>
> Ok.

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?

Uros.

Index: fold-const.c
===================================================================
--- fold-const.c	(revision 166974)
+++ fold-const.c	(working copy)
@@ -2097,6 +2097,7 @@
 {
   if (pedantic_lvalues)
     return non_lvalue_loc (loc, x);
+  x = unshare_expr (x);
   protected_set_expr_location (x, loc);
   return x;
 }

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

[Richard Guenther]

On Sun, Nov 21, 2010 at 8:02 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Sun, Nov 21, 2010 at 7:43 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Sat, Nov 20, 2010 at 8:03 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>>> Hello!
>>>
>>> Attached patch fixes PR 43057 by unsharing arg0 argument.
>>>
>>> 2010-11-20  Uros Bizjak  <ubizjak@gmail.com>
>>>
>>>        PR middle-end/43057
>>>        * fold-const.c (fold_ternary_loc) <COND_EXPR>: Unshare arg0 when
>>>        converting A ? 1 : 0 to A.
>>>
>>> Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu
>>> {,-m32}. FWIW, "--enable-checking=all" bootstrap is still running and
>>> it already compiled up to stage-2 gcc/dse.o. I will leave the
>>> bootstrap overnight to see if it breaks (it probably won't).
>>>
>>> OK for mainline and 4.5?
>>
>> Ok.
>
> 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.

Thanks,
Richard.

> Uros.
>
> Index: fold-const.c
> ===================================================================
> --- fold-const.c        (revision 166974)
> +++ fold-const.c        (working copy)
> @@ -2097,6 +2097,7 @@
>  {
>   if (pedantic_lvalues)
>     return non_lvalue_loc (loc, x);
> +  x = unshare_expr (x);
>   protected_set_expr_location (x, loc);
>   return x;
>  }
>

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

[Jakub Jelinek]

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.

	Jakub

Re: [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.

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

[Jakub Jelinek]

On Mon, Nov 22, 2010 at 09:34:11AM +0100, Uros Bizjak wrote:
> 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.

Fine with me; if Richi agrees to that, it would be better to put this into
a separate inline (fold-const.c local is ok) though, as it might be used
later on in multiple locations.

And then at the next phase get rid of all the protected_set_expr_location
and SET_EXPR_LOCATION uses in fold-const.c, either (when used on
the result of buildN), start using buildN_loc, or call this new inline
instead.

	Jakub

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

[Richard Guenther]

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.
>