[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