ExecutionEngine is not thread-safe

[programmerjake]

Describe the Bug The ExecutionEngine class in LLVM is thread safe and intended to be used from multiple threads. The ExecutionEngine wrapper in inkwell is not -- it uses Rc<...> internally.

https://github.com/llvm/llvm-project/blob/6ce1067f2ddd8623b163175ee9488673b9ced8d2/llvm/include/llvm/ExecutionEngine/ExecutionEngine.h#L159

LLVM Version (please complete the following information):

• LLVM Version: 11.1.0
• Inkwell Branch Used: llvm11-0

[TheDan64]

We don't currently support using Inkwell types across multiple threads as it would be a pretty big undertaking to get right. Thanks for pointing it out, though. https://github.com/TheDan64/inkwell/issues/28 is related

I'm not even sure this is possible to do safely in rust as the EE needs to have a lifetime associated with the parent Context that it is derived from, and you wouldn't be able to send that EE<'ctx> to another thread because of that lifetime, I think?

[programmerjake]

Hmm, maybe it could be done the other way around: Contexts would have the lifetime of the ExecutionEngine, since the ExecutionEngine handles multiple Contexts.

Alternative idea: the safe ExecutionEngine wrapper could hold on to Vec<Arc<ContextInsides>> for all the contexts it needs to keep live, and a Context would be more like struct Context(Option<ContextInsides>) where the ExecutionEngine would take out the insides and prevent using the context while LLVM could be using the Context from another thread internally.

[BadBastion]

I have vested interest in this usecase and would be more than happy to take a crack at solving it.

I believe ExecutionEngine doesn't directly depend on the lifetime of any specific Context, it is specifically the function pointers returned by execution engine that require both Context and ExecutionEngine to outlive the function pointer.

The most direct way to represent this in rust would be changing JitFunction to hold an Arc to both Context & ExecutionEngine objects.

Unfortunately I don't see any way this isn't a breaking change to the API. Even if it is just removing <'ctx> lifetimes.

[TheDan64]

Feel free to take a crack!

Its worth mentioning though, that EEs can be used multi threaded today with scoped threads!

Just wanted to throw that out there for anyone looking for a more immediate solution

[BadBastion]

Created #487 for discussion.

For the best user experience JitFunction would be Send, Sync, and have no lifetime. Having an Arc for Execution Engine is pretty easy. Using an Arc for Context would require additional changes. This likely means also changing Context to be Arc<ContextImpl>, but explicitly marking it !Send since it's unsafe to call methods on context across threads. The Arc is just used for ownership.

Great, but this bring us to where the changes start to cascade:

Problem: How do we get the context inside the ExecutionEngine.get_function so we can clone the Arc?

• Answer 1: Pass Context through Module and ExecutionEngine so we can just lookup self._context.
  • This means removing create_builder and create_module from ContextRef
  • This essentially means we force the ExecutionEngine to have a 1 to 1 mapping with Contexts
• Answer 2: Make the user figure it out and pass Context into get_function.
  • This is the least ergonomic, in addition if the user passes in the wrong Context it will be unsafe.
• Answer 3: Use global map LLVMContextRef -> Context for ref counting.
  • The would provide the best ergonomics to the user, but is by far the most significant change to the repo.