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.
Thanks for the review.
Richard.
