[PATCH][2/2] early LTO debug, main part
Richard Biener
rguenther@suse.de
Thu Aug 3 10:51:00 GMT 2017
On Wed, 2 Aug 2017, Jason Merrill wrote:
> 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.
You are right. I suspect I got confused by the comment when looking
for a way to paper over the check_die ICE removing the check causes.
We end up adding DW_AT_inline to the DIE which means check_die isn't
happy with us using it for the concrete instance.
That happens in two places, first add_abstract_origin_attribute
calling dwarf2out_abstract_function for origin != FUNCTION_DECL/BLOCK
and second from RTL expansion calling the same function via the
debug hook. Both of this should be obsoleted by the early debug
work on trunk. Then we do the same from gen_decl_die. I'm not sure
that those cases are obsolete by now - the clone case should
eventually be, but we can probably guard them with early_dwarf -- this
exposes the issue that gen_subprogram_die happily re-uses the old_die
even if DW_AT_inline is set, so the set_decl_origin_self must
matter here. During early dwarf cgraph_function_possibly_inlined_p
returns false. Not sure what is best here - either guarding
gen_subprogram_die or doing set_decl_origin_self in
dwarf2out_abstract_function? Doing the latter now with the idea
this origin might be important elsewhere. For in_lto_p we also need to
avoid dwarf2out_abstract_function, easiest by adding a
|| get_AT (old_die, DW_AT_abstract_origin) to the existing check
for DW_AT_inline (that might have fixed all of the issues above,
just in case you are hot happy with them).
I've changed the comment in add_abstract_origin_attribute as the
unwrapping is really because of the inlined subroutine case. It
also ends up applying to the case of clones where I'm less sure
if the abstract origin should point to the concrete instance or
the abstract copy (without LTO it points at the abstract copy
and thus matches what the patch ends up doing).
So with the following I see the DIEs used for the concrete instance
as intented and the abstract copy in the early object refered to
by inline instances (and the concrete instance of course).
Re-doing testing...
Thanks,
Richard.
Index: early-lto-debug/gcc/dwarf2out.c
===================================================================
--- early-lto-debug.orig/gcc/dwarf2out.c 2017-08-03 12:48:59.854268649 +0200
+++ early-lto-debug/gcc/dwarf2out.c 2017-08-03 12:44:13.545059174 +0200
@@ -20517,33 +20517,12 @@ add_abstract_origin_attribute (dw_die_re
{
dw_die_ref origin_die = NULL;
- if (TREE_CODE (origin) != FUNCTION_DECL
- && TREE_CODE (origin) != BLOCK)
- {
- /* We may have gotten separated from the block for the inlined
- function, if we're in an exception handler or some such; make
- sure that the abstract function has been written out.
-
- Doing this for nested functions is wrong, however; functions are
- distinct units, and our context might not even be inline. */
- tree fn = origin;
-
- if (TYPE_P (fn))
- fn = TYPE_STUB_DECL (fn);
-
- fn = decl_function_context (fn);
- if (fn)
- dwarf2out_abstract_function (fn);
- }
-
if (DECL_P (origin))
{
dw_die_ref c;
origin_die = lookup_decl_die (origin);
/* "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. */
+ We are looking for the abstract copy here. */
if (in_lto_p
&& origin_die
&& (c = get_AT_ref (origin_die, DW_AT_abstract_origin))
@@ -21879,7 +21858,8 @@ dwarf2out_abstract_function (tree decl)
old_die = lookup_decl_die (decl);
/* With early debug we always have an old DIE. */
gcc_assert (old_die != NULL);
- if (get_AT (old_die, DW_AT_inline))
+ if (get_AT (old_die, DW_AT_inline)
+ || get_AT (old_die, DW_AT_abstract_origin))
/* We've already generated the abstract instance. */
return;
@@ -21902,6 +21882,8 @@ dwarf2out_abstract_function (tree decl)
if (DECL_DECLARED_INLINE_P (decl)
&& lookup_attribute ("artificial", DECL_ATTRIBUTES (decl)))
add_AT_flag (old_die, DW_AT_artificial, 1);
+
+ set_decl_origin_self (decl);
}
/* Helper function of premark_used_types() which gets called through
@@ -22113,7 +22095,7 @@ gen_subprogram_die (tree decl, dw_die_re
&& debug_info_level > DINFO_LEVEL_TERSE)
old_die = force_decl_die (decl);
- /* An inlined instance, tag a new DIE with DW_AT_abstract_origin. */
+ /* A concrete instance, tag a new DIE with DW_AT_abstract_origin. */
if (origin != NULL)
{
gcc_assert (!declaration || local_scope_p (context_die));
@@ -22123,12 +22105,7 @@ gen_subprogram_die (tree decl, dw_die_re
if (old_die && old_die->die_parent == NULL)
add_child_die (context_die, old_die);
- 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 (old_die && get_AT_ref (old_die, DW_AT_abstract_origin))
{
/* If we have a DW_AT_abstract_origin we have a working
cached version. */
@@ -25496,8 +25473,13 @@ gen_decl_die (tree decl, tree origin, st
/* This is only a declaration. */;
#endif
+ /* We should have abstract copies already and should not generate
+ stray type DIEs in late LTO dumping. */
+ if (! early_dwarf)
+ ;
+
/* If we're emitting a clone, emit info for the abstract instance. */
- if (origin || DECL_ORIGIN (decl) != decl)
+ else if (origin || DECL_ORIGIN (decl) != decl)
dwarf2out_abstract_function (origin
? DECL_ORIGIN (origin)
: DECL_ABSTRACT_ORIGIN (decl));
@@ -25511,15 +25493,10 @@ gen_decl_die (tree decl, tree origin, st
a declaration. We must avoid setting DECL_ABSTRACT_ORIGIN in
that case, because that works only if we have a die. */
&& DECL_INITIAL (decl) != NULL_TREE)
- {
- dwarf2out_abstract_function (decl);
- set_decl_origin_self (decl);
- }
+ dwarf2out_abstract_function (decl);
/* Otherwise we're emitting the primary DIE for this decl. */
- else if (debug_info_level > DINFO_LEVEL_TERSE
- /* Do not generate stray type DIEs in late LTO dumping. */
- && early_dwarf)
+ else if (debug_info_level > DINFO_LEVEL_TERSE)
{
/* Before we describe the FUNCTION_DECL itself, make sure that we
have its containing type. */
Index: early-lto-debug/gcc/cfgexpand.c
===================================================================
--- early-lto-debug.orig/gcc/cfgexpand.c 2017-08-02 10:20:44.093608890 +0200
+++ early-lto-debug/gcc/cfgexpand.c 2017-08-03 12:21:08.348962776 +0200
@@ -6527,12 +6527,6 @@ pass_expand::execute (function *fun)
TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (parent)) = 1;
}
- /* We are now committed to emitting code for this function. Do any
- preparation, such as emitting abstract debug info for the inline
- before it gets mangled by optimization. */
- if (cgraph_function_possibly_inlined_p (current_function_decl))
- (*debug_hooks->outlining_inline_function) (current_function_decl);
-
TREE_ASM_WRITTEN (current_function_decl) = 1;
/* After expanding, the return labels are no longer needed. */
More information about the Gcc-patches
mailing list