[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 11:21 AM, Frédéric Riss <friss at apple.com> wrote:

>
> On 10 Sep 2014, at 19:34, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Wed, Sep 10, 2014 at 10:17 AM, Frédéric Riss <friss at apple.com> wrote:
>
>>
>> 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> 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)
>>
>>
>> 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?
>>
>
> I'm not sure I follow there - the CRTP should mean that the function needs
> to be written only once (in a class template, and it'll be generated twice
> - once for each of the two derived classes).
>
>
>
> I’m not sure we’re on the same line either. Let’s put things straight:
>
> Units can be extracted from 4 sections: .debug_info, .debug_types,
> .debug_info.dwo and .debug_types.dwo. There are 4 corresponding vectors in
> DWARFContext.
>
> Today we have DWARFContext::getUnitForOffset() that searches .debug_info.
> I wanted to add 3 helpers for the 3 other section (eg.
> getDWOCompileUnitForOffset) and another generic helper (getUnitForOffset)
> that would select the right search function from the unit kind.
>
> You suggested that rather than storing a unit kind, we could store a
> reference to the corresponding vector. If we do this, it seems logical to
> put the search code also in the unit. The reference to the list in itself
> is useless. This means putting the equivalent of getCompileUnitForOffset
> into DWARFUnit.
> There are quite a few calls to getUnitForOffset that don’t have a unit
> available, which means that we either:
>  - leave getCompileUnitForOffset in the context (slight code duplication)
>

Could avoid this duplication by refactoring the getUnitForOffset into a
standalone function template and call that from the CRTP base and from the
current getCompileUnitForOffset. (even in your proposal, this could be used
to simplify the getUnitForOffset function you've proposed, of course - by
just having it call a single function template, passing in the right
collection)


>  - replace calls to getCompileUnitForOffset by "CUSs.empty() ? nullptr :
> CUs[0].getCompileUnitForOffset()”
>
> Am I missing something?
>

No no, I think you've summarized it fairly accurately.


>
> Fred
>
>
>>
>>> 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).
>>
>
> *nod* my objection is more feeling and probably feeling about elegance
> than one of correctness... probably.
>
>
>>
>>
>
>> 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
>>>>> >
>>>>> > 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/d1a40f25/attachment.html>