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

[Lang Hames]

Hi Rafael,

I appear to have pasted my (very belated) response to "Don't own the buffer
in object::Binary" in the wrong thead. I'm not sure what I was thinking
with the RuntimeDyld comment - that all looks good. Your comment on
makeLTOModule makes perfect sense too. So no problems there after all.

This change looks good too. Please go for it!

- Lang.



On Tue, Aug 26, 2014 at 8:51 AM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> On 22 August 2014 19:42, Lang Hames <lhames at gmail.com> wrote:
> > Hi Rafael,
> >
> > Sorry about the delayed review.
>
> NP, sorry for the delay on my side too.
>
> > 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.
>
> What makes it possible for LTOModule to not own the buffer is the fact
> that it parses the entire module before LTOModule::makeLTOModule
> returns. It needs to do that to provide
> LTO_SYMBOL_SCOPE_DEFAULT_CAN_BE_HIDDEN to ld64.  At some point I would
> have wanted to make that optional, but given that tools/gold doesn't
> use LTOModule anymore, it seems better to just uses the simpler
> interface.
>
> > 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.
>
> Not sure I follow. With the patch the ownership ends up in
>
>  SmallVector<std::unique_ptr<MemoryBuffer>, 2> Buffers;
>
> which I added to MCJIT.
>
> Cheers,
> Rafael
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140826/70fed0e1/attachment.html>