[PATCH/RFC] Fix ICE in find_taken_edge_computed_goto (PR 84136)

[PATCH/RFC] Fix ICE in find_taken_edge_computed_goto (PR 84136)

[David Malcolm]

PR 84136 reports an ICE within sccvn_dom_walker when handling a
C/C++ source file that overuses the labels-as-values extension.
The code in question stores a jump label into a global, and then
jumps to it from another function, which ICEs after inlining:

void* a;

void foo() {
  if ((a = &&l))
      return;

  l:;
}

int main() {
  foo();
  goto *a;

  return 0;
}

This appears to be far beyond what we claim to support in this
extension - but we shouldn't ICE.

What's happening is that, after inlining, we have usage of a *copy*
of the label, which optimizes away the if-return logic, turning it
into an infinite loop.

On entry to the sccvn_dom_walker we have this gimple:

main ()
{
  void * a.0_1;

  <bb 2> [count: 0]:
  a = &l;

  <bb 3> [count: 0]:
l:
  a.0_1 = a;
  goto a.0_1;
}

and:
  edge taken = find_taken_edge (bb, vn_valueize (val));
reasonably valueizes the:
  goto a.0_1;
after the:
  a = &l;
  a.0_1 = a;
as if it were:
  goto *&l;

find_taken_edge_computed_goto then has:

2380	  dest = label_to_block (val);
2381	  if (dest)
2382	    {
2383	      e = find_edge (bb, dest);
2384	      gcc_assert (e != NULL);
2385	    }

which locates dest as a self-jump from block 3 back to itself.

However, the find_edge call returns NULL - it has a predecessor edge
from block 2, but no successor edges.

Hence the assertion fails and we ICE.

A successor edge from the computed goto could have been created by
make_edges if the label stmt had been in the function, but make_edges
only looks in the current function when handling computed gotos, and
the label only appeared after inlining.

The following patch removes the assertion, fixing the ICE.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.

If that's option (a), there could be some other approaches:

(b) convert the assertion into a warning/error/sorry, on the
    assumption that if we don't detect such an edge then the code is
    presumably abusing the labels-as-values feature
(c) have make_edges detect such a problematic computed goto (maybe
    converting make_edges_bb's return value to an enum and adding a 4th
    value - though it's not clear what to do then with it)
(d) detect this case on inlining and handle it somehow (e.g. adding
    edges for labels that have appeared since make_edges originally
    ran, for computed gotos that have no out-edges)
(e) do nothing, keeping the assertion, and accept that this is going
    to fail on a non-release build
(f) something else?

Of the above, (d) seems to me to be the most robust solution, but I
don't know how far we want to go "down the rabbit hole" of handling
such uses of labels-as-values (beyond not ICE-ing on them).

Thoughts?

gcc/testsuite/ChangeLog:
	PR tree-optimization/84136
	* gcc.c-torture/compile/pr84136.c: New test.

gcc/ChangeLog:
	PR tree-optimization/84136
	* tree-cfg.c (find_taken_edge_computed_goto): Remove assertion
	that the result of find_edge is non-NULL.
---
 gcc/testsuite/gcc.c-torture/compile/pr84136.c | 15 +++++++++++++++
 gcc/tree-cfg.c                                |  5 +----
 2 files changed, 16 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr84136.c

diff --git a/gcc/testsuite/gcc.c-torture/compile/pr84136.c b/gcc/testsuite/gcc.c-torture/compile/pr84136.c
new file mode 100644
index 0000000..0a70e4e
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr84136.c
@@ -0,0 +1,15 @@
+void* a;
+
+void foo() {
+    if ((a = &&l))
+        return;
+
+    l:;
+}
+
+int main() {
+    foo();
+    goto *a;
+
+    return 0;
+}
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index c5318b9..6b89307 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -2379,10 +2379,7 @@ find_taken_edge_computed_goto (basic_block bb, tree val)
 
   dest = label_to_block (val);
   if (dest)
-    {
-      e = find_edge (bb, dest);
-      gcc_assert (e != NULL);
-    }
+    e = find_edge (bb, dest);
 
   return e;
 }
-- 
1.8.5.3

Re: [PATCH/RFC] Fix ICE in find_taken_edge_computed_goto (PR 84136)

[Martin Sebor]

