[PATCH] Add a UnitKind field to DWARFUnit so that we can discriminate the unit's origin section.

[Frédéric Riss]

> On 10 Sep 2014, at 19:08, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Wed, Sep 10, 2014 at 10:01 AM, Frédéric Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
> 
>> On 10 Sep 2014, at 18:30, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>> 
>> 
>> 
>> On Wed, Sep 10, 2014 at 9:20 AM, Frédéric Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
>> 
>>> On 10 Sep 2014, at 18:10, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>> 
>>> 
>>> 
>>> On Wed, Sep 10, 2014 at 12:45 AM, Frédéric Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
>>> 
>>> > On 09 Sep 2014, at 23:40, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>> >
>>> >
>>> >
>>> > On Tue, Sep 9, 2014 at 5:53 AM, Frederic Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
>>> > Hi dblaikie, samsonov, echristo, aprantl,
>>> >
>>> > There is no simple way to get the origin section of a DWARFUnit. It is necessary
>>> > to know from which section a Unit has been extracted to be able to resolve
>>> > cross-unit references (to find the right list to search fo the reference
>>> > target).
>>> >
>>> > Would it make more sense to give it the actual section (or some kind of alias/handle to it) rather than a flag? That way it'd work for fission (split-dwarf) as well, and type units that may be in comdat sections?
>>> 
>>> Although this discriminates on origin section, the way it’s used is just to lookup the right Unit list in the context.
>>> 
>>> OK - then perhaps it should just be a pointer/reference/whatever to the relevant unit list? Or do you need this for other purposes as well?
>> 
>> Why not, but what type would you make that list then as it needs to be the same across all Units? Shall I change the SmallVector<std::unique_ptr<DWARF(Compile|Type)Unit> > to SmallVector<std::unique_ptr<DWARFUnit> > and add casts in all current users? Would that look better?
>> 
>> 
>> Nope, that wouldn't be much use. So the commonality is the result of  "getUnitForOffset" - that's generic (DWARFUnit*), though covariant if we cared.
>> 
>> You could do this without faux-RTTI as follows:
>> 
>> * Add virtual DWARFUnit *DWARFUnit::getSiblingUnitForOffset(uint32_t Offset) = 0
>> * add a CRTP between DWARFUnit and the derived classes, that would contain the type-specific SmallVector<std::unique_ptr<Derived>> reference (Or pointer, etc) - derived classes are constructed with a reference to that, pass it to their CRTP base. The CRTP base would implement getSiblingUnitForOffset.
>> 
>> Does that make sense? (at least is my explanation understandable - not necessarily a good idea, but making sure I've communicated it clearly)
>> 
>> Good/bad/indifferent? 
> 
> I’d need to code it to be sure, but it looks more complicated than my first approach. Here’s another one that I goes in the direction you describe, but does’t need changing the class hierarchy with CRTP:
> 
> * Add DWARFUnit *DWARFUnit::getUnitForOffset(uint32_t Offset) = 0 (getSiblingUnitForOffset name doesn’t sound great, sibling is already used in DWARF for inter-tag links. I won’t argue over the name though.)
> 
> Name is just a strawman. Just wanted something to indicate that this finds units in the same "bunch" of units.
> 
> Hmm, thinking about this some more - why is it valid to only look for cross-unit references in the same section as the source unit? Aren't these relocated values? Couldn't they point into other sections? (the most obvious example would be to avoid type unit references (DW_TAG_structure_type + DW_AT_signature) and just directly refer to the type in a type unit)

All the references that are described in dwarf (except for DW_FORM_ref_sig8) are section relative offsets. The signature stuff needs another interface anyway and we don’t have the infrastructure to handle that yet. 

> * Store a pointer to member function DWARFUnit *(DWARFContext::*UnitLookup)(uint32_t Offset) in the Unit that I initialize with the right Context function (once I introduced all the get*UnitForOffset() variants). And just have DWARFUnit::getUnitForOffset() call getContext().*UnitLookup(Offset).
> 
> I'd probably still favor the CRTP to avoid the need to have the repeated get*UnitForOffset/*UnitLookup functions, but there's not a /lot/ of difference in it.

OK, I’ll try it. But you agree this also means moving or duplicating the search functions from the context into the DWARFUnit?

> 
> How about that? (BTW, the helpers I’m talking about are waiting on the OffsetComparator fix. Is this one on your list of things to look at?)
> 
> I believe it is - but I'm in a conference all this week, so a lot of more detailed code review is being shelved until next week - just doing what seems urgent or easy.
> 
> My brief thought on the comparator stuff is that it still seems a bit off, but I haven't got all the state in my head right now.

OK, no worries, I just wanted to make sure it didn’t slip through the cracks. For what it’s worth, I had coded a small testcase exercising all offsets to see the behaviour on the limits and it seemed right (I’m speaking about the last iteration using upper_bound).

Fred

> 
> Thanks,
> Fred
> 
>> I have no other current use for the UnitKind than the one bellow.
>> 
>> Fred
>> 
>>>  
>>> It’s not the actual section that matters (the unit already contains a StringRef pointing to the section data). The next patch introduces:
>>> 
>>> DWARFUnit *DWARFContext::getUnitForOffset(const DWARFUnit *Unit, uint32_t Offset) {
>>>    switch (Unit->getKind()) {
>>>    case DWARFUnit::UK_Compile:
>>>      return getCompileUnitForOffset(Offset);
>>>    case DWARFUnit::UK_DWOCompile:
>>>      return getDWOCompileUnitForOffset(Offset);
>>>    case DWARFUnit::UK_Type:
>>>      return getTypeUnitForOffset(Offset);
>>>    case DWARFUnit::UK_DWOType:
>>>      return getDWOTypeUnitForOffset(Offset);
>>>    }
>>>    llvm_unreachable("Unkown unit type in getUnitForOffset.");
>>>  }
>>> 
>>> I’m open to any other way to achieve that. And this handles LTO and fission uses (or I can’t see what issues you’re thinking about).
>>> 
>>> Fred
>>> 
>>> >
>>> > This patch adds a field and an accessor to the DWARFUnit that indicates where
>>> > it originated from. This information will be used in a subsequent patch to
>>> > implement a generic version of DWARFContext::getCompilationUnitForOffset()
>>> > that will work with arbitrary unit types.
>>> >
>>> > Note that this is nearly implemented like the start of LLVM-style RTTI for
>>> > the DWARFUnit, but it seems that we don't need separate classes to represent
>>> > the DWO variants of Units as only their origin changes. Thus we only
>>> > introduce this discriminant field.
>>> >
>>> > http://reviews.llvm.org/D5264 <http://reviews.llvm.org/D5264>
>>> >
>>> > Files:
>>> >   lib/DebugInfo/DWARFCompileUnit.h
>>> >   lib/DebugInfo/DWARFContext.cpp
>>> >   lib/DebugInfo/DWARFTypeUnit.h
>>> >   lib/DebugInfo/DWARFUnit.cpp
>>> >   lib/DebugInfo/DWARFUnit.h
>>> >

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140910/81e8c52d/attachment.html>