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