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

[David Blaikie]

On Tue, Sep 16, 2014 at 11:35 PM, Frédéric Riss <friss at apple.com> wrote:

>
> On 17 Sep 2014, at 08:11, 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)
>
>
> Yes, this will be the case in object files. And the issue really is
> getUnitForOffset getting confused (after all the thinking we put into this
> one, that would be a pity, wouldn’t it? :-))
>
>
>
>> 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>
>
>
> I don’t have permission to access that file.
>

Sorry about that - seems gmail's a bit hit-or-miss with the "attach from
drive" permissions updates. It should be viewable (& commentable, for
whatever that's worth - not sure that Google Docs comments make for a good
review tool) by anyone with the link now.


> I guess it’s pretty similar to what my topic branch looks like.
>

Quite possibly, not sure. There's a few fun tools in there (had a bit of
fun adding some generic iterator utilities that I should probably find a
use for in tree and checkin anyway)


>
> ​
> 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.
>
>
> To test my patch, this is exactly the approach I used. I moved to
> protected inheritance and chose the APIs that would minimise the API
> surface while not requiring too much of a refactoring. The goal was simply
> to produce an easy to review ‘wrapper’ patch, and leave the bigger methods
> for later. I thought this is the approach you were suggesting. And yes,
> this introduces a bit of API that is meant to disappear soon, but it would
> have allowed me to propose the inheritance removal patch in parallel to
> more functionality refactoring ones.
>

Fair enough - sorry, that's certainly clear now but was less so when this
patch (& the obseleted precursor) was first provided - I wasn't sure if
this was the only effort to remove the inheritance.

For myself I'd (& I did, in the branch where I derived the attached patch
from) approach it by just doing the incremental API migrations up-front. So
long as it's clear that you're going to get to removing the inheritance
soon-ish and it's not going to languish (sorry, this is perhaps a case
where you're paying for the sins of previous developers (including
myself/ourselves, to be fair) - we tend to be a bit twitchy about "short
term workarounds" and vitally concerned about making sure they don't stick
around longer than they're welcome) in tree, there's no particular need to
front-load it than to just migrate an API at a time until the container
surface area goes to zero, then quietly remove the inheritance at the end.

It's all good though.

- David


>
> Fred
>
> (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.
>
>
>> Once this is done a bunch of accessors from the context can go away also.
>>
>> http://reviews.llvm.org/D5369
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140916/f92bd2b1/attachment.html>