Adjust subprogram DIE re-usal

Message ID alpine.LSU.2.20.1806261435290.5043@zhemvz.fhfr.qr
State New
Headers show
Series
  • Adjust subprogram DIE re-usal
Related show

Commit Message

Richard Biener June 26, 2018, 12:43 p.m.
A patch from Honza not LTO streaming DECL_ORIGINAL_TYPE ends up
ICEing during LTO bootstrap because we end up not re-using the
DIE we create late during LTRANS for a subprogram because its
parent is a namespace rather than a CU DIE (or a module).

I'm wondering of the general reason why we enforce (inconsistently)
"We always want the DIE for this function that has the *_pc attributes to 
be under comp_unit_die so the debugger can find it."
We indeed generate a specification DIE rooted at the CU in addition to the
declaration DIE inside the namespace for sth as simple as

namespace Foo { void foo () {} }

anyhow - the comment also says "We also need to do this [re-use the DIE]
for abstract instances of inlines, since the spec requires the out-of-line
copy to have the same parent.".  Not sure what condition this part of
the comment applies to.

So my fix is to move the || get_AT (old_die, DW_AT_abstract_origin)
check I added for early LTO debug to a global override - forcing
DIE re-use for any DIE with an abstract origin set.  That is, all
concrete instances are fine where they are.  That also avoids
double-indirection DW_AT_specification -> DW_AT_abstract_origin -> DIE.

But as said, I wonder about the overall condition, esp. the
DW_TAG_module special-casing (but not at the same time
special-casing DW_TAG_namespace or DW_TAG_partial_unit).

LTO bootstrap is in progress on x86_64-unknown-linux-gnu.

OK if that succeeds?

Thanks,
Richard.

2018-06-26  Richard Biener  <rguenther@suse.de>

	* dwarf2out.c (gen_subprogram_die): Always re-use DIEs with an
	DW_AT_abstract_origin attribute.

Comments

Jeff Law June 26, 2018, 8:37 p.m. | #1
On 06/26/2018 06:43 AM, Richard Biener wrote:
> 

> A patch from Honza not LTO streaming DECL_ORIGINAL_TYPE ends up

> ICEing during LTO bootstrap because we end up not re-using the

> DIE we create late during LTRANS for a subprogram because its

> parent is a namespace rather than a CU DIE (or a module).

> 

> I'm wondering of the general reason why we enforce (inconsistently)

> "We always want the DIE for this function that has the *_pc attributes to 

> be under comp_unit_die so the debugger can find it."

> We indeed generate a specification DIE rooted at the CU in addition to the

> declaration DIE inside the namespace for sth as simple as

> 

> namespace Foo { void foo () {} }

> 

> anyhow - the comment also says "We also need to do this [re-use the DIE]

> for abstract instances of inlines, since the spec requires the out-of-line

> copy to have the same parent.".  Not sure what condition this part of

> the comment applies to.

> 

> So my fix is to move the || get_AT (old_die, DW_AT_abstract_origin)

> check I added for early LTO debug to a global override - forcing

> DIE re-use for any DIE with an abstract origin set.  That is, all

> concrete instances are fine where they are.  That also avoids

> double-indirection DW_AT_specification -> DW_AT_abstract_origin -> DIE.

> 

> But as said, I wonder about the overall condition, esp. the

> DW_TAG_module special-casing (but not at the same time

> special-casing DW_TAG_namespace or DW_TAG_partial_unit).

> 

> LTO bootstrap is in progress on x86_64-unknown-linux-gnu.

> 

> OK if that succeeds?

> 

> Thanks,

> Richard.

> 

> 2018-06-26  Richard Biener  <rguenther@suse.de>

> 

> 	* dwarf2out.c (gen_subprogram_die): Always re-use DIEs with an

> 	DW_AT_abstract_origin attribute.

Explicitly deferring to Jason here.

jeff
Jason Merrill June 27, 2018, 11:26 p.m. | #2
On Tue, Jun 26, 2018 at 8:43 AM, Richard Biener <rguenther@suse.de> wrote:
>

> A patch from Honza not LTO streaming DECL_ORIGINAL_TYPE ends up

> ICEing during LTO bootstrap because we end up not re-using the

> DIE we create late during LTRANS for a subprogram because its

> parent is a namespace rather than a CU DIE (or a module).

>

> I'm wondering of the general reason why we enforce (inconsistently)

> "We always want the DIE for this function that has the *_pc attributes to

> be under comp_unit_die so the debugger can find it."

> We indeed generate a specification DIE rooted at the CU in addition to the

> declaration DIE inside the namespace for sth as simple as

>

> namespace Foo { void foo () {} }


The reason was to make it easier for debuggers to collect function
definitions by scanning the top-level DIEs.  I don't know how
useful/important that is to actual debuggers nowadays, since there are
various accelerated lookup tables as well.

> anyhow - the comment also says "We also need to do this [re-use the DIE]

> for abstract instances of inlines, since the spec requires the out-of-line

> copy to have the same parent.".  Not sure what condition this part of

> the comment applies to.


I think it's saying that we shouldn't re-use an in-class declaration
die for the abstract instance of an inline, just like we shouldn't
re-use it for a non-inline definition.

Incidentally, the same parent was only required by DWARF 2; DWARF 3+
say it's typical but not required.

> So my fix is to move the || get_AT (old_die, DW_AT_abstract_origin)

