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

[Jason Merrill]

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.

Jason