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

[Jeff Law <law@redhat.com>]

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