[PING][PATCH][2/2] Early LTO debug -- main part

[Richard Biener]

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.