[llvm] r216071 - Quick fix for an use after free.

[Lang Hames]

Hi Rafael,

Sorry about the delayed review.

Does this patch need to touch LTOModule? In any case, I think there's a bug
here: LTOModule has a create method that takes a file name, which means
LTOModule needs to own its underlying memory. Either LTOModule should own
its MemoryBuffer, or  we should remove the LTOModule::createFromFile
method. The former is consistent with the current behavior - it's probably
easier if we stick with that for now.

Regarding the RuntimeDyld changes - if we can avoid it at all, I'd really
like to keep the Object ownership up in the ExecutionEngine, rather than in
RuntimeDyld. The future direction for the JIT is pushing ownership
responsibility up to clients (or convenience adapters) and out of the JIT
itself. We can't go all the way with that yet, but we should manage the
ownership at the highest level that we can in-tree, which is the
ExecutionEngine.

If you're happy to re-work the patch to move the ownership out that would
be great, but otherwise I'll try to take a look some time soon.

Cheers,
Lang.






On Wed, Aug 20, 2014 at 10:00 PM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> On 20 August 2014 19:37, Lang Hames <lhames at gmail.com> wrote:
> > Hi Rafael,
> >
> > The ExecutionEngine already owns the LL module, right? I think we should
> > actually make addObjectFile an owning operation too, for consistency.
>
> What do you think of the attached patch?
>
> > Side note: In the long term I'd love to move to a world where
> > ExecutionEngine and its subclasses are non-owning and we provide
> convenience
> > classes to automatically manage ownership for clients with simple use
> cases.
> > A lot of JIT clients probably know better than ExecutionEngine how they
> want
> > their memory managed, and for them automatic ownership is just a
> > straightjacket.
>
> Yes. In general I feel that having Foo own Bar was more useful when
> memory management in C++ was manual. You only had to remember to
> delete Foo and Bar would go away. With std::unique_ptr and std::move
> most of that advantage goes away.
>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140822/9642de0e/attachment.html>