Store module name on wasmtime_environ::Module (#1309)

* Store module name on `wasmtime_environ::Module`

This keeps all name information in one place so we dont' have to keep
extra structures around in `wasmtime::Module`.

* rustfmt

cranelift/wasm/src/environ/dummy.rs

@@ -128,6 +128,9 @@ pub struct DummyEnvironment {
     /// Instructs to collect debug data during translation.
     debug_info: bool,
 
+    /// Name of the module from the wasm file.
+    pub module_name: Option<String>,
+
     /// Function names.
     function_names: SecondaryMap<FuncIndex, String>,
 }
@@ -141,6 +144,7 @@ impl DummyEnvironment {
             func_bytecode_sizes: Vec::new(),
             return_mode,
             debug_info,
+            module_name: None,
             function_names: SecondaryMap::new(),
         }
     }
@@ -726,6 +730,11 @@ impl<'data> ModuleEnvironment<'data> for DummyEnvironment {
         Ok(())
     }
 
+    fn declare_module_name(&mut self, name: &'data str) -> WasmResult<()> {
+        self.module_name = Some(String::from(name));
+        Ok(())
+    }
+
     fn declare_func_name(&mut self, func_index: FuncIndex, name: &'data str) -> WasmResult<()> {
         self.function_names[func_index] = String::from(name);
         Ok(())

cranelift/wasm/src/environ/spec.rs

@@ -648,6 +648,14 @@ pub trait ModuleEnvironment<'data>: TargetEnvironment {
         data: &'data [u8],
     ) -> WasmResult<()>;
 
+    /// Declares the name of a module to the environment.
+    ///
+    /// By default this does nothing, but implementations can use this to read
+    /// the module name subsection of the custom name section if desired.
+    fn declare_module_name(&mut self, _name: &'data str) -> WasmResult<()> {
+        Ok(())
+    }
+
     /// Declares the name of a function to the environment.
     ///
     /// By default this does nothing, but implementations can use this to read

cranelift/wasm/src/sections_translator.rs

@@ -429,9 +429,13 @@ pub fn parse_name_section<'data>(
                         environ.declare_func_name(index, name)?;
                     }
                 }
-                return Ok(());
             }
-            wasmparser::Name::Local(_) | wasmparser::Name::Module(_) => {}
+            wasmparser::Name::Module(module) => {
+                if let Ok(name) = module.get_name() {
+                    environ.declare_module_name(name)?;
+                }
+            }
+            wasmparser::Name::Local(_) => {}
         };
     }
     Ok(())

crates/api/src/frame_info.rs

@@ -1,8 +1,8 @@
-use crate::module::Names;
 use std::collections::BTreeMap;
 use std::sync::{Arc, RwLock};
 use wasmtime_environ::entity::EntityRef;
 use wasmtime_environ::wasm::FuncIndex;
