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

[David Blaikie]

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.


>
>
>
>>
>>
>>> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140917/ddaf436e/attachment.html>