<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">Actually, looking at the destructors for LLVMContext and Module I do not think the current ownership scheme makes sense, so this might be a good opportunity to re-think it.<div><br></div><div>Right now an LLVMContext owns a list of modules (see LLVMContextImpl::OwnedModules) that it destroys when its destructor is called.  Modules remove themselves from this list if they are destructed before the context:</div><div><br></div><div><div><font face="monospace, monospace">Module::~Module() {</font></div><div><font face="monospace, monospace">  Context.removeModule(this);</font></div></div><div><font face="monospace, monospace">  ...</font></div><div><br></div><div><font face="monospace, monospace">LLVMContextImpl::~LLVMContextImpl() {</font></div><div><font face="monospace, monospace">  // NOTE: We need to delete the contents of OwnedModules, but Module's dtor</font></div><div><font face="monospace, monospace">  // will call LLVMContextImpl::removeModule, thus invalidating iterators into</font></div><div><font face="monospace, monospace">  // the container. Avoid iterators during this operation:</font></div><div><font face="monospace, monospace">  while (!OwnedModules.empty())</font></div><div><font face="monospace, monospace">    delete *OwnedModules.begin();</font></div><div><font face="monospace, monospace">    ...</font></div><div><br></div><div>This makes it unsafe to hold a unique_ptr to a Module: If any Module is still alive when its context goes out of scope it will be double freed, first by the LLVMContextImpl destructor and then again by the unique ptr. Idiomatic scoping means that we tend not to see this in practice  (Module takes an LLVMContext reference, meaning we always declare the context first, so it goes out of scope last), but makes the context ownership redundant: the modules are always freed first via their unique_ptr's.</div><div><br></div><div>I don't think it makes sense for LLVMContext to own Modules. I think that Modules should share ownership of their LLVMContext via a shared_ptr.</div><div><br></div><div>Thoughts?</div><div><br></div><div>Cheers,</div><div>Lang.</div></div></div></div></div></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Sat, Sep 15, 2018 at 4:14 PM Lang Hames <<a href="mailto:lhames@gmail.com">lhames@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi All,<div><br></div><div>ORC's new concurrent compilation model generates some interesting lifetime and thread safety questions around LLVMContext: We need multiple LLVMContexts (one per module in the simplest case, but at least one per thread), and the lifetime of each context depends on the execution path of the JIT'd code. We would like to deallocate contexts once all modules associated with them have been compiled, but there is no safe or easy way to check that condition at the moment as LLVMContext does not expose how many modules are associated with it.</div><div><br></div><div>One way to fix this would be to add a mutex to LLVMContext, and expose this and the module count. Then in the IR-compiling layer of the JIT we could have something like:</div><div><br></div><div><font face="monospace, monospace">// Compile finished, time to deallocate the module.</font></div><div><font face="monospace, monospace">// Explicit deletes used for clarity, we would use unique_ptrs in practice.</font></div><div><font face="monospace, monospace">auto &Ctx = Mod->getContext();</font></div><div><font face="monospace, monospace">delete Mod;</font></div><div><font face="monospace, monospace">std::lock_guard<std::mutex> Lock(Ctx->getMutex());</font></div><div><font face="monospace, monospace">if (Ctx.getNumModules())</font></div><div><font face="monospace, monospace">  delete Ctx;</font></div><div><br></div><div>Another option would be to invert the ownership model and say that each Module shares ownership of its LLVMContext. That way LLVMContexts would be automatically deallocated when the last module using them is destructed (providing no other shared_ptrs to the context are held elsewhere).</div><div><br></div><div>There are other possible approaches (e.g. side tables for the mutex and module count) but before I spent too much time on it I wanted to see whether anyone else has encountered these issues or has opinions on solutions.</div><div><br></div><div>Cheers,</div><div>Lang.</div></div>
</blockquote></div>