[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 18:30, David Blaikie <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.)
* 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).

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

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/3af9153f/attachment.html>