Requesting response in regards to implementation of Orc engine.

[ErisianArchitect]

Hope I'm not bothering you too much.

I'm just wrapping up work on the Orc V1 implementation (it's taking longer than I thought), but I've come across some issues that I would like your feedback on before continuing on my implementation.

In the Orc V1 engine, the JIT Stack (basically the execution engine) takes ownership of modules that are added to it. This doesn't seem like a problem at first glance, because it's not too difficult to do this. Just pass the entire module to the Orc Engine and allow the orc engine to handle ownership. The problem comes with the lifetime of the Context, because the module will still be tied to the context that created it. That's why I think it would be a good idea to turn the context into a reference counted smart pointer wrapper, and give a clone of the context to the module to keep the context alive. Then when the module is added to the Orc Engine, the smart pointer to the context is moved into the Orc Engine's "keep alive" system. This could be made possible by placing ContextImpl into an Arc, and forcing Context to be !Send and !Sync by adding a PhantomData<*const ()> field. This could be changed later on so that Context could be thread-safe if need-be.

Another issue that I came across has to do with symbol resolution in the Orc Engine. As far as I'm aware, resolution goes like this: search external symbols in inverse order (LIFO) that modules were added, then when the symbol is not found in any of those modules, the user supplied symbol resolver for each module in LIFO order is searched. The goal for me is to make it so that there's a final fallback global symbol table for things to resolve to when all else fails. The solution that I've come up with to solve this problem is for the Orc Engine to create a dummy module that resolves to the global table for the express purpose of having a fallback lookup table. This dummy module would be added as soon as the engine is created.

Some other things aren't issues I needed feedback on, but I figured I would give you an update on.

I added lockfree_linked_list.rs to /src/. It's, as the name implies, a lock-free linked list. I use it in the Orc Engine implementation to make a thread-safe list to append to in order to keep lazy compile callbacks in memory. I added to the crate-level namespace because I figured that it might be useful in other parts of the crate, especially Orc2.

I also added listeners.rs, which is for JITEventListeners. I don't know if these event listeners are exclusive to Orc or not, so I just added the module to the crate-level namespace as a precaution. They didn't seem Orc specific since they aren't in the Orc namespace.

I created a neat FunctionAddress abstraction as well as a fn_addr! macro. This simplifies creating LLVMOrcTargetAddresses from from extern functions.

let add_addr0 = fn_addr!(jit_functions::add : (i32, i32) -> i32);
// optionally, you can include `fn`, `extern "C" fn`, or `unsafe extern "C" fn` before the parameter list:
let add_addr1 = fn_addr!(jit_functions::add : fn(i32, i32) -> i32);
let add_addr2 = fn_addr!(jit_functions::add : extern "C" fn(i32, i32) -> i32);
let add_addr3 = fn_addr!(jit_functions::add : unsafe extern "C" fn(i32, i32) -> i32);
// When the function has no arguments nor return type, you can exclude the signature.
let say_hello_addr = fn_addr!(jit_functions::say_hello);

The fn_addr macro makes it much easier to safely get a function address without allowing users to pass in raw values.
Here's another example from my sandbox:

extern "C" fn bar(num: i32) {
    println!("extern \"C\" fn bar({num})");
}

let func_addr = engine.create_lazy_compile_callback(|engine| {
    println!("Lazy Compile Callback was called.");
    fn_addr!(bar : (i32))
}).unwrap();

Here, you can see fn_addr in action, but you can also see how the creation of a lazy_compile callback works. The lazy compile callback is simply a trampoline function internal to the JIT stack that will call your provided function to lazily compile some function, then the address of the callback will be replaced with the address that you return. It's pretty neat, and it was a little complicated to make it work with a closure rather than a raw extern "C" function.

Lastly, I added an UnsafeOrcFn trait similar to the UnafeFunctionPointer trait. I even implemented it in a similar way. I just wanted to have 32 arguments for the Orc Engine so that there would be less limitation in the safe API. Ideally, I would like to create a custom proc macro for generating these parameterized implementations. That way the argument names for the call functions would not be PascalCase, and could instead be arg0: T0, arg1: T1, arg2: T2, arg3: T3, .... I didn't want to add a barely used proc macro to the crate without your blessing, though, so I didn't do so. And just in case you're curious, I tested compile times both with my 32 parameters macro and without it, and there was no discernible difference.

[ErisianArchitect]

Oh, I almost forgot. There's also a problem with Eager compilation that I believe is a Windows problem. External symbols from modules aren't exported for resolution, which means that if you add an eagerly compiled module, you can't lookup functions inside of it, which makes it functionally useless. When I looked into it, I recall reading something about the LLVM team not having enough time to implement the proper symbol export stuff for Windows, but I don't have a link to it at the moment. I think I mentioned this in the other thread. I didn't remove the ability to do eager compilation on Windows, I just added a disclaimer that it might not work. Until we can do further testing with different environments, I have no way to know if it's a Windows exclusive thing, or just something wrong with my build of LLVM.

