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

[David Blaikie]

Might be easier as separate patches. As for what I can find of the debug
info stuff:

* Might consider using llvm::function_ref rather than std::function if you
never need to copy/pass ownership of the std::function (I assume you don't,
since you're passing by const std::function&, rather than by value - so I
guess you're just using the std::function during getDWARFContext, not
squirrelling it away in the returned DWARFContext to be used more later)

* Rather than two ctors and an Initialize function, can you just use a
default argument? (not sure if it's better - might be)

* FWIW, I tend to prefer using ErrorOr<unique_ptr<T>>'s operator* rather
than .get(). When I see "x.get()" I wonder if I'm dealing with a case where
'x' is a unique_ptr and thus x.get() is a raw pointer. At least with
operator* I'm not worried about that and "f(std::move(*x))" doesn't have
any such scare/twitch reaction for me. Not sure if any of that makes sense
- but figured I'd mention it.


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/8fa4a3e7/attachment.html>