Address feedback, cleanup, fix memory leak in LLVM-backend

CHANGELOG.md

@@ -2,7 +2,7 @@
 
 ## **[Unreleased]**
 
-- [#1401](https://github.com/wasmerio/wasmer/pull/1401) Make breaking change to `RuntimeError`: `RuntimeError` is now more explicit about its possible error values allowing for better insight into why a call may have failed.
+- [#1401](https://github.com/wasmerio/wasmer/pull/1401) Make breaking change to `RuntimeError`: `RuntimeError` is now more explicit about its possible error values allowing for better insight into why a call into Wasm failed.
 - [#1382](https://github.com/wasmerio/wasmer/pull/1382) Refactored test infranstructure (part 2)
 - [#1380](https://github.com/wasmerio/wasmer/pull/1380) Refactored test infranstructure (part 1)
 - [#1357](https://github.com/wasmerio/wasmer/pull/1357) Refactored bin commands into separate files

examples/callback-guest/callback-guest.rs

@@ -4,6 +4,7 @@ extern "C" {
     fn host_callback() -> u32;
 }
 
+#[no_mangle]
 fn test_callback() -> u32 {
     42
 }

lib/clif-backend/src/signal/mod.rs

@@ -79,9 +79,6 @@ impl RunnableModule for Caller {
 
             match res {
                 Err(err) => {
-                    // probably makes the most sense to actually do a translation here to a
-                    // a generic type defined in runtime-core
-                    // TODO: figure out _this_ error return story
                     *error_out = Some(err.into());
                     false
                 }

lib/clif-backend/src/signal/windows.rs

@@ -48,9 +48,9 @@ fn get_signal_name(code: DWORD) -> &'static str {
         EXCEPTION_BREAKPOINT => "breakpoint",
         EXCEPTION_SINGLE_STEP => "single step",
         EXCEPTION_ARRAY_BOUNDS_EXCEEDED => "array bounds exceeded",
-        EXCEPTION_INT_DIVIDE_BY_ZERO => "int div by zero",
+        EXCEPTION_INT_DIVIDE_BY_ZERO => "integer division by zero",
-        EXCEPTION_INT_OVERFLOW => "int overflow",
+        EXCEPTION_INT_OVERFLOW => "integer overflow",
-        EXCEPTION_PRIV_INSTRUCTION => "priv instruction",
+        EXCEPTION_PRIV_INSTRUCTION => "privileged instruction",
         EXCEPTION_IN_PAGE_ERROR => "in page error",
         EXCEPTION_NONCONTINUABLE_EXCEPTION => "non continuable exception",
         EXCEPTION_STACK_OVERFLOW => "stack overflow",

lib/llvm-backend/cpp/object_loader.hh

@@ -49,7 +49,7 @@ typedef struct {
   visit_fde_t visit_fde;
 } callbacks_t;
 
-using runtime_error_t = void*;
+typedef struct runtime_error_t_privdecl runtime_error_t;
 
 enum WasmTrapType {
   Unreachable = 0,
@@ -63,7 +63,8 @@ enum WasmTrapType {
 
 extern "C" void callback_trampoline(void *, void *);
 
-extern "C" void copy_runtime_error(runtime_error_t src, runtime_error_t dst);
+extern "C" void copy_runtime_error(runtime_error_t *src, runtime_error_t *dst);
+extern "C" void free_runtime_error_without_drop(runtime_error_t*);
 
 struct MemoryManager : llvm::RuntimeDyld::MemoryManager {
 public:
@@ -121,7 +122,7 @@ private:
 
 struct WasmErrorSink {
   WasmTrapType *trap_out;
-  runtime_error_t user_error;
+  runtime_error_t *user_error;
 };
 
 struct WasmException : std::exception {
@@ -150,7 +151,13 @@ public:
 struct UserException : UncatchableException {
 public:
   UserException(size_t data) {
-      error_data = reinterpret_cast<runtime_error_t>(data);
+      error_data = reinterpret_cast<runtime_error_t*>(data);
+  }
+
+  UserException(const UserException &) = delete;
+
+  ~UserException() {
+    free_runtime_error_without_drop(error_data);
   }
 
   virtual std::string description() const noexcept override {
@@ -158,7 +165,7 @@ public:
   }
 
   // The pointer to `Option<RuntimeError>`.
-  runtime_error_t error_data;
+  runtime_error_t *error_data;
 
   virtual void write_error(WasmErrorSink &out) const noexcept override {
     copy_runtime_error(error_data, out.user_error);
@@ -290,7 +297,7 @@ void module_delete(WasmModule *module) { delete module; }
 
 bool cxx_invoke_trampoline(trampoline_t trampoline, void *ctx, void *func,
                        void *params, void *results, WasmTrapType *trap_out,
-                       runtime_error_t user_error, void *invoke_env) noexcept {
+                       runtime_error_t *user_error, void *invoke_env) noexcept {
   try {
     catch_unwind([trampoline, ctx, func, params, results]() {
       trampoline(ctx, func, params, results);

lib/llvm-backend/src/backend.rs

@@ -11,6 +11,7 @@ use inkwell::{
 };
 use libc::c_char;
 use std::{
+    alloc,
     cell::RefCell,
     ffi::{c_void, CString},
     mem,
@@ -69,6 +70,22 @@ extern "C" {
     ) -> bool;
 }
 
+/// Unsafe copy of `RuntimeError`. For use from C++.
+///
+/// This copy is unsafe because `RuntimeError` contains non-`Clone` types such as
+/// `Box<dyn Any>`.
+///
+/// This function should only be used when the ownership can be manually tracked.
+///
+/// For example, this is safe* when used indirectly through the C++ API with a pointer
+/// from `do_early_trap` because `do_early_trap` fully owns the `RuntimeError` and
+/// creates and leaks the `Box` itself.
+///
+/// *: it is only safe provided the following invariants are upheld:
+/// 1. The versions of memory that these 2 pointers point to is only dropped once;
+///    the memory itself can be freed provided the inner type is not dropped.
+/// 2. The duplicated memory is not brought back into Rust to violate standard
+///    mutable aliasing/ownership rules.
 #[no_mangle]
 pub unsafe extern "C" fn copy_runtime_error(
     src: *mut Option<RuntimeError>,
@@ -79,6 +96,18 @@ pub unsafe extern "C" fn copy_runtime_error(
     ptr::copy::<Option<RuntimeError>>(src, dst, 1);
 }
 
+/// Frees the memory of a `Option<RuntimeError>` without calling its destructor.
+/// For use from C++ to safely clean up after `copy_runtime_error`.
+#[no_mangle]
+pub unsafe extern "C" fn free_runtime_error_without_drop(rte: *mut Option<RuntimeError>) {
+    let rte_layout = alloc::Layout::from_size_align(
+        mem::size_of::<Option<RuntimeError>>(),
+        mem::align_of::<Option<RuntimeError>>(),
+    )
+    .expect("layout of `Option<RuntimeError>`");
+    alloc::dealloc(rte as *mut u8, rte_layout)
+}
+
 /// `invoke_trampoline` is a wrapper around `cxx_invoke_trampoline`, for fixing up the obsoleted
 /// `trap_out` in the C++ part.
 unsafe extern "C" fn invoke_trampoline(

lib/runtime-core/src/error.rs

@@ -181,7 +181,7 @@ pub enum InvokeError {
     FailedWithNoError,
     /// Indicates that a trap occurred that is not known to Wasmer.
     UnknownTrap {
-        /// The address that the trap occured at.
+        /// The address that the trap occurred at.
         address: usize,
         /// The name of the signal.
         signal: &'static str,
@@ -251,7 +251,7 @@ impl std::fmt::Display for InvokeError {
 /// extremely rare and impossible to handle.
 #[derive(Debug)]
 pub enum RuntimeError {
-    /// When an invoke returns an error
+    /// An error relating to the invocation of a Wasm function.
     InvokeError(InvokeError),
     /// A metering triggered error value.
     ///