[MCJIT][DebugInfo] [PATCH] Remove object-ownership from RuntimeDyld.

[Lang Hames]

Hi Dave,

Just for clarity (though I expect this is what you meant anyway) the changes are from ObjectImage to ObjectFile, not the other way around. Many functions already follow your suggested style (i.e. grabbing an ObjectFile reference from the ObjectImage at the start of the function), but it's not possible in most cases to move changes up in to the function signatures. The reason is that most functions end up transitively calling emitSection, which requires an ObjectImage, so all the other functions end up propagating that in their signatures.

Regarding the debug info changes some clarification is in order. As per our discussions offline:

(1) This patch fixes http://llvm.org/PR20722 by having RuntimeDyldELF clone incoming objects. The crash in that PR is caused when RuntimeDyldELF tries to write to read-only mmapped memory in order to update address fields required for debugging. Cloning the object into memory ensures the underlying memory is writable, which eliminates the crash. This solution is sub-optimal, and we would prefer not to rely on in-place modification for JIT debugging at all, but we're not ready for that change just yet. In lieu of that I've made the signature for loadObject sane and hidden the in-place modification inside RuntimeDyldELF, which brings us to the debug-info changes in this patch...

(2) In the llvm-rtdyld utility, the -printline option prints line table info from loaded objects using a DWARFContext. The DWARFContext had been inspecting the potentially-modified object that had been returned from RuntimeDyld::loadObject (and which is inaccessible as of this patch).
This scheme never worked for MachO (since MachO doesn't do in-place modification), and only sometimes worked on ELF (if you were lucky and MemoryBuffer::getFile(...) returned your object in-memory, rather that mmapped memory, so you survived the aforementioned crash). To make line table information work with unmodified objects, DWARFContext had to be taught how to get address info for symbols from somewhere other than the object file. That is the reason for the introduction of the SymLookup argument in DWARFContextInMemory's constructor: It gives DWARFContext a way to get the symbol information it needs from another source (in this case RuntimeDyld's own symbol tables).

It is this latter change, which actually touches libDebugInfo code, which I'm seeking your and Eric's input on. As a side-effect of this change, -printline may also begin working for MachO - I will check that and, if it is indeed working now, I'll also add a test case as per our offline discussion.

Regarding your other suggestions, while there are some opportunities for breaking bits of this patch up (e.g. renaming iterators to make interfaces look similar in order to reduce renaming), I'm not sure the benefit warrants the extra effort that would have to be put in at this stage.

Cheers,
Lang.


> On Wed, Oct 15, 2014 at 10:16 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
>> On Tue, Oct 14, 2014 at 9:52 PM, Lang Hames <lhames at gmail.com> wrote:
>> Hi Eric, Dave,
>> 
>> 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.
> 
> 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.
> 
> (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)
> 
> 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.
>  
>> 
>> Dave:
>> 
>> As per your suggestions I've switch to a function_ref and a single constructor with a default initialiser for DWARFContext.
> 
> Yep - looks about right.
>  
>> 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.
> 
> *nod* no doubt.
> 
> 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... 
>  
>> 
>> Updated patch with Dave's suggestions incorporated attached.
>> 
>> - Lang.
>> 
>> 
>>> On Tue, Oct 14, 2014 at 2:39 PM, Eric Christopher <echristo at gmail.com> wrote:
>>> Hi Lang,
>>> 
>>> In general the DI changes look ok (modulo Dave's comments). It's a
>>> really large patch so splitting it up would be appreciated for a
>>> closer look.
>>> 
>>> The symbol lookup via the passed in function feels awkward (no better
>>> idea offhand so I'm not rejecting it, just feels awkward), can we get
>>> that part of the patch especially split out and we can look at it? :)
>>> 
>>> -eric
>>> 
>>> On Fri, Oct 10, 2014 at 2:49 PM, Lang Hames <lhames at gmail.com> wrote:
>>> > Hi All,
>>> >
>>> > The attached patch removes object file ownership from RuntimeDyld. Key
>>> > changes are:
>>> >
>>> > (1) Instead of a taking a unique_ptr<ObjectFile>, the
>>> > RuntimeDyld::loadObject method will now take a const ObjectFile reference.
>>> > Instead of returning an owning wrapper (ObjectImage) for the loaded object,
>>> > RuntimeDyld::loadObject now returns a "LoadedObjectInfo" instance, which can
>>> > be queried for information about where the object's sections were loaded.
>>> >
>>> > (2) The classes that are currently used to manage object file ownership
>>> > within RuntimeDyld (ObjectBuffer and ObjectImage) are removed from the API
>>> > and turned into implementation details in MCJIT and RuntimeDyldELF
>>> > respectively.
>>> >
>>> > (3) Minor changes are made to the DIContext and DWARFContext classes to
>>> > support debugging objects loaded by RuntimeDyld.
>>> >
>>> > (4) All use-sites of loadObject are updated.
>>> >
>>> > Background:
>>> >
>>> > RuntimeDyld currently takes ownership of object files to be loaded/linked.
>>> > I'm not sure there was ever a strong reason for this, but these days the
>>> > only reason to have ownership in RuntimeDyld is to support the flawed
>>> > debugging registration system used by RuntimeDyldELF. This system relies on
>>> > rewriting objects in place to update address fields of the loaded
>>> > symbols/sections. The problem is that ObjectFile instances are backed by
>>> > MemoryBuffer instances, which (according to the MemoryBuffer contract)
>>> > represent constant memory. RuntimeDyldELF casts away the constness of the
>>> > underlying memory to make modifications. Usually the memory backing the
>>> > MemoryBuffer is writable, in which case this trick works. Sometimes the
>>> > memory is not writable (e.g. when mmaping files from disk), and you crash
>>> > the JIT. See http://llvm.org/PR20722 .
>>> >
>>> > ELF debugging is important to a number of clients, and we haven't yet
>>> > fleshed out a safe, generic system of debugger registration for the JIT, so
>>> > I don't want to remove it. In lieu of that, this patch opts to clean up the
>>> > interface to RuntimeDyld, and contain the existing craziness within
>>> > RuntimeDyldELF. ELF objects will be cloned to writable memory before being
>>> > modified (still a violation of the MemoryBuffer contract, but safe in
>>> > practice, and no worse than what we had). MachO support is unaffected. Once
>>> > we have a better debugging registration system for the JIT (which is in the
>>> > works) we can go back and clean the rest of the nastiness out of
>>> > RuntimeDyldELF.
>>> >
>>> > General feedback and style comments welcome. I could also particularly use
>>> > feedback from debugger experts (Dave, Eric, Adrian) on the changes to
>>> > DIContext/DWARFContext. I think they're sane, but I'm not intimately
>>> > familiar with the DebugInfo library.
>>> >
>>> > Hopefully another step towards a cleaner LLVM JIT.
>>> >
>>> > Cheers,
>>> > Lang.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141015/d80e8b03/attachment.html>