On 01/31/2018 08:39 AM, David Malcolm wrote:
> PR 84136 reports an ICE within sccvn_dom_walker when handling a
> C/C++ source file that overuses the labels-as-values extension.
> The code in question stores a jump label into a global, and then
> jumps to it from another function, which ICEs after inlining:
>
> void* a;
>
> void foo() {
>   if ((a = &&l))
>       return;
>
>   l:;
> }
>
> int main() {
>   foo();
>   goto *a;
>
>   return 0;
> }
>
> This appears to be far beyond what we claim to support in this
> extension - but we shouldn't ICE.
>
> What's happening is that, after inlining, we have usage of a *copy*
> of the label, which optimizes away the if-return logic, turning it
> into an infinite loop.
>
> On entry to the sccvn_dom_walker we have this gimple:
>
> main ()
> {
>   void * a.0_1;
>
>   <bb 2> [count: 0]:
>   a = &l;
>
>   <bb 3> [count: 0]:
> l:
>   a.0_1 = a;
>   goto a.0_1;
> }
>
> and:
>   edge taken = find_taken_edge (bb, vn_valueize (val));
> reasonably valueizes the:
>   goto a.0_1;
> after the:
>   a = &l;
>   a.0_1 = a;
> as if it were:
>   goto *&l;
>
> find_taken_edge_computed_goto then has:
>
> 2380	  dest = label_to_block (val);
> 2381	  if (dest)
> 2382	    {
> 2383	      e = find_edge (bb, dest);
> 2384	      gcc_assert (e != NULL);
> 2385	    }
>
> which locates dest as a self-jump from block 3 back to itself.
>
> However, the find_edge call returns NULL - it has a predecessor edge
> from block 2, but no successor edges.
>
> Hence the assertion fails and we ICE.
>
> A successor edge from the computed goto could have been created by
> make_edges if the label stmt had been in the function, but make_edges
> only looks in the current function when handling computed gotos, and
> the label only appeared after inlining.
>
> The following patch removes the assertion, fixing the ICE.
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
>
> If that's option (a), there could be some other approaches:
>
> (b) convert the assertion into a warning/error/sorry, on the
>     assumption that if we don't detect such an edge then the code is
>     presumably abusing the labels-as-values feature
> (c) have make_edges detect such a problematic computed goto (maybe
>     converting make_edges_bb's return value to an enum and adding a 4th
>     value - though it's not clear what to do then with it)
> (d) detect this case on inlining and handle it somehow (e.g. adding
>     edges for labels that have appeared since make_edges originally
>     ran, for computed gotos that have no out-edges)
> (e) do nothing, keeping the assertion, and accept that this is going
>     to fail on a non-release build
> (f) something else?
>
> Of the above, (d) seems to me to be the most robust solution, but I
> don't know how far we want to go "down the rabbit hole" of handling
> such uses of labels-as-values (beyond not ICE-ing on them).
>
> Thoughts?

If the source code is invalid or unsupported as you say and
should stay that way then one of the alternatives mentioned
in (b) sounds preferable to me, at least in the near term.
It will make it clear to users that it is, in fact, not
supported.

Loner term, I agree that (d) seems ideal if it's feasible
(I have very little experience with this extension to judge
that, or even how useful it might be).

Martin

>
> gcc/testsuite/ChangeLog:
> 	PR tree-optimization/84136
> 	* gcc.c-torture/compile/pr84136.c: New test.
>
> gcc/ChangeLog:
> 	PR tree-optimization/84136
> 	* tree-cfg.c (find_taken_edge_computed_goto): Remove assertion
> 	that the result of find_edge is non-NULL.
> ---
>  gcc/testsuite/gcc.c-torture/compile/pr84136.c | 15 +++++++++++++++
>  gcc/tree-cfg.c                                |  5 +----
>  2 files changed, 16 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr84136.c
>
> diff --git a/gcc/testsuite/gcc.c-torture/compile/pr84136.c b/gcc/testsuite/gcc.c-torture/compile/pr84136.c
> new file mode 100644
> index 0000000..0a70e4e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/compile/pr84136.c
> @@ -0,0 +1,15 @@
> +void* a;
> +
> +void foo() {
> +    if ((a = &&l))
> +        return;
> +
> +    l:;
> +}
> +
> +int main() {
> +    foo();
> +    goto *a;
> +
> +    return 0;
> +}
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index c5318b9..6b89307 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -2379,10 +2379,7 @@ find_taken_edge_computed_goto (basic_block bb, tree val)
>
>    dest = label_to_block (val);
>    if (dest)
> -    {
> -      e = find_edge (bb, dest);
> -      gcc_assert (e != NULL);
> -    }
> +    e = find_edge (bb, dest);
>
>    return e;
>  }
>

Re: [PATCH/RFC] Fix ICE in find_taken_edge_computed_goto (PR 84136)

[Richard Biener]

