<div dir="ltr">Hi Rafael,<div><br></div><div>I appear to have pasted my (very belated) response to "<span style="font-family:arial,sans-serif;font-size:13px">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.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">This change looks good too. Please go for it!</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br>
</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">- Lang.</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Tue, Aug 26, 2014 at 8:51 AM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="">On 22 August 2014 19:42, Lang Hames <<a href="mailto:lhames@gmail.com">lhames@gmail.com</a>> wrote:<br>
> Hi Rafael,<br>
><br>
</div><div class="">> Sorry about the delayed review.<br>
<br>
</div>NP, sorry for the delay on my side too.<br>
<div class=""><br>
> Does this patch need to touch LTOModule? In any case, I think there's a bug<br>
> here: LTOModule has a create method that takes a file name, which means<br>
> LTOModule needs to own its underlying memory. Either LTOModule should own<br>
> its MemoryBuffer, or  we should remove the LTOModule::createFromFile method.<br>
> The former is consistent with the current behavior - it's probably easier if<br>
> we stick with that for now.<br>
<br>
</div>What makes it possible for LTOModule to not own the buffer is the fact<br>
that it parses the entire module before LTOModule::makeLTOModule<br>
returns. It needs to do that to provide<br>
LTO_SYMBOL_SCOPE_DEFAULT_CAN_BE_HIDDEN to ld64.  At some point I would<br>
have wanted to make that optional, but given that tools/gold doesn't<br>
use LTOModule anymore, it seems better to just uses the simpler<br>
interface.<br>
<div class=""><br>
> Regarding the RuntimeDyld changes - if we can avoid it at all, I'd really<br>
> like to keep the Object ownership up in the ExecutionEngine, rather than in<br>
> RuntimeDyld. The future direction for the JIT is pushing ownership<br>
> responsibility up to clients (or convenience adapters) and out of the JIT<br>
> itself. We can't go all the way with that yet, but we should manage the<br>
> ownership at the highest level that we can in-tree, which is the<br>
> ExecutionEngine.<br>
<br>
</div>Not sure I follow. With the patch the ownership ends up in<br>
<br>
 SmallVector<std::unique_ptr<MemoryBuffer>, 2> Buffers;<br>
<br>
which I added to MCJIT.<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div>