[PATCH] Fix middle-end/67133, part 1

[PATCH] Fix middle-end/67133, part 1

[Marek Polacek]

As outlined in the PR, this fixes one ICE.  The code in question here
tries to determine whether OP can be derived as non-NULL.  In case the
function has the nonnull attribute that applies to all the arguments,
we want to see whether OP is in this argument list.  But nonnull only
appertains to pointers.  Some ssa_names don't have a type so check for
that first instead of segv before looking at its tree code.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2015-08-14  Marek Polacek  <polacek@redhat.com>

	PR middle-end/67133
	* gimple.c (infer_nonnull_range_by_attribute): Handle null TREE_TYPE.

	* g++.dg/torture/pr67133.C: New test.

diff --git gcc/gimple.c gcc/gimple.c
index cca328a..1482eb4 100644
--- gcc/gimple.c
+++ gcc/gimple.c
@@ -2678,14 +2678,16 @@ infer_nonnull_range_by_attribute (gimple stmt, tree op)
 	  if (attrs == NULL_TREE)
 	    return false;
 
-	  /* If "nonnull" applies to all the arguments, then ARG
+	  /* If "nonnull" applies to all the arguments, then OP
 	     is non-null if it's in the argument list.  */
 	  if (TREE_VALUE (attrs) == NULL_TREE)
 	    {
 	      for (unsigned int i = 0; i < gimple_call_num_args (stmt); i++)
 		{
-		  if (POINTER_TYPE_P (TREE_TYPE (gimple_call_arg (stmt, i)))
-		      && operand_equal_p (op, gimple_call_arg (stmt, i), 0))
+		  tree arg = gimple_call_arg (stmt, i);
+		  if (TREE_TYPE (arg) != NULL_TREE
+		      && POINTER_TYPE_P (TREE_TYPE (arg))
+		      && operand_equal_p (op, arg, 0))
 		    return true;
 		}
 	      return false;
diff --git gcc/testsuite/g++.dg/torture/pr67133.C gcc/testsuite/g++.dg/torture/pr67133.C
index e69de29..0f23572 100644
--- gcc/testsuite/g++.dg/torture/pr67133.C
+++ gcc/testsuite/g++.dg/torture/pr67133.C
@@ -0,0 +1,46 @@
+// { dg-do compile }
+// { dg-additional-options "-fisolate-erroneous-paths-attribute" }
+
+class A;
+struct B {
+  typedef A type;
+};
+template <typename> struct I : B {};
+class C {
+public:
+  C(char *);
+  int size();
+};
+template <typename> struct D;
+template <typename _Tp, typename = D<_Tp>> class F {
+  class G {
+    template <typename> static _Tp *__test();
+    typedef int _Del;
+
+  public:
+    typedef decltype(__test<_Del>()) type;
+  };
+
+public:
+  typename I<_Tp>::type operator*() {
+    typename G::type a = 0;
+    return *a;
+  }
+};
+class H {
+  F<A> Out;
+  H();
+};
+void fn1(void *, void *, int) __attribute__((__nonnull__));
+class A {
+  int OutBufEnd, OutBufCur;
+
+public:
+  void operator<<(C p1) {
+    int b, c = p1.size();
+    if (OutBufEnd)
+      fn1(&OutBufCur, &b, c);
+  }
+};
+char* a;
+H::H() { *Out << a; }

	Marek

Re: [PATCH] Fix middle-end/67133, part 1

[Richard Biener]

On Fri, Aug 14, 2015 at 1:20 PM, Marek Polacek <polacek@redhat.com> wrote:
> As outlined in the PR, this fixes one ICE.  The code in question here
> tries to determine whether OP can be derived as non-NULL.  In case the
> function has the nonnull attribute that applies to all the arguments,
> we want to see whether OP is in this argument list.  But nonnull only
> appertains to pointers.  Some ssa_names don't have a type so check for
> that first instead of segv before looking at its tree code.

Huh?  All but released SSA names have a type.  So this gets invoked on dead code
somehow?

RIchard.

>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2015-08-14  Marek Polacek  <polacek@redhat.com>
>
>         PR middle-end/67133
>         * gimple.c (infer_nonnull_range_by_attribute): Handle null TREE_TYPE.
>
>         * g++.dg/torture/pr67133.C: New test.
>
> diff --git gcc/gimple.c gcc/gimple.c
> index cca328a..1482eb4 100644
> --- gcc/gimple.c
> +++ gcc/gimple.c
> @@ -2678,14 +2678,16 @@ infer_nonnull_range_by_attribute (gimple stmt, tree op)
>           if (attrs == NULL_TREE)
>             return false;
>
> -         /* If "nonnull" applies to all the arguments, then ARG
> +         /* If "nonnull" applies to all the arguments, then OP
>              is non-null if it's in the argument list.  */
>           if (TREE_VALUE (attrs) == NULL_TREE)
>             {
>               for (unsigned int i = 0; i < gimple_call_num_args (stmt); i++)
>                 {
> -                 if (POINTER_TYPE_P (TREE_TYPE (gimple_call_arg (stmt, i)))
> -                     && operand_equal_p (op, gimple_call_arg (stmt, i), 0))
> +                 tree arg = gimple_call_arg (stmt, i);
> +                 if (TREE_TYPE (arg) != NULL_TREE
> +                     && POINTER_TYPE_P (TREE_TYPE (arg))
> +                     && operand_equal_p (op, arg, 0))
>                     return true;
>                 }
>               return false;
> diff --git gcc/testsuite/g++.dg/torture/pr67133.C gcc/testsuite/g++.dg/torture/pr67133.C
> index e69de29..0f23572 100644
> --- gcc/testsuite/g++.dg/torture/pr67133.C
> +++ gcc/testsuite/g++.dg/torture/pr67133.C
> @@ -0,0 +1,46 @@
> +// { dg-do compile }
> +// { dg-additional-options "-fisolate-erroneous-paths-attribute" }
> +
> +class A;
> +struct B {
> +  typedef A type;
> +};
> +template <typename> struct I : B {};
> +class C {
> +public:
> +  C(char *);
> +  int size();
> +};
> +template <typename> struct D;
> +template <typename _Tp, typename = D<_Tp>> class F {
> +  class G {
> +    template <typename> static _Tp *__test();
> +    typedef int _Del;
> +
> +  public:
> +    typedef decltype(__test<_Del>()) type;
> +  };
> +
> +public:
> +  typename I<_Tp>::type operator*() {
> +    typename G::type a = 0;
> +    return *a;
> +  }
> +};
> +class H {
> +  F<A> Out;
> +  H();
> +};
> +void fn1(void *, void *, int) __attribute__((__nonnull__));
> +class A {
> +  int OutBufEnd, OutBufCur;
> +
> +public:
> +  void operator<<(C p1) {
> +    int b, c = p1.size();
> +    if (OutBufEnd)
> +      fn1(&OutBufCur, &b, c);
> +  }
> +};
> +char* a;
> +H::H() { *Out << a; }
>
>         Marek

Re: [PATCH] Fix middle-end/67133, part 1

[Marek Polacek]

On Fri, Aug 14, 2015 at 03:14:10PM +0200, Richard Biener wrote:
> On Fri, Aug 14, 2015 at 1:20 PM, Marek Polacek <polacek@redhat.com> wrote:
> > As outlined in the PR, this fixes one ICE.  The code in question here
> > tries to determine whether OP can be derived as non-NULL.  In case the
> > function has the nonnull attribute that applies to all the arguments,
> > we want to see whether OP is in this argument list.  But nonnull only
> > appertains to pointers.  Some ssa_names don't have a type so check for
> > that first instead of segv before looking at its tree code.
> 
> Huh?  All but released SSA names have a type.  So this gets invoked on dead code

I suppose so.  It gets
 <ssa_name 0x7ffff1890948 nothrow var <var_decl 0x7ffff188df30 c>def_stmt 

    version 13 in-free-list>

(it didn't ICE before r209891 because the operand_equal_p check came first and
returned false so we didn't check the type).

	Marek

Re: [PATCH] Fix middle-end/67133, part 1

[Jeff Law]

On 08/14/2015 07:29 AM, Marek Polacek wrote:
> On Fri, Aug 14, 2015 at 03:14:10PM +0200, Richard Biener wrote:
>> On Fri, Aug 14, 2015 at 1:20 PM, Marek Polacek <polacek@redhat.com> wrote:
>>> As outlined in the PR, this fixes one ICE.  The code in question here
>>> tries to determine whether OP can be derived as non-NULL.  In case the
>>> function has the nonnull attribute that applies to all the arguments,
>>> we want to see whether OP is in this argument list.  But nonnull only
>>> appertains to pointers.  Some ssa_names don't have a type so check for
>>> that first instead of segv before looking at its tree code.
>>
>> Huh?  All but released SSA names have a type.  So this gets invoked on dead code
>
> I suppose so.  It gets
>   <ssa_name 0x7ffff1890948 nothrow var <var_decl 0x7ffff188df30 c>def_stmt
>
>      version 13 in-free-list>
>
> (it didn't ICE before r209891 because the operand_equal_p check came first and
> returned false so we didn't check the type).
Let's track this down -- nothing should be referencing anything in the 
SSA_NAME freelist.

Jeff

Re: [PATCH] Fix middle-end/67133, part 1

[Marek Polacek]

On Fri, Aug 14, 2015 at 08:50:22AM -0600, Jeff Law wrote:
> On 08/14/2015 07:29 AM, Marek Polacek wrote:
> >On Fri, Aug 14, 2015 at 03:14:10PM +0200, Richard Biener wrote:
> >>On Fri, Aug 14, 2015 at 1:20 PM, Marek Polacek <polacek@redhat.com> wrote:
> >>>As outlined in the PR, this fixes one ICE.  The code in question here
> >>>tries to determine whether OP can be derived as non-NULL.  In case the
> >>>function has the nonnull attribute that applies to all the arguments,
> >>>we want to see whether OP is in this argument list.  But nonnull only
> >>>appertains to pointers.  Some ssa_names don't have a type so check for
> >>>that first instead of segv before looking at its tree code.
> >>
> >>Huh?  All but released SSA names have a type.  So this gets invoked on dead code
> >
> >I suppose so.  It gets
> >  <ssa_name 0x7ffff1890948 nothrow var <var_decl 0x7ffff188df30 c>def_stmt
> >
> >     version 13 in-free-list>
> >
> >(it didn't ICE before r209891 because the operand_equal_p check came first and
> >returned false so we didn't check the type).
> Let's track this down -- nothing should be referencing anything in the
> SSA_NAME freelist.

Ok, I'll investigate and come back to y'all when/if I find something.

	Marek

Re: [PATCH] Fix middle-end/67133, part 1

[Jeff Law]

On 08/14/2015 09:32 AM, Marek Polacek wrote:
> On Fri, Aug 14, 2015 at 08:50:22AM -0600, Jeff Law wrote:
>> On 08/14/2015 07:29 AM, Marek Polacek wrote:
>>> On Fri, Aug 14, 2015 at 03:14:10PM +0200, Richard Biener wrote:
>>>> On Fri, Aug 14, 2015 at 1:20 PM, Marek Polacek <polacek@redhat.com> wrote:
>>>>> As outlined in the PR, this fixes one ICE.  The code in question here
>>>>> tries to determine whether OP can be derived as non-NULL.  In case the
>>>>> function has the nonnull attribute that applies to all the arguments,
>>>>> we want to see whether OP is in this argument list.  But nonnull only
>>>>> appertains to pointers.  Some ssa_names don't have a type so check for
>>>>> that first instead of segv before looking at its tree code.
>>>>
>>>> Huh?  All but released SSA names have a type.  So this gets invoked on dead code
>>>
>>> I suppose so.  It gets
>>>   <ssa_name 0x7ffff1890948 nothrow var <var_decl 0x7ffff188df30 c>def_stmt
>>>
>>>      version 13 in-free-list>
>>>
>>> (it didn't ICE before r209891 because the operand_equal_p check came first and
>>> returned false so we didn't check the type).
>> Let's track this down -- nothing should be referencing anything in the
>> SSA_NAME freelist.
>
> Ok, I'll investigate and come back to y'all when/if I find something.
Thanks.  I still regret using the TREE_TYPE as a way to chain elements 
in the free list:(  I didn't want to add another pointer field...

jeff

Re: [PATCH] Fix middle-end/67133, part 1

[Marek Polacek]

On Fri, Aug 14, 2015 at 09:33:46AM -0600, Jeff Law wrote:
> >Ok, I'll investigate and come back to y'all when/if I find something.
> Thanks.  I still regret using the TREE_TYPE as a way to chain elements in
> the free list:(  I didn't want to add another pointer field...

It's actually plain to see what's happening.  Before isolate-paths we've got

  <bb 2>:
  ...
  SR.5_10 = MEM[(const struct A &)0B];
  ...
  c_13 = C::size (&p1);
  ...
  if (_14 != 0)
    goto <bb 3>; 
  else
    goto <bb 4>; 

  <bb 3>: 
  fn1 (&D.2434.OutBufCur, &b, c_13);

Then in isolate-paths in find_explicit_erroneous_behaviour we're walking the
stmts in bb 2 and we find a null dereference, so insert_trap_and_remove_trailing_statements
comes in play and turns bb 2 into: 

  <bb 2>:
  ...
  SR.5_10 = MEM[(const struct A &)0B];
  __builtin_trap ();

i.e. it removs the defining statement for c_13.  Then find_explicit_erroneous_behaviour
walks over bb 3, hits the fn1 (&D.2434.OutBufCur, &b, c_13); statement, and
ICEs on the c_13 argument: it's a released SSA name with NULL TREE_TYPE.

The question now is what to do with that.  Skip SSA_NAME_IN_FREE_LIST?  That
sounds weird.  Note that we're going to remove bb 3 and bb 4 anyway... 

	Marek

Re: [PATCH] Fix middle-end/67133, part 1

[Jeff Law]

On 08/14/2015 01:58 PM, Marek Polacek wrote:
> On Fri, Aug 14, 2015 at 09:33:46AM -0600, Jeff Law wrote:
>>> Ok, I'll investigate and come back to y'all when/if I find something.
>> Thanks.  I still regret using the TREE_TYPE as a way to chain elements in
>> the free list:(  I didn't want to add another pointer field...
>
> It's actually plain to see what's happening.  Before isolate-paths we've got
>
>    <bb 2>:
>    ...
>    SR.5_10 = MEM[(const struct A &)0B];
>    ...
>    c_13 = C::size (&p1);
>    ...
>    if (_14 != 0)
>      goto <bb 3>;
>    else
>      goto <bb 4>;
>
>    <bb 3>:
>    fn1 (&D.2434.OutBufCur, &b, c_13);
>
> Then in isolate-paths in find_explicit_erroneous_behaviour we're walking the
> stmts in bb 2 and we find a null dereference, so insert_trap_and_remove_trailing_statements
> comes in play and turns bb 2 into:
>
>    <bb 2>:
>    ...
>    SR.5_10 = MEM[(const struct A &)0B];
>    __builtin_trap ();
>
> i.e. it removs the defining statement for c_13.  Then find_explicit_erroneous_behaviour
> walks over bb 3, hits the fn1 (&D.2434.OutBufCur, &b, c_13); statement, and
> ICEs on the c_13 argument: it's a released SSA name with NULL TREE_TYPE.
>
> The question now is what to do with that.  Skip SSA_NAME_IN_FREE_LIST?  That
> sounds weird.  Note that we're going to remove bb 3 and bb 4 anyway...
Jeez, looking at the code N years later, I feel like a complete idiot. 
Of course that's not going to work.

We certainly don't want to peek at SSA_NAME_IN_FREE_LIST.

I wonder if we should be walking in backwards dominator order to avoid 
these effects.  Or maybe just ignoring any BB with no preds.  I'll 
ponder those over the weekend.

Jeff

Re: [PATCH] Fix middle-end/67133, part 1

[Marek Polacek]

On Fri, Aug 14, 2015 at 02:32:20PM -0600, Jeff Law wrote:
> On 08/14/2015 01:58 PM, Marek Polacek wrote:
> >On Fri, Aug 14, 2015 at 09:33:46AM -0600, Jeff Law wrote:
> >>>Ok, I'll investigate and come back to y'all when/if I find something.
> >>Thanks.  I still regret using the TREE_TYPE as a way to chain elements in
> >>the free list:(  I didn't want to add another pointer field...
> >
> >It's actually plain to see what's happening.  Before isolate-paths we've got
> >
> >   <bb 2>:
> >   ...
> >   SR.5_10 = MEM[(const struct A &)0B];
> >   ...
> >   c_13 = C::size (&p1);
> >   ...
> >   if (_14 != 0)
> >     goto <bb 3>;
> >   else
> >     goto <bb 4>;
> >
> >   <bb 3>:
> >   fn1 (&D.2434.OutBufCur, &b, c_13);
> >
> >Then in isolate-paths in find_explicit_erroneous_behaviour we're walking the
> >stmts in bb 2 and we find a null dereference, so insert_trap_and_remove_trailing_statements
> >comes in play and turns bb 2 into:
> >
> >   <bb 2>:
> >   ...
> >   SR.5_10 = MEM[(const struct A &)0B];
> >   __builtin_trap ();
> >
> >i.e. it removs the defining statement for c_13.  Then find_explicit_erroneous_behaviour
> >walks over bb 3, hits the fn1 (&D.2434.OutBufCur, &b, c_13); statement, and
> >ICEs on the c_13 argument: it's a released SSA name with NULL TREE_TYPE.
> >
> >The question now is what to do with that.  Skip SSA_NAME_IN_FREE_LIST?  That
> >sounds weird.  Note that we're going to remove bb 3 and bb 4 anyway...
> Jeez, looking at the code N years later, I feel like a complete idiot. Of
> course that's not going to work.
> 
> We certainly don't want to peek at SSA_NAME_IN_FREE_LIST.

Yeh, I thought as much.
 
> I wonder if we should be walking in backwards dominator order to avoid these
> effects.  Or maybe just ignoring any BB with no preds.  I'll ponder those
> over the weekend.

I suppose both ought to work.  Or at least theoretically we could run e.g.
cleanup_cfg to prune the IR after we've inserted trap and removed trailing
stmts so that it gets rid of unreachable bbs.  Would that make sense?

Anyway, if you think of how would you like to solve this I can take a crack
at it next week.

	Marek

Re: [PATCH] Fix middle-end/67133, part 1

[Jeff Law]

On 08/14/2015 02:46 PM, Marek Polacek wrote:
>>> Then in isolate-paths in find_explicit_erroneous_behaviour we're walking the
>>> stmts in bb 2 and we find a null dereference, so insert_trap_and_remove_trailing_statements
>>> comes in play and turns bb 2 into:
>>>
>>>    <bb 2>:
>>>    ...
>>>    SR.5_10 = MEM[(const struct A &)0B];
>>>    __builtin_trap ();
>>>
>>> i.e. it removs the defining statement for c_13.  Then find_explicit_erroneous_behaviour
>>> walks over bb 3, hits the fn1 (&D.2434.OutBufCur, &b, c_13); statement, and
>>> ICEs on the c_13 argument: it's a released SSA name with NULL TREE_TYPE.
>>>
>>> The question now is what to do with that.  Skip SSA_NAME_IN_FREE_LIST?  That
>>> sounds weird.  Note that we're going to remove bb 3 and bb 4 anyway...
>> Jeez, looking at the code N years later, I feel like a complete idiot. Of
>> course that's not going to work.
>>
>> We certainly don't want to peek at SSA_NAME_IN_FREE_LIST.
>
> Yeh, I thought as much.
>
>> I wonder if we should be walking in backwards dominator order to avoid these
>> effects.  Or maybe just ignoring any BB with no preds.  I'll ponder those
>> over the weekend.
>
> I suppose both ought to work.  Or at least theoretically we could run e.g.
> cleanup_cfg to prune the IR after we've inserted trap and removed trailing
> stmts so that it gets rid of unreachable bbs.  Would that make sense?
>
> Anyway, if you think of how would you like to solve this I can take a crack
> at it next week.
The funny thing here is we remove the statements after the trap to avoid 
this exact situation!

I think the problem with schemes that either change the order of block 
processing, or which ignore some blocks are going to run into issues. 
By walking blocks and statements in a backwards order, we address 99% of 
the problems, including uses in PHIs in a direct successor block.

What's not handled is a use in a PHI at the frontier of a subgraph that 
becomes unreachable.  We'd have to do the usual unreachable block 
analysis to catch and handle those properly.

I don't particularly like that idea....

But in walking through all that, I think I've stumbled on a simpler 
solution.  Specifically do as a little as possible and let the standard 
mechanisms clean things up :-)

1. Delete the code that removes instructions after the trap.

2. Split the block immediately after the trap and remove the edge
    from the original block (with the trap) to the new block.


THen let the standard mechanisms handle things when that pass is complete.

By setting cfg_altered, we'll  get unreachable code removal which will 
capture most of the intended effect.  DCE fires a couple more passes 
down in the pipeline to pick up the remaining tidbits.

Do you want to try and tackle this?

jeff

Re: [PATCH] Fix middle-end/67133, part 1

[Marek Polacek]

On Mon, Aug 17, 2015 at 11:31:57AM -0600, Jeff Law wrote:
> The funny thing here is we remove the statements after the trap to avoid
> this exact situation!
> 
> I think the problem with schemes that either change the order of block
> processing, or which ignore some blocks are going to run into issues. By
> walking blocks and statements in a backwards order, we address 99% of the
> problems, including uses in PHIs in a direct successor block.
> 
> What's not handled is a use in a PHI at the frontier of a subgraph that
> becomes unreachable.  We'd have to do the usual unreachable block analysis
> to catch and handle those properly.
> 
> I don't particularly like that idea....
> 
> But in walking through all that, I think I've stumbled on a simpler
> solution.  Specifically do as a little as possible and let the standard
> mechanisms clean things up :-)
> 
> 1. Delete the code that removes instructions after the trap.
> 
> 2. Split the block immediately after the trap and remove the edge
>    from the original block (with the trap) to the new block.
> 
> 
> THen let the standard mechanisms handle things when that pass is complete.
> 
> By setting cfg_altered, we'll  get unreachable code removal which will
> capture most of the intended effect.  DCE fires a couple more passes down in
> the pipeline to pick up the remaining tidbits.

Ok, thanks.
 
> Do you want to try and tackle this?

Sure.  I should have a patch tomorrow :-).

	Marek

Re: [PATCH] Fix middle-end/67133, part 1

[Richard Biener]

On Mon, Aug 17, 2015 at 7:31 PM, Jeff Law <law@redhat.com> wrote:
> On 08/14/2015 02:46 PM, Marek Polacek wrote:
>>>>
>>>> Then in isolate-paths in find_explicit_erroneous_behaviour we're walking
>>>> the
>>>> stmts in bb 2 and we find a null dereference, so
>>>> insert_trap_and_remove_trailing_statements
>>>> comes in play and turns bb 2 into:
>>>>
>>>>    <bb 2>:
>>>>    ...
>>>>    SR.5_10 = MEM[(const struct A &)0B];
>>>>    __builtin_trap ();
>>>>
>>>> i.e. it removs the defining statement for c_13.  Then
>>>> find_explicit_erroneous_behaviour
>>>> walks over bb 3, hits the fn1 (&D.2434.OutBufCur, &b, c_13); statement,
>>>> and
>>>> ICEs on the c_13 argument: it's a released SSA name with NULL TREE_TYPE.
>>>>
>>>> The question now is what to do with that.  Skip SSA_NAME_IN_FREE_LIST?
>>>> That
>>>> sounds weird.  Note that we're going to remove bb 3 and bb 4 anyway...
>>>
>>> Jeez, looking at the code N years later, I feel like a complete idiot. Of
>>> course that's not going to work.
>>>
>>> We certainly don't want to peek at SSA_NAME_IN_FREE_LIST.
>>
>>
>> Yeh, I thought as much.
>>
>>> I wonder if we should be walking in backwards dominator order to avoid
>>> these
>>> effects.  Or maybe just ignoring any BB with no preds.  I'll ponder those
>>> over the weekend.
>>
>>
>> I suppose both ought to work.  Or at least theoretically we could run e.g.
>> cleanup_cfg to prune the IR after we've inserted trap and removed trailing
>> stmts so that it gets rid of unreachable bbs.  Would that make sense?
>>
>> Anyway, if you think of how would you like to solve this I can take a
>> crack
>> at it next week.
>
> The funny thing here is we remove the statements after the trap to avoid
> this exact situation!

Not sure, when I came along that code in the past I wondered why we
don't just do like other passes - split the block, insert the unreachable()
at the end of the first and leave the actual cleanup to cfg-cleanup.

> I think the problem with schemes that either change the order of block
> processing, or which ignore some blocks are going to run into issues. By
> walking blocks and statements in a backwards order, we address 99% of the
> problems, including uses in PHIs in a direct successor block.
>
> What's not handled is a use in a PHI at the frontier of a subgraph that
> becomes unreachable.  We'd have to do the usual unreachable block analysis
> to catch and handle those properly.

So you are after second-level effects that require earlier unreachable paths to
be pruned?

> I don't particularly like that idea....
>
> But in walking through all that, I think I've stumbled on a simpler
> solution.  Specifically do as a little as possible and let the standard
> mechanisms clean things up :-)
>
> 1. Delete the code that removes instructions after the trap.
>
> 2. Split the block immediately after the trap and remove the edge
>    from the original block (with the trap) to the new block.

cfg-cleanup will do that for you if you have a not returning stmt ending
the previous block.

Richard.

>
> THen let the standard mechanisms handle things when that pass is complete.
>
> By setting cfg_altered, we'll  get unreachable code removal which will
> capture most of the intended effect.  DCE fires a couple more passes down in
> the pipeline to pick up the remaining tidbits.
>
> Do you want to try and tackle this?
>
> jeff
>
>

Re: [PATCH] Fix middle-end/67133, part 1

[Marek Polacek]

On Tue, Aug 18, 2015 at 10:45:21AM +0200, Richard Biener wrote:
> On Mon, Aug 17, 2015 at 7:31 PM, Jeff Law <law@redhat.com> wrote:
> > But in walking through all that, I think I've stumbled on a simpler
> > solution.  Specifically do as a little as possible and let the standard
> > mechanisms clean things up :-)
> >
> > 1. Delete the code that removes instructions after the trap.
> >
> > 2. Split the block immediately after the trap and remove the edge
> >    from the original block (with the trap) to the new block.
> 
> cfg-cleanup will do that for you if you have a not returning stmt ending
> the previous block.

The following patch hopefully does what's oulined above.
Arguably I should have renamed the insert_trap_and_remove_trailing_statements
to something more descriptive, e.g. insert_trap_and_split_block.  Your
call.

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2015-08-18  Marek Polacek  <polacek@redhat.com>

	PR middle-end/67133
	* gimple-ssa-isolate-paths.c
	(insert_trap_and_remove_trailing_statements): Rename to ...
	(insert_trap): ... this.  Don't remove trailing statements; split
	block instead.
	(find_explicit_erroneous_behaviour): Don't remove all outgoing edges.

	* g++.dg/torture/pr67133.C: New test.

diff --git gcc/gimple-ssa-isolate-paths.c gcc/gimple-ssa-isolate-paths.c
index 6f84f85..ca2322d 100644
--- gcc/gimple-ssa-isolate-paths.c
+++ gcc/gimple-ssa-isolate-paths.c
@@ -66,10 +66,10 @@ check_loadstore (gimple stmt, tree op, tree, void *data)
   return false;
 }
 
-/* Insert a trap after SI and remove SI and all statements after the trap.  */
+/* Insert a trap after SI and split the block after the trap.  */
 
 static void
-insert_trap_and_remove_trailing_statements (gimple_stmt_iterator *si_p, tree op)
+insert_trap (gimple_stmt_iterator *si_p, tree op)
 {
   /* We want the NULL pointer dereference to actually occur so that
      code that wishes to catch the signal can do so.
@@ -115,18 +115,8 @@ insert_trap_and_remove_trailing_statements (gimple_stmt_iterator *si_p, tree op)
   else
     gsi_insert_before (si_p, seq, GSI_NEW_STMT);
 
-  /* We must remove statements from the end of the block so that we
-     never reference a released SSA_NAME.  */
-  basic_block bb = gimple_bb (gsi_stmt (*si_p));
-  for (gimple_stmt_iterator si = gsi_last_bb (bb);
-       gsi_stmt (si) != gsi_stmt (*si_p);
-       si = gsi_last_bb (bb))
-    {
-      stmt = gsi_stmt (si);
-      unlink_stmt_vdef (stmt);
-      gsi_remove (&si, true);
-      release_defs (stmt);
-    }
+  split_block (gimple_bb (new_stmt), new_stmt);
+  *si_p = gsi_for_stmt (stmt);
 }
 
 /* BB when reached via incoming edge E will exhibit undefined behaviour
@@ -215,7 +205,7 @@ isolate_path (basic_block bb, basic_block duplicate,
 	  update_stmt (ret);
 	}
       else
-	insert_trap_and_remove_trailing_statements (&si2, op);
+	insert_trap (&si2, op);
     }
 
   return duplicate;
@@ -422,14 +412,8 @@ find_explicit_erroneous_behaviour (void)
 		    continue;
 		}
 
-	      insert_trap_and_remove_trailing_statements (&si,
-							  null_pointer_node);
-
-	      /* And finally, remove all outgoing edges from BB.  */
-	      edge e;
-	      for (edge_iterator ei = ei_start (bb->succs);
-		   (e = ei_safe_edge (ei)); )
-		remove_edge (e);
+	      insert_trap (&si, null_pointer_node);
+	      bb = gimple_bb (gsi_stmt (si));
 
 	      /* Ignore any more operands on this statement and
 		 continue the statement iterator (which should
diff --git gcc/testsuite/g++.dg/torture/pr67133.C gcc/testsuite/g++.dg/torture/pr67133.C
index e69de29..0f23572 100644
--- gcc/testsuite/g++.dg/torture/pr67133.C
+++ gcc/testsuite/g++.dg/torture/pr67133.C
@@ -0,0 +1,46 @@
+// { dg-do compile }
+// { dg-additional-options "-fisolate-erroneous-paths-attribute" }
+
+class A;
+struct B {
+  typedef A type;
+};
+template <typename> struct I : B {};
+class C {
+public:
+  C(char *);
+  int size();
+};
+template <typename> struct D;
+template <typename _Tp, typename = D<_Tp>> class F {
+  class G {
+    template <typename> static _Tp *__test();
+    typedef int _Del;
+
+  public:
+    typedef decltype(__test<_Del>()) type;
+  };
+
+public:
+  typename I<_Tp>::type operator*() {
+    typename G::type a = 0;
+    return *a;
+  }
+};
+class H {
+  F<A> Out;
+  H();
+};
+void fn1(void *, void *, int) __attribute__((__nonnull__));
+class A {
+  int OutBufEnd, OutBufCur;
+
+public:
+  void operator<<(C p1) {
+    int b, c = p1.size();
+    if (OutBufEnd)
+      fn1(&OutBufCur, &b, c);
+  }
+};
+char* a;
+H::H() { *Out << a; }

	Marek

Re: [PATCH] Fix middle-end/67133, part 1

[Richard Biener]

On Tue, Aug 18, 2015 at 9:49 PM, Marek Polacek <polacek@redhat.com> wrote:
> On Tue, Aug 18, 2015 at 10:45:21AM +0200, Richard Biener wrote:
>> On Mon, Aug 17, 2015 at 7:31 PM, Jeff Law <law@redhat.com> wrote:
>> > But in walking through all that, I think I've stumbled on a simpler
>> > solution.  Specifically do as a little as possible and let the standard
>> > mechanisms clean things up :-)
>> >
>> > 1. Delete the code that removes instructions after the trap.
>> >
>> > 2. Split the block immediately after the trap and remove the edge
>> >    from the original block (with the trap) to the new block.
>>
>> cfg-cleanup will do that for you if you have a not returning stmt ending
>> the previous block.
>
> The following patch hopefully does what's oulined above.
> Arguably I should have renamed the insert_trap_and_remove_trailing_statements
> to something more descriptive, e.g. insert_trap_and_split_block.  Your
> call.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?

Looks good to me.

Richard.

> 2015-08-18  Marek Polacek  <polacek@redhat.com>
>
>         PR middle-end/67133
>         * gimple-ssa-isolate-paths.c
>         (insert_trap_and_remove_trailing_statements): Rename to ...
>         (insert_trap): ... this.  Don't remove trailing statements; split
>         block instead.
>         (find_explicit_erroneous_behaviour): Don't remove all outgoing edges.
>
>         * g++.dg/torture/pr67133.C: New test.
>
> diff --git gcc/gimple-ssa-isolate-paths.c gcc/gimple-ssa-isolate-paths.c
> index 6f84f85..ca2322d 100644
> --- gcc/gimple-ssa-isolate-paths.c
> +++ gcc/gimple-ssa-isolate-paths.c
> @@ -66,10 +66,10 @@ check_loadstore (gimple stmt, tree op, tree, void *data)
>    return false;
>  }
>
> -/* Insert a trap after SI and remove SI and all statements after the trap.  */
> +/* Insert a trap after SI and split the block after the trap.  */
>
>  static void
> -insert_trap_and_remove_trailing_statements (gimple_stmt_iterator *si_p, tree op)
> +insert_trap (gimple_stmt_iterator *si_p, tree op)
>  {
>    /* We want the NULL pointer dereference to actually occur so that
>       code that wishes to catch the signal can do so.
> @@ -115,18 +115,8 @@ insert_trap_and_remove_trailing_statements (gimple_stmt_iterator *si_p, tree op)
>    else
>      gsi_insert_before (si_p, seq, GSI_NEW_STMT);
>
> -  /* We must remove statements from the end of the block so that we
> -     never reference a released SSA_NAME.  */
> -  basic_block bb = gimple_bb (gsi_stmt (*si_p));
> -  for (gimple_stmt_iterator si = gsi_last_bb (bb);
> -       gsi_stmt (si) != gsi_stmt (*si_p);
> -       si = gsi_last_bb (bb))
> -    {
> -      stmt = gsi_stmt (si);
> -      unlink_stmt_vdef (stmt);
> -      gsi_remove (&si, true);
> -      release_defs (stmt);
> -    }
> +  split_block (gimple_bb (new_stmt), new_stmt);
> +  *si_p = gsi_for_stmt (stmt);
>  }
>
>  /* BB when reached via incoming edge E will exhibit undefined behaviour
> @@ -215,7 +205,7 @@ isolate_path (basic_block bb, basic_block duplicate,
>           update_stmt (ret);
>         }
>        else
> -       insert_trap_and_remove_trailing_statements (&si2, op);
> +       insert_trap (&si2, op);
>      }
>
>    return duplicate;
> @@ -422,14 +412,8 @@ find_explicit_erroneous_behaviour (void)
>                     continue;
>                 }
>
> -             insert_trap_and_remove_trailing_statements (&si,
> -                                                         null_pointer_node);
> -
> -             /* And finally, remove all outgoing edges from BB.  */
> -             edge e;
> -             for (edge_iterator ei = ei_start (bb->succs);
> -                  (e = ei_safe_edge (ei)); )
> -               remove_edge (e);
> +             insert_trap (&si, null_pointer_node);
> +             bb = gimple_bb (gsi_stmt (si));
>
>               /* Ignore any more operands on this statement and
>                  continue the statement iterator (which should
> diff --git gcc/testsuite/g++.dg/torture/pr67133.C gcc/testsuite/g++.dg/torture/pr67133.C
> index e69de29..0f23572 100644
> --- gcc/testsuite/g++.dg/torture/pr67133.C
> +++ gcc/testsuite/g++.dg/torture/pr67133.C
> @@ -0,0 +1,46 @@
> +// { dg-do compile }
> +// { dg-additional-options "-fisolate-erroneous-paths-attribute" }
> +
> +class A;
> +struct B {
> +  typedef A type;
> +};
> +template <typename> struct I : B {};
> +class C {
> +public:
> +  C(char *);
> +  int size();
> +};
> +template <typename> struct D;
> +template <typename _Tp, typename = D<_Tp>> class F {
> +  class G {
> +    template <typename> static _Tp *__test();
> +    typedef int _Del;
> +
> +  public:
> +    typedef decltype(__test<_Del>()) type;
> +  };
> +
> +public:
> +  typename I<_Tp>::type operator*() {
> +    typename G::type a = 0;
> +    return *a;
> +  }
> +};
> +class H {
> +  F<A> Out;
> +  H();
> +};
> +void fn1(void *, void *, int) __attribute__((__nonnull__));
> +class A {
> +  int OutBufEnd, OutBufCur;
> +
> +public:
> +  void operator<<(C p1) {
> +    int b, c = p1.size();
> +    if (OutBufEnd)
> +      fn1(&OutBufCur, &b, c);
> +  }
> +};
> +char* a;
> +H::H() { *Out << a; }
>
>         Marek

Re: [PATCH] Fix middle-end/67133, part 1

[Marek Polacek]

On Wed, Aug 19, 2015 at 11:48:12AM +0200, Richard Biener wrote:
> Looks good to me.

Thanks!  I'll wait for Jeff if he has any comments.

	Marek

Re: [PATCH] Fix middle-end/67133, part 1

[Jeff Law]

On 08/18/2015 01:49 PM, Marek Polacek wrote:
> On Tue, Aug 18, 2015 at 10:45:21AM +0200, Richard Biener wrote:
>> On Mon, Aug 17, 2015 at 7:31 PM, Jeff Law <law@redhat.com> wrote:
>>> But in walking through all that, I think I've stumbled on a simpler
>>> solution.  Specifically do as a little as possible and let the standard
>>> mechanisms clean things up :-)
>>>
>>> 1. Delete the code that removes instructions after the trap.
>>>
>>> 2. Split the block immediately after the trap and remove the edge
>>>     from the original block (with the trap) to the new block.
>>
>> cfg-cleanup will do that for you if you have a not returning stmt ending
>> the previous block.
>
> The following patch hopefully does what's oulined above.
> Arguably I should have renamed the insert_trap_and_remove_trailing_statements
> to something more descriptive, e.g. insert_trap_and_split_block.  Your
> call.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2015-08-18  Marek Polacek  <polacek@redhat.com>
>
> 	PR middle-end/67133
> 	* gimple-ssa-isolate-paths.c
> 	(insert_trap_and_remove_trailing_statements): Rename to ...
> 	(insert_trap): ... this.  Don't remove trailing statements; split
> 	block instead.
> 	(find_explicit_erroneous_behaviour): Don't remove all outgoing edges.
>
> 	* g++.dg/torture/pr67133.C: New test.
Looks good to me too.
jeff

Re: [PATCH] Fix middle-end/67133, part 1

[Andreas Schwab]

Marek Polacek <polacek@redhat.com> writes:

> 	PR middle-end/67133
> 	* gimple-ssa-isolate-paths.c
> 	(insert_trap_and_remove_trailing_statements): Rename to ...
> 	(insert_trap): ... this.  Don't remove trailing statements; split
> 	block instead.
> 	(find_explicit_erroneous_behaviour): Don't remove all outgoing edges.

This breaks go on aarch64:

../../../libgo/go/encoding/gob/decode.go: In function ‘gob.decIgnoreOpFor.pN20_encoding_gob.Decoder’:
../../../libgo/go/encoding/gob/decode.go:843:1: internal compiler error: in operator[], at vec.h:714
 func (dec *Decoder) decIgnoreOpFor(wireId typeId) decOp {
 ^
0xac5c3b vec<edge_def*, va_gc, vl_embed>::operator[](unsigned int)
        ../../gcc/vec.h:714
0xac5c3b extract_true_false_edges_from_block(basic_block_def*, edge_def**, edge_def**)
        ../../gcc/tree-cfg.c:8456
0xace9bf gimple_verify_flow_info
        ../../gcc/tree-cfg.c:5260
0x6ea1ab verify_flow_info()
        ../../gcc/cfghooks.c:260
0xadeca3 cleanup_tree_cfg_noloop
        ../../gcc/tree-cfgcleanup.c:739
0xadeca3 cleanup_tree_cfg()
        ../../gcc/tree-cfgcleanup.c:788
0x9d21c3 execute_function_todo
        ../../gcc/passes.c:1900
0x9d2b07 execute_todo
        ../../gcc/passes.c:2005

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix middle-end/67133, part 1

[Marek Polacek]

On Thu, Aug 20, 2015 at 11:02:17AM +0200, Andreas Schwab wrote:
> Marek Polacek <polacek@redhat.com> writes:
> 
> > 	PR middle-end/67133
> > 	* gimple-ssa-isolate-paths.c
> > 	(insert_trap_and_remove_trailing_statements): Rename to ...
> > 	(insert_trap): ... this.  Don't remove trailing statements; split
> > 	block instead.
> > 	(find_explicit_erroneous_behaviour): Don't remove all outgoing edges.
> 
> This breaks go on aarch64:
> 
> ../../../libgo/go/encoding/gob/decode.go: In function ‘gob.decIgnoreOpFor.pN20_encoding_gob.Decoder’:
> ../../../libgo/go/encoding/gob/decode.go:843:1: internal compiler error: in operator[], at vec.h:714
>  func (dec *Decoder) decIgnoreOpFor(wireId typeId) decOp {
>  ^
> 0xac5c3b vec<edge_def*, va_gc, vl_embed>::operator[](unsigned int)
>         ../../gcc/vec.h:714
> 0xac5c3b extract_true_false_edges_from_block(basic_block_def*, edge_def**, edge_def**)
>         ../../gcc/tree-cfg.c:8456
> 0xace9bf gimple_verify_flow_info
>         ../../gcc/tree-cfg.c:5260
> 0x6ea1ab verify_flow_info()
>         ../../gcc/cfghooks.c:260
> 0xadeca3 cleanup_tree_cfg_noloop
>         ../../gcc/tree-cfgcleanup.c:739
> 0xadeca3 cleanup_tree_cfg()
>         ../../gcc/tree-cfgcleanup.c:788
> 0x9d21c3 execute_function_todo
>         ../../gcc/passes.c:1900
> 0x9d2b07 execute_todo
>         ../../gcc/passes.c:2005

Whilst I'm struggling with building cross libgo to reproduce this, is
there something like preprocessed source for go?  So that ideally I'd
just run ./go1 foo.go?  That'd help tremendously.

	Marek

Re: [PATCH] Fix middle-end/67133, part 1

[Andreas Schwab]

Marek Polacek <polacek@redhat.com> writes:

> Whilst I'm struggling with building cross libgo to reproduce this, is
> there something like preprocessed source for go?

I don't think so.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix middle-end/67133, part 1

[Jeff Law]

On 08/20/2015 04:37 AM, Marek Polacek wrote:
> On Thu, Aug 20, 2015 at 11:02:17AM +0200, Andreas Schwab wrote:
>> Marek Polacek <polacek@redhat.com> writes:
>>
>>> 	PR middle-end/67133
>>> 	* gimple-ssa-isolate-paths.c
>>> 	(insert_trap_and_remove_trailing_statements): Rename to ...
>>> 	(insert_trap): ... this.  Don't remove trailing statements; split
>>> 	block instead.
>>> 	(find_explicit_erroneous_behaviour): Don't remove all outgoing edges.
>>
>> This breaks go on aarch64:
>>
>> ../../../libgo/go/encoding/gob/decode.go: In function ‘gob.decIgnoreOpFor.pN20_encoding_gob.Decoder’:
>> ../../../libgo/go/encoding/gob/decode.go:843:1: internal compiler error: in operator[], at vec.h:714
>>   func (dec *Decoder) decIgnoreOpFor(wireId typeId) decOp {
>>   ^
>> 0xac5c3b vec<edge_def*, va_gc, vl_embed>::operator[](unsigned int)
>>          ../../gcc/vec.h:714
>> 0xac5c3b extract_true_false_edges_from_block(basic_block_def*, edge_def**, edge_def**)
>>          ../../gcc/tree-cfg.c:8456
>> 0xace9bf gimple_verify_flow_info
>>          ../../gcc/tree-cfg.c:5260
>> 0x6ea1ab verify_flow_info()
>>          ../../gcc/cfghooks.c:260
>> 0xadeca3 cleanup_tree_cfg_noloop
>>          ../../gcc/tree-cfgcleanup.c:739
>> 0xadeca3 cleanup_tree_cfg()
>>          ../../gcc/tree-cfgcleanup.c:788
>> 0x9d21c3 execute_function_todo
>>          ../../gcc/passes.c:1900
>> 0x9d2b07 execute_todo
>>          ../../gcc/passes.c:2005
>
> Whilst I'm struggling with building cross libgo to reproduce this, is
> there something like preprocessed source for go?  So that ideally I'd
> just run ./go1 foo.go?  That'd help tremendously.
The process for finding out what Go's doing is, umm, exceedingly 
difficult.  Though at least for gcc-go, using "-v" will help.

RTH might have some ideas.


Based on the error, I suspect we've got a block ending with a 
GIMPLE_COND with no successors in the CFG.

jeff

Re: [PATCH] Fix middle-end/67133, part 1

[Marek Polacek]

On Thu, Aug 20, 2015 at 10:41:28AM -0600, Jeff Law wrote:
> On 08/20/2015 04:37 AM, Marek Polacek wrote:
> >On Thu, Aug 20, 2015 at 11:02:17AM +0200, Andreas Schwab wrote:
> >>Marek Polacek <polacek@redhat.com> writes:
> >>
> >>>	PR middle-end/67133
> >>>	* gimple-ssa-isolate-paths.c
> >>>	(insert_trap_and_remove_trailing_statements): Rename to ...
> >>>	(insert_trap): ... this.  Don't remove trailing statements; split
> >>>	block instead.
> >>>	(find_explicit_erroneous_behaviour): Don't remove all outgoing edges.
> >>
> >>This breaks go on aarch64:
> >>
> >>../../../libgo/go/encoding/gob/decode.go: In function ‘gob.decIgnoreOpFor.pN20_encoding_gob.Decoder’:
> >>../../../libgo/go/encoding/gob/decode.go:843:1: internal compiler error: in operator[], at vec.h:714
> >>  func (dec *Decoder) decIgnoreOpFor(wireId typeId) decOp {
> >>  ^
> >>0xac5c3b vec<edge_def*, va_gc, vl_embed>::operator[](unsigned int)
> >>         ../../gcc/vec.h:714
> >>0xac5c3b extract_true_false_edges_from_block(basic_block_def*, edge_def**, edge_def**)
> >>         ../../gcc/tree-cfg.c:8456
> >>0xace9bf gimple_verify_flow_info
> >>         ../../gcc/tree-cfg.c:5260
> >>0x6ea1ab verify_flow_info()
> >>         ../../gcc/cfghooks.c:260
> >>0xadeca3 cleanup_tree_cfg_noloop
> >>         ../../gcc/tree-cfgcleanup.c:739
> >>0xadeca3 cleanup_tree_cfg()
> >>         ../../gcc/tree-cfgcleanup.c:788
> >>0x9d21c3 execute_function_todo
> >>         ../../gcc/passes.c:1900
> >>0x9d2b07 execute_todo
> >>         ../../gcc/passes.c:2005
> >
> >Whilst I'm struggling with building cross libgo to reproduce this, is
> >there something like preprocessed source for go?  So that ideally I'd
> >just run ./go1 foo.go?  That'd help tremendously.
> The process for finding out what Go's doing is, umm, exceedingly difficult.
> Though at least for gcc-go, using "-v" will help.
 
Yeah :/.  I resorted to adding debug_function (cfun->decl, 0) and also some
debug_bb_n () to see how the cfg looks like...

But at least I have reproduced the ICE.

> RTH might have some ideas.
> 
> 
> Based on the error, I suspect we've got a block ending with a GIMPLE_COND
> with no successors in the CFG.

Except that I'm also seeing a different error:
/home/brq/mpolacek/gcc/libgo/go/text/template/exec.go:303:1: error: wrong
outgoing edge flags at end of bb 6
We have this bb:

<bb 6>:
# iftmp.1693_53 = PHI <0B(4)>
_54 = t_5(D)->Pipe;
GOTMP.163 = template.evalPipeline.pN19_text_template.state (s_7(D), dot, _31);
[return slot optimization]
dot = GOTMP.163;
_61 = __go_new (&__go_tdn_text_template..text_template.state, 64);
*_35 = *s_7(D);
# DEBUG newState => _35
_35->tmpl = iftmp.1693_55;
GOTMP.166.value = dot;
_66 = __go_new (&__go_td_AN22_text_template.variable1e, 40);
SR.4170_67 = "$";
SR.4171_68 = 1;
MEM[(struct .text/template.variable *)&GOTMP.166] = "$";
MEM[(struct .text/template.variable *)&GOTMP.166 + 8B] = 1;
MEM[(struct .text/template.variable[1] *)_40][0] = GOTMP.166;
_35->vars.__values = _40;
_35->vars.__count = 1;
_35->vars.__capacity = 1;
_75 ={v} iftmp.1693_55->Tree;
__builtin_trap ();
_76 = _46->Root;
D.8248.__methods =
&__go_pimt__I25_.text_template_parse.treeFrpN24_text_template_parse.Treeee4_CopyFrN24_text_template_parse.Nodeee8_PositionFrN23_text_template_parse.Posee6_StringFrN6_stringee4_TypeFrN28_text_template_parse.NodeTypeeee__N28_text_template_parse.ListNode;
D.8248.__object = _47;
template.walk.pN19_text_template.state (_35, dot, D.8248);
return;

and single_succ_p (bb) is not satisfied, so it must have more outgoing edges.
Not sure how can that happen...

Looking more.

	Marek

Re: [PATCH] Fix middle-end/67133, part 1

[Jeff Law]

On 08/20/2015 10:51 AM, Marek Polacek wrote:
>>
>> Based on the error, I suspect we've got a block ending with a GIMPLE_COND
>> with no successors in the CFG.
>
> Except that I'm also seeing a different error:
> /home/brq/mpolacek/gcc/libgo/go/text/template/exec.go:303:1: error: wrong
> outgoing edge flags at end of bb 6
> We have this bb:
>
> <bb 6>:
> # iftmp.1693_53 = PHI <0B(4)>
> _54 = t_5(D)->Pipe;
> GOTMP.163 = template.evalPipeline.pN19_text_template.state (s_7(D), dot, _31);
> [return slot optimization]
> dot = GOTMP.163;
> _61 = __go_new (&__go_tdn_text_template..text_template.state, 64);
> *_35 = *s_7(D);
> # DEBUG newState => _35
> _35->tmpl = iftmp.1693_55;
> GOTMP.166.value = dot;
> _66 = __go_new (&__go_td_AN22_text_template.variable1e, 40);
> SR.4170_67 = "$";
> SR.4171_68 = 1;
> MEM[(struct .text/template.variable *)&GOTMP.166] = "$";
> MEM[(struct .text/template.variable *)&GOTMP.166 + 8B] = 1;
> MEM[(struct .text/template.variable[1] *)_40][0] = GOTMP.166;
> _35->vars.__values = _40;
> _35->vars.__count = 1;
> _35->vars.__capacity = 1;
> _75 ={v} iftmp.1693_55->Tree;
> __builtin_trap ();
> _76 = _46->Root;
> D.8248.__methods =
> &__go_pimt__I25_.text_template_parse.treeFrpN24_text_template_parse.Treeee4_CopyFrN24_text_template_parse.Nodeee8_PositionFrN23_text_template_parse.Posee6_StringFrN6_stringee4_TypeFrN28_text_template_parse.NodeTypeeee__N28_text_template_parse.ListNode;
> D.8248.__object = _47;
> template.walk.pN19_text_template.state (_35, dot, D.8248);
> return;
>
> and single_succ_p (bb) is not satisfied, so it must have more outgoing edges.
> Not sure how can that happen...
OK, iftmp.1693_55 is NULL (via the PHI).  We inserted the trap.  That 
looks reasonable.

What does the CFG look like after splitting the block?   There should be 
flags you can pass to get the edges & flags as part of the debugging output.

My first guess would be some kind of exception handling edge, but I 
thought we avoided the transformation in that case.

Hmm, maybe seeing the CFG with edges & flags at the time of trap 
insertion would be useful too.


Jeff

Re: [PATCH] Fix middle-end/67133, part 1

[Marek Polacek]

On Thu, Aug 20, 2015 at 06:51:45PM +0200, Marek Polacek wrote:
> and single_succ_p (bb) is not satisfied, so it must have more outgoing edges.
> Not sure how can that happen...

Actually the problem seems to be that the BB ends with return but it has
*no* outgoing edges.

	Marek

Re: [PATCH] Fix middle-end/67133, part 1

[Jeff Law]

On 08/20/2015 11:00 AM, Marek Polacek wrote:
> On Thu, Aug 20, 2015 at 06:51:45PM +0200, Marek Polacek wrote:
>> and single_succ_p (bb) is not satisfied, so it must have more outgoing edges.
>> Not sure how can that happen...
>
> Actually the problem seems to be that the BB ends with return but it has
> *no* outgoing edges.
But isn't that block removed -- it ought to be unreachable -- unless 
something didn't handle splitting the block after the trap.

jeff

Re: [PATCH] Fix middle-end/67133, part 1

[Jan-Benedict Glaw]

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

On Thu, 2015-08-20 11:02:17 +0200, Andreas Schwab <schwab@suse.de> wrote:
> Marek Polacek <polacek@redhat.com> writes:
> 
> > 	PR middle-end/67133
> > 	* gimple-ssa-isolate-paths.c
> > 	(insert_trap_and_remove_trailing_statements): Rename to ...
> > 	(insert_trap): ... this.  Don't remove trailing statements; split
> > 	block instead.
> > 	(find_explicit_erroneous_behaviour): Don't remove all outgoing edges.
> 
> This breaks go on aarch64:
> 
> ../../../libgo/go/encoding/gob/decode.go: In function ‘gob.decIgnoreOpFor.pN20_encoding_gob.Decoder’:
> ../../../libgo/go/encoding/gob/decode.go:843:1: internal compiler error: in operator[], at vec.h:714
>  func (dec *Decoder) decIgnoreOpFor(wireId typeId) decOp {
>  ^
> 0xac5c3b vec<edge_def*, va_gc, vl_embed>::operator[](unsigned int)
>         ../../gcc/vec.h:714
> 0xac5c3b extract_true_false_edges_from_block(basic_block_def*, edge_def**, edge_def**)
>         ../../gcc/tree-cfg.c:8456
> 0xace9bf gimple_verify_flow_info
>         ../../gcc/tree-cfg.c:5260
> 0x6ea1ab verify_flow_info()
>         ../../gcc/cfghooks.c:260
> 0xadeca3 cleanup_tree_cfg_noloop
>         ../../gcc/tree-cfgcleanup.c:739
> 0xadeca3 cleanup_tree_cfg()
>         ../../gcc/tree-cfgcleanup.c:788
> 0x9d21c3 execute_function_todo
>         ../../gcc/passes.c:1900
> 0x9d2b07 execute_todo
>         ../../gcc/passes.c:2005
> "And now for something completely different."

Not quite. As it seems, it breaks any build:

	configure --prefix=... --disable-multilib --enable-languages=all,ada,go
	make

Seems the Go code triggers this ICE, so you just need to enable Go.

MfG, JBG

-- 
      Jan-Benedict Glaw      jbglaw@lug-owl.de              +49-172-7608481
Signature of:                     Eine Freie Meinung in einem Freien Kopf
the second  :                   für einen Freien Staat voll Freier Bürger.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] Fix middle-end/67133, part 1

[Jeff Law]

On 08/23/2015 04:40 AM, Jan-Benedict Glaw wrote:
> On Thu, 2015-08-20 11:02:17 +0200, Andreas Schwab <schwab@suse.de> wrote:
>> Marek Polacek <polacek@redhat.com> writes:
>>
>>> 	PR middle-end/67133
>>> 	* gimple-ssa-isolate-paths.c
>>> 	(insert_trap_and_remove_trailing_statements): Rename to ...
>>> 	(insert_trap): ... this.  Don't remove trailing statements; split
>>> 	block instead.
>>> 	(find_explicit_erroneous_behaviour): Don't remove all outgoing edges.
>>
>> This breaks go on aarch64:
>>
>> ../../../libgo/go/encoding/gob/decode.go: In function ‘gob.decIgnoreOpFor.pN20_encoding_gob.Decoder’:
>> ../../../libgo/go/encoding/gob/decode.go:843:1: internal compiler error: in operator[], at vec.h:714
>>   func (dec *Decoder) decIgnoreOpFor(wireId typeId) decOp {
>>   ^
>> 0xac5c3b vec<edge_def*, va_gc, vl_embed>::operator[](unsigned int)
>>          ../../gcc/vec.h:714
>> 0xac5c3b extract_true_false_edges_from_block(basic_block_def*, edge_def**, edge_def**)
>>          ../../gcc/tree-cfg.c:8456
>> 0xace9bf gimple_verify_flow_info
>>          ../../gcc/tree-cfg.c:5260
>> 0x6ea1ab verify_flow_info()
>>          ../../gcc/cfghooks.c:260
>> 0xadeca3 cleanup_tree_cfg_noloop
>>          ../../gcc/tree-cfgcleanup.c:739
>> 0xadeca3 cleanup_tree_cfg()
>>          ../../gcc/tree-cfgcleanup.c:788
>> 0x9d21c3 execute_function_todo
>>          ../../gcc/passes.c:1900
>> 0x9d2b07 execute_todo
>>          ../../gcc/passes.c:2005
>> "And now for something completely different."
>
> Not quite. As it seems, it breaks any build:
>
> 	configure --prefix=... --disable-multilib --enable-languages=all,ada,go
> 	make
>
> Seems the Go code triggers this ICE, so you just need to enable Go.
Right.  Marek is working on the problem.  In fact, I think he and Ian 
have agreed on a patch to the Go front-end to fix this issue.

jeff

Re: [PATCH] Fix middle-end/67133, part 1

[Marek Polacek]

On Mon, Aug 24, 2015 at 09:54:31AM -0600, Jeff Law wrote:
> >Not quite. As it seems, it breaks any build:
> >
> >	configure --prefix=... --disable-multilib --enable-languages=all,ada,go
> >	make
> >
> >Seems the Go code triggers this ICE, so you just need to enable Go.
> Right.  Marek is working on the problem.  In fact, I think he and Ian have
> agreed on a patch to the Go front-end to fix this issue.

I've now committed the patch so libgo ought to build again.

	Marek