Remove the DataContext wrapper around DataDescription (#6170)

* Remove the DataContext wrapper around DataDescription

It doesn't have much of a purpose while making it harder to for example
rewrite the function and data object declarations within it as is
necessary for deserializing a serialized module.

* Derive Debug for DataDescription

cranelift/jit/src/backend.rs

@@ -8,7 +8,7 @@ use cranelift_codegen::{binemit::Reloc, CodegenError};
 use cranelift_control::ControlPlane;
 use cranelift_entity::SecondaryMap;
 use cranelift_module::{
-    DataContext, DataDescription, DataId, FuncId, Init, Linkage, Module, ModuleCompiledFunction,
+    DataDescription, DataId, FuncId, Init, Linkage, Module, ModuleCompiledFunction,
     ModuleDeclarations, ModuleError, ModuleExtName, ModuleReloc, ModuleResult,
 };
 use log::info;
@@ -837,7 +837,7 @@ impl Module for JITModule {
         Ok(ModuleCompiledFunction { size: total_size })
     }
 
-    fn define_data(&mut self, id: DataId, data: &DataContext) -> ModuleResult<()> {
+    fn define_data(&mut self, id: DataId, data: &DataDescription) -> ModuleResult<()> {
         let decl = self.declarations.get_data_decl(id);
         if !decl.linkage.is_definable() {
             return Err(ModuleError::InvalidImportDefinition(decl.name.clone()));
@@ -857,7 +857,7 @@ impl Module for JITModule {
             data_relocs: _,
             custom_segment_section: _,
             align,
-        } = data.description();
+        } = data;
 
         let size = init.size();
         let ptr = if decl.writable {
@@ -896,10 +896,7 @@ impl Module for JITModule {
             PointerWidth::U32 => Reloc::Abs4,
             PointerWidth::U64 => Reloc::Abs8,
         };
-        let relocs = data
+        let relocs = data.all_relocs(pointer_reloc).collect::<Vec<_>>();
-            .description()
-            .all_relocs(pointer_reloc)
-            .collect::<Vec<_>>();
 
         self.compiled_data_objects[id] = Some(CompiledBlob { ptr, size, relocs });
         self.data_objects_to_finalize.push(id);

cranelift/module/src/data_context.rs

@@ -40,7 +40,7 @@ impl Init {
 }
 
 /// A description of a data object.
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub struct DataDescription {
     /// How the data should be initialized.
     pub init: Init,
@@ -60,6 +60,85 @@ pub struct DataDescription {
 }
 
 impl DataDescription {
+    /// Allocate a new `DataDescription`.
+    pub fn new() -> Self {
+        Self {
+            init: Init::Uninitialized,
+            function_decls: PrimaryMap::new(),
+            data_decls: PrimaryMap::new(),
+            function_relocs: vec![],
+            data_relocs: vec![],
+            custom_segment_section: None,
+            align: None,
+        }
+    }
+
+    /// Clear all data structures in this `DataDescription`.
+    pub fn clear(&mut self) {
+        self.init = Init::Uninitialized;
+        self.function_decls.clear();
+        self.data_decls.clear();
+        self.function_relocs.clear();
+        self.data_relocs.clear();
+        self.custom_segment_section = None;
+        self.align = None;
+    }
+
+    /// Define a zero-initialized object with the given size.
+    pub fn define_zeroinit(&mut self, size: usize) {
+        debug_assert_eq!(self.init, Init::Uninitialized);
+        self.init = Init::Zeros { size };
+    }
+
+    /// Define an object initialized with the given contents.
+    ///
+    /// TODO: Can we avoid a Box here?
+    pub fn define(&mut self, contents: Box<[u8]>) {
+        debug_assert_eq!(self.init, Init::Uninitialized);
+        self.init = Init::Bytes { contents };
+    }
+
+    /// Override the segment/section for data, only supported on Object backend
+    pub fn set_segment_section(&mut self, seg: &str, sec: &str) {
+        self.custom_segment_section = Some((seg.to_owned(), sec.to_owned()))
+    }
+
+    /// Set the alignment for data. The alignment must be a power of two.
+    pub fn set_align(&mut self, align: u64) {
+        assert!(align.is_power_of_two());
+        self.align = Some(align);
+    }
+
+    /// Declare an external function import.
+    ///
+    /// Users of the `Module` API generally should call
+    /// `Module::declare_func_in_data` instead, as it takes care of generating
+    /// the appropriate `ExternalName`.
+    pub fn import_function(&mut self, name: ModuleExtName) -> ir::FuncRef {
+        self.function_decls.push(name)
+    }
+
+    /// Declares a global value import.
+    ///
+    /// TODO: Rename to import_data?
+    ///
+    /// Users of the `Module` API generally should call
+    /// `Module::declare_data_in_data` instead, as it takes care of generating
+    /// the appropriate `ExternalName`.
+    pub fn import_global_value(&mut self, name: ModuleExtName) -> ir::GlobalValue {
+        self.data_decls.push(name)
+    }
+
+    /// Write the address of `func` into the data at offset `offset`.
+    pub fn write_function_addr(&mut self, offset: CodeOffset, func: ir::FuncRef) {
+        self.function_relocs.push((offset, func))
+    }
+
+    /// Write the address of `data` into the data at offset `offset`.
+    pub fn write_data_addr(&mut self, offset: CodeOffset, data: ir::GlobalValue, addend: Addend) {
+        self.data_relocs.push((offset, data, addend))
+    }
+
     /// An iterator over all relocations of the data object.
     pub fn all_relocs<'a>(
         &'a self,
@@ -87,167 +166,60 @@ impl DataDescription {
     }
 }
 
-/// This is to data objects what cranelift_codegen::Context is to functions.
-pub struct DataContext {
-    description: DataDescription,
-}
-
-impl DataContext {
-    /// Allocate a new context.
-    pub fn new() -> Self {
-        Self {
-            description: DataDescription {
-                init: Init::Uninitialized,
-                function_decls: PrimaryMap::new(),
-                data_decls: PrimaryMap::new(),
-                function_relocs: vec![],
-                data_relocs: vec![],
-                custom_segment_section: None,
-                align: None,
-            },
-        }
-    }
-
-    /// Clear all data structures in this context.
-    pub fn clear(&mut self) {
-        self.description.init = Init::Uninitialized;
-        self.description.function_decls.clear();
-        self.description.data_decls.clear();
-        self.description.function_relocs.clear();
-        self.description.data_relocs.clear();
-        self.description.custom_segment_section = None;
-        self.description.align = None;
-    }
-
-    /// Define a zero-initialized object with the given size.
-    pub fn define_zeroinit(&mut self, size: usize) {
-        debug_assert_eq!(self.description.init, Init::Uninitialized);
-        self.description.init = Init::Zeros { size };
-    }
-
-    /// Define an object initialized with the given contents.
-    ///
-    /// TODO: Can we avoid a Box here?
-    pub fn define(&mut self, contents: Box<[u8]>) {
-        debug_assert_eq!(self.description.init, Init::Uninitialized);
-        self.description.init = Init::Bytes { contents };
-    }
-
-    /// Override the segment/section for data, only supported on Object backend
-    pub fn set_segment_section(&mut self, seg: &str, sec: &str) {
-        self.description.custom_segment_section = Some((seg.to_owned(), sec.to_owned()))
-    }
-
-    /// Set the alignment for data. The alignment must be a power of two.
-    pub fn set_align(&mut self, align: u64) {
-        assert!(align.is_power_of_two());
-        self.description.align = Some(align);
-    }
-
-    /// Declare an external function import.
-    ///
-    /// Users of the `Module` API generally should call
-    /// `Module::declare_func_in_data` instead, as it takes care of generating
-    /// the appropriate `ExternalName`.
-    pub fn import_function(&mut self, name: ModuleExtName) -> ir::FuncRef {
-        self.description.function_decls.push(name)
-    }
-
-    /// Declares a global value import.
-    ///
-    /// TODO: Rename to import_data?
-    ///
-    /// Users of the `Module` API generally should call
-    /// `Module::declare_data_in_data` instead, as it takes care of generating
-    /// the appropriate `ExternalName`.
-    pub fn import_global_value(&mut self, name: ModuleExtName) -> ir::GlobalValue {
-        self.description.data_decls.push(name)
-    }
-
-    /// Write the address of `func` into the data at offset `offset`.
-    pub fn write_function_addr(&mut self, offset: CodeOffset, func: ir::FuncRef) {
-        self.description.function_relocs.push((offset, func))
-    }
-
-    /// Write the address of `data` into the data at offset `offset`.
-    pub fn write_data_addr(&mut self, offset: CodeOffset, data: ir::GlobalValue, addend: Addend) {
-        self.description.data_relocs.push((offset, data, addend))
-    }
-
-    /// Reference the initializer data.
-    pub fn description(&self) -> &DataDescription {
-        debug_assert!(
-            self.description.init != Init::Uninitialized,
-            "data must be initialized first"
-        );
-        &self.description
-    }
-}
-
 #[cfg(test)]
 mod tests {
     use crate::ModuleExtName;
 
-    use super::{DataContext, Init};
+    use super::{DataDescription, Init};
 
     #[test]
     fn basic_data_context() {
-        let mut data_ctx = DataContext::new();
+        let mut data = DataDescription::new();
-        {
+        assert_eq!(data.init, Init::Uninitialized);
-            let description = &data_ctx.description;
+        assert!(data.function_decls.is_empty());
-            assert_eq!(description.init, Init::Uninitialized);
+        assert!(data.data_decls.is_empty());
-            assert!(description.function_decls.is_empty());
+        assert!(data.function_relocs.is_empty());
-            assert!(description.data_decls.is_empty());
+        assert!(data.data_relocs.is_empty());
-            assert!(description.function_relocs.is_empty());
-            assert!(description.data_relocs.is_empty());
-        }
 
-        data_ctx.define_zeroinit(256);
+        data.define_zeroinit(256);
 
-        let _func_a = data_ctx.import_function(ModuleExtName::user(0, 0));
+        let _func_a = data.import_function(ModuleExtName::user(0, 0));
-        let func_b = data_ctx.import_function(ModuleExtName::user(0, 1));
+        let func_b = data.import_function(ModuleExtName::user(0, 1));
-        let func_c = data_ctx.import_function(ModuleExtName::user(0, 2));
+        let func_c = data.import_function(ModuleExtName::user(0, 2));
-        let _data_a = data_ctx.import_global_value(ModuleExtName::user(0, 3));
+        let _data_a = data.import_global_value(ModuleExtName::user(0, 3));
-        let data_b = data_ctx.import_global_value(ModuleExtName::user(0, 4));
+        let data_b = data.import_global_value(ModuleExtName::user(0, 4));
 
-        data_ctx.write_function_addr(8, func_b);
+        data.write_function_addr(8, func_b);
-        data_ctx.write_function_addr(16, func_c);
+        data.write_function_addr(16, func_c);
-        data_ctx.write_data_addr(32, data_b, 27);
+        data.write_data_addr(32, data_b, 27);
 
-        {
+        assert_eq!(data.init, Init::Zeros { size: 256 });
-            let description = data_ctx.description();
+        assert_eq!(data.function_decls.len(), 3);
-            assert_eq!(description.init, Init::Zeros { size: 256 });
+        assert_eq!(data.data_decls.len(), 2);
-            assert_eq!(description.function_decls.len(), 3);
+        assert_eq!(data.function_relocs.len(), 2);
-            assert_eq!(description.data_decls.len(), 2);
+        assert_eq!(data.data_relocs.len(), 1);
-            assert_eq!(description.function_relocs.len(), 2);
-            assert_eq!(description.data_relocs.len(), 1);
-        }
 
-        data_ctx.clear();
+        data.clear();
-        {
+
-            let description = &data_ctx.description;
+        assert_eq!(data.init, Init::Uninitialized);
-            assert_eq!(description.init, Init::Uninitialized);
+        assert!(data.function_decls.is_empty());
-            assert!(description.function_decls.is_empty());
+        assert!(data.data_decls.is_empty());
-            assert!(description.data_decls.is_empty());
+        assert!(data.function_relocs.is_empty());
-            assert!(description.function_relocs.is_empty());
+        assert!(data.data_relocs.is_empty());
-            assert!(description.data_relocs.is_empty());
-        }
 
         let contents = vec![33, 34, 35, 36];
         let contents_clone = contents.clone();
-        data_ctx.define(contents.into_boxed_slice());
+        data.define(contents.into_boxed_slice());
-        {
+
-            let description = data_ctx.description();
         assert_eq!(
-                description.init,
+            data.init,
             Init::Bytes {
                 contents: contents_clone.into_boxed_slice()
             }
         );
-            assert_eq!(description.function_decls.len(), 0);
+        assert_eq!(data.function_decls.len(), 0);
-            assert_eq!(description.data_decls.len(), 0);
+        assert_eq!(data.data_decls.len(), 0);
-            assert_eq!(description.function_relocs.len(), 0);
+        assert_eq!(data.function_relocs.len(), 0);
-            assert_eq!(description.data_relocs.len(), 0);
+        assert_eq!(data.data_relocs.len(), 0);
-        }
     }
 }

cranelift/module/src/lib.rs

@@ -40,7 +40,7 @@ mod data_context;
 mod module;
 mod traps;
 
-pub use crate::data_context::{DataContext, DataDescription, Init};
+pub use crate::data_context::{DataDescription, Init};
 pub use crate::module::{
     DataDeclaration, DataId, FuncId, FuncOrDataId, FunctionDeclaration, Linkage, Module,
     ModuleCompiledFunction, ModuleDeclarations, ModuleError, ModuleExtName, ModuleReloc,

cranelift/module/src/module.rs

@@ -3,10 +3,10 @@
 // TODO: Should `ir::Function` really have a `name`?
 
 // TODO: Factor out `ir::Function`'s `ext_funcs` and `global_values` into a struct
-// shared with `DataContext`?
+// shared with `DataDescription`?
 
 use super::HashMap;
-use crate::data_context::DataContext;
+use crate::data_context::DataDescription;
 use core::fmt::Display;
 use cranelift_codegen::binemit::{CodeOffset, Reloc};
 use cranelift_codegen::entity::{entity_impl, PrimaryMap};
@@ -348,7 +348,7 @@ impl DataDeclaration {
 }
 
 /// A translated `ExternalName` into something global we can handle.
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub enum ModuleExtName {
     /// User defined function, converted from `ExternalName::User`.
     User {
@@ -641,13 +641,13 @@ pub trait Module {
     }
 
     /// TODO: Same as above.
-    fn declare_func_in_data(&self, func: FuncId, ctx: &mut DataContext) -> ir::FuncRef {
+    fn declare_func_in_data(&self, func_id: FuncId, data: &mut DataDescription) -> ir::FuncRef {
-        ctx.import_function(ModuleExtName::user(0, func.as_u32()))
+        data.import_function(ModuleExtName::user(0, func_id.as_u32()))
     }
 
     /// TODO: Same as above.
-    fn declare_data_in_data(&self, data: DataId, ctx: &mut DataContext) -> ir::GlobalValue {
+    fn declare_data_in_data(&self, data_id: DataId, data: &mut DataDescription) -> ir::GlobalValue {
-        ctx.import_global_value(ModuleExtName::user(1, data.as_u32()))
+        data.import_global_value(ModuleExtName::user(1, data_id.as_u32()))
     }
 
     /// Define a function, producing the function body from the given `Context`.
@@ -697,7 +697,7 @@ pub trait Module {
     ) -> ModuleResult<ModuleCompiledFunction>;
 
     /// Define a data object, producing the data contents from the given `DataContext`.
-    fn define_data(&mut self, data: DataId, data_ctx: &DataContext) -> ModuleResult<()>;
+    fn define_data(&mut self, data_id: DataId, data: &DataDescription) -> ModuleResult<()>;
 }
 
 impl<M: Module> Module for &mut M {
@@ -768,12 +768,12 @@ impl<M: Module> Module for &mut M {
         (**self).declare_data_in_func(data, func)
     }
 
-    fn declare_func_in_data(&self, func: FuncId, ctx: &mut DataContext) -> ir::FuncRef {
+    fn declare_func_in_data(&self, func_id: FuncId, data: &mut DataDescription) -> ir::FuncRef {
-        (**self).declare_func_in_data(func, ctx)
+        (**self).declare_func_in_data(func_id, data)
     }
 
-    fn declare_data_in_data(&self, data: DataId, ctx: &mut DataContext) -> ir::GlobalValue {
+    fn declare_data_in_data(&self, data_id: DataId, data: &mut DataDescription) -> ir::GlobalValue {
-        (**self).declare_data_in_data(data, ctx)
+        (**self).declare_data_in_data(data_id, data)
     }
 
     fn define_function(
@@ -804,7 +804,7 @@ impl<M: Module> Module for &mut M {
         (**self).define_function_bytes(func_id, func, alignment, bytes, relocs)
     }
 
-    fn define_data(&mut self, data: DataId, data_ctx: &DataContext) -> ModuleResult<()> {
+    fn define_data(&mut self, data_id: DataId, data: &DataDescription) -> ModuleResult<()> {
-        (**self).define_data(data, data_ctx)
+        (**self).define_data(data_id, data)
     }
 }

cranelift/object/src/backend.rs

@@ -10,7 +10,7 @@ use cranelift_codegen::{
 };
 use cranelift_control::ControlPlane;
 use cranelift_module::{
-    DataContext, DataDescription, DataId, FuncId, Init, Linkage, Module, ModuleCompiledFunction,
+    DataDescription, DataId, FuncId, Init, Linkage, Module, ModuleCompiledFunction,
     ModuleDeclarations, ModuleError, ModuleExtName, ModuleReloc, ModuleResult,
 };
 use log::info;
@@ -394,7 +394,7 @@ impl Module for ObjectModule {
         Ok(ModuleCompiledFunction { size: total_size })
     }
 
-    fn define_data(&mut self, data_id: DataId, data_ctx: &DataContext) -> ModuleResult<()> {
+    fn define_data(&mut self, data_id: DataId, data: &DataDescription) -> ModuleResult<()> {
         let decl = self.declarations.get_data_decl(data_id);
         if !decl.linkage.is_definable() {
             return Err(ModuleError::InvalidImportDefinition(decl.name.clone()));
@@ -414,15 +414,14 @@ impl Module for ObjectModule {
             data_relocs: _,
             ref custom_segment_section,
             align,
-        } = data_ctx.description();
+        } = data;
 
         let pointer_reloc = match self.isa.triple().pointer_width().unwrap() {
             PointerWidth::U16 => unimplemented!("16bit pointers"),
             PointerWidth::U32 => Reloc::Abs4,
             PointerWidth::U64 => Reloc::Abs8,
         };
-        let relocs = data_ctx
+        let relocs = data
-            .description()
             .all_relocs(pointer_reloc)
             .map(|record| self.process_reloc(&record))
             .collect::<Vec<_>>();