[PATCH] Move DWARFUnitSection from the vector API to something more semantically accurate.

Alexey Samsonov vonosmas at gmail.com
Wed Sep 17 16:30:00 PDT 2014


On Wed, Sep 17, 2014 at 4:19 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Wed, Sep 17, 2014 at 3:43 PM, Alexey Samsonov <vonosmas at gmail.com>
> wrote:
>
>>
>> On Tue, Sep 16, 2014 at 11:11 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Tue, Sep 16, 2014 at 10:47 PM, Frederic Riss <friss at apple.com> wrote:
>>>
>>>> The Units method was suggested by Alexey I have no strong feeling
>>>> either way. With this patch applied, the inheritance of SmallVector isn't
>>>> necessary anymore, I didn't fold the move from inheritance to composition
>>>> in this patch, but it could be done.
>>>>
>>>
>>> Yeah, I saw from his CR feedback.
>>>
>>> To be honest, since seeing this patch I've been dabbling with some
>>> changes of my own to demonstrate a possible direction.
>>>
>>>
>>>> The next step for me is to factor the the parsing code into this class.
>>>
>>>
>>> The thing is, I think factoring more of the logic, such as the parsing,
>>> into this class will actually eventually remove the need to expose the
>>> container-like API of this class at all.
>>>
>>> My patch is probably a bit over the top, but for demonstration, I've
>>> included it (locally I've got it broken down into smaller steps).
>>>
>>> The only thing missing to make DWARFUnitSection not expose begin/end
>>> (and thus, not expose any container-like API) is to
>>>
>>>
>>>> There is an intermediate step though: the actual code is semantically
>>>> wrong when it comes to type units. Type units might come from different
>>>> sections, but we put them all in the same DWARFUnitSection object.
>>>
>>>
>>> Why is this a problem? Oh, I suppose getUnitForOffset doesn't work so
>>> well when it's not a contiguous non-overlapping range?
>>>
>>> Fair enough.
>>>
>>> Note that it's only non-fission type units that come from different
>>> sections. (& I don't know how comdat sections look once they're all linked
>>> and deduplicated - so I guess it might be only type units in object files
>>> (not executables or .dwo files) that are in their own sections)
>>>
>>>
>>>> Once this is fixed, we can move parsing into DWARFUnitSection and
>>>> remove at least the addUnit method.
>>>>
>>>
>>> Fair enough - the attached patch just moves it as-is, which is no worse
>>> than the current state, I don't think.
>>>>>>  DWARFUnitSection.diff
>>> <https://docs.google.com/file/d/0B0jpkch3iC_7ZXRwR1d5cGFOcVk/edit?usp=drive_web>
>>>>>> There's some extra tidying up I could do on that patch (& locally I've
>>> got it more granular) - but I figured it might be some use to you and/or
>>> Alexey to see where it might go. Like your patch, mine doesn't actually
>>> remove the inheritance - it just demonstrates (by making the inheritance
>>> private and not exposing any container-like operations or typedefs) that
>>> it's no longer relied upon by anything other than the implementation. At
>>> which point it's fairly easy to refactor it to composition.
>>>
>>> (personally, I think moving the parsing and probably the dumping support
>>> there makes sense - the rest of it... I'd be just as happy to have
>>> begin/end exposed on the DWARFUnitSection and keep the last couple (the
>>> line table and arange handling) where they were, probably)
>>>
>>> Alexey - you mentioned preferring not to make DWARFUnitSection not a
>>> container itself (but rather to have a "units" member on it) - I don't feel
>>> strongly, but we do have at least some prior art/tradition of having things
>>> /be/ containers in LLVM (Modules are containers of functions, functions are
>>> containers of basic blocks, basic blocks are containers of instructions,
>>> etc) so it's not a problem to do that & in some cases I think it's rather
>>> nice. Interested to hear your thoughts/perspective.
>>>
>>
>> I think I just referred to inheritance vs. composition. IMO it's better
>> to have SmallVector<DWARFUnit> as a member in DWARFUnitSection. The latter
>> will certainly serve as "container" for its users, that's fine.
>> llvm::Module contains a list of functions, not inherits from it, and
>> defines its own begin()/end() methods. I think we should do the same here.
>>
>
> Right - I wasn't suggesting (especially public) inheritance was the
> solution here, I was just (/slightly/) arguing in favor of exposing
> begin/end wrappers around the (composed) container member, rather than
> "units()" returning a range. A minor point.
>

Sure, I'm fine with either of this.


>
>
>>
>>
>>
>>>
>>>
>>>> Once this is done a bunch of accessors from the context can go away
>>>> also.
>>>>
>>>> http://reviews.llvm.org/D5369
>>>>
>>>>
>>>>
>>>
>>
>>
>> --
>> Alexey Samsonov
>> vonosmas at gmail.com
>>
>
>


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


More information about the llvm-commits mailing list