> Jan Hubicka <hubi...@ucw.cz> writes: > >> The testcase has failed since r9-5035, because obj_type_ref_class > >> tries to look up an ODR type when no ODR type information is > >> available. (The information was available earlier in the > >> compilation, but was freed during pass_ipa_free_lang_data.) > >> We then crash dereferencing the null get_odr_type result. > >> > >> The test passes with -O2. However, it fails again if -fdump-tree-all > >> is used, since obj_type_ref_class is called indirectly from the > >> dump routines. > >> > >> Other code seems to create ODR type entries on the fly by passing > >> "true" as the insert parameter. But since obj_type_ref_class is > >> used by dump routines, I guess it should have no side-effects and > >> should simply punt in this case. > > Thanks for looking into this! > > > > The ODR type hash is indeed supposed to be populated lazilly except for > > LTO when we populate it at stream in time. I think just making function > > return NULL in case it should not is fragile, because we may hit missed > > optimizations and other surprised elsewhere. > > > > What about adding new "for_dump" parameter set implicitly to false that > > will make the function return ret in case get_odr_type is NULL? > > For dumping it probably does not matter what ODR variant you see, since > > we end up with a type name in the dump anyway. > > I think there are two problematic cases (at least as far as this > testcase goes). One is the direct call to obj_type_ref_class in > tree-pretty-print.c. The other is an indirect call via > virtual_method_call_p, which is also called by tree-pretty-print.c. > So I guess both of them would need a "for_dump" parameter. > > TBH doing it that way feels fragile too. Even if we get it right now, > it's going to be hard to ensure that all indirect calls to obj_type_ref_class > in future make the correct choice between lazy initialisation and remaining > side-effect free.
Yep, it is not very pretty :(. In general the output of optimizations does not depend much on what is inserted into the type inheritance graph (because we must assume that types are derived in other units anyway) except for anonymous types. However speculative devirt is an exception here, so I guess we want to go woth for_dump markers (it is useful to have obj type ref pretty printed in a meaningful way). I will try to think of this bit more - assuming that type used by the program is touched by the unit is a meaningful heuristics. Adding type entries proactively when ODR type is created is an option but then the speculative devirt will drag in types that are otherwise unused (which will lead to various fun with headers and visibility). Perhaps speculative devirt should care only about types that does have constructors in the callgraph (like it does for anonymous types) that would make it stable, but then I can imagine units that does use the given type but does not construct it quite easily. Honza
