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

[Richard Biener]

On Thu, Feb 8, 2018 at 6:04 AM, Jeff Law <law@redhat.com> wrote:
> On 02/02/2018 02:35 PM, David Malcolm wrote:
>> 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.
> Agreed.

So remove the assert and add a comment in its place explaining the
situation.

OK with that.
Richard.

> jeff