On 02/01/2018 04:05 AM, 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. 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
