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

[David Blaikie]

On Wed, Sep 10, 2014 at 10:01 AM, Frédéric Riss <friss at apple.com> wrote:

>
> 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> wrote:
>
>>
>> On 10 Sep 2014, at 18:10, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Wed, Sep 10, 2014 at 12:45 AM, Frédéric Riss <friss at apple.com> wrote:
>>
>>>
>>> > On 09 Sep 2014, at 23:40, David Blaikie <dblaikie at gmail.com> wrote:
>>> >
>>> >
>>> >
>>> > On Tue, Sep 9, 2014 at 5:53 AM, Frederic Riss <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)


> * 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.


>
> 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.


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