Move almost all logic out of Module

cranelift/module/src/backend.rs

@@ -5,7 +5,7 @@ use crate::FuncId;
 use crate::Linkage;
 use crate::ModuleDeclarations;
 use crate::ModuleResult;
-use crate::{DataContext, FuncOrDataId};
+use crate::DataContext;
 use core::marker;
 use cranelift_codegen::isa::TargetIsa;
 use cranelift_codegen::Context;
@@ -13,7 +13,7 @@ use cranelift_codegen::{binemit, ir};
 
 use std::boxed::Box;
 use std::string::String;
-use std::{borrow::ToOwned, collections::HashMap};
+use std::borrow::ToOwned;
 
 /// A `Backend` implements the functionality needed to support a `Module`.
 ///
@@ -99,11 +99,7 @@ where
 
     /// Consume this `Backend` and return a result. Some implementations may
     /// provide additional functionality through this result.
-    fn finish(
+    fn finish(self, declarations: ModuleDeclarations) -> Self::Product;
-        self,
-        names: HashMap<String, FuncOrDataId>,
-        declarations: ModuleDeclarations,
-    ) -> Self::Product;
 }
 
 /// Default names for `ir::LibCall`s. A function by this name is imported into the object as

cranelift/module/src/module.rs

@@ -202,11 +202,18 @@ impl DataDeclaration {
 /// This provides a view to the state of a module which allows `ir::ExternalName`s to be translated
 /// into `FunctionDeclaration`s and `DataDeclaration`s.
 pub struct ModuleDeclarations {
+    names: HashMap<String, FuncOrDataId>,
     functions: PrimaryMap<FuncId, FunctionDeclaration>,
     data_objects: PrimaryMap<DataId, DataDeclaration>,
 }
 
 impl ModuleDeclarations {
+    /// Get the module identifier for a given name, if that name
+    /// has been declared.
+    pub fn get_name(&self, name: &str) -> Option<FuncOrDataId> {
+        self.names.get(name).copied()
+    }
+
     /// Get the `FuncId` for the function named by `name`.
     pub fn get_function_id(&self, name: &ir::ExternalName) -> FuncId {
         if let ir::ExternalName::User { namespace, index } = *name {
@@ -245,6 +252,75 @@ impl ModuleDeclarations {
             panic!("unexpected ExternalName kind {}", name)
         }
     }
+
+    /// Declare a function in this module.
+    pub fn declare_function(
+        &mut self,
+        name: &str,
+        linkage: Linkage,
+        signature: &ir::Signature,
+    ) -> ModuleResult<(FuncId, &FunctionDeclaration)> {
+        // TODO: Can we avoid allocating names so often?
+        use super::hash_map::Entry::*;
+        match self.names.entry(name.to_owned()) {
+            Occupied(entry) => match *entry.get() {
+                FuncOrDataId::Func(id) => {
+                    let existing = &mut self.functions[id];
+                    existing.merge(linkage, signature)?;
+                    Ok((id, existing))
+                }
+                FuncOrDataId::Data(..) => {
+                    Err(ModuleError::IncompatibleDeclaration(name.to_owned()))
+                }
+            },
+            Vacant(entry) => {
+                let id = self.functions.push(FunctionDeclaration {
+                    name: name.to_owned(),
+                    linkage,
+                    signature: signature.clone(),
+                });
+                entry.insert(FuncOrDataId::Func(id));
+                Ok((id, &self.functions[id]))
+            }
+        }
+    }
+
+    /// Declare a data object in this module.
+    pub fn declare_data(
+        &mut self,
+        name: &str,
+        linkage: Linkage,
+        writable: bool,
+        tls: bool,
+        align: Option<u8>, // An alignment bigger than 128 is unlikely
+    ) -> ModuleResult<(DataId, &DataDeclaration)> {
+        // TODO: Can we avoid allocating names so often?
+        use super::hash_map::Entry::*;
+        match self.names.entry(name.to_owned()) {
+            Occupied(entry) => match *entry.get() {
+                FuncOrDataId::Data(id) => {
+                    let existing = &mut self.data_objects[id];
+                    existing.merge(linkage, writable, tls, align);
+                    Ok((id, existing))
+                }
+
+                FuncOrDataId::Func(..) => {
+                    Err(ModuleError::IncompatibleDeclaration(name.to_owned()))
+                }
+            },
+            Vacant(entry) => {
+                let id = self.data_objects.push(DataDeclaration {
+                    name: name.to_owned(),
+                    linkage,
+                    writable,
+                    tls,
+                    align,
+                });
+                entry.insert(FuncOrDataId::Data(id));
+                Ok((id, &self.data_objects[id]))
+            }
+        }
+    }
 }
 
 /// A `Module` is a utility for collecting functions and data objects, and linking them together.
@@ -252,7 +328,6 @@ pub struct Module<B>
 where
     B: Backend,
 {
-    names: HashMap<String, FuncOrDataId>,
     declarations: ModuleDeclarations,
     backend: B,
 }
@@ -268,8 +343,8 @@ where
     /// Create a new `Module`.
     pub fn new(backend_builder: B::Builder) -> Self {
         Self {
-            names: HashMap::new(),
             declarations: ModuleDeclarations {
+                names: HashMap::new(),
                 functions: PrimaryMap::new(),
                 data_objects: PrimaryMap::new(),
             },
@@ -280,7 +355,7 @@ where
     /// Get the module identifier for a given name, if that name
     /// has been declared.
     pub fn get_name(&self, name: &str) -> Option<FuncOrDataId> {
-        self.names.get(name).cloned()
+        self.declarations.names.get(name).cloned()
     }
 
     /// Return the target information needed by frontends to produce Cranelift IR
@@ -330,32 +405,12 @@ where
         linkage: Linkage,
         signature: &ir::Signature,
     ) -> ModuleResult<FuncId> {
-        // TODO: Can we avoid allocating names so often?
+        let (id, decl) = self
-        use super::hash_map::Entry::*;
+            .declarations
-        match self.names.entry(name.to_owned()) {
+            .declare_function(name, linkage, signature)?;
-            Occupied(entry) => match *entry.get() {
+        self.backend.declare_function(id, name, decl.linkage);
-                FuncOrDataId::Func(id) => {
-                    let existing = &mut self.declarations.functions[id];
-                    existing.merge(linkage, signature)?;
-                    self.backend.declare_function(id, name, existing.linkage);
         Ok(id)
     }
-                FuncOrDataId::Data(..) => {
-                    Err(ModuleError::IncompatibleDeclaration(name.to_owned()))
-                }
-            },
-            Vacant(entry) => {
-                let id = self.declarations.functions.push(FunctionDeclaration {
-                    name: name.to_owned(),
-                    linkage,
-                    signature: signature.clone(),
-                });
-                entry.insert(FuncOrDataId::Func(id));
-                self.backend.declare_function(id, name, linkage);
-                Ok(id)
-            }
-        }
-    }
 
     /// An iterator over functions that have been declared in this module.
     pub fn declared_functions(&self) -> core::slice::Iter<'_, FunctionDeclaration> {
@@ -371,44 +426,20 @@ where
         tls: bool,
         align: Option<u8>, // An alignment bigger than 128 is unlikely
     ) -> ModuleResult<DataId> {
-        // TODO: Can we avoid allocating names so often?
+        let (id, decl) = self
-        use super::hash_map::Entry::*;
+            .declarations
-        match self.names.entry(name.to_owned()) {
+            .declare_data(name, linkage, writable, tls, align)?;
-            Occupied(entry) => match *entry.get() {
-                FuncOrDataId::Data(id) => {
-                    let existing = &mut self.declarations.data_objects[id];
-                    existing.merge(linkage, writable, tls, align);
         self.backend.declare_data(
             id,
             name,
-                        existing.linkage,
+            decl.linkage,
-                        existing.writable,
+            decl.writable,
-                        existing.tls,
+            decl.tls,
-                        existing.align,
+            decl.align,
         );
         Ok(id)
     }
 
-                FuncOrDataId::Func(..) => {
-                    Err(ModuleError::IncompatibleDeclaration(name.to_owned()))
-                }
-            },
-            Vacant(entry) => {
-                let id = self.declarations.data_objects.push(DataDeclaration {
-                    name: name.to_owned(),
-                    linkage,
-                    writable,
-                    tls,
-                    align,
-                });
-                entry.insert(FuncOrDataId::Data(id));
-                self.backend
-                    .declare_data(id, name, linkage, writable, tls, align);
-                Ok(id)
-            }
-        }
-    }
-
     /// Use this when you're building the IR of a function to reference a function.
     ///
     /// TODO: Coalesce redundant decls and signatures.
@@ -542,6 +573,6 @@ where
     /// implementations may provide additional functionality available after
     /// a `Module` is complete.
     pub fn finish(self) -> B::Product {
-        self.backend.finish(self.names, self.declarations)
+        self.backend.finish(self.declarations)
     }
 }

cranelift/object/src/backend.rs

@@ -8,8 +8,8 @@ use cranelift_codegen::entity::SecondaryMap;
 use cranelift_codegen::isa::TargetIsa;
 use cranelift_codegen::{self, ir};
 use cranelift_module::{
-    Backend, DataContext, DataDescription, DataId, FuncId, FuncOrDataId, Init, Linkage,
+    Backend, DataContext, DataDescription, DataId, FuncId, Init, Linkage, ModuleDeclarations,
-    ModuleDeclarations, ModuleError, ModuleResult,
+    ModuleError, ModuleResult,
 };
 use object::write::{
     Object, Relocation, SectionId, StandardSection, Symbol, SymbolId, SymbolSection,
@@ -410,11 +410,7 @@ impl Backend for ObjectBackend {
         Ok(())
     }
 
-    fn finish(
+    fn finish(mut self, declarations: ModuleDeclarations) -> ObjectProduct {
-        mut self,
-        _names: HashMap<String, FuncOrDataId>,
-        declarations: ModuleDeclarations,
-    ) -> ObjectProduct {
         let symbol_relocs = mem::take(&mut self.relocs);
         for symbol in symbol_relocs {
             for &RelocRecord {

cranelift/simplejit/src/backend.rs

@@ -172,7 +172,7 @@ struct SimpleJITMemoryHandle {
 /// defined in the original module.
 pub struct SimpleJITProduct {
     memory: SimpleJITMemoryHandle,
-    names: HashMap<String, FuncOrDataId>,
+    declarations: ModuleDeclarations,
     functions: SecondaryMap<FuncId, Option<SimpleJITCompiledFunction>>,
     data_objects: SecondaryMap<DataId, Option<SimpleJITCompiledData>>,
 }
@@ -194,7 +194,7 @@ impl SimpleJITProduct {
 
     /// Get the `FuncOrDataId` associated with the given name.
     pub fn func_or_data_for_func(&self, name: &str) -> Option<FuncOrDataId> {
-        self.names.get(name).copied()
+        self.declarations.get_name(name)
     }
 
     /// Return the address of a function.
@@ -583,11 +583,7 @@ impl<'simple_jit_backend> Backend for SimpleJITBackend {
     ///
     /// This method does not need to be called when access to the memory
     /// handle is not required.
-    fn finish(
+    fn finish(mut self, declarations: ModuleDeclarations) -> Self::Product {
-        mut self,
-        names: HashMap<String, FuncOrDataId>,
-        declarations: ModuleDeclarations,
-    ) -> Self::Product {
         for func in std::mem::take(&mut self.functions_to_finalize) {
             let decl = declarations.get_function_decl(func);
             debug_assert!(decl.linkage.is_definable());
@@ -605,7 +601,7 @@ impl<'simple_jit_backend> Backend for SimpleJITBackend {
 
         SimpleJITProduct {
             memory: self.memory,
-            names,
+            declarations,
             functions: self.functions,
             data_objects: self.data_objects,
         }