| Message ID | alpine.LSU.2.20.1806261435290.5043@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
| Series | Adjust subprogram DIE re-usal | expand |
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
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
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.
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;
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.