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