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

[David Blaikie]

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/813baab5/attachment.html>