[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: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).


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