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

[David Malcolm <dmalcolm@redhat.com>]

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