> check I added for early LTO debug to a global override - forcing

> DIE re-use for any DIE with an abstract origin set.  That is, all

> concrete instances are fine where they are.  That also avoids

> double-indirection DW_AT_specification -> DW_AT_abstract_origin -> DIE.

>

> But as said, I wonder about the overall condition, esp. the

> DW_TAG_module special-casing (but not at the same time

> special-casing DW_TAG_namespace or DW_TAG_partial_unit).


Hmm, the DW_TAG_module check seems to have come in with the
early-debug merge.  I don't know what the rationale was for that; it
certainly does weaken the case for treating CU scope specially.

As for DW_TAG_partial_unit, probably the is_cu_die check should change
to is_unit_die.

> LTO bootstrap is in progress on x86_64-unknown-linux-gnu.

>

> OK if that succeeds?


OK.

Jason
Richard Biener June 28, 2018, 7:45 a.m. | #3
On Wed, 27 Jun 2018, Jason Merrill wrote:

> On Tue, Jun 26, 2018 at 8:43 AM, Richard Biener <rguenther@suse.de> wrote:

> >

> > A patch from Honza not LTO streaming DECL_ORIGINAL_TYPE ends up

> > ICEing during LTO bootstrap because we end up not re-using the

> > DIE we create late during LTRANS for a subprogram because its

> > parent is a namespace rather than a CU DIE (or a module).

> >

> > I'm wondering of the general reason why we enforce (inconsistently)

> > "We always want the DIE for this function that has the *_pc attributes to

> > be under comp_unit_die so the debugger can find it."

> > We indeed generate a specification DIE rooted at the CU in addition to the

> > declaration DIE inside the namespace for sth as simple as

> >

> > namespace Foo { void foo () {} }

> 

> The reason was to make it easier for debuggers to collect function

> definitions by scanning the top-level DIEs.  I don't know how

> useful/important that is to actual debuggers nowadays, since there are

> various accelerated lookup tables as well.


Ok.

> > anyhow - the comment also says "We also need to do this [re-use the DIE]

> > for abstract instances of inlines, since the spec requires the out-of-line

> > copy to have the same parent.".  Not sure what condition this part of

> > the comment applies to.

> 

> I think it's saying that we shouldn't re-use an in-class declaration

> die for the abstract instance of an inline, just like we shouldn't

> re-use it for a non-inline definition.

> 

> Incidentally, the same parent was only required by DWARF 2; DWARF 3+

> say it's typical but not required.


Ah, I see.

> > So my fix is to move the || get_AT (old_die, DW_AT_abstract_origin)

> > check I added for early LTO debug to a global override - forcing

> > DIE re-use for any DIE with an abstract origin set.  That is, all

> > concrete instances are fine where they are.  That also avoids

> > double-indirection DW_AT_specification -> DW_AT_abstract_origin -> DIE.

> >

> > But as said, I wonder about the overall condition, esp. the

> > DW_TAG_module special-casing (but not at the same time

> > special-casing DW_TAG_namespace or DW_TAG_partial_unit).

> 

> Hmm, the DW_TAG_module check seems to have come in with the

> early-debug merge.  I don't know what the rationale was for that; it

> certainly does weaken the case for treating CU scope specially.


The comment says it was done to fix some ICE.

> As for DW_TAG_partial_unit, probably the is_cu_die check should change

> to is_unit_die.


I'll bootstrap and test that change separately.

> > LTO bootstrap is in progress on x86_64-unknown-linux-gnu.

> >

> > OK if that succeeds?

> 

> OK.


It did, so applied.

Thanks,
Richard.

Patch

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 262132)
+++ gcc/dwarf2out.c	(working copy)
@@ -22780,26 +22780,25 @@  gen_subprogram_die (tree decl, dw_die_re
 	 apply; we just use the old DIE.  */
       expanded_location s = expand_location (DECL_SOURCE_LOCATION (decl));
       struct dwarf_file_data * file_index = lookup_filename (s.file);
-      if ((is_cu_die (old_die->die_parent)
-	   /* This condition fixes the inconsistency/ICE with the
-	      following Fortran test (or some derivative thereof) while
-	      building libgfortran:
+      if (((is_cu_die (old_die->die_parent)
+	    /* This condition fixes the inconsistency/ICE with the
+	       following Fortran test (or some derivative thereof) while
+	       building libgfortran:
 
-		 module some_m
-		 contains
-		    logical function funky (FLAG)
-		      funky = .true.
-		   end function
-		 end module
-	   */
-	   || (old_die->die_parent
-	       && old_die->die_parent->die_tag == DW_TAG_module)
-	   || context_die == NULL)
+		  module some_m
+		  contains
+		     logical function funky (FLAG)
+		       funky = .true.
+		    end function
+		  end module
+	     */
+	    || (old_die->die_parent
+		&& old_die->die_parent->die_tag == DW_TAG_module)
+	    || context_die == NULL)
 	   && (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)
@@ -22807,6 +22806,10 @@  gen_subprogram_die (tree decl, dw_die_re
 		       || s.column == 0
 		       || (get_AT_unsigned (old_die, DW_AT_decl_column)
 			   == (unsigned) s.column)))))
+	  /* With LTO if there's an abstract instance for
+	     the old DIE, this is a concrete instance and
+	     thus re-use the DIE.  */
+	  || get_AT (old_die, DW_AT_abstract_origin))
 	{
 	  subr_die = old_die;