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

[Rafael Espíndola]

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