This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][2/2] early LTO debug, main part
- From: Jason Merrill <jason at redhat dot com>
- To: Richard Biener <rguenther at suse dot de>, gcc-patches at gcc dot gnu dot org
- Date: Wed, 2 Aug 2017 00:29:36 -0400
- Subject: Re: [PATCH][2/2] early LTO debug, main part
- Authentication-results: sourceware.org; auth=none
- References: <alpine.LSU.2.20.1705191235260.20726@zhemvz.fhfr.qr>
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.
+ /* ??? 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.
! /* ??? 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.
! unit_die->comdat_type_p = comdat_p;
! }
!
! static void
! compute_section_prefix (dw_die_ref unit_die)
! {
! compute_section_prefix_1 (unit_die, true);
! comdat_symbol_id = unit_die->die_id.die_symbol;
comdat_symbol_number = 0;
}
Let's set the comdat_type_p flag in this function rather than add a
parameter to the existing function. And when -fEDD goes away, we don't
need this entry point at all.
Also, for LTO debug, it seems you aren't actually using the symbol as a
section prefix, so the name becomes inaccurate. Maybe
compute_comp_unit_symbol rather than compute_section_prefix_1?
+ /* For LTO cross unit DIE refs we want a symbol on the start of the
+ debuginfo section, not on the CU DIE.
+ ??? We could simply use the symbol as it would be output by output_die
+ and account for the extra offset produced by the CU header which has fixed
+ size. OTOH it currently only supports linkonce globals which would
+ be less than ideal?. */
I think the way you're doing it now is better than this alternative,
since die_offset is relative to the beginning of the CU header.
! /* 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.
! 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?
! /* ??? In LTO we do not see any of the location attributes. */
! && ((DECL_ARTIFICIAL (decl) || in_lto_p)
Perhaps get_AT_ref (old_die, DW_AT_abstract_origin) instead of in_lto_p?
And you don't need the added parentheses here.
if (parm_die && parm_die->die_parent != context_die)
{
! /* ??? The DIE parent is the "abstract" copy and the context_die
! is the specification "copy". */
! if (!DECL_ABSTRACT_P (node) && !in_lto_p)
And a bit below in the current source,
/* FIXME: Reuse DIE even with a differing context.
This can happen when calling
dwarf2out_abstract_function to build debug info for
the abstract instance of a function for which we have
already generated a DIE in
dwarf2out_early_global_decl.
Once we remove dwarf2out_abstract_function, we should
have a call to gcc_unreachable here. */
Now that you've done away with the old dwarf2out_abstract_function, I
think dwarf2out shouldn't need to consider DECL_ABSTRACT_P at all.
In the hunk above, it seems that you want to reuse the parm DIEs from
d*_register_external_die if origin is NULL, which seems appropriate.
So, are there any cases remaining where we actually want to clear
parm_die there? I suspect not, in which case we should be able to
remove the entire "if (parm_die && parm_die->die_parent != context_die)"
block.
! /* Early created DIEs do not have a parent as the decls refer
! to the function as DECL_CONTEXT rather than the BLOCK. */
! if (in_lto_p
! && die && die->die_parent == NULL)
! add_child_die (context_die, die);
Perhaps in_lto_p should be asserted rather than a condition?
! /* Do not generate stray type DIEs in late LTO dumping. */
! && early_dwarf)
...
! /* Avoid generating stray type DIEs during late dwarf dumping.
! All types have been dumped early. */
! if (! lookup_decl_die (decl_or_origin)
...
! /* Avoid generating stray type DIEs during late dwarf dumping.
! All types have been dumped early. */
! if (! lookup_decl_die (decl_or_origin)
Should these use the same test (early_dwarf vs. lookup_decl_die)?
Jason