Merge wasmerio#1097

1097: Move inline breakpoint outside of runtime backend r=syrusakbary a=syrusakbary  # Description There was some code smell leaking inline breakpoint implementation into the runtime core backend instead of the compiler itself. This PR fixes it.  # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Syrus <[email protected]>
igrep · Dec 21, 2019 · 7390372 · 7390372
2 parents 0419df9 + ad82bef
commit 7390372
Show file tree

Hide file tree

Showing 14 changed files with 116 additions and 132 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## **[Unreleased]**
 
+- [#1097](https://github.com/wasmerio/wasmer/pull/1097) Move inline breakpoint outside of runtime backend
 - [#1095](https://github.com/wasmerio/wasmer/pull/1095) Update to cranelift 0.52.
 - [#1092](https://github.com/wasmerio/wasmer/pull/1092) Add `get_utf8_string_with_nul` to `WasmPtr` to read nul-terminated strings from memory.
 - [#1071](https://github.com/wasmerio/wasmer/pull/1071) Add support for non-trapping float-to-int conversions, enabled by default.

diff --git a/lib/clif-backend/src/module.rs b/lib/clif-backend/src/module.rs
@@ -42,9 +42,8 @@ impl Module {
         let runnable_module = Caller::new(handler_data, trampolines, func_resolver);
 
         Ok(ModuleInner {
-            runnable_module: Box::new(runnable_module),
+            runnable_module: Arc::new(Box::new(runnable_module)),
             cache_gen,
-
             info,
         })
     }

diff --git a/lib/llvm-backend/src/code.rs b/lib/llvm-backend/src/code.rs
@@ -30,6 +30,7 @@ use std::{
     rc::Rc,
     sync::{Arc, RwLock},
 };
+
 use wasmer_runtime_core::{
     backend::{Backend, CacheGen, CompilerConfig, Token},
     cache::{Artifact, Error as CacheError},
@@ -8982,9 +8983,8 @@ impl<'ctx> ModuleCodeGenerator<LLVMFunctionCodeGenerator<'ctx>, LLVMBackend, Cod
             LLVMBackend::from_buffer(memory).map_err(CacheError::DeserializeError)?;
 
         Ok(ModuleInner {
-            runnable_module: Box::new(backend),
+            runnable_module: Arc::new(Box::new(backend)),
             cache_gen: Box::new(cache_gen),
-
             info,
         })
     }

diff --git a/lib/middleware-common-tests/src/lib.rs b/lib/middleware-common-tests/src/lib.rs
@@ -3,6 +3,7 @@ mod tests {
     use wabt::wat2wasm;
 
     use wasmer_middleware_common::metering::*;
+    use wasmer_runtime_core::backend::RunnableModule;
     use wasmer_runtime_core::codegen::{MiddlewareChain, StreamingCompiler};
     use wasmer_runtime_core::fault::{pop_code_version, push_code_version};
     use wasmer_runtime_core::state::CodeVersion;
@@ -123,6 +124,7 @@ mod tests {
                 baseline: true,
                 msm: msm,
                 base: instance.module.runnable_module.get_code().unwrap().as_ptr() as usize,
+                runnable_module: instance.module.runnable_module.clone(),
                 backend: backend_id,
             });
             true
@@ -165,6 +167,7 @@ mod tests {
                 msm: msm,
                 base: instance.module.runnable_module.get_code().unwrap().as_ptr() as usize,
                 backend: backend_id,
+                runnable_module: instance.module.runnable_module.clone(),
             });
             true
         } else {

diff --git a/lib/runtime-core/src/backend.rs b/lib/runtime-core/src/backend.rs
@@ -104,83 +104,6 @@ pub struct InlineBreakpoint {
     pub ty: InlineBreakpointType,
 }
 
-/// Inline breakpoint size for (x86-64, singlepass).
-pub const INLINE_BREAKPOINT_SIZE_X86_SINGLEPASS: usize = 7;
-
-/// Inline breakpoint size for (aarch64, singlepass).
-pub const INLINE_BREAKPOINT_SIZE_AARCH64_SINGLEPASS: usize = 12;
-
-/// Returns the inline breakpoint size corresponding to an (Architecture, Backend) pair.
-pub fn get_inline_breakpoint_size(arch: Architecture, backend: Backend) -> Option<usize> {
-    match (arch, backend) {
-        (Architecture::X64, Backend::Singlepass) => Some(INLINE_BREAKPOINT_SIZE_X86_SINGLEPASS),
-        (Architecture::Aarch64, Backend::Singlepass) => {
-            Some(INLINE_BREAKPOINT_SIZE_AARCH64_SINGLEPASS)
-        }
-        _ => None,
-    }
-}
-
-/// Attempts to read an inline breakpoint from the code.
-///
-/// Inline breakpoints are detected by special instruction sequences that never
-/// appear in valid code.
-pub fn read_inline_breakpoint(
-    arch: Architecture,
-    backend: Backend,
-    code: &[u8],
-) -> Option<InlineBreakpoint> {
-    match arch {
-        Architecture::X64 => match backend {
-            Backend::Singlepass => {
-                if code.len() < INLINE_BREAKPOINT_SIZE_X86_SINGLEPASS {
-                    None
-                } else if &code[..INLINE_BREAKPOINT_SIZE_X86_SINGLEPASS - 1]
-                    == &[
-                        0x0f, 0x0b, // ud2
-                        0x0f, 0xb9, // ud
-                        0xcd, 0xff, // int 0xff
-                    ]
-                {
-                    Some(InlineBreakpoint {
-                        size: INLINE_BREAKPOINT_SIZE_X86_SINGLEPASS,
-                        ty: match code[INLINE_BREAKPOINT_SIZE_X86_SINGLEPASS - 1] {
-                            0 => InlineBreakpointType::Middleware,
-                            _ => return None,
-                        },
-                    })
-                } else {
-                    None
-                }
-            }
-            _ => None,
-        },
-        Architecture::Aarch64 => match backend {
-            Backend::Singlepass => {
-                if code.len() < INLINE_BREAKPOINT_SIZE_AARCH64_SINGLEPASS {
-                    None
-                } else if &code[..INLINE_BREAKPOINT_SIZE_AARCH64_SINGLEPASS - 4]
-                    == &[
-                        0, 0, 0, 0, // udf #0
-                        0xff, 0xff, 0x00, 0x00, // udf #65535
-                    ]
-                {
-                    Some(InlineBreakpoint {
-                        size: INLINE_BREAKPOINT_SIZE_AARCH64_SINGLEPASS,
-                        ty: match code[INLINE_BREAKPOINT_SIZE_AARCH64_SINGLEPASS - 4] {
-                            0 => InlineBreakpointType::Middleware,
-                            _ => return None,
-                        },
-                    })
-                } else {
-                    None
-                }
-            }
-            _ => None,
-        },
-    }
-}
-
 #[cfg(test)]
 mod backend_test {
     use super::*;
@@ -316,6 +239,23 @@ pub trait RunnableModule: Send + Sync {
     fn get_local_function_offsets(&self) -> Option<Vec<usize>> {
         None
     }
+
+    /// Returns the inline breakpoint size corresponding to an Architecture (None in case is not implemented)
+    fn get_inline_breakpoint_size(&self, _arch: Architecture) -> Option<usize> {
+        None
+    }
+
+    /// Attempts to read an inline breakpoint from the code.
+    ///
+    /// Inline breakpoints are detected by special instruction sequences that never
+    /// appear in valid code.
+    fn read_inline_breakpoint(
+        &self,
+        _arch: Architecture,
+        _code: &[u8],
+    ) -> Option<InlineBreakpoint> {
+        None
+    }
 }
 
 pub trait CacheGen: Send + Sync {

diff --git a/lib/runtime-core/src/codegen.rs b/lib/runtime-core/src/codegen.rs
@@ -249,7 +249,7 @@ impl<
                 })?;
         Ok(ModuleInner {
             cache_gen,
-            runnable_module: Box::new(exec_context),
+            runnable_module: Arc::new(Box::new(exec_context)),
             info: Arc::try_unwrap(info).unwrap().into_inner().unwrap(),
         })
     }

diff --git a/lib/runtime-core/src/fault.rs b/lib/runtime-core/src/fault.rs
@@ -272,9 +272,7 @@ extern "C" fn signal_trap_handler(
     siginfo: *mut siginfo_t,
     ucontext: *mut c_void,
 ) {
-    use crate::backend::{
-        get_inline_breakpoint_size, read_inline_breakpoint, Architecture, InlineBreakpointType,
-    };
+    use crate::backend::{Architecture, InlineBreakpointType};
 
     #[cfg(target_arch = "x86_64")]
     static ARCH: Architecture = Architecture::X64;
@@ -291,17 +289,17 @@ extern "C" fn signal_trap_handler(
             CURRENT_CODE_VERSIONS.with(|versions| {
                 let versions = versions.borrow();
                 for v in versions.iter() {
-                    let magic_size = if let Some(x) = get_inline_breakpoint_size(ARCH, v.backend) {
-                        x
-                    } else {
-                        continue;
-                    };
+                    let magic_size =
+                        if let Some(x) = v.runnable_module.get_inline_breakpoint_size(ARCH) {
+                            x
+                        } else {
+                            continue;
+                        };
                     let ip = fault.ip.get();
                     let end = v.base + v.msm.total_size;
                     if ip >= v.base && ip < end && ip + magic_size <= end {
-                        if let Some(ib) = read_inline_breakpoint(
+                        if let Some(ib) = v.runnable_module.read_inline_breakpoint(
                             ARCH,
-                            v.backend,
                             std::slice::from_raw_parts(ip as *const u8, magic_size),
                         ) {
                             match ib.ty {

diff --git a/lib/runtime-core/src/instance.rs b/lib/runtime-core/src/instance.rs
@@ -146,7 +146,7 @@ impl Instance {
 
     /// Load an `Instance` using the given loader.
     pub fn load<T: Loader>(&self, loader: T) -> ::std::result::Result<T::Instance, T::Error> {
-        loader.load(&*self.module.runnable_module, &self.module.info, unsafe {
+        loader.load(&**self.module.runnable_module, &self.module.info, unsafe {
             &*self.inner.vmctx
         })
     }
@@ -362,7 +362,7 @@ impl Instance {
 
         call_func_with_index(
             &self.module.info,
-            &*self.module.runnable_module,
+            &**self.module.runnable_module,
             &self.inner.import_backing,
             self.inner.vmctx,
             func_index,
@@ -790,7 +790,7 @@ impl<'a> DynFunc<'a> {
 
         call_func_with_index(
             &self.module.info,
-            &*self.module.runnable_module,
+            &**self.module.runnable_module,
             &self.instance_inner.import_backing,
             self.instance_inner.vmctx,
             self.func_index,

diff --git a/lib/runtime-core/src/module.rs b/lib/runtime-core/src/module.rs
@@ -23,9 +23,8 @@ use std::sync::Arc;
 /// This is used to instantiate a new WebAssembly module.
 #[doc(hidden)]
 pub struct ModuleInner {
-    pub runnable_module: Box<dyn RunnableModule>,
+    pub runnable_module: Arc<Box<dyn RunnableModule>>,
     pub cache_gen: Box<dyn CacheGen>,
-
     pub info: ModuleInfo,
 }
 

diff --git a/lib/runtime-core/src/state.rs b/lib/runtime-core/src/state.rs
@@ -2,9 +2,10 @@
 //! state could read or updated at runtime. Use cases include generating stack traces, switching
 //! generated code from one tier to another, or serializing state of a running instace.
 
-use crate::backend::Backend;
+use crate::backend::{Backend, RunnableModule};
 use std::collections::BTreeMap;
 use std::ops::Bound::{Included, Unbounded};
+use std::sync::Arc;
 
 /// An index to a register
 #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, Serialize, Deserialize)]
@@ -173,7 +174,7 @@ pub struct InstanceImage {
 }
 
 /// A `CodeVersion` is a container for a unit of generated code for a module.
-#[derive(Debug, Clone)]
+#[derive(Clone)]
 pub struct CodeVersion {
     /// Indicates if this code version is the baseline version.
     pub baseline: bool,
@@ -186,6 +187,9 @@ pub struct CodeVersion {
 
     /// The backend used to compile this module.
     pub backend: Backend,
+
+    /// `RunnableModule` for this code version.
+    pub runnable_module: Arc<Box<dyn RunnableModule>>,
 }
 
 impl ModuleStateMap {

diff --git a/lib/runtime-core/src/tiering.rs b/lib/runtime-core/src/tiering.rs
@@ -129,6 +129,7 @@ pub unsafe fn run_tiering<F: Fn(InteractiveShellContext) -> ShellExitOperation>(
             .unwrap(),
         base: baseline.module.runnable_module.get_code().unwrap().as_ptr() as usize,
         backend: baseline_backend,
+        runnable_module: baseline.module.runnable_module.clone(),
     });
     let n_versions: Cell<usize> = Cell::new(1);
 
@@ -191,6 +192,7 @@ pub unsafe fn run_tiering<F: Fn(InteractiveShellContext) -> ShellExitOperation>(
                     .unwrap()
                     .as_ptr() as usize,
                 backend: backend_id,
+                runnable_module: optimized.module.runnable_module.clone(),
             });
             n_versions.set(n_versions.get() + 1);
 

diff --git a/lib/runtime-core/src/vm.rs b/lib/runtime-core/src/vm.rs
@@ -975,6 +975,7 @@ mod vm_ctx_tests {
     use crate::module::{ModuleInfo, ModuleInner, StringTable};
     use crate::structures::Map;
     use std::ffi::c_void;
+    use std::sync::Arc;
 
     struct TestData {
         x: u32,
@@ -1095,7 +1096,7 @@ mod vm_ctx_tests {
         }
 
         ModuleInner {
-            runnable_module: Box::new(Placeholder),
+            runnable_module: Arc::new(Box::new(Placeholder)),
             cache_gen: Box::new(Placeholder),
             info: ModuleInfo {
                 memories: Map::new(),