On Thu, Feb 8, 2018 at 6:04 AM, Jeff Law <l...@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 <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.
So remove the assert and add a comment in its place explaining the situation. OK with that. Richard. > jeff