+use wasmtime_environ::Module;
 use wasmtime_jit::CompiledModule;
 
 lazy_static::lazy_static! {
@@ -39,7 +39,7 @@ pub struct GlobalFrameInfoRegistration {
 struct ModuleFrameInfo {
     start: usize,
     functions: BTreeMap<usize, (usize, FuncIndex)>,
-    names: Arc<Names>,
+    module: Arc<Module>,
 }
 
 impl GlobalFrameInfo {
@@ -49,11 +49,7 @@ impl GlobalFrameInfo {
     /// compiled functions within `module`. If the `module` has no functions
     /// then `None` will be returned. Otherwise the returned object, when
     /// dropped, will be used to unregister all name information from this map.
-    pub fn register(
+    pub fn register(&self, module: &CompiledModule) -> Option<GlobalFrameInfoRegistration> {
-        &self,
-        names: &Arc<Names>,
-        module: &CompiledModule,
-    ) -> Option<GlobalFrameInfoRegistration> {
         let mut min = usize::max_value();
         let mut max = 0;
         let mut functions = BTreeMap::new();
@@ -92,7 +88,7 @@ impl GlobalFrameInfo {
             ModuleFrameInfo {
                 start: min,
                 functions,
-                names: names.clone(),
+                module: module.module().clone(),
             },
         );
         assert!(prev.is_none());
@@ -114,9 +110,9 @@ impl GlobalFrameInfo {
             return None;
         }
         Some(FrameInfo {
-            module_name: info.names.module_name.clone(),
+            module_name: info.module.name.clone(),
             func_index: func_index.index() as u32,
-            func_name: info.names.module.func_names.get(func_index).cloned(),
+            func_name: info.module.func_names.get(func_index).cloned(),
         })
     }
 }

crates/api/src/module.rs

@@ -7,10 +7,7 @@ use crate::types::{
 use anyhow::{bail, Error, Result};
 use std::path::Path;
 use std::sync::{Arc, Mutex};
-use wasmparser::{
+use wasmparser::{validate, ExternalKind, ImportSectionEntryType, ModuleReader, SectionCode};
-    validate, CustomSectionKind, ExternalKind, ImportSectionEntryType, ModuleReader, Name,
-    SectionCode,
-};
 use wasmtime_jit::CompiledModule;
 
 fn into_memory_type(mt: wasmparser::MemoryType) -> Result<MemoryType> {
@@ -141,12 +138,6 @@ struct ModuleInner {
     exports: Box<[ExportType]>,
     compiled: CompiledModule,
     frame_info_registration: Mutex<Option<Option<GlobalFrameInfoRegistration>>>,
-    names: Arc<Names>,
-}
-
-pub struct Names {
-    pub module: Arc<wasmtime_environ::Module>,
-    pub module_name: Option<String>,
 }
 
 impl Module {
@@ -234,7 +225,7 @@ impl Module {
     pub fn new_with_name(store: &Store, bytes: impl AsRef<[u8]>, name: &str) -> Result<Module> {
         let mut module = Module::new(store, bytes.as_ref())?;
         let inner = Arc::get_mut(&mut module.inner).unwrap();
-        Arc::get_mut(&mut inner.names).unwrap().module_name = Some(name.to_string());
+        Arc::get_mut(inner.compiled.module_mut()).unwrap().name = Some(name.to_string());
         Ok(module)
     }
 
@@ -379,16 +370,11 @@ impl Module {
             store.engine().config().profiler.as_ref(),
         )?;
 
-        let names = Arc::new(Names {
-            module_name: None,
-            module: compiled.module().clone(),
-        });
         Ok(Module {
             inner: Arc::new(ModuleInner {
                 store: store.clone(),
                 imports: Box::new([]),
                 exports: Box::new([]),
-                names,
                 compiled,
                 frame_info_registration: Mutex::new(None),
             }),
@@ -424,7 +410,7 @@ impl Module {
     /// # }
     /// ```
     pub fn name(&self) -> Option<&str> {
-        self.inner.names.module_name.as_deref()
+        self.inner.compiled.module().name.as_deref()
     }
 
     /// Returns the list of imports that this [`Module`] has and must be
@@ -644,23 +630,6 @@ impl Module {
                         exports.push(ExportType::new(entry.field, r#type));
                     }
                 }
-                SectionCode::Custom {
-                    kind: CustomSectionKind::Name,
-                    ..
-                } => {
-                    // Read name section. Per spec, ignore invalid custom section.
-                    if let Ok(mut reader) = section.get_name_section_reader() {
-                        while let Ok(entry) = reader.read() {
-                            if let Name::Module(name) = entry {
-                                if let Ok(name) = name.get_name() {
-                                    Arc::get_mut(&mut inner.names).unwrap().module_name =
-                                        Some(name.to_string());
-                                }
-                                break;
-                            }
-                        }
-                    }
-                }
                 SectionCode::Custom {
                     name: "webidl-bindings",
                     ..
@@ -702,6 +671,6 @@ and for re-adding support for interface types you can see this issue:
         if info.is_some() {
             return;
         }
-        *info = Some(FRAME_INFO.register(&self.inner.names, &self.inner.compiled));
+        *info = Some(FRAME_INFO.register(&self.inner.compiled));
     }
 }

crates/environ/src/module.rs

@@ -141,6 +141,9 @@ pub struct Module {
     /// A unique identifier (within this process) for this module.
     pub id: usize,
 
+    /// The name of this wasm module, often found in the wasm file.
+    pub name: Option<String>,
+
     /// Local information about a module which is the bare minimum necessary to
     /// translate a function body. This is derived as `Hash` whereas this module
     /// isn't, since it contains too much information needed to translate a
@@ -222,6 +225,7 @@ impl Module {
 
         Self {
             id: NEXT_ID.fetch_add(1, SeqCst),
+            name: None,
             imported_funcs: PrimaryMap::new(),
             imported_tables: PrimaryMap::new(),
             imported_memories: PrimaryMap::new(),

crates/environ/src/module_environ.rs

@@ -409,6 +409,11 @@ impl<'data> cranelift_wasm::ModuleEnvironment<'data> for ModuleEnvironment<'data
         Ok(())
     }
 
+    fn declare_module_name(&mut self, name: &'data str) -> WasmResult<()> {
+        self.result.module.name = Some(name.to_string());
+        Ok(())
+    }
+
     fn declare_func_name(&mut self, func_index: FuncIndex, name: &'data str) -> WasmResult<()> {
         self.result
             .module

crates/jit/src/instantiate.rs

@@ -252,6 +252,11 @@ impl CompiledModule {
         &self.module
     }
 
+    /// Return a reference-counting pointer to a module.
+    pub fn module_mut(&mut self) -> &mut Arc<Module> {
+        &mut self.module
+    }
+
     /// Return a reference to a module.
     pub fn module_ref(&self) -> &Module {
         &self.module