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

[Lang Hames]

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.

Dave:

As per your suggestions I've switch to a function_ref and a single
constructor with a default initialiser for DWARFContext. 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.

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/20141014/e22ec0e0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: remove-ownership-from-rtdyld-2.patch
Type: application/octet-stream
Size: 92364 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141014/e22ec0e0/attachment.obj>