On Thu, 24 Nov 2016, Richard Biener wrote:
> On Tue, 22 Nov 2016, Jason Merrill wrote:
>
> > On 11/11/2016 03:06 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. */
> >
> > The comment for DECL_CONTEXT says that a VAR_DECL can have 'NULL_TREE or a
> > TRANSLATION_UNIT_DECL if the given decl has "file scope"'
> >
> > So this doesn't seem like a FE bug.
>
> True - though with LTO we rely on all entities be associated with
> a TRANSLATION_UNIT_DECL (well, "rely" only in terms of how debuginfo
> is emitted with or without this patch). It should be easy to fix this
> up in the LTO streamer though.
>
> > > + /* ??? 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).
> >
> > I'd be happy to remove or disable -feliminate-dwarf2-dups at this point,
> > since
> > it's already useless for C++ without reimplementation.
>
> Ok, I'd favor removal in that case, I'll see to that independently
> of this patch.
>
> > > + /* "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. */
> >
> > Can you elaborate more on the refactoring? dwarf2out_function_decl is
> > already
> > very small, I'm guessing you mean gen_subprogram_die?
>
> Yes, refactor gen_subprogram_die into the early part and the part needed
> by dwarf2out_function_decl.
>
> > > + /* ??? We can't annotate types late, but for LTO we may not
> > > + generate a location early either (gfortran.dg/save_5.f90).
> > > + The proper way is to handle it like VLAs though it is told
> > > + that DW_AT_string_length does not support this. */
> >
> > I think go ahead and handle it like VLAs, this is an obvious generalization
> > and should go into the spec soon enough. This can happen later.
>
> Ok, note that VLAs are now handled by re-emitting types late to avoid
> some guality regressions. With inlining VLA types also get copied
> so we'd need to re-design how we handle them a bit. I'll see what
> the DWARF people come up with.
>
> > > + /* ??? This all (and above) should probably be simply
> > > + a ! early_dwarf check somehow. */
> > > + && ((DECL_ARTIFICIAL (decl) || in_lto_p)
> > > || (get_AT_file (old_die, DW_AT_decl_file) == file_index
> > > && (get_AT_unsigned (old_die, DW_AT_decl_line)
> > > == (unsigned) s.line))))
> >
> > Why doesn't the existing source position check handle the LTO case? Also the
> > extra parens aren't necessary.
>
> Because in LTRANS we do not see those attributes anymore but they are
> present in the early created DIEs. The LTRANS old_die looks basically
> like
>
> DW_TAG_subprogram
> DW_AT_abstract_origin : <reference to early DIE via $symbol + offset>
>
> refactoring gen_subprogram might also help here (I tried this three
> times alrady but it quickly becomes unwieldly).
>
> > > /* If we're emitting an out-of-line copy of an inline function,
> > > emit info for the abstract instance and set up to refer to it. */
> > > + /* ??? We have output an abstract instance early already and
> > > + could just re-use that. This is how LTO treats all functions
> > > + for example. */
> >
> > Isn't this what you do now?
>
> Yes. dwarf2out_abstract_function only sets DW_AT_inline after the patch.
> I'll adjust the comment to
>
> /* If we're emitting a possibly inlined function emit it as
> abstract instance. */
>
> >
> > > + /* Avoid generating stray type DIEs during late dwarf dumping.
> > > + All types have been dumped early. */
> > > + if (! (decl ? lookup_decl_die (decl) : NULL)
> >
> > Why do you still want to gen_type_die if decl_or_origin is origin?
>
> Probably an oversight on my side -- will change.
>
> > > +init_sections_and_labels (bool early_lto_debug)
> >
> > You're changing this function to do the same thing in four slightly
> > different
> > ways rather than two. I'd rather control each piece as appropriate; we
> > ought
> > to make SECTION_DEBUG or SECTION_DEBUG|SECTION_EXCLUDE a local variable, and
> > select between *_SECTION and the DWO variant at each statement rather than
> > in
> > different blocks.
>
> Note that the section names change between LTO, LTO_DWO, DWO and
> regular section names. It's basically modeled after what we have now
> which switches between regular and DWO section names. We might
> be able to refactor this with a new array
>
> enum section_kind { NORMAL, DWO, LTO, LTO_DWO };
> char **section_names[section_kind][] = { { DEBUG_INFO_SECTION, ... },
> { DEBUG_DWO_INFO_SECTION, ... },
> { DEBUG_LTO_INFO_SECTION, ... },
> { DEBUG_LTO_DWO_INFO_SECTION, .. } };
>
> would you prefer that?
>
> > > + /* Remove DW_AT_macro from the early output. */
> > > + if (have_macinfo)
> > > + remove_AT (comp_unit_die (),
> > > + dwarf_strict ? DW_AT_macro_info : DW_AT_GNU_macros);
> >
> > This will need adjustment for Jakub's DWARF 5 work. Please make the choice
> > of
> > AT value a macro.
>
> Ah, I see elsewhere
>
> if (have_macinfo)
> add_AT_macptr (comp_unit_die (),
> dwarf_version >= 5 ? DW_AT_macros
> : dwarf_strict ? DW_AT_macro_info : DW_AT_GNU_macros,
> macinfo_section_label);
>
> I'll add
>
> /* Attribute used to refer to the macro section. */
> #define DEBUG_MACRO_ATTRIBUTE (dwarf_version >= 5 ? DW_AT_macros \
> : dwarf_strict ? DW_AT_macro_info : DW_AT_GNU_macros)
>
>
> > > + /* ??? Mostly duplicated from dwarf2out_finish. */
> >
> > :(
>
> Whoops - that got off my radar somehow. I'll try to factor out sth
> like a output_dwarf function.
Ok, so while there are duplications there are a lot of omissions
(everything outputting location stuff) plus some LTO specific changes.
The dwarf2out_finish output part is 328 lines of code while the
redacted "copy" in dwarf2out_early_finish is 187 lines. I don't
see much that can be literally shared, some dwarf2out refactoring
could make thinks like
/* Traverse the DIE's and add add sibling attributes to those DIE's
that have children. */
add_sibling_attributes (comp_unit_die ());
for (limbo_die_node *node = limbo_die_list; node; node = node->next)
add_sibling_attributes (node->die);
for (comdat_type_node *ctnode = comdat_type_list;
ctnode != NULL; ctnode = ctnode->next)
add_sibling_attributes (ctnode->root_die);
smaller (if we'd had only a single list of CUs).
Or
/* Output the abbreviation table. */
if (vec_safe_length (abbrev_die_table) != 1)
{
switch_to_section (debug_abbrev_section);
ASM_OUTPUT_LABEL (asm_out_file, abbrev_section_label);
output_abbrev_section ();
}
could be fully put into output_abbrev_section ().
Other than that there's not much to do I fear. For now I'll amend
the ??? comment (and remove the ???).
Thanks,
Richard.