[TheDan64]

Changing the entire crate to be Arc based is less than ideal, as it means most types are no longer simple to copy and move around without explicit clones everywhere

[ErisianArchitect]

That can be a difficult thing to work around, since that means that users will be expected to keep the context that created the module alive while the Orc Engine is alive. It's undefined behavior if they destroy the context before the code has been compiled.

[TheDan64]

Can't we move the context into the orc engine? I suppose that may not be possible with the global context

[ErisianArchitect]

I don't actually think it would be a problem to move ContextImpl into an Arc inside of Context, since Context isn't Clone nor Copy. Then Module could own an Arc<ContextImpl> to keep the context alive, then for Module::get_context, it could just return a ContextRef from the Arc<ContextImpl>, but of course the ContextRef would have a lifetime associated with the Module to prevent use-after free errors. I already tried swapping out the ContextImpl with Arc<ContextImpl>, and there was only a few lines of code that I needed to change to make it work. If this change were implemented, Module and Builder would no longer need lifetime parameters because they would keep the context alive. Neither Module nor Builder are Clone or Copy, so adding in the Arc wouldn't change anything either.

[ErisianArchitect]

One problem would be the create_builder and create_module functions in ContextImpl, they wouldn't have an Arc<ContextImpl> to pass to the Module nor Builder.

[ErisianArchitect]

Oh, and to answer your question, moving the context into the OrcEngine is one option, but then you wouldn't be able to use that context anymore. Another option is to create a thread-safe context for the OrcEngine.

Edit: Sorry for the excessive comments, I should have consolidated this all into one comment.

[TheDan64]

No worries. We can still dole out ContextRef which is a type we already provide

[TheDan64]

Or the orc module could take a ContextRef as input 🤔

Edit: as long as it's associated with the context lifetime, it doesn't even need a full context ref as a field

[ErisianArchitect]

as long as it's associated with the context lifetime, it doesn't even need a full context ref as a field

Yeah, that's a problem, really. You either restrict it to one context per orc engine, pass the context and module into the orc engine, or you make it so that you can keep the context alive. I think the easiest option is to make it so that you can keep the context alive, and it doesn't really break that much. It just means that you would have to clone the context instead of copying a ContextRef. Cloning is pretty cheap with Arc, so it's just a little extra syntax to create a new one.

I think all the options aren't ideal, because no matter what they restrict the user is some way, or make software written with the orc engine unsafe. But the option that restricts the user the least is the Arc option.

Edit: It also removes the necessity for the lifetime in Module and Builder, but the lifetime could be kept anyway just so that you can still create a Module/Builder from a ContextRef.

[ErisianArchitect]

I just discovered another interesting quirk. If you manually drop the Context before you drop the OrcEngine, there will be a status access violation. I think that the OrcEngine needs the context to be alive while it's being dropped. I added a mechanism to keep objects alive with the OrcEngine, but it's not an ideal solution.

One possible solution is to use a single context for the entire OrcEngine, then require users to make modules using that context, but that's not ideal because then you wouldn't be able to do multi-threaded compilation since the Context isn't thread-safe.

[TheDan64]

Yea, I expected as much, that's why I said we should tie it to the context lifetime somehow

[ErisianArchitect]

It's not so simple to tie the OrcEngine to the context lifetime, though, because that would make it impossible to compile modules from multiple threads because all modules would need to be created with the same context, which isn't thread-safe. Having lifetimes attached also makes the types more difficult to use. I'm really leaning into the idea of putting ContextImpl into an Arc, and having Module and Builder own their own copies, then when OrcEngine needs to keep the Context alive longer than the JIT Stack, it could be added to an internal list to prevent the Context from being dropped before the OrcEngine. That would also eliminate the need to attach a lifetime to Module and Builder, making them more versatile. Then, when the user requests the context from the module, instead of calling the internal LLVM function to create a ContextRef, it could clone the Arc<ContextImpl> that it owns and create a new Context from that.

This would make it possible to compile Modules from multiple threads, and when some type needs to take ownership of a module and keep the context that created it alive, it can deconstruct the module and take its Arc<ContextImpl> to keep the context alive.

Context, Builder, and Module already do not implement Clone or Copy, so this doesn't really change much.

Edit: Also, I forgot to mention, we can use PhantomData<*const ()> to prevent types from being Send or Sync where we need to.

Side Note: I was just looking at Context and ContextRef, and I was just thinking that it would be much simpler if ContextImpl were made public, then have Context and ContextRef deref to ContextImpl so that all the functions don't need to be mirrored. Seems error prone.

[TheDan64]

We already only minimally support multithreaded usecase. Full support has never been there because LLVM's complexity is too difficult to try and map fully imo. Scoped threads are probably the best case if someone wants to do multithreading