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.
> > ! 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?
Done.
> > + /* 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.
Removed the ??? part of the comment.
> > ! /* 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?
> > ! 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).
That is the underlying issue is that when we register the external DIE
we really register abstract origins for the decls but the easiest way
to store those is to create an actual DIE for the decl just containing
that abstract origin link. For function decls that will end up as
the DIE for the concrete instance so we have to jump through this hoop
to get the abstract origin for the inline instances correct. The
check in gen_subprogram_die
/* An inlined instance, tag a new DIE with DW_AT_abstract_origin. */
if (origin != NULL)
{
...
dw_die_ref c;
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))
{
/* If we have a DW_AT_abstract_origin we have a working
cached version. */
subr_die = old_die;
makes sure we do _not_ re-use this concrete instance DIE (it's
not the cached variant).
Maybe we can move the add_abstract_origin_attribute check to
some of the callers if we provide a variant that gets a origin DIE
rather than a tree ...
Can we leave this for future enhancement? (just making a list here)
>
> > ! /* ??? 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?
Good idea. Any reason why not get_AT (...)? Using that now.
> And you don't need the added parentheses here.
Fixed.
> > 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.
I suspect not either. But the dwarf2out_abstract_function change
should cause DECL_ABSTRACT_P to _not_ be set (unless we have any
"real" DECL_ABSTRACT params) and thus we always run into the
param_die = NULL case (if the guarding condition is true, that is,
which I think happens for in_lto_p -- thus the change should have
been to amend the if (param_die && param_die->die_parent !=
context_die) with && !in_lto_p.
I've moved the && !in_lto_p check and the comment and added an item
on my TODO (remove this whole block). And I added gcc_unreachable
as suggested by the comment above.
Oh, and if the later check, param_die->die_parent == NULL then
we've always run into the earlier block and if ! DECL_ABSTRACT_P
cleared param_die.
This code is somewhat confusing as it exists right now.
> > ! /* 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?
You mean
if (die && die->die_parent == NULL)
{
gcc_assert (in_lto_p);
add_child_die (context_die, die);
}
? Done.
> > ! /* 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)?
Yeah, fixed.
Below is an incremental patch with the changes you requested, minus
those I put on the TODO.
I'll put this version through my usual testing now and will post
the full updated patch once that succeeded.
Thanks,
Richard.
Index: early-lto-debug/gcc/dwarf2out.c
===================================================================
--- early-lto-debug.orig/gcc/dwarf2out.c 2017-08-02 12:33:27.392750835
+0200
+++ early-lto-debug/gcc/dwarf2out.c 2017-08-02 12:29:13.128290085 +0200
@@ -5087,6 +5087,21 @@ get_AT_file (dw_die_ref die, enum dwarf_
return a ? AT_file (a) : NULL;
}
+/* Returns the ultimate TRANSLATION_UNIT_DECL context of DECL or NULL. */
+
+static const_tree
+get_ultimate_context (const_tree decl)
+{
+ while (decl && TREE_CODE (decl) != TRANSLATION_UNIT_DECL)
+ {
+ if (TREE_CODE (decl) == BLOCK)
+ decl = BLOCK_SUPERCONTEXT (decl);
+ else
+ decl = get_containing_scope (decl);
+ }
+ return decl;
+}
+
/* Return TRUE if the language is C++. */
static inline bool
@@ -5105,14 +5120,7 @@ is_cxx (const_tree decl)
{
if (in_lto_p)
{
- const_tree context = decl;
- while (context && TREE_CODE (context) != TRANSLATION_UNIT_DECL)
- {
- if (TREE_CODE (context) == BLOCK)
- context = BLOCK_SUPERCONTEXT (context);
- else
- context = get_containing_scope (context);
- }
+ const_tree context = get_ultimate_context (decl);
if (context && TRANSLATION_UNIT_LANGUAGE (context))
return strncmp (TRANSLATION_UNIT_LANGUAGE (context), "GNU C++", 7) == 0;
}
@@ -5133,6 +5141,21 @@ is_fortran (void)
|| lang == DW_LANG_Fortran08);
}
+static inline bool
+is_fortran (const_tree decl)
+{
+ if (in_lto_p)
+ {
+ const_tree context = get_ultimate_context (decl);
+ if (context && TRANSLATION_UNIT_LANGUAGE (context))
+ return (strncmp (TRANSLATION_UNIT_LANGUAGE (context),
+ "GNU Fortran", 11) == 0
+ || strcmp (TRANSLATION_UNIT_LANGUAGE (context),
+ "GNU F77") == 0);
+ }
+ return is_fortran ();
+}
+
/* Return TRUE if the language is Ada. */
static inline bool
@@ -5597,10 +5620,8 @@ dwarf2out_register_external_die (tree de
parent = lookup_decl_die (ctx);
}
else
- /* ??? 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. */
+ /* In some cases the FEs fail to set DECL_CONTEXT properly.
+ Handle this case gracefully by globalizing stuff. */
parent = comp_unit_die ();
/* Create a DIE "stub". */
switch (TREE_CODE (decl))
@@ -5619,10 +5640,10 @@ dwarf2out_register_external_die (tree de
die = new_die (DW_TAG_compile_unit, NULL, decl);
break;
case NAMESPACE_DECL:
- /* ??? 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. */
- die = new_die (DW_TAG_namespace, parent, decl);
+ if (is_fortran (decl))
+ die = new_die (DW_TAG_module, parent, decl);
+ else
+ die = new_die (DW_TAG_namespace, parent, decl);
break;
case FUNCTION_DECL:
die = new_die (DW_TAG_subprogram, parent, decl);
@@ -7461,10 +7482,10 @@ static const char *comdat_symbol_id;
static unsigned int comdat_symbol_number;
/* Calculate the MD5 checksum of the compilation unit DIE UNIT_DIE and its
- children, and set comdat_symbol_id accordingly. */
+ children, and set die_symbol. */
static void
-compute_section_prefix_1 (dw_die_ref unit_die, bool comdat_p)
+compute_comp_unit_symbol (dw_die_ref unit_die)
{
const char *die_name = get_AT_string (unit_die, DW_AT_name);
const char *base = die_name ? lbasename (die_name) : "anonymous";
@@ -7498,13 +7519,13 @@ compute_section_prefix_1 (dw_die_ref uni
}
unit_die->die_id.die_symbol = xstrdup (name);
- unit_die->comdat_type_p = comdat_p;
}
static void
compute_section_prefix (dw_die_ref unit_die)
{
- compute_section_prefix_1 (unit_die, true);
+ compute_comp_unit_symbol (unit_die);
+ unit_die->comdat_type_p = true;
comdat_symbol_id = unit_die->die_id.die_symbol;
comdat_symbol_number = 0;
}
@@ -10684,11 +10705,7 @@ output_comp_unit (dw_die_ref die, int ou
}
/* 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?. */
+ debuginfo section, not on the CU DIE. */
if (flag_generate_lto && oldsym)
{
/* ??? No way to get visibility assembled without a decl. */
@@ -21505,12 +21522,13 @@ gen_formal_parameter_die (tree node, tre
parm_die = lookup_decl_die (node);
/* If the contexts differ, we may not be talking about the same
- thing. */
- if (parm_die && parm_die->die_parent != context_die)
+ thing.
+ ??? When in LTO the DIE parent is the "abstract" copy and the
+ context_die is the specification "copy". But this whole block
+ should eventually be no longer needed. */
+ if (parm_die && parm_die->die_parent != context_die && !in_lto_p)
{
- /* ??? The DIE parent is the "abstract" copy and the context_die
- is the specification "copy". */
- if (!DECL_ABSTRACT_P (node) && !in_lto_p)
+ if (!DECL_ABSTRACT_P (node))
{
/* This can happen when creating an inlined instance, in
which case we need to create a new DIE that will get
@@ -21518,18 +21536,7 @@ gen_formal_parameter_die (tree node, tre
parm_die = NULL;
}
else
- {
- /* 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. */
- }
+ gcc_unreachable ();
}
if (parm_die && parm_die->die_parent == NULL)
@@ -22189,8 +22196,11 @@ gen_subprogram_die (tree decl, dw_die_re
|| (old_die->die_parent
&& old_die->die_parent->die_tag == DW_TAG_module)
|| context_die == NULL)
- /* ??? In LTO we do not see any of the location attributes. */
- && ((DECL_ARTIFICIAL (decl) || in_lto_p)
+ && (DECL_ARTIFICIAL (decl)
+ /* The location attributes may be in the abstract origin
+ which in the case of LTO might be not available to
+ look at. */
+ || get_AT (old_die, DW_AT_abstract_origin)
|| (get_AT_file (old_die, DW_AT_decl_file) == file_index
&& (get_AT_unsigned (old_die, DW_AT_decl_line)
== (unsigned) s.line)
@@ -25067,9 +25077,11 @@ process_scope_var (tree stmt, tree decl,
/* 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);
+ if (die && die->die_parent == NULL)
+ {
+ gcc_assert (in_lto_p);
+ add_child_die (context_die, die);
+ }
}
gen_decl_die (decl, origin, NULL, context_die);
@@ -25576,7 +25588,7 @@ gen_decl_die (tree decl, tree origin, st
/* Avoid generating stray type DIEs during late dwarf dumping.
All types have been dumped early. */
- if (! lookup_decl_die (decl_or_origin)
+ if (early_dwarf
/* ??? But in LTRANS we cannot annotate early created variably
modified type DIEs without copying them and adjusting all
references to them. Dump them again as happens for inlining
@@ -25635,7 +25647,7 @@ gen_decl_die (tree decl, tree origin, st
case PARM_DECL:
/* Avoid generating stray type DIEs during late dwarf dumping.
All types have been dumped early. */
- if (! lookup_decl_die (decl_or_origin)
+ if (early_dwarf
/* ??? But in LTRANS we cannot annotate early created variably
modified type DIEs without copying them and adjusting all
references to them. Dump them again as happens for inlining
@@ -30863,7 +30875,7 @@ dwarf2out_early_finish (const char *file
add_AT_pubnames (comp_unit_die ());
/* Stick a unique symbol to the main debuginfo section. */
- compute_section_prefix_1 (comp_unit_die (), false);
+ compute_comp_unit_symbol (comp_unit_die ());
/* Output the main compilation unit. We always need it if only for
the CU symbol. */