On Wed, Jan 31, 2018 at 4:39 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> PR 84136 reports an ICE within sccvn_dom_walker when handling a
> C/C++ source file that overuses the labels-as-values extension.
> The code in question stores a jump label into a global, and then
> jumps to it from another function, which ICEs after inlining:
>
> void* a;
>
> void foo() {
>   if ((a = &&l))
>       return;
>
>   l:;
> }
>
> int main() {
>   foo();
>   goto *a;
>
>   return 0;
> }
>
> This appears to be far beyond what we claim to support in this
> extension - but we shouldn't ICE.
>
> What's happening is that, after inlining, we have usage of a *copy*
> of the label, which optimizes away the if-return logic, turning it
> into an infinite loop.
>
> On entry to the sccvn_dom_walker we have this gimple:
>
> main ()
> {
>   void * a.0_1;
>
>   <bb 2> [count: 0]:
>   a = &l;
>
>   <bb 3> [count: 0]:
> l:
>   a.0_1 = a;
>   goto a.0_1;
> }
>
> and:
>   edge taken = find_taken_edge (bb, vn_valueize (val));
> reasonably valueizes the:
>   goto a.0_1;
> after the:
>   a = &l;
>   a.0_1 = a;
> as if it were:
>   goto *&l;
>
> find_taken_edge_computed_goto then has:
>
> 2380      dest = label_to_block (val);
> 2381      if (dest)
> 2382        {
> 2383          e = find_edge (bb, dest);
> 2384          gcc_assert (e != NULL);
> 2385        }
>
> which locates dest as a self-jump from block 3 back to itself.
>
> However, the find_edge call returns NULL - it has a predecessor edge
> from block 2, but no successor edges.
>
> Hence the assertion fails and we ICE.
>
> A successor edge from the computed goto could have been created by
> make_edges if the label stmt had been in the function, but make_edges
> only looks in the current function when handling computed gotos, and
> the label only appeared after inlining.
>
> The following patch removes the assertion, fixing the ICE.
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
>
> If that's option (a), there could be some other approaches:
>
> (b) convert the assertion into a warning/error/sorry, on the
>     assumption that if we don't detect such an edge then the code is
>     presumably abusing the labels-as-values feature
> (c) have make_edges detect such a problematic computed goto (maybe
>     converting make_edges_bb's return value to an enum and adding a 4th
>     value - though it's not clear what to do then with it)
> (d) detect this case on inlining and handle it somehow (e.g. adding
>     edges for labels that have appeared since make_edges originally
>     ran, for computed gotos that have no out-edges)
> (e) do nothing, keeping the assertion, and accept that this is going
>     to fail on a non-release build
> (f) something else?
>
> Of the above, (d) seems to me to be the most robust solution, but I
> don't know how far we want to go "down the rabbit hole" of handling
> such uses of labels-as-values (beyond not ICE-ing on them).
>
> Thoughts?

I think you can preserve the assert for ! DECL_NONLOCAL (val) thus

gcc_assert (e != NULL || DECL_NONLOCAL (val));

does the label in this case properly have DECL_NONLOCAL set?  Probably
not given we shouldn't have duplicated it in this case.  So the issue is really
that the FE doesn't set this bit for "escaped" labels... but I'm not sure how
to easily constrain the extension here.

The label should be FORCED_LABEL though so that's maybe a weaker
check.

Joseph?

Richard.

>
> gcc/testsuite/ChangeLog:
>         PR tree-optimization/84136
>         * gcc.c-torture/compile/pr84136.c: New test.
>
> gcc/ChangeLog:
>         PR tree-optimization/84136
>         * tree-cfg.c (find_taken_edge_computed_goto): Remove assertion
>         that the result of find_edge is non-NULL.
> ---
>  gcc/testsuite/gcc.c-torture/compile/pr84136.c | 15 +++++++++++++++
>  gcc/tree-cfg.c                                |  5 +----
>  2 files changed, 16 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr84136.c
>
> diff --git a/gcc/testsuite/gcc.c-torture/compile/pr84136.c b/gcc/testsuite/gcc.c-torture/compile/pr84136.c
> new file mode 100644
> index 0000000..0a70e4e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/compile/pr84136.c
> @@ -0,0 +1,15 @@
> +void* a;
> +
> +void foo() {
> +    if ((a = &&l))
> +        return;
> +
> +    l:;
> +}
> +
> +int main() {
> +    foo();
> +    goto *a;
> +
> +    return 0;
> +}
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index c5318b9..6b89307 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -2379,10 +2379,7 @@ find_taken_edge_computed_goto (basic_block bb, tree val)
>
>    dest = label_to_block (val);
>    if (dest)
> -    {
> -      e = find_edge (bb, dest);
> -      gcc_assert (e != NULL);
> -    }
> +    e = find_edge (bb, dest);
>
>    return e;
>  }
> --
> 1.8.5.3
>

Re: [PATCH/RFC] Fix ICE in find_taken_edge_computed_goto (PR 84136)

[David Malcolm]

