Started changing EE to take ownership of modules

to prevent double frees/other UB and to more closely mimick LLVM's actual representation

src/execution_engine.rs

@@ -1,11 +1,11 @@
-use llvm_sys::execution_engine::{LLVMGetExecutionEngineTargetData, LLVMExecutionEngineRef, LLVMRunFunction, LLVMRunFunctionAsMain, LLVMDisposeExecutionEngine, LLVMGetFunctionAddress, LLVMAddModule, LLVMFindFunction};
+use llvm_sys::execution_engine::{LLVMGetExecutionEngineTargetData, LLVMExecutionEngineRef, LLVMRunFunction, LLVMRunFunctionAsMain, LLVMDisposeExecutionEngine, LLVMGetFunctionAddress, LLVMAddModule, LLVMFindFunction, LLVMLinkInMCJIT, LLVMLinkInInterpreter, LLVMRemoveModule};
 
 use module::Module;
 use targets::TargetData;
 use values::{AsValueRef, FunctionValue};
 
 use std::ffi::CString;
-use std::mem::zeroed;
+use std::mem::{uninitialized, zeroed};
 
 #[derive(Debug, PartialEq, Eq)]
 pub enum FunctionLookupError {
@@ -16,6 +16,7 @@ pub enum FunctionLookupError {
 #[derive(PartialEq, Eq)]
 pub struct ExecutionEngine {
     execution_engine: LLVMExecutionEngineRef,
+    pub(crate) modules: Vec<Module>,
     jit_mode: bool,
 }
 
@@ -25,14 +26,59 @@ impl ExecutionEngine {
 
         ExecutionEngine {
             execution_engine: execution_engine,
+            modules: vec![],
             jit_mode: jit_mode,
         }
     }
 
-    pub fn add_module(&self, module: &Module) {
+    pub fn link_in_mc_jit() {
+        unsafe {
+            LLVMLinkInMCJIT()
+        }
+    }
+
+    pub fn link_in_interpreter() {
+        unsafe {
+            LLVMLinkInInterpreter();
+        }
+    }
+
+    // TODOC: EE must *own* modules and deal out references
+    pub fn add_module(&mut self, module: Module) -> &Module {
         unsafe {
             LLVMAddModule(self.execution_engine, module.module)
         }
+
+        self.modules.push(module);
+
+        &self.modules[self.modules.len() - 1]
+    }
+
+    pub fn remove_module(&mut self, module: &Module) -> Result<Module, String> {
+        let mut new_module = unsafe { uninitialized() };
+        let mut err_str = unsafe { zeroed() };
+
+        let code = unsafe {
+            LLVMRemoveModule(self.execution_engine, module.module, &mut new_module, err_str)
+        };
+
+        // REVIEW: This might end up a hashtable for better performance
+        let mut index = None;
+
+        for (i, owned_module) in self.modules.iter().enumerate() {
+            if module == owned_module {
+                index = Some(i);
+            }
+        }
+
+        self.modules.remove(index.unwrap());
+
+        Ok(Module::new(new_module))
+    }
+
+    // FIXME: Workaround until we can think of a better API
+    pub fn get_module_at(&self, index: usize) -> &Module {
+        &self.modules[index]
     }
 
     /// WARNING: The returned address *will* be invalid if the EE drops first

src/module.rs

@@ -1,7 +1,7 @@
 use llvm_sys::analysis::{LLVMVerifyModule, LLVMVerifierFailureAction};
 use llvm_sys::bit_writer::{LLVMWriteBitcodeToFile, LLVMWriteBitcodeToMemoryBuffer, LLVMWriteBitcodeToFD};
 use llvm_sys::core::{LLVMAddFunction, LLVMAddGlobal, LLVMCreateFunctionPassManagerForModule, LLVMDisposeMessage, LLVMDumpModule, LLVMGetNamedFunction, LLVMGetTypeByName, LLVMSetDataLayout, LLVMSetInitializer, LLVMSetTarget, LLVMCloneModule, LLVMDisposeModule, LLVMGetTarget, LLVMGetDataLayout, LLVMModuleCreateWithName, LLVMGetModuleContext, LLVMGetFirstFunction, LLVMGetLastFunction, LLVMSetLinkage, LLVMAddGlobalInAddressSpace};
-use llvm_sys::execution_engine::{LLVMCreateExecutionEngineForModule, LLVMLinkInInterpreter, LLVMLinkInMCJIT};
+use llvm_sys::execution_engine::{LLVMCreateExecutionEngineForModule, LLVMLinkInInterpreter, LLVMLinkInMCJIT, LLVMCreateJITCompilerForModule, LLVMCreateMCJITCompilerForModule};
 use llvm_sys::prelude::LLVMModuleRef;
 use llvm_sys::LLVMLinkage;
 
@@ -87,6 +87,7 @@ impl Linkage {
     }
 }
 
+#[derive(PartialEq, Eq)]
 pub struct Module {
     pub(crate) module: LLVMModuleRef,
 }
@@ -191,7 +192,10 @@ impl Module {
         }
     }
 
-    pub fn create_execution_engine(&self, jit_mode: bool) -> Result<ExecutionEngine, String> {
+    // TODOC: EE must *own* modules and deal out references
+    // REVIEW: I'm wondering if this should form be disallowed altogether in favor of the explicit
+    // EE types. Having to call these global methods beforehand is a huge hassle but seems avoidable
+    pub fn create_execution_engine(self, jit_mode: bool) -> Result<ExecutionEngine, String> {
         let mut execution_engine = unsafe { uninitialized() };
         let mut err_str = unsafe { zeroed() };
 
@@ -221,7 +225,40 @@ impl Module {
             return Err(rust_str);
         }
 
-        Ok(ExecutionEngine::new(execution_engine, jit_mode))
+        let mut execution_engine = ExecutionEngine::new(execution_engine, jit_mode);
+
+        execution_engine.modules.push(self);
+
+        Ok(execution_engine)
+    }
+
+    // TODOC: EE must *own* modules and deal out references
+    pub fn create_jit_execution_engine(self) -> Result<ExecutionEngine, String> {
+        let mut execution_engine = unsafe { uninitialized() };
+        let mut err_str = unsafe { zeroed() };
+        let opt_level = 1; // TODO: Param
+
+        let code = unsafe {
+            LLVMCreateJITCompilerForModule(&mut execution_engine, self.module, opt_level, &mut err_str) // Should take ownership of module
+        };
+
+        if code == 1 {
+            let rust_str = unsafe {
+                let rust_str = CStr::from_ptr(err_str).to_string_lossy().into_owned();
+
+                LLVMDisposeMessage(&mut *err_str);
+
+                rust_str
+            };
+
+            return Err(rust_str);
+        }
+
+        let mut execution_engine = ExecutionEngine::new(execution_engine, true);
+
+        execution_engine.modules.push(self);
+
+        Ok(execution_engine)
     }
 
     pub fn create_function_pass_manager(&self) -> PassManager {

tests/test_builder.rs

@@ -42,6 +42,7 @@ fn test_null_checked_ptr_ops() {
     let module = context.create_module("unsafe");
     let builder = context.create_builder();
     let execution_engine = module.create_execution_engine(true).unwrap();
+    let module = execution_engine.get_module_at(0);
 
     // Here we're going to create a function that looks roughly like:
     // fn check_null_index1(ptr: *const i8) -> i8 {

tests/test_execution_engine.rs

@@ -2,70 +2,65 @@ extern crate inkwell;
 
 use self::inkwell::context::Context;
 use self::inkwell::execution_engine::FunctionLookupError;
-// use self::inkwell::targets::{InitializationConfig, Target};
-
-// use std::mem::forget;
+use self::inkwell::targets::{InitializationConfig, Target};
 
 #[test]
 fn test_get_function_address() {
     let context = Context::create();
     let module = context.create_module("errors_abound");
     let builder = context.create_builder();
-    let execution_engine = module.create_execution_engine(false).unwrap();
-
     let void_type = context.void_type();
     let fn_type = void_type.fn_type(&[], false);
-    let fn_value = module.add_function("func", &fn_type, None);
-    let basic_block = fn_value.append_basic_block("entry");
 
-    builder.position_at_end(&basic_block);
-    builder.build_return(None);
+    assert_eq!(module.create_jit_execution_engine().err(), Some("Unable to find target for this triple (no targets are registered)".into()));
+
+    let module = context.create_module("errors_abound");
 
-    assert_eq!(execution_engine.get_function_address("errors"), Err(FunctionLookupError::JITNotEnabled));
+    Target::initialize_native(&InitializationConfig::default()).expect("Failed to initialize native target");
 
-    // FIXME: The following results in an invalid memory access somehow. Commenting out EE drop stops it from happenening
-    // which is weird because both created EEs are separate instances(verify!)
-    // forget(execution_engine); // FIXME: This is a workaround so drop isn't called
+    let execution_engine = module.create_jit_execution_engine().unwrap();
 
-    // Target::initialize_native(&InitializationConfig::default()).expect("Failed to initialize native target");
+    assert_eq!(execution_engine.get_function_address("errors"), Err(FunctionLookupError::FunctionNotFound));
 
-    // let execution_engine = module.create_execution_engine(true).unwrap();
+    let module = context.create_module("errors_abound");
+    let fn_value = module.add_function("func", &fn_type, None);
+    let basic_block = context.append_basic_block(&fn_value, "entry");
 
-    // assert_eq!(execution_engine.get_function_address("errors"), Err(FunctionLookupError::FunctionNotFound));
+    builder.position_at_end(&basic_block);
+    builder.build_return(None);
 
-    // // FIXME: Ocassionally fails with `cargo test test_get_function`
-    // // Will either return Err(?) or SF? Inconsistent test..
-    // assert!(execution_engine.get_function_address("func").is_ok());
+    let execution_engine = module.create_jit_execution_engine().unwrap();
 
-    // forget(execution_engine); // FIXME
+    assert_eq!(execution_engine.get_function_address("errors"), Err(FunctionLookupError::FunctionNotFound));
+
+    assert!(execution_engine.get_function_address("func").is_ok());
 }
 
 // #[test]
 // fn test_get_function_value() {
 //     let context = Context::create();
 //     let builder = context.create_builder();
 //     let module = context.create_module("errors_abound");
-//     let execution_engine = module.create_execution_engine(false).unwrap();
-
+//     let mut execution_engine = module.create_jit_execution_engine().unwrap();
+//     let module = execution_engine.get_module_at(0);
 //     let void_type = context.void_type();
 //     let fn_type = void_type.fn_type(&[], false);
 //     let fn_value = module.add_function("func", &fn_type, None);
-//     let basic_block = fn_value.append_basic_block("entry");
+//     let basic_block = context.append_basic_block(&fn_value, "entry");
 
 //     builder.position_at_end(&basic_block);
 //     builder.build_return(None);
 
 //     assert_eq!(execution_engine.get_function_value("errors"), Err(FunctionLookupError::JITNotEnabled));
 
-//     forget(execution_engine); // FIXME: Same issue as in test_get_function_address_errors
+//     // Regain ownership of module
+//     let module = execution_engine.remove_module(&module).unwrap();
 
 //     Target::initialize_native(&InitializationConfig::default()).expect("Failed to initialize native target");
 
-//     let execution_engine = module.create_execution_engine(true).unwrap();
+//     let execution_engine = module.create_jit_execution_engine().unwrap();
 
 //     assert_eq!(execution_engine.get_function_value("errors"), Err(FunctionLookupError::FunctionNotFound));
 
 //     assert!(execution_engine.get_function_value("func").is_ok());
-
-//     forget(execution_engine); // FIXME
 // }

tests/test_tari_example.rs

@@ -12,6 +12,7 @@ fn test_tari_example() {
     let module = context.create_module("sum");
     let builder = context.create_builder();
     let execution_engine = module.create_execution_engine(true).unwrap();
+    let module = execution_engine.get_module_at(0);
 
     let i64_type = context.i64_type();
     let fn_type = i64_type.fn_type(&[&i64_type, &i64_type, &i64_type], false);