<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 14, 2014 at 9:52 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Eric, Dave,<div><br></div><div>I took a couple of cracks at breaking this up, but unfortunately it doesn't decompose nicely. The ownership change is mostly all or nothing - there are only a few true leaf-functions that can be changed from ObjectImage* to const ObjectFile& without affecting everything else. The debug-info part seems like should be able to be broken out, but it's pretty intimately tied to the change in signature of RuntimeDyld::loadObject (which means it depends on the ownership change), and test cases break if the debug-info issue isn't fixed (which means the ownership change depends on the debug-info change). With some effort I could break the debug-info part out into a separate patch, but it would involve doing violence to the signature of loadObject (it would need to return both an ObjectImage and a LoadedObjectInfo), which would then have to be immediately un-done for the ownership patch. I don't think it'll be a net win for readability.</div></div></blockquote><div><br></div><div>It looks like at least some parts of this could be pulled out - given that an ObjectFile can be retrieved from an ObjectImage, most of the API changes from ObjectFile to ObjectImage could be frontloaded (pushing all the way up to loadImage - essentially rename the unique_ptr<ObjectImage> parameter to ObjIm or something, then immediately introduce an ObjectFile &Obj local variable (= *Obj->getObjectFile()) - flush out all the ObjectImage uses that could be ObjectFile uses that you've refactored here. Commit that as a refactoring. That I think would substantially reduce the noise in the patch & make it easier to see the 'real' work that's going on.<br><br>(also - depending on what the refactoring path looks like, making ObjectImage and ObjectFile more similar (canonicalizing section_end/begin versus end/begin_sections, etc) and adding const would be even smaller increments that would make each step more obvious by allowing more drop in replacement/changes between ObjectImage and ObjectFile as you migrate uses)<br><br>I'm not sure what else could be pulled out - but that seems to constitute a chunk of the changes in this patch. With those aside, perhaps we could see other pieces to separate.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>Dave:</div><div><br></div><div>As per your suggestions I've switch to a function_ref and a single constructor with a default initialiser for DWARFContext.</div></div></blockquote><div><br></div><div>Yep - looks about right.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div> I'll consider switching from .get() to operator* for ErrorOr in a follow up patch. There's much more cleanup to be done in this area.</div></div></blockquote><div><br></div><div>*nod* no doubt.<br><br>btw - do you need error-handling for calling getAddress in getAddrFromObject? (it returns an error_code - could possibly return ErrorOr<uint64_t> to help enforce error handling if this is a real error path that should be handled) Looks like this only returns errors in COFFObjectFile::getSymbolAddress... </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>Updated patch with Dave's suggestions incorporated attached.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>- Lang.</div><div><br></div></font></span></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 14, 2014 at 2:39 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Lang,<br>
<br>
In general the DI changes look ok (modulo Dave's comments). It's a<br>
really large patch so splitting it up would be appreciated for a<br>
closer look.<br>
<br>
The symbol lookup via the passed in function feels awkward (no better<br>
idea offhand so I'm not rejecting it, just feels awkward), can we get<br>
that part of the patch especially split out and we can look at it? :)<br>
<span><font color="#888888"><br>
-eric<br>
</font></span><span><br>
On Fri, Oct 10, 2014 at 2:49 PM, Lang Hames <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>> wrote:<br>
</span><div><div>> Hi All,<br>
><br>
> The attached patch removes object file ownership from RuntimeDyld. Key<br>
> changes are:<br>
><br>
> (1) Instead of a taking a unique_ptr<ObjectFile>, the<br>
> RuntimeDyld::loadObject method will now take a const ObjectFile reference.<br>
> Instead of returning an owning wrapper (ObjectImage) for the loaded object,<br>
> RuntimeDyld::loadObject now returns a "LoadedObjectInfo" instance, which can<br>
> be queried for information about where the object's sections were loaded.<br>
><br>
> (2) The classes that are currently used to manage object file ownership<br>
> within RuntimeDyld (ObjectBuffer and ObjectImage) are removed from the API<br>
> and turned into implementation details in MCJIT and RuntimeDyldELF<br>
> respectively.<br>
><br>
> (3) Minor changes are made to the DIContext and DWARFContext classes to<br>
> support debugging objects loaded by RuntimeDyld.<br>
><br>
> (4) All use-sites of loadObject are updated.<br>
><br>
> Background:<br>
><br>
> RuntimeDyld currently takes ownership of object files to be loaded/linked.<br>
> I'm not sure there was ever a strong reason for this, but these days the<br>
> only reason to have ownership in RuntimeDyld is to support the flawed<br>
> debugging registration system used by RuntimeDyldELF. This system relies on<br>
> rewriting objects in place to update address fields of the loaded<br>
> symbols/sections. The problem is that ObjectFile instances are backed by<br>
> MemoryBuffer instances, which (according to the MemoryBuffer contract)<br>
> represent constant memory. RuntimeDyldELF casts away the constness of the<br>
> underlying memory to make modifications. Usually the memory backing the<br>
> MemoryBuffer is writable, in which case this trick works. Sometimes the<br>
> memory is not writable (e.g. when mmaping files from disk), and you crash<br>
> the JIT. See <a href="http://llvm.org/PR20722" target="_blank">http://llvm.org/PR20722</a> .<br>
><br>
> ELF debugging is important to a number of clients, and we haven't yet<br>
> fleshed out a safe, generic system of debugger registration for the JIT, so<br>
> I don't want to remove it. In lieu of that, this patch opts to clean up the<br>
> interface to RuntimeDyld, and contain the existing craziness within<br>
> RuntimeDyldELF. ELF objects will be cloned to writable memory before being<br>
> modified (still a violation of the MemoryBuffer contract, but safe in<br>
> practice, and no worse than what we had). MachO support is unaffected. Once<br>
> we have a better debugging registration system for the JIT (which is in the<br>
> works) we can go back and clean the rest of the nastiness out of<br>
> RuntimeDyldELF.<br>
><br>
> General feedback and style comments welcome. I could also particularly use<br>
> feedback from debugger experts (Dave, Eric, Adrian) on the changes to<br>
> DIContext/DWARFContext. I think they're sane, but I'm not intimately<br>
> familiar with the DebugInfo library.<br>
><br>
> Hopefully another step towards a cleaner LLVM JIT.<br>
><br>
> Cheers,<br>
> Lang.<br>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>