Page MenuHomePhabricator

1/3: DWARFDIE split out to DWARFBaseDIE
ClosedPublic

Authored by jankratochvil on May 23 2018, 12:00 PM.

Details

Summary

As Pavel Labath said in D46810 this is new DWARFBasicDIE to be used for DWARFUnit::GetUnitDIEOnly(). This patch is only mechanical split without any use of it.

Diff Detail

Repository
rL LLVM

Event Timeline

jankratochvil created this revision.May 23 2018, 12:00 PM

Marked things that don't belong in DWARFBasicDIE.

Also DWARFBasicDIE doesn't really explain what it actually is. Maybe we should rename this DWARFUnitDIE? DWARFTopDIE? DWARFRootDIE?

source/Plugins/SymbolFile/DWARF/DWARFBasicDIE.h
49 ↗(On Diff #148267)

This will never be true for DWARFBasicDIE. Remove?

115 ↗(On Diff #148267)

This will never be useful for DWARFBasicDIE since it will represent the first DIE. Remove?

117 ↗(On Diff #148267)

This will never be useful for DWARFBasicDIE since it will represent the first DIE. Remove?

119 ↗(On Diff #148267)

This will never be useful for DWARFBasicDIE since it will represent the first DIE. Remove?

151–155 ↗(On Diff #148267)

This will never be useful for DWARFBasicDIE since it will represent the first DIE. Remove?

jankratochvil marked 5 inline comments as done.May 24 2018, 2:21 AM

Marked things that don't belong in DWARFBasicDIE.

OK, sorry, thanks for reviewing it.

Also DWARFBasicDIE doesn't really explain what it actually is. Maybe we should rename this DWARFUnitDIE? DWARFTopDIE? DWARFRootDIE?

I do not mind any name but if you do not like Pavel's DWARFBasicDIE and you are not sure with the name yourself I have chosen DWARFFirstDIE.

I don't think a name like DWARFUnitDIE is a good one bacause it would make a weird is-a relationship (a DWARFDIE represetning a DW_TAG_variable is certainly not a "unit DIE" yet you could assign it to a DWARFUnitDIE&). We could have a DWARFUnitDIE type if we wanted to, but that would have to be a special type in addition to DWARFBasicDIE. However, I think that would be overkill.

That said, if everyone who is going to be calling IsStructOrClass and friends will see the type as DWARFDIE then keeping those methods on that class makes sense.

Looks like we missed each other, but all I said about DWARFUnitDIE applies to DWARFFirstDIE as well. I doesn't have to be called "basic" die, but the reason I proposed that name is that it does not sound weird when you say that any die "is a" basic die. Other possibility would be to keep DWARFDIE as the "basic" die, and have a something like a "navigatable" die for those that allow you to access children and stuff.

clayborg accepted this revision.May 24 2018, 9:25 AM

I like DWARFFirstDIE. Pavel should ok this too.

This revision is now accepted and ready to land.May 24 2018, 9:25 AM

I like DWARFFirstDIE. Pavel should ok this too.

I said what I think of DWARFFirstDIE. I'd like to hear from you what you think about the is-a issue I mentioned.

I don't think a name like DWARFUnitDIE is a good one bacause it would make a weird is-a relationship (a DWARFDIE represetning a DW_TAG_variable is certainly not a "unit DIE" yet you could assign it to a DWARFUnitDIE&). We could have a DWARFUnitDIE type if we wanted to, but that would have to be a special type in addition to DWARFBasicDIE. However, I think that would be overkill.

Yeah, we just need to be able to tell the difference between the top level DIE we hand out with no children and the one that has all the abilities.

That said, if everyone who is going to be calling IsStructOrClass and friends will see the type as DWARFDIE then keeping those methods on that class makes sense.

Yes, that is the case. No top level DIE can be a struct, union or class. Only a compile unit, type unit or partial unit.

I don't think a name like DWARFUnitDIE is a good one bacause it would make a weird is-a relationship (a DWARFDIE represetning a DW_TAG_variable is certainly not a "unit DIE" yet you could assign it to a DWARFUnitDIE&). We could have a DWARFUnitDIE type if we wanted to, but that would have to be a special type in addition to DWARFBasicDIE. However, I think that would be overkill.

Yeah, we just need to be able to tell the difference between the top level DIE we hand out with no children and the one that has all the abilities.

Yes, but this is not what this patch is doing. The class inheritance makes is such that any "die with all abilities" we hand out, also is-a "top level die with no children". This is where my problem comes from.

If I rephrase your comment a bit

we just need to be able to tell the difference between a DIE that has no ability to access children and the one that has all the abilities.

then it is fine because a "die which has all abilities" is also a "die which has only a certain smaller set of abilities". However in this case a name like DWARFUnitDIE is not appropriate.

If we wanted to have a type called DwarfUnitDIE die then it should be a special type inheriting from DWARFBasicDIE (or whatever it's called). Then again it would be fine, because then it would be a "DIE that has no ability to access children and is a root DIE", which is a refinement of "DIE that has no ability to access children".

I don't think a name like DWARFUnitDIE is a good one bacause it would make a weird is-a relationship (a DWARFDIE represetning a DW_TAG_variable is certainly not a "unit DIE" yet you could assign it to a DWARFUnitDIE&). We could have a DWARFUnitDIE type if we wanted to, but that would have to be a special type in addition to DWARFBasicDIE. However, I think that would be overkill.

Yeah, we just need to be able to tell the difference between the top level DIE we hand out with no children and the one that has all the abilities.

Yes, but this is not what this patch is doing. The class inheritance makes is such that any "die with all abilities" we hand out, also is-a "top level die with no children". This is where my problem comes from.

Yeah, I see where the name doesn't fit now. DWARFBaseDIE maybe?

If I rephrase your comment a bit

we just need to be able to tell the difference between a DIE that has no ability to access children and the one that has all the abilities.

then it is fine because a "die which has all abilities" is also a "die which has only a certain smaller set of abilities". However in this case a name like DWARFUnitDIE is not appropriate.

If we wanted to have a type called DwarfUnitDIE die then it should be a special type inheriting from DWARFBasicDIE (or whatever it's called). Then again it would be fine, because then it would be a "DIE that has no ability to access children and is a root DIE", which is a refinement of "DIE that has no ability to access children".

DWARFBaseDIE might work then. Name doesn't violate any of the assumptions you mention above. All of your issues revolve around the naming right?

Yes, Base is fine. Thank you.

clayborg requested changes to this revision.May 24 2018, 11:46 AM

ok, just rename DWARFFirstDIE to DWARFBaseDIE and this is good to go.

This revision now requires changes to proceed.May 24 2018, 11:46 AM
This revision was not accepted when it landed; it landed in state Needs Revision.May 24 2018, 1:50 PM
Closed by commit rL333222: DWARFDIE split out to DWARFBaseDIE (authored by jankratochvil, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
jankratochvil retitled this revision from 1/3: DWARFDIE split out to DWARFBasicDIE to 1/3: DWARFDIE split out to DWARFBaseDIE.May 24 2018, 1:50 PM