On Thu, Aug 3, 2017 at 6:51 AM, Richard Biener <rguent...@suse.de> wrote: > On Wed, 2 Aug 2017, Jason Merrill wrote: > >> On Wed, Aug 2, 2017 at 6:35 AM, Richard Biener <rguent...@suse.de> wrote: >> > On Wed, 2 Aug 2017, Jason Merrill wrote: >> > >> >> On 05/19/2017 06:42 AM, Richard Biener wrote: >> >> > + /* ??? In some cases the C++ FE (at least) fails to >> >> > + set DECL_CONTEXT properly. Simply globalize stuff >> >> > + in this case. For example >> >> > + __dso_handle created via iostream line 74 col 25. */ >> >> > + parent = comp_unit_die (); >> >> >> >> I've now fixed __dso_handle, so that can be removed from the comment, but >> >> it >> >> still makes sense to have this fall-back for the (permitted) case of null >> >> DECL_CONTEXT. >> > >> > Adjusted the comment. >> > >> >> > + /* ??? LANG issue - DW_TAG_module for fortran. Either look >> >> > + at the input language (if we have enough DECL_CONTEXT to follow) >> >> > + or use a bit in tree_decl_with_vis to record the distinction. */ >> >> >> >> Sure, you should be able to look at TRANSLATION_UNIT_LANGUAGE. >> > >> > Yeah, the comment says we might be able to walk DECL_CONTEXT up to >> > a TRANSLATION_UNIT_DECL. I've amended is_fortran similar to as I >> > amended is_cxx, providing an overload for a decl, factoring out common >> > code. So this is now if (is_fortran (decl)) ... = new_die >> > (DW_TAG_module,...). >> > >> >> > ! /* ??? We cannot unconditionally output die_offset if >> >> > ! non-zero - at least -feliminate-dwarf2-dups will >> >> > ! create references to those DIEs via symbols. And we >> >> > ! do not clear its DIE offset after outputting it >> >> > ! (and the label refers to the actual DIEs, not the >> >> > ! DWARF CU unit header which is when using label + offset >> >> > ! would be the correct thing to do). >> >> >> >> As in our previous discussion, I think -feliminate-dwarf2-dups can go away >> >> now. But this is a more general issue: die_offset has meant the offset >> >> from >> >> the beginning of the CU, but if with_offset is set it means an offset from >> >> die_symbol. Since with_offset changes the meaning of die_symbol and >> >> die_offset, having different code here depending on that flag makes sense. >> >> >> >> It seems likely that when -fEDD goes away, we will never again want to do >> >> direct symbolic references to DIEs, in which case we could drop the >> >> current >> >> meaning of die_symbol, and so we wouldn't need the with_offset flag. >> > >> > Yeah, I've been playing with a patch to remove -fEDD but it has conflicts >> > with the early LTO debug work and thus I wanted to postpone it until >> > after that goes in to avoid further churn. I hope that's fine, it's >> > sth I'll tackle soon after this patch lands on trunk. >> >> Sure. >> >> >> > ! /* Don't output the symbol twice. For LTO we want the label >> >> > ! on the section beginning, not on the actual DIE. */ >> >> > ! && (!flag_generate_lto >> >> > ! || die->die_tag != DW_TAG_compile_unit)) >> >> >> >> I think this check should just be !with_offset; if that flag is set the >> >> DIE >> >> doesn't actually have its own symbol. >> > >> > with_offset is set only during LTRANS phase for the DIEs refering to >> > the early DIEs via the CU label. But the above is guarding the >> > early phase when we do not want to output that CU label twice. >> > >> > Can we revisit this check when -fEDD has gone away? >> >> Yes. >> >> >> > ! if (old_die >> >> > ! && (c = get_AT_ref (old_die, DW_AT_abstract_origin)) >> >> > ! /* ??? In LTO all origin DIEs still refer to the early >> >> > ! debug copy. Detect that. */ >> >> > ! && get_AT (c, DW_AT_inline)) >> >> ... >> >> > ! /* "Unwrap" the decls DIE which we put in the imported unit >> >> > context. >> >> > ! ??? If we finish dwarf2out_function_decl refactoring we can >> >> > ! do this in a better way from the start and only lazily emit >> >> > ! the early DIE references. */ >> >> >> >> It seems like in gen_subprogram_die you deliberately avoid reusing the DIE >> >> from dwarf2out_register_external_die (since it doesn't have >> >> DW_AT_inline), and >> >> then in add_abstract_origin_attribute you need to look through that >> >> redundant >> >> die. Why not reuse it? >> > >> > What we're doing here is dealing with the case of an inlined >> > instance which is adjusted to point back to the early debug copy >> > directly than to the wrapping DIE (supposed to eventually contain >> > the concrete instance). >> >> But we enter this block when we're emitting the concrete out-of-line >> instance, and the DW_AT_inline check prevents us from using the >> wrapping DIE for the out-of-line instance. >> >> The comment should really change "inlined instance" to "concrete >> instance"; inlined instances are handled in >> gen_inlined_subroutine_die. > > You are right. I suspect I got confused by the comment when looking > for a way to paper over the check_die ICE removing the check causes. > We end up adding DW_AT_inline to the DIE which means check_die isn't > happy with us using it for the concrete instance. > > That happens in two places, first add_abstract_origin_attribute > calling dwarf2out_abstract_function for origin != FUNCTION_DECL/BLOCK > and second from RTL expansion calling the same function via the > debug hook. Both of this should be obsoleted by the early debug > work on trunk. Then we do the same from gen_decl_die. I'm not sure > that those cases are obsolete by now - the clone case should > eventually be, but we can probably guard them with early_dwarf -- this > exposes the issue that gen_subprogram_die happily re-uses the old_die > even if DW_AT_inline is set, so the set_decl_origin_self must > matter here. During early dwarf cgraph_function_possibly_inlined_p > returns false. Not sure what is best here - either guarding > gen_subprogram_die or doing set_decl_origin_self in > dwarf2out_abstract_function? Doing the latter now with the idea > this origin might be important elsewhere. For in_lto_p we also need to > avoid dwarf2out_abstract_function, easiest by adding a > || get_AT (old_die, DW_AT_abstract_origin) to the existing check > for DW_AT_inline (that might have fixed all of the issues above, > just in case you are hot happy with them). > > I've changed the comment in add_abstract_origin_attribute as the > unwrapping is really because of the inlined subroutine case. It > also ends up applying to the case of clones where I'm less sure > if the abstract origin should point to the concrete instance or > the abstract copy (without LTO it points at the abstract copy > and thus matches what the patch ends up doing). > > So with the following I see the DIEs used for the concrete instance > as intented and the abstract copy in the early object refered to > by inline instances (and the concrete instance of course). > > Re-doing testing...
OK if testing passes. Jason