Add updates from feedback

Co-authored-by: Ivan Enderlin <ivan.enderlin@wanadoo.fr>

ATTRIBUTIONS.md

@@ -17,8 +17,8 @@ These are the projects that were used as inspiration and/or that we are using co
     of generating debug information for each function with Cranelift
     (for example, the sorting of the extended basic blocks before
     processing the instructions), and the API for transforming DWARF
-    see wasm-debug's attribution file for more information (TODO: link
+    see [wasm-debug's attribution file](https://github.com/wasmerio/wasm-debug/blob/master/ATTRIBUTIONS.md)
-    and update here).
+    for more information
 - [stackoverflow](https://stackoverflow.com/a/45795699/1072990): to create an efficient HashMap with pair keys
 - [Emscripten](https://github.com/kripken/emscripten): for emtests test sources to ensure compatibility
 - [The WebAssembly spec](https://github.com/WebAssembly/spec/tree/master/test): for implementation details of WebAssembly and spectests

lib/clif-backend/src/code.rs

@@ -71,7 +71,7 @@ impl ModuleCodeGenerator<CraneliftFunctionCodeGenerator, Caller, CodegenError>
     fn next_function(
         &mut self,
         module_info: Arc<RwLock<ModuleInfo>>,
-        loc: (u32, u32),
+        loc: WasmSpan,
     ) -> Result<&mut CraneliftFunctionCodeGenerator, CodegenError> {
         // define_function_body(
 
@@ -102,8 +102,7 @@ impl ModuleCodeGenerator<CraneliftFunctionCodeGenerator, Caller, CodegenError>
                 target_config: self.isa.frontend_config().clone(),
                 clif_signatures: self.clif_signatures.clone(),
             },
-            start: loc.0,
+            loc,
-            end: loc.1,
         };
 
         let generate_debug_info = module_info.read().unwrap().generate_debug_info;
@@ -120,7 +119,7 @@ impl ModuleCodeGenerator<CraneliftFunctionCodeGenerator, Caller, CodegenError>
             &mut func_env.position,
         );
 
-        builder.set_srcloc(ir::SourceLoc::new(loc.0));
+        builder.set_srcloc(ir::SourceLoc::new(loc.start()));
 
         let entry_block = builder.create_ebb();
         builder.append_ebb_params_for_function_params(entry_block);
@@ -156,9 +155,9 @@ impl ModuleCodeGenerator<CraneliftFunctionCodeGenerator, Caller, CodegenError>
         ),
         CodegenError,
     > {
-        let mut func_bodies: Map<LocalFuncIndex, (ir::Function, (u32, u32))> = Map::new();
+        let mut func_bodies: Map<LocalFuncIndex, (ir::Function, WasmSpan)> = Map::new();
         for f in self.functions.into_iter() {
-            func_bodies.push((f.func, (f.start, f.end)));
+            func_bodies.push((f.func, f.loc));
         }
 
         let (func_resolver_builder, debug_metadata, handler_data) =
@@ -256,12 +255,8 @@ pub struct CraneliftFunctionCodeGenerator {
     next_local: usize,
     position: Position,
     func_env: FunctionEnvironment,
-    /// Start location of the function as an offset in bytes in the Wasm module
+    /// Where the function lives in the Wasm module as a span of bytes
-    /// from the beginning of the code section.
+    loc: WasmSpan,
-    start: u32,
-    /// End location of the function as an offset in bytes in the Wasm module
-    /// from the beginning of the code section.
-    end: u32,
 }
 
 pub struct FunctionEnvironment {

lib/clif-backend/src/resolver.rs

@@ -26,6 +26,7 @@ use wasmer_runtime_core::{
         SigRegistry,
     },
     cache::Error as CacheError,
+    codegen::WasmSpan,
     error::{CompileError, CompileResult},
     module::ModuleInfo,
     structures::{Map, SliceMap, TypedIndex},
@@ -96,7 +97,7 @@ impl FuncResolverBuilder {
 
     pub fn new(
         isa: &dyn isa::TargetIsa,
-        function_bodies: Map<LocalFuncIndex, (ir::Function, (u32, u32))>,
+        function_bodies: Map<LocalFuncIndex, (ir::Function, WasmSpan)>,
         info: &ModuleInfo,
     ) -> CompileResult<(
         Self,
@@ -127,7 +128,7 @@ impl FuncResolverBuilder {
             .par_iter()
             .map_init(
                 || Context::new(),
-                |ctx, (lfi, (func, (start, end)))| {
+                |ctx, (lfi, (func, loc))| {
                     let mut code_buf = Vec::new();
                     ctx.func = func.to_owned();
                     let mut reloc_sink = RelocSink::new();
@@ -179,8 +180,8 @@ impl FuncResolverBuilder {
 
                         let entry = CompiledFunctionData {
                             instructions,
-                            start_srcloc: wasm_debug::types::SourceLoc::new(*start),
+                            start_srcloc: wasm_debug::types::SourceLoc::new(loc.start()),
-                            end_srcloc: wasm_debug::types::SourceLoc::new(*end),
+                            end_srcloc: wasm_debug::types::SourceLoc::new(loc.end()),
                             // this not being 0 breaks inst-level debugging
                             body_offset: 0,
                             body_len: code_buf.len(),

lib/llvm-backend/src/code.rs

@@ -8739,7 +8739,7 @@ impl<'ctx> ModuleCodeGenerator<LLVMFunctionCodeGenerator<'ctx>, LLVMBackend, Cod
     fn next_function(
         &mut self,
         _module_info: Arc<RwLock<ModuleInfo>>,
-        _loc: (u32, u32),
+        _loc: WasmSpan,
     ) -> Result<&mut LLVMFunctionCodeGenerator<'ctx>, CodegenError> {
         // Creates a new function and returns the function-scope code generator for it.
         let (context, builder, intrinsics) = match self.functions.last_mut() {

lib/runtime-core/src/codegen.rs

@@ -65,6 +65,45 @@ impl fmt::Debug for InternalEvent {
     }
 }
 
+/// Type representing an area of Wasm code in bytes as an offset from the
+/// beginning of the code section.
+///
+/// `start` must be less than or equal to `end`.
+#[derive(Copy, Clone, Debug)]
+pub struct WasmSpan {
+    /// Start offset in bytes from the beginning of the Wasm code section
+    start: u32,
+    /// End offset in bytes from the beginning of the Wasm code section
+    end: u32,
+}
+
+impl WasmSpan {
+    /// Create a new `WasmSpan`.
+    ///
+    /// `start` must be less than or equal to `end`.
+    // TODO: mark this function as `const` when asserts get stabilized as `const`
+    // see: https://github.com/rust-lang/rust/issues/57563
+    pub fn new(start: u32, end: u32) -> Self {
+        debug_assert!(start <= end);
+        Self { start, end }
+    }
+
+    /// Start offset in bytes from the beginning of the Wasm code section
+    pub const fn start(&self) -> u32 {
+        self.start
+    }
+
+    /// End offset in bytes from the beginning of the Wasm code section
+    pub const fn end(&self) -> u32 {
+        self.end
+    }
+
+    /// Size in bytes of the span
+    pub const fn size(&self) -> u32 {
+        self.end - self.start
+    }
+}
+
 /// Information for a breakpoint
 #[cfg(unix)]
 pub struct BreakpointInfo<'a> {
@@ -115,7 +154,7 @@ pub trait ModuleCodeGenerator<FCG: FunctionCodeGenerator<E>, RM: RunnableModule,
     fn next_function(
         &mut self,
         module_info: Arc<RwLock<ModuleInfo>>,
-        loc: (u32, u32),
+        loc: WasmSpan,
     ) -> Result<&mut FCG, E>;
     /// Finalizes this module.
     fn finalize(

lib/runtime-core/src/jit_debug.rs

@@ -1,8 +1,11 @@
 //! Code for interacting with the
-//! [GDB JIT inteface](https://sourceware.org/gdb/current/onlinedocs/gdb.html#JIT-Interface).
+//! [GDB JIT interface](https://sourceware.org/gdb/current/onlinedocs/gdb.html#JIT-Interface).
 
+use lazy_static::lazy_static;
+
+use std::os::raw::c_char;
 use std::ptr;
-use std::sync::Arc;
+use std::sync::{Arc, Mutex};
 
 /// Entrypoint that the debugger will use to trigger a read from the
 /// [`__jit_debug_descriptor`] global variable.
@@ -17,45 +20,43 @@ use std::sync::Arc;
 extern "C" fn __jit_debug_register_code() {
     // This code exists to prevent optimization of this function so that the
     // GDB JIT interface behaves as expected
-    let x = 3;
+    let x = 42;
     unsafe {
         std::ptr::read_volatile(&x);
     }
 }
 
 /// The operation that the debugger should perform with the entry that we gave it.
-#[allow(non_camel_case_types)]
 #[derive(Debug)]
 #[repr(u32)]
-pub(crate) enum JITAction {
+enum JitAction {
     /// Do nothing.
-    JIT_NOACTION = 0,
+    NoAction = 0,
     /// Register the given code.
-    JIT_REGISTER_FN = 1,
+    RegisterFn = 1,
     /// Unregister the given code.
-    JIT_UNREGISTER_FN = 2,
+    UnregisterFn = 2,
 }
 
 /// Node of the doubly linked list that the GDB JIT interface reads from.
-#[no_mangle]
 #[repr(C)]
-pub(crate) struct JITCodeEntry {
+struct JitCodeEntry {
     /// Next entry in the linked list.
-    next: *mut JITCodeEntry,
+    next: *mut Self,
     /// Previous entry in the linked list.
-    prev: *mut JITCodeEntry,
+    prev: *mut Self,
     /// Pointer to the data we want the debugger to read.
-    symfile_addr: *mut u8,
+    symfile_addr: *const c_char,
     /// The amount of data at the `symfile_addr` pointer.
     symfile_size: u64,
 }
 
-impl Default for JITCodeEntry {
+impl Default for JitCodeEntry {
     fn default() -> Self {
         Self {
             next: ptr::null_mut(),
             prev: ptr::null_mut(),
-            symfile_addr: ptr::null_mut(),
+            symfile_addr: ptr::null(),
             symfile_size: 0,
         }
     }
@@ -64,15 +65,15 @@ impl Default for JITCodeEntry {
 /// Head node of the doubly linked list that the GDB JIT interface expects.
 #[no_mangle]
 #[repr(C)]
-pub(crate) struct JitDebugDescriptor {
+struct JitDebugDescriptor {
-    /// The version of the JIT interface to use (presumably, TODO: double check this)
+    /// The version of the JIT interface to use.
     version: u32,
-    /// Which action to perform, see [`JITAction`].
+    /// Which action to perform.
-    action_flag: u32,
+    action_flag: JitAction,
     /// The entry in the list that the `action_flag` applies to.
-    relevant_entry: *mut JITCodeEntry,
+    relevant_entry: *mut JitCodeEntry,
     /// The first entry in the doubly linked list.
-    first_entry: *mut JITCodeEntry,
+    first_entry: *mut JitCodeEntry,
 }
 
 /// Global variable that the GDB JIT interface will read the data from.
@@ -81,24 +82,33 @@ pub(crate) struct JitDebugDescriptor {
 /// debugger to perform.
 #[no_mangle]
 #[allow(non_upper_case_globals)]
-pub(crate) static mut __jit_debug_descriptor: JitDebugDescriptor = JitDebugDescriptor {
+static mut __jit_debug_descriptor: JitDebugDescriptor = JitDebugDescriptor {
     version: 1,
-    action_flag: JITAction::JIT_NOACTION as _,
+    action_flag: JitAction::NoAction,
     relevant_entry: ptr::null_mut(),
     first_entry: ptr::null_mut(),
 };
 
+lazy_static! {
+    /// Global lock on [`__jit_debug_descriptor`]. Acquire this lock when
+    /// reading or writing to the global variable. This includes calls to
+    /// [`__jit_debug_register_code`] which may cause a debugger to read from
+    /// the global variable.
+    static ref JIT_DEBUG_DESCRIPTOR_LOCK: Mutex<()> = Mutex::new(());
+}
+
 /// Prepend an item to the front of the `__jit_debug_descriptor` entry list
 ///
 /// # Safety
-/// - Access to underlying global variable is unsynchronized.
+/// - Access to underlying global variable is unsynchronized: acquire a lock on
-/// - Pointer to [`JITCodeEntry`] should point to a valid entry.
+///   [`JIT_DEBUG_DESCRIPTOR_LOCK`] before calling this function.
-unsafe fn push_front(jce: *mut JITCodeEntry) {
+/// - Pointer to [`JitCodeEntry`] must point to a valid entry.
+unsafe fn push_front(jce: *mut JitCodeEntry) {
     if __jit_debug_descriptor.first_entry.is_null() {
         __jit_debug_descriptor.first_entry = jce;
     } else {
         let old_first = __jit_debug_descriptor.first_entry;
-        debug_assert!((*old_first).prev.is_null());
+        assert!((*old_first).prev.is_null());
         (*jce).next = old_first;
         (*old_first).prev = jce;
         __jit_debug_descriptor.first_entry = jce;
@@ -109,11 +119,12 @@ unsafe fn push_front(jce: *mut JITCodeEntry) {
 /// connected to.
 ///
 /// # Safety
-/// - Access to underlying global variable is unsynchronized.
+/// - Access to underlying global variable is unsynchronized: acquire a lock on
-/// - Pointer must point to a valid `JitCodeEntry`.
+///   [`JIT_DEBUG_DESCRIPTOR_LOCK`] before calling this function.
-unsafe fn remove_node(jce: *mut JITCodeEntry) {
+/// - Pointer to [`JitCodeEntry`] must point to a valid entry.
+unsafe fn remove_node(jce: *mut JitCodeEntry) {
     if __jit_debug_descriptor.first_entry == jce {
-        debug_assert!((*jce).prev.is_null());
+        assert!((*jce).prev.is_null());
         __jit_debug_descriptor.first_entry = (*jce).next;
     }
     if !(*jce).prev.is_null() {
@@ -126,25 +137,26 @@ unsafe fn remove_node(jce: *mut JITCodeEntry) {
 
 /// Type for implementing Drop on the memory shared with the debugger.
 #[derive(Debug)]
-struct JITCodeDebugInfoEntryHandleInner(*mut JITCodeEntry);
+struct JitCodeDebugInfoEntryHandleInner(*mut JitCodeEntry);
 
 /// Handle to debug info about JIT code registered with a debugger
 #[derive(Debug, Clone)]
-pub(crate) struct JITCodeDebugInfoEntryHandle(Arc<JITCodeDebugInfoEntryHandleInner>);
+pub(crate) struct JitCodeDebugInfoEntryHandle(Arc<JitCodeDebugInfoEntryHandleInner>);
 
-impl Drop for JITCodeDebugInfoEntryHandleInner {
+impl Drop for JitCodeDebugInfoEntryHandleInner {
     fn drop(&mut self) {
+        let _guard = JIT_DEBUG_DESCRIPTOR_LOCK.lock().unwrap();
         unsafe {
             // unregister the function when dropping the JIT code entry
             __jit_debug_descriptor.relevant_entry = self.0;
-            __jit_debug_descriptor.action_flag = JITAction::JIT_UNREGISTER_FN as u32;
+            __jit_debug_descriptor.action_flag = JitAction::UnregisterFn;
             __jit_debug_register_code();
             __jit_debug_descriptor.relevant_entry = ptr::null_mut();
-            __jit_debug_descriptor.action_flag = JITAction::JIT_NOACTION as u32;
+            __jit_debug_descriptor.action_flag = JitAction::NoAction;
             remove_node(self.0);
-            let entry: Box<JITCodeEntry> = Box::from_raw(self.0);
+            let entry: Box<JitCodeEntry> = Box::from_raw(self.0);
             Vec::from_raw_parts(
-                entry.symfile_addr,
+                entry.symfile_addr as *mut u8,
                 entry.symfile_size as _,
                 entry.symfile_size as _,
             );
@@ -154,16 +166,16 @@ impl Drop for JITCodeDebugInfoEntryHandleInner {
 
 /// Manager of debug info registered with the debugger.
 #[derive(Debug, Clone, Default)]
-pub struct JITCodeDebugInfoManager {
+pub(crate) struct JitCodeDebugInfoManager {
-    inner: Vec<JITCodeDebugInfoEntryHandle>,
+    inner: Vec<JitCodeDebugInfoEntryHandle>,
 }
 
-impl JITCodeDebugInfoManager {
+impl JitCodeDebugInfoManager {
     /// Register debug info relating to JIT code with the debugger.
     pub(crate) fn register_new_jit_code_entry(
         &mut self,
         bytes: &[u8],
-    ) -> JITCodeDebugInfoEntryHandle {
+    ) -> JitCodeDebugInfoEntryHandle {
         let mut owned_bytes = bytes.iter().cloned().collect::<Vec<u8>>();
         // ensure length == capacity to simplify memory freeing code
         owned_bytes.shrink_to_fit();
@@ -172,22 +184,23 @@ impl JITCodeDebugInfoManager {
 
         std::mem::forget(owned_bytes);
 
-        let entry: *mut JITCodeEntry = Box::into_raw(Box::new(JITCodeEntry {
+        let entry: *mut JitCodeEntry = Box::into_raw(Box::new(JitCodeEntry {
-            symfile_addr: ptr,
+            symfile_addr: ptr as *const _,
             symfile_size: len as _,
-            ..JITCodeEntry::default()
+            ..JitCodeEntry::default()
         }));
 
         unsafe {
+            let _guard = JIT_DEBUG_DESCRIPTOR_LOCK.lock().unwrap();
             push_front(entry);
             __jit_debug_descriptor.relevant_entry = entry;
-            __jit_debug_descriptor.action_flag = JITAction::JIT_REGISTER_FN as u32;
+            __jit_debug_descriptor.action_flag = JitAction::RegisterFn;
             __jit_debug_register_code();
             __jit_debug_descriptor.relevant_entry = ptr::null_mut();
-            __jit_debug_descriptor.action_flag = JITAction::JIT_NOACTION as u32;
+            __jit_debug_descriptor.action_flag = JitAction::NoAction;
         }
 
-        let handle = JITCodeDebugInfoEntryHandle(Arc::new(JITCodeDebugInfoEntryHandleInner(entry)));
+        let handle = JitCodeDebugInfoEntryHandle(Arc::new(JitCodeDebugInfoEntryHandleInner(entry)));
         self.inner.push(handle.clone());
 
         handle

lib/runtime-core/src/module.rs

@@ -87,7 +87,7 @@ pub struct ModuleInfo {
     #[cfg(feature = "generate-debug-information")]
     #[serde(skip)]
     /// Resource manager of debug information being used by a debugger.
-    pub debug_info_manager: jit_debug::JITCodeDebugInfoManager,
+    pub(crate) debug_info_manager: jit_debug::JitCodeDebugInfoManager,
 }
 
 impl ModuleInfo {

lib/runtime-core/src/parse.rs

@@ -229,7 +229,10 @@ pub fn read_module<
                 }
 
                 let fcg = mcg
-                    .next_function(Arc::clone(&info), (range.start as u32, range.end as u32))
+                    .next_function(
+                        Arc::clone(&info),
+                        WasmSpan::new(range.start as u32, range.end as u32),
+                    )
                     .map_err(|x| LoadError::Codegen(format!("{:?}", x)))?;
 
                 {

lib/singlepass-backend/src/codegen_x64.rs

@@ -682,7 +682,7 @@ impl ModuleCodeGenerator<X64FunctionCode, X64ExecutionContext, CodegenError>
     fn next_function(
         &mut self,
         _module_info: Arc<RwLock<ModuleInfo>>,
-        _loc: (u32, u32),
+        _loc: WasmSpan,
     ) -> Result<&mut X64FunctionCode, CodegenError> {
         let (mut assembler, mut function_labels, breakpoints, exception_table) =
             match self.functions.last_mut() {