From 67c85b883ed63c07217a40016f171eca9284497a Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Thu, 6 Apr 2023 19:13:55 +0200 Subject: [PATCH] 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 | 11 +- cranelift/module/src/data_context.rs | 268 ++++++++++++--------------- cranelift/module/src/lib.rs | 2 +- cranelift/module/src/module.rs | 28 +-- cranelift/object/src/backend.rs | 9 +- 5 files changed, 143 insertions(+), 175 deletions(-) diff --git a/cranelift/jit/src/backend.rs b/cranelift/jit/src/backend.rs index 04e4d4c722..79e5db86af 100644 --- a/cranelift/jit/src/backend.rs +++ b/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 - .description() - .all_relocs(pointer_reloc) - .collect::>(); + let relocs = data.all_relocs(pointer_reloc).collect::>(); self.compiled_data_objects[id] = Some(CompiledBlob { ptr, size, relocs }); self.data_objects_to_finalize.push(id); diff --git a/cranelift/module/src/data_context.rs b/cranelift/module/src/data_context.rs index 3ec4079968..3188ad90c4 100644 --- a/cranelift/module/src/data_context.rs +++ b/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 description = &data_ctx.description; - assert_eq!(description.init, Init::Uninitialized); - assert!(description.function_decls.is_empty()); - assert!(description.data_decls.is_empty()); - assert!(description.function_relocs.is_empty()); - assert!(description.data_relocs.is_empty()); - } + let mut data = DataDescription::new(); + assert_eq!(data.init, Init::Uninitialized); + assert!(data.function_decls.is_empty()); + assert!(data.data_decls.is_empty()); + assert!(data.function_relocs.is_empty()); + assert!(data.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_b = data_ctx.import_function(ModuleExtName::user(0, 1)); - let func_c = data_ctx.import_function(ModuleExtName::user(0, 2)); - let _data_a = data_ctx.import_global_value(ModuleExtName::user(0, 3)); - let data_b = data_ctx.import_global_value(ModuleExtName::user(0, 4)); + let _func_a = data.import_function(ModuleExtName::user(0, 0)); + let func_b = data.import_function(ModuleExtName::user(0, 1)); + let func_c = data.import_function(ModuleExtName::user(0, 2)); + let _data_a = data.import_global_value(ModuleExtName::user(0, 3)); + let data_b = data.import_global_value(ModuleExtName::user(0, 4)); - data_ctx.write_function_addr(8, func_b); - data_ctx.write_function_addr(16, func_c); - data_ctx.write_data_addr(32, data_b, 27); + data.write_function_addr(8, func_b); + data.write_function_addr(16, func_c); + data.write_data_addr(32, data_b, 27); - { - let description = data_ctx.description(); - assert_eq!(description.init, Init::Zeros { size: 256 }); - assert_eq!(description.function_decls.len(), 3); - assert_eq!(description.data_decls.len(), 2); - assert_eq!(description.function_relocs.len(), 2); - assert_eq!(description.data_relocs.len(), 1); - } + assert_eq!(data.init, Init::Zeros { size: 256 }); + assert_eq!(data.function_decls.len(), 3); + assert_eq!(data.data_decls.len(), 2); + assert_eq!(data.function_relocs.len(), 2); + assert_eq!(data.data_relocs.len(), 1); - data_ctx.clear(); - { - let description = &data_ctx.description; - assert_eq!(description.init, Init::Uninitialized); - assert!(description.function_decls.is_empty()); - assert!(description.data_decls.is_empty()); - assert!(description.function_relocs.is_empty()); - assert!(description.data_relocs.is_empty()); - } + data.clear(); + + assert_eq!(data.init, Init::Uninitialized); + assert!(data.function_decls.is_empty()); + assert!(data.data_decls.is_empty()); + assert!(data.function_relocs.is_empty()); + assert!(data.data_relocs.is_empty()); let contents = vec![33, 34, 35, 36]; let contents_clone = contents.clone(); - data_ctx.define(contents.into_boxed_slice()); - { - let description = data_ctx.description(); - assert_eq!( - description.init, - Init::Bytes { - contents: contents_clone.into_boxed_slice() - } - ); - assert_eq!(description.function_decls.len(), 0); - assert_eq!(description.data_decls.len(), 0); - assert_eq!(description.function_relocs.len(), 0); - assert_eq!(description.data_relocs.len(), 0); - } + data.define(contents.into_boxed_slice()); + + assert_eq!( + data.init, + Init::Bytes { + contents: contents_clone.into_boxed_slice() + } + ); + assert_eq!(data.function_decls.len(), 0); + assert_eq!(data.data_decls.len(), 0); + assert_eq!(data.function_relocs.len(), 0); + assert_eq!(data.data_relocs.len(), 0); } } diff --git a/cranelift/module/src/lib.rs b/cranelift/module/src/lib.rs index 405a9b543a..bf561083be 100644 --- a/cranelift/module/src/lib.rs +++ b/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, diff --git a/cranelift/module/src/module.rs b/cranelift/module/src/module.rs index 540ee67ff9..5eddbe07cb 100644 --- a/cranelift/module/src/module.rs +++ b/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 { - ctx.import_function(ModuleExtName::user(0, func.as_u32())) + fn declare_func_in_data(&self, func_id: FuncId, data: &mut DataDescription) -> ir::FuncRef { + 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 { - ctx.import_global_value(ModuleExtName::user(1, data.as_u32())) + fn declare_data_in_data(&self, data_id: DataId, data: &mut DataDescription) -> ir::GlobalValue { + 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; /// 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 Module for &mut M { @@ -768,12 +768,12 @@ impl Module for &mut M { (**self).declare_data_in_func(data, func) } - fn declare_func_in_data(&self, func: FuncId, ctx: &mut DataContext) -> ir::FuncRef { - (**self).declare_func_in_data(func, ctx) + fn declare_func_in_data(&self, func_id: FuncId, data: &mut DataDescription) -> ir::FuncRef { + (**self).declare_func_in_data(func_id, data) } - fn declare_data_in_data(&self, data: DataId, ctx: &mut DataContext) -> ir::GlobalValue { - (**self).declare_data_in_data(data, ctx) + fn declare_data_in_data(&self, data_id: DataId, data: &mut DataDescription) -> ir::GlobalValue { + (**self).declare_data_in_data(data_id, data) } fn define_function( @@ -804,7 +804,7 @@ impl 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<()> { - (**self).define_data(data, data_ctx) + fn define_data(&mut self, data_id: DataId, data: &DataDescription) -> ModuleResult<()> { + (**self).define_data(data_id, data) } } diff --git a/cranelift/object/src/backend.rs b/cranelift/object/src/backend.rs index d717883142..5a2421329a 100644 --- a/cranelift/object/src/backend.rs +++ b/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 - .description() + let relocs = data .all_relocs(pointer_reloc) .map(|record| self.process_reloc(&record)) .collect::>();