On Thu, 2018-02-01 at 12:05 +0100, Richard Biener wrote:
> On Wed, Jan 31, 2018 at 4:39 PM, David Malcolm <dmalcolm@redhat.com>
> wrote:
> > PR 84136 reports an ICE within sccvn_dom_walker when handling a
> > C/C++ source file that overuses the labels-as-values extension.
> > The code in question stores a jump label into a global, and then
> > jumps to it from another function, which ICEs after inlining:
> > 
> > void* a;
> > 
> > void foo() {
> >   if ((a = &&l))
> >       return;
> > 
> >   l:;
> > }
> > 
> > int main() {
> >   foo();
> >   goto *a;
> > 
> >   return 0;
> > }
> > 
> > This appears to be far beyond what we claim to support in this
> > extension - but we shouldn't ICE.
> > 
> > What's happening is that, after inlining, we have usage of a *copy*
> > of the label, which optimizes away the if-return logic, turning it
> > into an infinite loop.
> > 
> > On entry to the sccvn_dom_walker we have this gimple:
> > 
> > main ()
> > {
> >   void * a.0_1;
> > 
> >   <bb 2> [count: 0]:
> >   a = &l;
> > 
> >   <bb 3> [count: 0]:
> > l:
> >   a.0_1 = a;
> >   goto a.0_1;
> > }
> > 
> > and:
> >   edge taken = find_taken_edge (bb, vn_valueize (val));
> > reasonably valueizes the:
> >   goto a.0_1;
> > after the:
> >   a = &l;
> >   a.0_1 = a;
> > as if it were:
> >   goto *&l;
> > 
> > find_taken_edge_computed_goto then has:
> > 
> > 2380      dest = label_to_block (val);
> > 2381      if (dest)
> > 2382        {
> > 2383          e = find_edge (bb, dest);
> > 2384          gcc_assert (e != NULL);
> > 2385        }
> > 
> > which locates dest as a self-jump from block 3 back to itself.
> > 
> > However, the find_edge call returns NULL - it has a predecessor
> > edge
> > from block 2, but no successor edges.
> > 
> > Hence the assertion fails and we ICE.
> > 
> > A successor edge from the computed goto could have been created by
> > make_edges if the label stmt had been in the function, but
> > make_edges
> > only looks in the current function when handling computed gotos,
> > and
> > the label only appeared after inlining.
> > 
> > The following patch removes the assertion, fixing the ICE.
> > 
> > Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
> > 
> > If that's option (a), there could be some other approaches:
> > 
> > (b) convert the assertion into a warning/error/sorry, on the
> >     assumption that if we don't detect such an edge then the code
> > is
> >     presumably abusing the labels-as-values feature
> > (c) have make_edges detect such a problematic computed goto (maybe
> >     converting make_edges_bb's return value to an enum and adding a
> > 4th
> >     value - though it's not clear what to do then with it)
> > (d) detect this case on inlining and handle it somehow (e.g. adding
> >     edges for labels that have appeared since make_edges originally
> >     ran, for computed gotos that have no out-edges)
> > (e) do nothing, keeping the assertion, and accept that this is
> > going
> >     to fail on a non-release build
> > (f) something else?
> > 
> > Of the above, (d) seems to me to be the most robust solution, but I
> > don't know how far we want to go "down the rabbit hole" of handling
> > such uses of labels-as-values (beyond not ICE-ing on them).
> > 
> > Thoughts?
> 
> I think you can preserve the assert for ! DECL_NONLOCAL (val) thus
> 
> gcc_assert (e != NULL || DECL_NONLOCAL (val));
> 
> does the label in this case properly have DECL_NONLOCAL
> set?  Probably
> not given we shouldn't have duplicated it in this case.  

Indeed, the inlined copy of the label doesn't have DECL_NONLOCAL set:

(gdb) p val->decl_common.nonlocal_flag 
$5 = 0

> So the issue is really
> that the FE doesn't set this bit for "escaped" labels... but I'm not
> sure how
> to easily constrain the extension here.
> 
> The label should be FORCED_LABEL though so that's maybe a weaker
> check.

It does have FORCED_LABEL set:

(gdb) p val->base.side_effects_flag 
$6 = 1

...though presumably that's going to be set for just about any label
that a computed goto jumps to?  Hence this is presumably of little
benefit for adjusting the assertion.

> Joseph?
> 
> Richard.
> 
> > 
> > gcc/testsuite/ChangeLog:
> >         PR tree-optimization/84136
> >         * gcc.c-torture/compile/pr84136.c: New test.
> > 
> > gcc/ChangeLog:
> >         PR tree-optimization/84136
> >         * tree-cfg.c (find_taken_edge_computed_goto): Remove
> > assertion
> >         that the result of find_edge is non-NULL.
> > ---
> >  gcc/testsuite/gcc.c-torture/compile/pr84136.c | 15 +++++++++++++++
> >  gcc/tree-cfg.c                                |  5 +----
> >  2 files changed, 16 insertions(+), 4 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr84136.c
> > 
> > diff --git a/gcc/testsuite/gcc.c-torture/compile/pr84136.c
> > b/gcc/testsuite/gcc.c-torture/compile/pr84136.c
> > new file mode 100644
> > index 0000000..0a70e4e
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.c-torture/compile/pr84136.c
> > @@ -0,0 +1,15 @@
> > +void* a;
> > +
> > +void foo() {
> > +    if ((a = &&l))
> > +        return;
> > +
> > +    l:;
> > +}
> > +
> > +int main() {
> > +    foo();
> > +    goto *a;
> > +
> > +    return 0;
> > +}
> > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> > index c5318b9..6b89307 100644
> > --- a/gcc/tree-cfg.c
> > +++ b/gcc/tree-cfg.c
> > @@ -2379,10 +2379,7 @@ find_taken_edge_computed_goto (basic_block
> > bb, tree val)
> > 
> >    dest = label_to_block (val);
> >    if (dest)
> > -    {
> > -      e = find_edge (bb, dest);
> > -      gcc_assert (e != NULL);
> > -    }
> > +    e = find_edge (bb, dest);
> > 
> >    return e;
> >  }
> > --
> > 1.8.5.3
> > 

