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

[Alexey Samsonov]

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.



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