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

[David Blaikie]

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.


> 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/4124a1a8/attachment.html>