Re: [PATCH/RFC] Fix ICE in find_taken_edge_computed_goto (PR 84136)

[Jeff Law]

On 01/31/2018 08:39 AM, David Malcolm wrote:
> PR 84136 reports an ICE within sccvn_dom_walker when handling a
> C/C++ source file that overuses the labels-as-values extension.
> The code in question stores a jump label into a global, and then
> jumps to it from another function, which ICEs after inlining:
That's not "overuse", that's simply invalid.

The docs are pretty clear.  Quoting from extend.texi:

--

You may not use this mechanism to jump to code in a different function.
If you do that, totally unpredictable things happen.  The best way to
avoid this is to store the label address only in automatic variables and
never pass it as an argument.

--

So at the source level this is bogus.  THe fact that inlining brings the
context of foo into main is beside the point.

> 
> This appears to be far beyond what we claim to support in this
> extension - but we shouldn't ICE.
Agreed.  An ICE is bad.  One could make the argument that we should warn
on an escaped label value, but I doubt it's worth the headache.

> 
> What's happening is that, after inlining, we have usage of a *copy*
> of the label, which optimizes away the if-return logic, turning it
> into an infinite loop.
> 
> On entry to the sccvn_dom_walker we have this gimple:
> 
> main ()
> {
>   void * a.0_1;
> 
>   <bb 2> [count: 0]:
>   a = &l;
> 
>   <bb 3> [count: 0]:
> l:
>   a.0_1 = a;
>   goto a.0_1;
> }

ACK.

> 
> and:
>   edge taken = find_taken_edge (bb, vn_valueize (val));
> reasonably valueizes the:
>   goto a.0_1;
> after the:
>   a = &l;
>   a.0_1 = a;
> as if it were:
>   goto *&l;
> 
> find_taken_edge_computed_goto then has:
> 
> 2380	  dest = label_to_block (val);
> 2381	  if (dest)
> 2382	    {
> 2383	      e = find_edge (bb, dest);
> 2384	      gcc_assert (e != NULL);
> 2385	    }
> 
> which locates dest as a self-jump from block 3 back to itself.
> 
> However, the find_edge call returns NULL - it has a predecessor edge
> from block 2, but no successor edges.
> 
> Hence the assertion fails and we ICE.
> 
> A successor edge from the computed goto could have been created by
> make_edges if the label stmt had been in the function, but make_edges
> only looks in the current function when handling computed gotos, and
> the label only appeared after inlining.
Which is the core problem.  Invalid input which breaks assumptions later.

I do not believe make_edges should be looking outside the current function.


