Re: [PATCH][2/2] early LTO debug, main part

[Jason Merrill <jason at redhat dot com>]

On Thu, Aug 3, 2017 at 6:51 AM, Richard Biener <rguenther@suse.de> wrote:
> On Wed, 2 Aug 2017, Jason Merrill wrote:
>
>> On Wed, Aug 2, 2017 at 6:35 AM, Richard Biener <rguenther@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