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