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

[Richard Biener]

Late response now that I'm finished refreshing the patches.

On Mon, Nov 28, 2016 at 6:20 PM, Jason Merrill  wrote:
> On Thu, Nov 24, 2016 at 8:50 AM, Richard Biener  wrote:
>>> > +  /* ???  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.
>
> Does the discussion in
> https://sourceware.org/bugzilla/show_bug.cgi?id=20426 cover the issue?

This covers the VLA issue, yes.  With the DW_OP_GNU_variable_value extension
we might be able to handle those better though gdb still lacks support.

For the Fortran case quoted above it might be possible to use
DW_OP_GNU_variable_value
as well but I'll leave that for followup improvements (handling it
"like VLAs" without
this extension isn't possible Jakub told me).

I've removed the reference to VLAs in the comment, added a -flto -g variant of
save_5.f90 to the testsuite and refer to that.

>
>>> > + /* ???  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.
>
> Ah, OK.  But the comment seems wrong, since we go through here in
> early dwarf for local class methods.

Changed the comment to

/* ???  In LTO we do not see any of the location attributes.  */

>>> > +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?
>
> That sounds better, thanks.

I tried a few variants but they all end up even more awkward ...

Given the ugliness is isolated in init_sections_and_labels I think
keeping it the
way it is is best.

Updated patches posted separately (I posted the diff to the previous
state already).

Thanks,
Richard.

>
> Jason

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

[Jason Merrill]

On Thu, Nov 24, 2016 at 8:50 AM, Richard Biener  wrote:
>> > +  /* ???  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.

Does the discussion in
https://sourceware.org/bugzilla/show_bug.cgi?id=20426 cover the issue?

>> > + /* ???  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.

Ah, OK.  But the comment seems wrong, since we go through here in
early dwarf for local class methods.

>> > +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?

That sounds better, thanks.

Jason

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

[Richard Biener]

On Thu, 24 Nov 2016, Richard Biener wrote:

> 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 : 
> 
> 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 

Re: [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 : 

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][] = 

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

[Jason Merrill]

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.


+ /* ???  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.



+  /* "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?



+  /* ???  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.



+ /* ???  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.



   /* 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?


+  /* 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?


+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.



+  /* 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.



+  /* ???  Mostly duplicated from dwarf2out_finish.  */


:(

Jason

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

[Richard Biener]

On Fri, 21 Oct 2016, Richard Biener wrote:

> 
> This is the main part of the early LTO debug support.  The main parts
> of the changes are to dwarf2out.c where most of the changes are related
> to the fact that we eventually have to output debug info twice, once
> for the early LTO part and once for the fat part of the object file.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu with ASAN and TSAN
> extra FAILs (see PR78063, a libbacktrace missing feature or libsanitizer
> being too pessimistic).  There's an extra
> 
> XPASS: gcc.dg/guality/inline-params.c   -O2 -flto -fuse-linker-plugin 
> -fno-fat-lto-objects  execution test
> 
> the previously reported extra VLA guality FAILs are gone.
> 
> I've compared testresults with -flto -g added for all languages and
> only see expected differences (libstdc++ pretty printers now work,
> most scan-assembler-times debug testcases fail because we have everything
> twice now).
> 
> See https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01842.html for
> the last posting of this patch which has a high-level overview of
> Early LTO debug.  You may want to refer to the slides I presented
> at the GNU Cauldron as well.

I have refreshed the patch after the DWARF5 changes, re-LTO-bootstrapped
and retested (also comparing -g -flto with/without the patch) with no
changes in results.  Patch [1/2] still applies without changes.

Thanks,
Richard.

2016-10-21  Richard Biener  

* debug.h (struct gcc_debug_hooks): Add die_ref_for_decl and
register_external_die hooks.
(debug_false_tree_charstarstar_uhwistar): Declare.
(debug_nothing_tree_charstar_uhwi): Likewise.
* debug.c (do_nothing_debug_hooks): Adjust.
(debug_false_tree_charstarstar_uhwistar): New do nothing.
(debug_nothing_tree_charstar_uhwi): Likewise.
* dbxout.c (dbx_debug_hooks): Adjust.
(xcoff_debug_hooks): Likewise.
* sdbout.c (sdb_debug_hooks): Likewise.
* vmsdbgout.c (vmsdbg_debug_hooks): Likewise.

* dwarf2out.c (macinfo_label_base): New global.
(dwarf2out_register_external_die): New function for the
register_external_die hook.
(dwarf2out_die_ref_for_decl): Likewise for die_ref_for_decl.
(dwarf2_debug_hooks): Use them.
(dwarf2_lineno_debug_hooks): Adjust.
(struct die_struct): Add with_offset flag.
(DEBUG_LTO_DWO_INFO_SECTION, DEBUG_LTO_INFO_SECTION,
DEBUG_LTO_DWO_ABBREV_SECTION, DEBUG_LTO_ABBREV_SECTION,
DEBUG_LTO_DWO_MACINFO_SECTION, DEBUG_LTO_MACINFO_SECTION,
DEBUG_LTO_DWO_MACRO_SECTION, DEBUG_LTO_MACRO_SECTION,
DEBUG_LTO_LINE_SECTION, DEBUG_LTO_DWO_STR_OFFSETS_SECTION,
DEBUG_LTO_STR_DWO_SECTION, DEBUG_STR_LTO_SECTION): New macros
defining section names for the early LTO debug variants.
(reset_indirect_string): New helper.
(add_AT_external_die_ref): Helper for dwarf2out_register_external_die.
(print_dw_val): Add support for offsetted symbol references.
(compute_section_prefix_1): Split out worker to distinguish
the comdat from the LTO case.
(compute_section_prefix): Wrap old comdat case here.
(output_die): Skip DIE symbol output for the LTO added one.
Handle DIE symbol references with offset.
(output_comp_unit): Guard section name mangling properly.
For LTO debug sections emit a symbol at the section beginning
which we use to refer to its DIEs.
(add_abstract_origin_attribute): For DIEs registered via
dwarf2out_register_external_die directly refer to the early
DIE rather than indirectly through the shadow one we created.
(gen_array_type_die): When generating early LTO debug do
not emit DW_AT_string_length.
(gen_formal_parameter_die): Do not re-create DIEs for PARM_DECLs
late when in LTO.
(gen_subprogram_die): Adjust the check for whether we face
a concrete instance DIE for an inline we can reuse for the
late LTO case.  Likewise avoid another specification DIE
for early built declarations/definitions for the late LTO case.
(gen_variable_die): Add type references for late duplicated VLA dies
when in late LTO.
(gen_inlined_subroutine_die): Do not call dwarf2out_abstract_function,
we have the abstract instance already.
(process_scope_var): Adjust decl DIE contexts in LTO which
first puts them in limbo.
(gen_decl_die): Do not generate type DIEs late apart from
types for VLAs or for decls we do not yet have a DIE.
(dwarf2out_early_global_decl): Make sure to create DIEs
for abstract instances of a decl first.
(dwarf2out_late_global_decl): Adjust comment.
(output_macinfo_op): With multiple macro sections use
macinfo_label_base to distinguish labels.
(output_macinfo): Likewise.  Update macinfo_label_base.
Pass in