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

Thu Sep 11 12:48:49 PDT 2014

OK, here's one more suggestion. Can we introduce a new entity:
DWARFUnitSection that would represent the contents of .debug_info /
.debug_types / .debug_info.dwo or .debug_types.dwo.
DWARFUnitSection will essentially be a wrapper around
SmallVector<std::unique_ptr<DWARF(Compile|Type)Unit>, 1>

DWARFContext will then have:
  DWARFUnitSection<DWARFCompileUnit> CUs, DWOCUs;
  DWARFUnitSection<DWARFTypeUnit> TUs, DWOTUs;

It is DWARFUnitSection that will have getUnitForOffset() method
implementation. Each DWARFUnit can store a reference to DWARFUnitSection
it's located in.
As an additional bonus, we might be able to declare generic
DWARFUnitSection::parse(), DWARFUnitSection()::units() iterator pair, and
so on.

WDYT?

On Wed, Sep 10, 2014 at 11:35 AM, David Blaikie <dblaikie at gmail.com> wrote:

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

-- 
Alexey Samsonov
vonosmas at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140911/add9b352/attachment.html>