> 
> The following patch removes the assertion, fixing the ICE.
> 
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
> 
> If that's option (a), there could be some other approaches:
> 
> (b) convert the assertion into a warning/error/sorry, on the
>     assumption that if we don't detect such an edge then the code is
>     presumably abusing the labels-as-values feature
> (c) have make_edges detect such a problematic computed goto (maybe
>     converting make_edges_bb's return value to an enum and adding a 4th
>     value - though it's not clear what to do then with it)
> (d) detect this case on inlining and handle it somehow (e.g. adding
>     edges for labels that have appeared since make_edges originally
>     ran, for computed gotos that have no out-edges)
> (e) do nothing, keeping the assertion, and accept that this is going
>     to fail on a non-release build
> (f) something else?
> 
> Of the above, (d) seems to me to be the most robust solution, but I
> don't know how far we want to go "down the rabbit hole" of handling
> such uses of labels-as-values (beyond not ICE-ing on them).
(b) and (d) seem like the best choices to me.

The problem with (d) is we don't have any kind of reasonable escape
analysis for the label.  Even if you can determine the label escapes,
does the mere escape trigger an error?  I could argue either way.


So I'd lean towards (b).  Though it seems awful late in the pipeline to
diagnose this kind of error.

Jeff

Re: [PATCH/RFC] Fix ICE in find_taken_edge_computed_goto (PR 84136)

[Jeff Law]

On 02/01/2018 04:05 AM, Richard Biener wrote:
> On Wed, Jan 31, 2018 at 4:39 PM, David Malcolm <dmalcolm@redhat.com> wrote:
>> PR 84136 reports an ICE within sccvn_dom_walker when handling a
>> C/C++ source file that overuses the labels-as-values extension.
>> The code in question stores a jump label into a global, and then
>> jumps to it from another function, which ICEs after inlining:
>>
>> void* a;
>>
>> void foo() {
>>   if ((a = &&l))
>>       return;
>>
>>   l:;
>> }
>>
>> int main() {
>>   foo();
>>   goto *a;
>>
>>   return 0;
>> }
>>
>> This appears to be far beyond what we claim to support in this
>> extension - but we shouldn't ICE.
>>
>> What's happening is that, after inlining, we have usage of a *copy*
>> of the label, which optimizes away the if-return logic, turning it
>> into an infinite loop.
>>
>> On entry to the sccvn_dom_walker we have this gimple:
>>
>> main ()
>> {
>>   void * a.0_1;
>>
>>   <bb 2> [count: 0]:
>>   a = &l;
>>
>>   <bb 3> [count: 0]:
>> l:
>>   a.0_1 = a;
>>   goto a.0_1;
>> }
>>
>> and:
>>   edge taken = find_taken_edge (bb, vn_valueize (val));
>> reasonably valueizes the:
>>   goto a.0_1;
>> after the:
>>   a = &l;
>>   a.0_1 = a;
>> as if it were:
>>   goto *&l;
>>
>> find_taken_edge_computed_goto then has:
>>
>> 2380      dest = label_to_block (val);
>> 2381      if (dest)
>> 2382        {
>> 2383          e = find_edge (bb, dest);
>> 2384          gcc_assert (e != NULL);
>> 2385        }
>>
>> which locates dest as a self-jump from block 3 back to itself.
>>
>> However, the find_edge call returns NULL - it has a predecessor edge
>> from block 2, but no successor edges.
>>
>> Hence the assertion fails and we ICE.
>>
>> A successor edge from the computed goto could have been created by
>> make_edges if the label stmt had been in the function, but make_edges
>> only looks in the current function when handling computed gotos, and
>> the label only appeared after inlining.
>>
>> The following patch removes the assertion, fixing the ICE.
>>
>> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
>>
>> If that's option (a), there could be some other approaches:
>>
>> (b) convert the assertion into a warning/error/sorry, on the
>>     assumption that if we don't detect such an edge then the code is
>>     presumably abusing the labels-as-values feature
>> (c) have make_edges detect such a problematic computed goto (maybe
>>     converting make_edges_bb's return value to an enum and adding a 4th
>>     value - though it's not clear what to do then with it)
>> (d) detect this case on inlining and handle it somehow (e.g. adding
>>     edges for labels that have appeared since make_edges originally
>>     ran, for computed gotos that have no out-edges)
>> (e) do nothing, keeping the assertion, and accept that this is going
>>     to fail on a non-release build
>> (f) something else?
>>
>> Of the above, (d) seems to me to be the most robust solution, but I
>> don't know how far we want to go "down the rabbit hole" of handling
>> such uses of labels-as-values (beyond not ICE-ing on them).
>>
>> Thoughts?
> 
> I think you can preserve the assert for ! DECL_NONLOCAL (val) thus
> 
> gcc_assert (e != NULL || DECL_NONLOCAL (val));
> 
> does the label in this case properly have DECL_NONLOCAL set?  Probably
> not given we shouldn't have duplicated it in this case.  So the issue is really
> that the FE doesn't set this bit for "escaped" labels... but I'm not sure how
> to easily constrain the extension here.
> 
> The label should be FORCED_LABEL though so that's maybe a weaker
> check.
As David mentioned, I don't think that checking FORCED_LABEL is going to
be useful here.

Ideally we'd tighten the extension's language so that we could issue an
error out of the front-end.


Jeff

Re: [PATCH/RFC] Fix ICE in find_taken_edge_computed_goto (PR 84136)

[Jeff Law]

On 02/02/2018 02:35 PM, David Malcolm wrote:
> On Thu, 2018-02-01 at 12:05 +0100, Richard Biener wrote:
>> On Wed, Jan 31, 2018 at 4:39 PM, David Malcolm <dmalcolm@redhat.com>
>> wrote:
>>> PR 84136 reports an ICE within sccvn_dom_walker when handling a
>>> C/C++ source file that overuses the labels-as-values extension.
>>> The code in question stores a jump label into a global, and then
>>> jumps to it from another function, which ICEs after inlining:
>>>
>>> void* a;
>>>
>>> void foo() {
>>>   if ((a = &&l))
>>>       return;
>>>
>>>   l:;
>>> }
>>>
>>> int main() {
>>>   foo();
>>>   goto *a;
>>>
>>>   return 0;
>>> }
>>>
>>> This appears to be far beyond what we claim to support in this
>>> extension - but we shouldn't ICE.
>>>
>>> What's happening is that, after inlining, we have usage of a *copy*
>>> of the label, which optimizes away the if-return logic, turning it
>>> into an infinite loop.
>>>
>>> On entry to the sccvn_dom_walker we have this gimple:
>>>
>>> main ()
>>> {
>>>   void * a.0_1;
>>>
>>>   <bb 2> [count: 0]:
>>>   a = &l;
>>>
>>>   <bb 3> [count: 0]:
>>> l:
>>>   a.0_1 = a;
>>>   goto a.0_1;
>>> }
>>>
>>> and:
>>>   edge taken = find_taken_edge (bb, vn_valueize (val));
>>> reasonably valueizes the:
>>>   goto a.0_1;
>>> after the:
>>>   a = &l;
>>>   a.0_1 = a;
>>> as if it were:
>>>   goto *&l;
>>>
>>> find_taken_edge_computed_goto then has:
>>>
>>> 2380      dest = label_to_block (val);
>>> 2381      if (dest)
>>> 2382        {
>>> 2383          e = find_edge (bb, dest);
>>> 2384          gcc_assert (e != NULL);
>>> 2385        }
>>>
>>> which locates dest as a self-jump from block 3 back to itself.
>>>
>>> However, the find_edge call returns NULL - it has a predecessor
>>> edge
>>> from block 2, but no successor edges.
>>>
>>> Hence the assertion fails and we ICE.
>>>
>>> A successor edge from the computed goto could have been created by
>>> make_edges if the label stmt had been in the function, but
>>> make_edges
>>> only looks in the current function when handling computed gotos,
>>> and
>>> the label only appeared after inlining.
>>>
>>> The following patch removes the assertion, fixing the ICE.
>>>
>>> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
>>>
>>> If that's option (a), there could be some other approaches:
>>>
>>> (b) convert the assertion into a warning/error/sorry, on the
>>>     assumption that if we don't detect such an edge then the code
>>> is
>>>     presumably abusing the labels-as-values feature
>>> (c) have make_edges detect such a problematic computed goto (maybe
>>>     converting make_edges_bb's return value to an enum and adding a
>>> 4th
>>>     value - though it's not clear what to do then with it)
>>> (d) detect this case on inlining and handle it somehow (e.g. adding
>>>     edges for labels that have appeared since make_edges originally
>>>     ran, for computed gotos that have no out-edges)
>>> (e) do nothing, keeping the assertion, and accept that this is
>>> going
>>>     to fail on a non-release build
>>> (f) something else?
>>>
>>> Of the above, (d) seems to me to be the most robust solution, but I
>>> don't know how far we want to go "down the rabbit hole" of handling
>>> such uses of labels-as-values (beyond not ICE-ing on them).
>>>
>>> Thoughts?
>>
>> I think you can preserve the assert for ! DECL_NONLOCAL (val) thus
>>
>> gcc_assert (e != NULL || DECL_NONLOCAL (val));
>>
>> does the label in this case properly have DECL_NONLOCAL
>> set?  Probably
>> not given we shouldn't have duplicated it in this case.  
> 
> Indeed, the inlined copy of the label doesn't have DECL_NONLOCAL set:
> 
> (gdb) p val->decl_common.nonlocal_flag 
> $5 = 0
> 
>> So the issue is really
>> that the FE doesn't set this bit for "escaped" labels... but I'm not
>> sure how
>> to easily constrain the extension here.
>>
>> The label should be FORCED_LABEL though so that's maybe a weaker
>> check.
> 
> It does have FORCED_LABEL set:
> 
> (gdb) p val->base.side_effects_flag 
> $6 = 1
> 
> ...though presumably that's going to be set for just about any label
> that a computed goto jumps to?  Hence this is presumably of little
> benefit for adjusting the assertion.
Agreed.

jeff

Re: [PATCH/RFC] Fix ICE in find_taken_edge_computed_goto (PR 84136)

[Richard Biener]

On Thu, Feb 8, 2018 at 6:04 AM, Jeff Law <law@redhat.com> wrote:
> On 02/02/2018 02:35 PM, David Malcolm wrote:
>> On Thu, 2018-02-01 at 12:05 +0100, Richard Biener wrote:
>>> On Wed, Jan 31, 2018 at 4:39 PM, David Malcolm <dmalcolm@redhat.com>
>>> wrote:
>>>> PR 84136 reports an ICE within sccvn_dom_walker when handling a
>>>> C/C++ source file that overuses the labels-as-values extension.
>>>> The code in question stores a jump label into a global, and then
>>>> jumps to it from another function, which ICEs after inlining:
>>>>
>>>> void* a;
>>>>
>>>> void foo() {
>>>>   if ((a = &&l))
>>>>       return;
>>>>
>>>>   l:;
>>>> }
>>>>
>>>> int main() {
>>>>   foo();
>>>>   goto *a;
>>>>
>>>>   return 0;
>>>> }
>>>>
>>>> This appears to be far beyond what we claim to support in this
>>>> extension - but we shouldn't ICE.
>>>>
>>>> What's happening is that, after inlining, we have usage of a *copy*
>>>> of the label, which optimizes away the if-return logic, turning it
>>>> into an infinite loop.
>>>>
>>>> On entry to the sccvn_dom_walker we have this gimple:
>>>>
>>>> main ()
>>>> {
>>>>   void * a.0_1;
>>>>
>>>>   <bb 2> [count: 0]:
>>>>   a = &l;
>>>>
>>>>   <bb 3> [count: 0]:
>>>> l:
>>>>   a.0_1 = a;
>>>>   goto a.0_1;
>>>> }
>>>>
>>>> and:
>>>>   edge taken = find_taken_edge (bb, vn_valueize (val));
>>>> reasonably valueizes the:
>>>>   goto a.0_1;
>>>> after the:
>>>>   a = &l;
>>>>   a.0_1 = a;
>>>> as if it were:
>>>>   goto *&l;
>>>>
>>>> find_taken_edge_computed_goto then has:
>>>>
>>>> 2380      dest = label_to_block (val);
>>>> 2381      if (dest)
>>>> 2382        {
>>>> 2383          e = find_edge (bb, dest);
>>>> 2384          gcc_assert (e != NULL);
>>>> 2385        }
>>>>
>>>> which locates dest as a self-jump from block 3 back to itself.
>>>>
>>>> However, the find_edge call returns NULL - it has a predecessor
>>>> edge
>>>> from block 2, but no successor edges.
>>>>
>>>> Hence the assertion fails and we ICE.
>>>>
>>>> A successor edge from the computed goto could have been created by
>>>> make_edges if the label stmt had been in the function, but
>>>> make_edges
>>>> only looks in the current function when handling computed gotos,
>>>> and
>>>> the label only appeared after inlining.
>>>>
>>>> The following patch removes the assertion, fixing the ICE.
>>>>
>>>> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
>>>>
>>>> If that's option (a), there could be some other approaches:
>>>>
>>>> (b) convert the assertion into a warning/error/sorry, on the
>>>>     assumption that if we don't detect such an edge then the code
>>>> is
>>>>     presumably abusing the labels-as-values feature
>>>> (c) have make_edges detect such a problematic computed goto (maybe
>>>>     converting make_edges_bb's return value to an enum and adding a
>>>> 4th
>>>>     value - though it's not clear what to do then with it)
>>>> (d) detect this case on inlining and handle it somehow (e.g. adding
>>>>     edges for labels that have appeared since make_edges originally
>>>>     ran, for computed gotos that have no out-edges)
>>>> (e) do nothing, keeping the assertion, and accept that this is
>>>> going
>>>>     to fail on a non-release build
>>>> (f) something else?
>>>>
>>>> Of the above, (d) seems to me to be the most robust solution, but I
>>>> don't know how far we want to go "down the rabbit hole" of handling
>>>> such uses of labels-as-values (beyond not ICE-ing on them).
>>>>
>>>> Thoughts?
>>>
>>> I think you can preserve the assert for ! DECL_NONLOCAL (val) thus
>>>
>>> gcc_assert (e != NULL || DECL_NONLOCAL (val));
>>>
>>> does the label in this case properly have DECL_NONLOCAL
>>> set?  Probably
>>> not given we shouldn't have duplicated it in this case.
>>
>> Indeed, the inlined copy of the label doesn't have DECL_NONLOCAL set:
>>
>> (gdb) p val->decl_common.nonlocal_flag
>> $5 = 0
>>
>>> So the issue is really
>>> that the FE doesn't set this bit for "escaped" labels... but I'm not
>>> sure how
>>> to easily constrain the extension here.
>>>
>>> The label should be FORCED_LABEL though so that's maybe a weaker
>>> check.
>>
>> It does have FORCED_LABEL set:
>>
>> (gdb) p val->base.side_effects_flag
>> $6 = 1
>>
>> ...though presumably that's going to be set for just about any label
>> that a computed goto jumps to?  Hence this is presumably of little
>> benefit for adjusting the assertion.
> Agreed.

So remove the assert and add a comment in its place explaining the
situation.

OK with that.
Richard.

> jeff

Re: [PATCH/RFC] Fix ICE in find_taken_edge_computed_goto (PR 84136)

[Joseph Myers]

On Wed, 7 Feb 2018, Jeff Law wrote:

> Ideally we'd tighten the extension's language so that we could issue an
> error out of the front-end.

It seems to me to be the sort of thing that's only undefined at execution 
time - it's perfectly valid to store the address of a label in a global, 
and later jump to it if you haven't left the function execution within 
which you took the address.  I.e., if you detect this case and wish to 
diagnose it, it should be a warning plus generating a call to 
__builtin_trap, not an error.

-- 
Joseph S. Myers
joseph@codesourcery.com