<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 17, 2014 at 1:56 AM, Frédéric Riss <span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span class=""><blockquote type="cite"><div>On 17 Sep 2014, at 08:47, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><br><br style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote" style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On Tue, Sep 16, 2014 at 11:35 PM, Frédéric Riss<span> </span><span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span><blockquote type="cite"><div>On 17 Sep 2014, at 08:11, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Sep 16, 2014 at 10:47 PM, Frederic Riss<span> </span><span dir="ltr"><<a href="mailto:friss@apple.com" target="_blank">friss@apple.com</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">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.<br></blockquote><div><br></div><div>Yeah, I saw from his CR feedback.<br><br>To be honest, since seeing this patch I've been dabbling with some changes of my own to demonstrate a possible direction.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">The next step for me is to factor the the parsing code into this class.<span> </span></blockquote><div><br></div><div>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. <br><br>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).<br><br>The only thing missing to make DWARFUnitSection not expose begin/end (and thus, not expose any container-like API) is to </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">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.<span> </span></blockquote><div><br></div><div>Why is this a problem? Oh, I suppose getUnitForOffset doesn't work so well when it's not a contiguous non-overlapping range?<br><br>Fair enough.<br><br>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)</div></div></div></div></div></blockquote><div><br></div></span><div>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? :-))</div><span><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Once this is fixed, we can move parsing into DWARFUnitSection and remove at least the addUnit method.<br></blockquote><div><br></div><div>Fair enough - the attached patch just moves it as-is, which is no worse than the current state, I don't think.</div><div>​<div class="gmail_chip gmail_drive_chip" style="width:396px;min-height:18px;max-height:18px;padding:5px;color:rgb(34,34,34);font-family:arial;font-style:normal;font-weight:bold;font-size:13px;border:1px solid rgb(221,221,221);line-height:1;background-color:rgb(245,245,245)"><a href="https://docs.google.com/file/d/0B0jpkch3iC_7ZXRwR1d5cGFOcVk/edit?usp=drive_web" style="display:inline-block;max-width:366px;overflow:hidden;text-overflow:ellipsis;white-space:nowrap;text-decoration:none;padding:1px 0px;border:none" target="_blank"><img src="https://ssl.gstatic.com/docs/doclist/images/icon_10_generic_list.png" style="vertical-align: bottom; border: none;"> <span dir="ltr" style="color:rgb(17,85,204);text-decoration:none;vertical-align:bottom">DWARFUnitSection.diff</span></a></div></div></div></div></div></div></blockquote><div><br></div></span><div>I don’t have permission to access that file.</div></div></div></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>I guess it’s pretty similar to what my topic branch looks like.</div></div></div></blockquote><div><br></div><div>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)</div></div></div></blockquote><div><br></div></span><div>The end result in the DWARF classes is very similar, but yeah, I didn’t have the iterator niceties.</div></div></div></blockquote><div><br></div><div>They wouldn't be necessary if there weren't any callers wanting to pass multiple sections - which, as you pointed out, is not quite right anyway. So once that's addressed the fancy iterator usage would go away.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div> Few things I would have made differently:</div><div> - I would have returned iterators rather than implement forEachLineTable </div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div> - 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.</div></div></blockquote><div><div><br class="">Yep - if I didn't mention it previously, the last two methods (for line table and address range handling) seemed somewhat excessive. I'd probably just expose begin/end and keep them where they are.<br><br>Used only once isn't itself an issue - it can still help separate logic into... logical (and named) chunks, but yeah. I don't think it's particularly helpful.</div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div> - I wouldn’t use the empty() property of the vector to check if the section has already been parsed. </div></div></blockquote><div><br></div><div>Yep - just propagating the existing implementation - I agree, though.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div>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.</div></div></blockquote><div><br></div><div>It seems like we should be able to deal with it better. A boolean somewhere, (one option would be to use Optional<DWARFUnitSection>s - and do the parsing in the ctor - pushing the laziness up into DWARFContext, essentially - rather than having DWARFContext unconditionally call parse on every call to "*_unit()" and having that call no-op if parsing had already happened). <br><br>Possibly it'd be nice if we could just avoid the laziness and put it somewhere clear, but I think the use of compile_units() from multiple places (not just for dumping the compile units - but for accessing the line tables, address ranges, etc) makes that a bit harder (maybe we only need the laziness for compile_units? Not sure).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div><div>I guess it’s Alexey’s call how he wants to move forward on this.</div><div><br></div><div>Fred</div><span class=""><div><br></div><div><blockquote type="cite"><div><div class="gmail_quote" style="font-family:Menlo-Regular;font-size:11px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><span><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>​<br>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.<br></div></div></div></div></div></blockquote><div><br></div></span><div>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.</div></div></div></blockquote><div><br></div><div>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.<br><br>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.<br><br>It's all good though.<br><br>- David</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><br></div><div>Fred</div><span><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>(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)<br><br>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Once this is done a bunch of accessors from the context can go away also.<br><br><a href="http://reviews.llvm.org/D5369" target="_blank">http://reviews.llvm.org/D5369</a></blockquote></div></div></div></div></blockquote></span></div></div></blockquote></div></div></blockquote></div><br></span></div></blockquote></div><br></div></div>