On Thu, 2018-02-01 at 12:05 +0100, Richard Biener wrote:
> On Wed, Jan 31, 2018 at 4:39 PM, David Malcolm <dmalc...@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®rtested 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
> >
