[banditopazzo]

Reversed the relationship between module and ee.
Made the module independent from ee.
ee owns the modules added to it, giving back a handle to eventually remove them.

Description

• Reversed ownership when a module is given to the ee removing the owned_by_ee from a module.
• data layout in module is not cached anymore

Related Issue

Closes #723

How This Has Been Tested

llvm 22 on my pc

Option<Breaking Changes>

• ee constructors take modules as self instead of &self
• ee constructors return Result<(ExecutionEngine<'ctx>, EngineModule<'ctx>), LLVMString> instead of Result<ExecutionEngine<'ctx>, LLVMString>
• Module::get_data_layout returns DataLayout (an owned copy)
• ExecutionEngine::add_module(&self, &Module) -> Result<(), ()> becomes add_module(&self, Module<'ctx>) -> EngineModule<'ctx> returing the handle to an engine module
• ExecutionEngine::remove_module(&self, &Module) -> Result<(), RemoveModuleError> becomes remove_module(&self, EngineModule<'ctx>) -> Result<Module<'ctx>, RemoveModuleError> consuming the handle and returning the module
• JitFunction<'ctx, F> becomes JitFunction<'ee, F> borrowing the engine instead of holding an Rc to it
• Mutating a module after handing it to an engine is no longer possible from safe code (the handle has no accessors). Code that created the engine first and added functions afterwards must build first, then create the engine

Checklist

• I have read the Contributing Guide

[ErisianArchitect]

I haven't finished the review because I'm on my phone, but I left a couple comments.

[ErisianArchitect]

I'm not sure why LLVMStringOrRaw was replaced here, could you explain that?

[banditopazzo]

LLVMStringOrRaw::Borrowed only existed so that Module::get_data_layout could return a Ref<DataLayout>. The module cached a DataLayout wrapping the raw pointer from LLVMGetDataLayoutStr. this is a pointer into the module's own std::string. LLVM replaces that string every time it calls Module::setDataLayout, but the cache was only refreshed by inkwell's set_data_layout.

when llvm changes datalayout internally the borrowed datalayout is stale or freed memory, even while a Ref is held, example:

let m = context.create_module("m");
m.set_data_layout(&TargetData::create("e-i64:64-n8:16:32:64-S128").get_data_layout());
let dl = m.get_data_layout();
target_machine.write_to_memory_buffer(&m, FileType::Object).unwrap();
println!("{:?}", dl.as_str()); // invalid

you can try doublecheck with something like valgrind

so I made it owned. I think it requires a a better rewrite but it's too invasive for this pr and out of scope

[ErisianArchitect]

I have doubts that this is even necessary. Is this being done to keep the modules alive?

[banditopazzo]

the llvm C remove module function is bugged:

LLVMBool LLVMRemoveModule(LLVMExecutionEngineRef EE, LLVMModuleRef M,
                          LLVMModuleRef *OutMod, char **OutError) {
  Module *Mod = unwrap(M);
  unwrap(EE)->removeModule(Mod);
  *OutMod = wrap(Mod);
  return 0;
}

always return 0;, it discards the the return value of unwrap(EE)->removeModule(Mod); so it's not possible to know if that module was inside the ee

[TheDan64]

Shouldn't the lifetime of the EngineModule be the module, not the context?

[banditopazzo]

This an internal helper to avoid breaking too much in this PR. The real reason is on a limitation I found on the public api of Module::create_execution_engine.

In a ideal world EngineModule should have the lifetime EngineModule<'ee> so tied to the ExecutionEngine that has ownership of the module you added to it.

The problem is that in the public function

pub fn create_execution_engine(self) -> Result<(ExecutionEngine<'ctx>, EngineModule<'ctx>), LLVMString>

rust doesn't allow you to return in the tuple an ExecutionEngine<'ctx> and an EngineModule<'ee> where the EngineModule have the lifetime of the ExecutionEngine part of the same tuple.

So I had this compromise: EngineModule can only be consumed by an ExecutionEngine to eventually return again a Module after removal from the ee if the module is really owned by that ee. If you drop the ExecutionEngine before the EngineModule, the EngineModule is inert even if it has the lifetime of the context. It can't be consumed (and turned back into a Module) anymore because of the check in ExecutionEngine::remove_module.

I am open to other proposals

[TheDan64]

Sorry, yes, I meant the ee lifetime not module

[TheDan64]

rust doesn't allow you to return in the tuple an ExecutionEngine<'ctx> and an EngineModule<'ee>

Are you sure? I feel like this should be possible

[TheDan64]

Oh I see - because it's returned side by side

[TheDan64]

but couldnt track_module / add_module still return with 'ee?

[banditopazzo]

yes but I don't like either having different lifetime between the first and the following added modules.

we could remove the first engine module from the tuple and add something like:

impl ExecutionEngine {
 pub fn initial_module(&self) -> Option<EngineModule<'_>>; 
}

in this way the lifetime is correct.

in the end remove_module is not a commonly used api and having some frictions is probably fine.

while basically everyone will discards the engine module from the tuple I added. this will become the standard:

let (ee, _) = module.create_jit_execution_engine(OptimizationLevel::None)?;

[banditopazzo]

major changes since last time:

• ee constructor return ExecutionEngine only; the first module’s handle comes from take_initial_loaded_module()
• removed the jit bool: derived from LLVMGetExecutionEngineTargetMachine. only MCJIT has one, interpreter is NULL
• unified TargetData and DataLayout into DataLayout + DataLayoutRef<'a> . same LLVM class, TargetData was an old name
• added can_finalize to protect static constructors/destructors run on empty ee

[banditopazzo]

I discoved 2 other issues already present on main branch:

• a module passed to the ee needs to have default data layout or same as the ee. in case of incompatible data layout, llvm crashes when the compilation is triggered by the jit. in case of interpreter, doesnt crash, but it is undefined behavior
• on add_module the ee can accept a module from a different context. at this point the lifetime is tied to 2 different contexts at the same time. the current implementation doesn't capture that, only looks at the lifetime of the first context, and could ends in an use after free.

tell me how do you prefer to tackle these, here or in a next pr after this? asking because this pr already has changes to the api. implementing on main first doubles the work.

plus the second is impossible to completely solve on the current main, the only fix possible is to limit add_module to accept modules from the same context. you can't modify the lifetime of ee because it's already stored in owned_by_ee field of previous modules.

[TheDan64]

Separate PR please, almost finished reviewing this PR

[TheDan64]

It'd probably be simpler to just have only DataLayout<'> type, where 'static = owned, 'static: 'a for borrowed, then drop is only implemented for 'static

[banditopazzo]

it's not possible to specialize with lifetime, rust doesn't allow that: Drop impls cannot be specialized

other libraries do the same, serde for example has Deserialize<'de> and DeserializeOwned

[TheDan64]

Shoot, that's annoying

[ErisianArchitect]

Shoot, that's annoying

One of the reasons I've been experimenting with switching to Zig. Hence why I haven't been working on Orc2 lately. I've been increasingly frustrated with Rust in the last few months.

[TheDan64]

Zig is really cool but I find the lack of safety guarantees less compelling

[TheDan64]

Thanks!