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

[Alexey Samsonov]

On Wed, Sep 17, 2014 at 1:56 AM, Frédéric Riss <friss at apple.com> wrote:

>
> On 17 Sep 2014, at 08:47, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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)
>
>
> The end result in the DWARF classes is very similar, but yeah, I didn’t
> have the iterator niceties. Few things I would have made differently:
>  - I would have returned iterators rather than implement forEachLineTable
>  - The collectAddressRanges looks strange (at least I wouldn’t have put
> the first argument in there). I don’t think it brings us anything, as it is
> used only once anyway.
>  - I wouldn’t use the empty() property of the vector to check if the
> section has already been parsed. Having managed built Units and having
> tried to parse the section look different enough to me that I’d put a
> separate boolean for the parsing.
>
> I guess it’s Alexey’s call how he wants to move forward on this.
>

Overall, I'm really satisfied with David's direction. Callables in
forEachLineTable and collectAddressRanges() do indeed seem like an
overkill, as these routines (esp. the last one, as Frederic points out)
doesn't seem to be
generally useful. I'm also mildly concerned about getAddressByteSize()
piece - is it theoretically possible to have multiple compile units with
different address byte sizes in real life? It looks strange to me, but as
it's possible in DWARF, I think we should try to not hard-wire same address
byte sizes if possible. Other than that, I'd like to see most of this patch
landed :)


>
> Fred
>
>
>> ​
>> 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
>>
>>
>


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