From 88baac4ca6f52d96ebc448f1c7100fbd656d326a Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Tue, 11 Jan 2022 14:40:53 +0100 Subject: [PATCH 01/11] Move the TestCodeSink functionality to MachBufferFinalized --- .../src/isa/aarch64/inst/emit_tests.rs | 5 +- .../codegen/src/isa/arm32/inst/emit_tests.rs | 5 +- cranelift/codegen/src/isa/mod.rs | 3 - .../codegen/src/isa/s390x/inst/emit_tests.rs | 5 +- cranelift/codegen/src/isa/test_utils.rs | 74 ------------------- .../codegen/src/isa/x64/inst/emit_tests.rs | 5 +- cranelift/codegen/src/machinst/buffer.rs | 11 +++ 7 files changed, 15 insertions(+), 93 deletions(-) delete mode 100644 cranelift/codegen/src/isa/test_utils.rs diff --git a/cranelift/codegen/src/isa/aarch64/inst/emit_tests.rs b/cranelift/codegen/src/isa/aarch64/inst/emit_tests.rs index 915657c2f5..3d7016af2d 100644 --- a/cranelift/codegen/src/isa/aarch64/inst/emit_tests.rs +++ b/cranelift/codegen/src/isa/aarch64/inst/emit_tests.rs @@ -1,7 +1,6 @@ use crate::ir::types::*; use crate::ir::TrapCode; use crate::isa::aarch64::inst::*; -use crate::isa::test_utils; use crate::isa::CallConv; use crate::settings; @@ -6523,12 +6522,10 @@ fn test_aarch64_binemit() { let actual_printing = insn.show_rru(Some(&rru)); assert_eq!(expected_printing, actual_printing); - let mut sink = test_utils::TestCodeSink::new(); let mut buffer = MachBuffer::new(); insn.emit(&mut buffer, &emit_info, &mut Default::default()); let buffer = buffer.finish(); - buffer.emit(&mut sink); - let actual_encoding = &sink.stringify(); + let actual_encoding = &buffer.stringify_code_bytes(); assert_eq!(expected_encoding, actual_encoding); } } diff --git a/cranelift/codegen/src/isa/arm32/inst/emit_tests.rs b/cranelift/codegen/src/isa/arm32/inst/emit_tests.rs index 73269be999..421cc2e548 100644 --- a/cranelift/codegen/src/isa/arm32/inst/emit_tests.rs +++ b/cranelift/codegen/src/isa/arm32/inst/emit_tests.rs @@ -1,5 +1,4 @@ use crate::isa::arm32::inst::*; -use crate::isa::test_utils; use crate::settings; use alloc::vec::Vec; @@ -1948,12 +1947,10 @@ fn test_arm32_emit() { // Check the printed text is as expected. let actual_printing = insn.show_rru(Some(&rru)); assert_eq!(expected_printing, actual_printing); - let mut sink = test_utils::TestCodeSink::new(); let mut buffer = MachBuffer::new(); insn.emit(&mut buffer, &flags, &mut Default::default()); let buffer = buffer.finish(); - buffer.emit(&mut sink); - let actual_encoding = &sink.stringify(); + let actual_encoding = &buffer.stringify_code_bytes(); assert_eq!(expected_encoding, actual_encoding, "{}", expected_printing); } } diff --git a/cranelift/codegen/src/isa/mod.rs b/cranelift/codegen/src/isa/mod.rs index e92dc21627..f50cb83f9d 100644 --- a/cranelift/codegen/src/isa/mod.rs +++ b/cranelift/codegen/src/isa/mod.rs @@ -76,9 +76,6 @@ pub mod unwind; mod call_conv; -#[cfg(test)] -mod test_utils; - /// Returns a builder that can create a corresponding `TargetIsa` /// or `Err(LookupError::SupportDisabled)` if not enabled. macro_rules! isa_builder { diff --git a/cranelift/codegen/src/isa/s390x/inst/emit_tests.rs b/cranelift/codegen/src/isa/s390x/inst/emit_tests.rs index 15f32bed74..9d60355fd1 100644 --- a/cranelift/codegen/src/isa/s390x/inst/emit_tests.rs +++ b/cranelift/codegen/src/isa/s390x/inst/emit_tests.rs @@ -1,7 +1,6 @@ use crate::ir::MemFlags; use crate::isa::s390x::inst::*; use crate::isa::s390x::settings as s390x_settings; -use crate::isa::test_utils; use crate::settings; use alloc::vec::Vec; @@ -8112,12 +8111,10 @@ fn test_s390x_binemit() { let actual_printing = insn.show_rru(Some(&rru)); assert_eq!(expected_printing, actual_printing); - let mut sink = test_utils::TestCodeSink::new(); let mut buffer = MachBuffer::new(); insn.emit(&mut buffer, &emit_info, &mut Default::default()); let buffer = buffer.finish(); - buffer.emit(&mut sink); - let actual_encoding = &sink.stringify(); + let actual_encoding = &buffer.stringify_code_bytes(); assert_eq!(expected_encoding, actual_encoding); } } diff --git a/cranelift/codegen/src/isa/test_utils.rs b/cranelift/codegen/src/isa/test_utils.rs deleted file mode 100644 index ed5eed09b4..0000000000 --- a/cranelift/codegen/src/isa/test_utils.rs +++ /dev/null @@ -1,74 +0,0 @@ -// This is unused when no platforms with the new backend are enabled. -#![allow(dead_code)] - -use crate::binemit::{Addend, CodeOffset, CodeSink, Reloc}; -use crate::ir::{ExternalName, Opcode, SourceLoc, TrapCode}; - -use alloc::vec::Vec; -use std::string::String; - -pub struct TestCodeSink { - bytes: Vec, -} - -impl TestCodeSink { - /// Create a new TestCodeSink. - pub fn new() -> TestCodeSink { - TestCodeSink { bytes: vec![] } - } - - /// Return the code emitted to this sink as a hex string. - pub fn stringify(&self) -> String { - // This is pretty lame, but whatever .. - use std::fmt::Write; - let mut s = String::with_capacity(self.bytes.len() * 2); - for b in &self.bytes { - write!(&mut s, "{:02X}", b).unwrap(); - } - s - } -} - -impl CodeSink for TestCodeSink { - fn offset(&self) -> CodeOffset { - self.bytes.len() as CodeOffset - } - - fn put1(&mut self, x: u8) { - self.bytes.push(x); - } - - fn put2(&mut self, x: u16) { - self.bytes.push((x >> 0) as u8); - self.bytes.push((x >> 8) as u8); - } - - fn put4(&mut self, mut x: u32) { - for _ in 0..4 { - self.bytes.push(x as u8); - x >>= 8; - } - } - - fn put8(&mut self, mut x: u64) { - for _ in 0..8 { - self.bytes.push(x as u8); - x >>= 8; - } - } - - fn reloc_external( - &mut self, - _srcloc: SourceLoc, - _rel: Reloc, - _name: &ExternalName, - _addend: Addend, - ) { - } - - fn trap(&mut self, _code: TrapCode, _srcloc: SourceLoc) {} - - fn end_codegen(&mut self) {} - - fn add_call_site(&mut self, _opcode: Opcode, _srcloc: SourceLoc) {} -} diff --git a/cranelift/codegen/src/isa/x64/inst/emit_tests.rs b/cranelift/codegen/src/isa/x64/inst/emit_tests.rs index acb335b80b..e8f905bcc5 100644 --- a/cranelift/codegen/src/isa/x64/inst/emit_tests.rs +++ b/cranelift/codegen/src/isa/x64/inst/emit_tests.rs @@ -13,7 +13,6 @@ //! -- isa::x64::inst::emit_tests::test_x64_emit use super::*; -use crate::isa::test_utils; use crate::isa::x64; use alloc::vec::Vec; @@ -4460,7 +4459,6 @@ fn test_x64_emit() { // Check the printed text is as expected. let actual_printing = insn.show_rru(Some(&rru)); assert_eq!(expected_printing, actual_printing); - let mut sink = test_utils::TestCodeSink::new(); let mut buffer = MachBuffer::new(); insn.emit(&mut buffer, &emit_info, &mut Default::default()); @@ -4470,8 +4468,7 @@ fn test_x64_emit() { buffer.bind_label(label); let buffer = buffer.finish(); - buffer.emit(&mut sink); - let actual_encoding = &sink.stringify(); + let actual_encoding = &buffer.stringify_code_bytes(); assert_eq!(expected_encoding, actual_encoding, "{}", expected_printing); } } diff --git a/cranelift/codegen/src/machinst/buffer.rs b/cranelift/codegen/src/machinst/buffer.rs index 298ee90cd1..87c2566e9e 100644 --- a/cranelift/codegen/src/machinst/buffer.rs +++ b/cranelift/codegen/src/machinst/buffer.rs @@ -1420,6 +1420,17 @@ impl MachBufferFinalized { self.data.len() as CodeOffset } + /// Return the code in this mach buffer as a hex string for testing purposes. + pub fn stringify_code_bytes(&self) -> String { + // This is pretty lame, but whatever .. + use std::fmt::Write; + let mut s = String::with_capacity(self.data.len() * 2); + for b in &self.data { + write!(&mut s, "{:02X}", b).unwrap(); + } + s + } + /// Emit this buffer to the given CodeSink. pub fn emit(&self, sink: &mut CS) { // N.B.: we emit every section into the .text section as far as From 354c4f7bf855aa4851debd12744d6bbc63fe7b33 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Tue, 11 Jan 2022 14:45:21 +0100 Subject: [PATCH 02/11] Remove unused CodeSink methods --- cranelift/codegen/src/binemit/memorysink.rs | 24 ++++----------------- cranelift/codegen/src/binemit/mod.rs | 12 ----------- cranelift/codegen/src/machinst/buffer.rs | 12 ----------- cranelift/filetests/src/test_compile.rs | 16 -------------- 4 files changed, 4 insertions(+), 60 deletions(-) diff --git a/cranelift/codegen/src/binemit/memorysink.rs b/cranelift/codegen/src/binemit/memorysink.rs index 6635bff876..d40d5e7281 100644 --- a/cranelift/codegen/src/binemit/memorysink.rs +++ b/cranelift/codegen/src/binemit/memorysink.rs @@ -99,26 +99,10 @@ impl<'a> MemoryCodeSink<'a> { } impl<'a> CodeSink for MemoryCodeSink<'a> { - fn offset(&self) -> CodeOffset { - self.offset as CodeOffset - } - fn put1(&mut self, x: u8) { self.write(x); } - fn put2(&mut self, x: u16) { - self.write(x); - } - - fn put4(&mut self, x: u32) { - self.write(x); - } - - fn put8(&mut self, x: u64) { - self.write(x); - } - fn reloc_external( &mut self, srcloc: SourceLoc, @@ -126,17 +110,17 @@ impl<'a> CodeSink for MemoryCodeSink<'a> { name: &ExternalName, addend: Addend, ) { - let ofs = self.offset(); + let ofs = self.offset as CodeOffset; self.relocs.reloc_external(ofs, srcloc, rel, name, addend); } fn trap(&mut self, code: TrapCode, srcloc: SourceLoc) { - let ofs = self.offset(); + let ofs = self.offset as CodeOffset; self.traps.trap(ofs, srcloc, code); } fn end_codegen(&mut self) { - self.info.total_size = self.offset(); + self.info.total_size = self.offset as CodeOffset; } fn add_call_site(&mut self, opcode: Opcode, loc: SourceLoc) { @@ -144,7 +128,7 @@ impl<'a> CodeSink for MemoryCodeSink<'a> { opcode.is_call(), "adding call site info for a non-call instruction." ); - let ret_addr = self.offset(); + let ret_addr = self.offset as CodeOffset; self.relocs.add_call_site(opcode, ret_addr, loc); } } diff --git a/cranelift/codegen/src/binemit/mod.rs b/cranelift/codegen/src/binemit/mod.rs index 157a123d7e..ff124ec6fa 100644 --- a/cranelift/codegen/src/binemit/mod.rs +++ b/cranelift/codegen/src/binemit/mod.rs @@ -105,21 +105,9 @@ pub struct CodeInfo { /// A `CodeSink` will receive all of the machine code for a function. It also accepts relocations /// which are locations in the code section that need to be fixed up when linking. pub trait CodeSink { - /// Get the current position. - fn offset(&self) -> CodeOffset; - /// Add 1 byte to the code section. fn put1(&mut self, _: u8); - /// Add 2 bytes to the code section. - fn put2(&mut self, _: u16); - - /// Add 4 bytes to the code section. - fn put4(&mut self, _: u32); - - /// Add 8 bytes to the code section. - fn put8(&mut self, _: u64); - /// Add a relocation referencing an external symbol plus the addend at the current offset. fn reloc_external(&mut self, _: SourceLoc, _: Reloc, _: &ExternalName, _: Addend); diff --git a/cranelift/codegen/src/machinst/buffer.rs b/cranelift/codegen/src/machinst/buffer.rs index 87c2566e9e..2f46f7110e 100644 --- a/cranelift/codegen/src/machinst/buffer.rs +++ b/cranelift/codegen/src/machinst/buffer.rs @@ -2079,21 +2079,9 @@ mod test { relocs: Vec<(CodeOffset, Reloc)>, } impl CodeSink for TestCodeSink { - fn offset(&self) -> CodeOffset { - self.offset - } fn put1(&mut self, _: u8) { self.offset += 1; } - fn put2(&mut self, _: u16) { - self.offset += 2; - } - fn put4(&mut self, _: u32) { - self.offset += 4; - } - fn put8(&mut self, _: u64) { - self.offset += 8; - } fn reloc_external(&mut self, _: SourceLoc, r: Reloc, _: &ExternalName, _: Addend) { self.relocs.push((self.offset, r)); } diff --git a/cranelift/filetests/src/test_compile.rs b/cranelift/filetests/src/test_compile.rs index 4072735ad3..ec8bbbb120 100644 --- a/cranelift/filetests/src/test_compile.rs +++ b/cranelift/filetests/src/test_compile.rs @@ -81,26 +81,10 @@ struct SizeSink { } impl binemit::CodeSink for SizeSink { - fn offset(&self) -> binemit::CodeOffset { - self.offset - } - fn put1(&mut self, _: u8) { self.offset += 1; } - fn put2(&mut self, _: u16) { - self.offset += 2; - } - - fn put4(&mut self, _: u32) { - self.offset += 4; - } - - fn put8(&mut self, _: u64) { - self.offset += 8; - } - fn reloc_external( &mut self, _srcloc: ir::SourceLoc, From 37598ad17058011b16db8e61e46009adabeb7e4f Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Tue, 11 Jan 2022 14:51:27 +0100 Subject: [PATCH 03/11] Remove end_codegen method from CodeSink --- cranelift/codegen/src/binemit/memorysink.rs | 14 +++++++------- cranelift/codegen/src/binemit/mod.rs | 3 --- cranelift/codegen/src/context.rs | 2 +- cranelift/codegen/src/machinst/buffer.rs | 3 --- cranelift/filetests/src/test_compile.rs | 1 - 5 files changed, 8 insertions(+), 15 deletions(-) diff --git a/cranelift/codegen/src/binemit/memorysink.rs b/cranelift/codegen/src/binemit/memorysink.rs index d40d5e7281..96a1ac4619 100644 --- a/cranelift/codegen/src/binemit/memorysink.rs +++ b/cranelift/codegen/src/binemit/memorysink.rs @@ -36,8 +36,6 @@ pub struct MemoryCodeSink<'a> { offset: isize, relocs: &'a mut dyn RelocSink, traps: &'a mut dyn TrapSink, - /// Information about the generated code and read-only data. - pub info: CodeInfo, } impl<'a> MemoryCodeSink<'a> { @@ -55,11 +53,17 @@ impl<'a> MemoryCodeSink<'a> { Self { data, offset: 0, - info: CodeInfo { total_size: 0 }, relocs, traps, } } + + /// Information about the generated code and read-only data. + pub fn info(&self) -> CodeInfo { + CodeInfo { + total_size: self.offset as CodeOffset, + } + } } /// A trait for receiving relocations for code that is emitted directly into memory. @@ -119,10 +123,6 @@ impl<'a> CodeSink for MemoryCodeSink<'a> { self.traps.trap(ofs, srcloc, code); } - fn end_codegen(&mut self) { - self.info.total_size = self.offset as CodeOffset; - } - fn add_call_site(&mut self, opcode: Opcode, loc: SourceLoc) { debug_assert!( opcode.is_call(), diff --git a/cranelift/codegen/src/binemit/mod.rs b/cranelift/codegen/src/binemit/mod.rs index ff124ec6fa..ee2ff47490 100644 --- a/cranelift/codegen/src/binemit/mod.rs +++ b/cranelift/codegen/src/binemit/mod.rs @@ -114,9 +114,6 @@ pub trait CodeSink { /// Add trap information for the current offset. fn trap(&mut self, _: TrapCode, _: SourceLoc); - /// Read-only data output is complete, we're done. - fn end_codegen(&mut self); - /// Add a call site for a call with the given opcode, returning at the current offset. fn add_call_site(&mut self, _: Opcode, _: SourceLoc) { // Default implementation doesn't need to do anything. diff --git a/cranelift/codegen/src/context.rs b/cranelift/codegen/src/context.rs index 67fc48d77c..d5adc868ec 100644 --- a/cranelift/codegen/src/context.rs +++ b/cranelift/codegen/src/context.rs @@ -200,7 +200,7 @@ impl Context { .as_ref() .expect("only using mach backend now"); result.buffer.emit(&mut sink); - let info = sink.info; + let info = sink.info(); // New backends do not emit StackMaps through the `CodeSink` because its interface // requires `Value`s; instead, the `StackMap` objects are directly accessible via // `result.buffer.stack_maps()`. diff --git a/cranelift/codegen/src/machinst/buffer.rs b/cranelift/codegen/src/machinst/buffer.rs index 2f46f7110e..1c487ab1dc 100644 --- a/cranelift/codegen/src/machinst/buffer.rs +++ b/cranelift/codegen/src/machinst/buffer.rs @@ -1470,8 +1470,6 @@ impl MachBufferFinalized { } sink.put1(*byte); } - - sink.end_codegen(); } /// Get the stack map metadata for this code. @@ -2088,7 +2086,6 @@ mod test { fn trap(&mut self, t: TrapCode, _: SourceLoc) { self.traps.push((self.offset, t)); } - fn end_codegen(&mut self) {} fn add_call_site(&mut self, op: Opcode, _: SourceLoc) { self.callsites.push((self.offset, op)); } diff --git a/cranelift/filetests/src/test_compile.rs b/cranelift/filetests/src/test_compile.rs index ec8bbbb120..5c03e96bec 100644 --- a/cranelift/filetests/src/test_compile.rs +++ b/cranelift/filetests/src/test_compile.rs @@ -94,7 +94,6 @@ impl binemit::CodeSink for SizeSink { ) { } fn trap(&mut self, _code: ir::TrapCode, _srcloc: ir::SourceLoc) {} - fn end_codegen(&mut self) {} } fn check_precise_output(text: &str, context: &Context) -> Result<()> { From 379c9c65a3cb817359d7e14dae72ad4cb9c073f8 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Tue, 11 Jan 2022 15:10:02 +0100 Subject: [PATCH 04/11] Inline MemoryCodeSink::write --- cranelift/codegen/src/binemit/memorysink.rs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/cranelift/codegen/src/binemit/memorysink.rs b/cranelift/codegen/src/binemit/memorysink.rs index 96a1ac4619..42ff627b6f 100644 --- a/cranelift/codegen/src/binemit/memorysink.rs +++ b/cranelift/codegen/src/binemit/memorysink.rs @@ -92,19 +92,12 @@ pub trait TrapSink { fn trap(&mut self, _: CodeOffset, _: SourceLoc, _: TrapCode); } -impl<'a> MemoryCodeSink<'a> { - fn write(&mut self, x: T) { - unsafe { - #[cfg_attr(feature = "cargo-clippy", allow(clippy::cast_ptr_alignment))] - write_unaligned(self.data.offset(self.offset) as *mut T, x); - self.offset += core::mem::size_of::() as isize; - } - } -} - impl<'a> CodeSink for MemoryCodeSink<'a> { fn put1(&mut self, x: u8) { - self.write(x); + unsafe { + write_unaligned(self.data.offset(self.offset), x); + self.offset += 1; + } } fn reloc_external( From 38aaa6e1daecfbf52b2f5265bbfec4dcff9832bb Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Tue, 11 Jan 2022 16:32:57 +0100 Subject: [PATCH 05/11] Remove add_call_site from CodeSink and RelocSink And introduce MachBufferFinalized::call_sites() in the place. --- cranelift/codegen/src/binemit/memorysink.rs | 15 +------- cranelift/codegen/src/binemit/mod.rs | 7 +--- cranelift/codegen/src/lib.rs | 2 +- cranelift/codegen/src/machinst/buffer.rs | 38 ++++++++++++--------- 4 files changed, 24 insertions(+), 38 deletions(-) diff --git a/cranelift/codegen/src/binemit/memorysink.rs b/cranelift/codegen/src/binemit/memorysink.rs index 42ff627b6f..8b5e02e738 100644 --- a/cranelift/codegen/src/binemit/memorysink.rs +++ b/cranelift/codegen/src/binemit/memorysink.rs @@ -15,7 +15,7 @@ //! `CodeSink::put*` methods, so the performance impact of the virtual callbacks is less severe. use super::{Addend, CodeInfo, CodeOffset, CodeSink, Reloc}; use crate::binemit::stack_map::StackMap; -use crate::ir::{ExternalName, Opcode, SourceLoc, TrapCode}; +use crate::ir::{ExternalName, SourceLoc, TrapCode}; use core::ptr::write_unaligned; /// A `CodeSink` that writes binary machine code directly into memory. @@ -77,10 +77,6 @@ pub trait RelocSink { _: &ExternalName, _: Addend, ); - - /// Track a call site whose return address is the given CodeOffset, for the given opcode. Does - /// nothing in general, only useful for certain embedders (SpiderMonkey). - fn add_call_site(&mut self, _: Opcode, _: CodeOffset, _: SourceLoc) {} } /// A trait for receiving trap codes and offsets. @@ -115,15 +111,6 @@ impl<'a> CodeSink for MemoryCodeSink<'a> { let ofs = self.offset as CodeOffset; self.traps.trap(ofs, srcloc, code); } - - fn add_call_site(&mut self, opcode: Opcode, loc: SourceLoc) { - debug_assert!( - opcode.is_call(), - "adding call site info for a non-call instruction." - ); - let ret_addr = self.offset as CodeOffset; - self.relocs.add_call_site(opcode, ret_addr, loc); - } } /// A `RelocSink` implementation that does nothing, which is convenient when diff --git a/cranelift/codegen/src/binemit/mod.rs b/cranelift/codegen/src/binemit/mod.rs index ee2ff47490..b2c345a37a 100644 --- a/cranelift/codegen/src/binemit/mod.rs +++ b/cranelift/codegen/src/binemit/mod.rs @@ -11,7 +11,7 @@ pub use self::memorysink::{ TrapSink, }; pub use self::stack_map::StackMap; -use crate::ir::{ExternalName, Opcode, SourceLoc, TrapCode}; +use crate::ir::{ExternalName, SourceLoc, TrapCode}; use core::fmt; #[cfg(feature = "enable-serde")] use serde::{Deserialize, Serialize}; @@ -113,9 +113,4 @@ pub trait CodeSink { /// Add trap information for the current offset. fn trap(&mut self, _: TrapCode, _: SourceLoc); - - /// Add a call site for a call with the given opcode, returning at the current offset. - fn add_call_site(&mut self, _: Opcode, _: SourceLoc) { - // Default implementation doesn't need to do anything. - } } diff --git a/cranelift/codegen/src/lib.rs b/cranelift/codegen/src/lib.rs index 426a9c0c4d..182de0a359 100644 --- a/cranelift/codegen/src/lib.rs +++ b/cranelift/codegen/src/lib.rs @@ -88,7 +88,7 @@ pub mod verifier; pub mod write; pub use crate::entity::packed_option; -pub use crate::machinst::buffer::MachSrcLoc; +pub use crate::machinst::buffer::{MachCallSite, MachSrcLoc}; pub use crate::machinst::TextSectionBuilder; mod bitset; diff --git a/cranelift/codegen/src/machinst/buffer.rs b/cranelift/codegen/src/machinst/buffer.rs index 1c487ab1dc..302a81f630 100644 --- a/cranelift/codegen/src/machinst/buffer.rs +++ b/cranelift/codegen/src/machinst/buffer.rs @@ -1350,6 +1350,10 @@ impl MachBuffer { /// Add a call-site record at the current offset. pub fn add_call_site(&mut self, srcloc: SourceLoc, opcode: Opcode) { + debug_assert!( + opcode.is_call(), + "adding call site info for a non-call instruction." + ); self.call_sites.push(MachCallSite { ret_addr: self.data.len() as CodeOffset, srcloc, @@ -1446,7 +1450,6 @@ impl MachBufferFinalized { let mut next_reloc = 0; let mut next_trap = 0; - let mut next_call_site = 0; for (idx, byte) in self.data.iter().enumerate() { while next_reloc < self.relocs.len() && self.relocs[next_reloc].offset == idx as CodeOffset @@ -1461,13 +1464,6 @@ impl MachBufferFinalized { sink.trap(trap.code, trap.srcloc); next_trap += 1; } - while next_call_site < self.call_sites.len() - && self.call_sites[next_call_site].ret_addr == idx as CodeOffset - { - let call_site = &self.call_sites[next_call_site]; - sink.add_call_site(call_site.opcode, call_site.srcloc); - next_call_site += 1; - } sink.put1(*byte); } } @@ -1476,6 +1472,11 @@ impl MachBufferFinalized { pub fn stack_maps(&self) -> &[MachStackMap] { &self.stack_maps[..] } + + /// Get the list of call sites for this code. + pub fn call_sites(&self) -> &[MachCallSite] { + &self.call_sites[..] + } } /// A constant that is deferred to the next constant-pool opportunity. @@ -1531,13 +1532,14 @@ struct MachTrap { } /// A call site record resulting from a compilation. -struct MachCallSite { +#[derive(Clone, Debug)] +pub struct MachCallSite { /// The offset of the call's return address, *relative to the containing section*. - ret_addr: CodeOffset, + pub ret_addr: CodeOffset, /// The original source location. - srcloc: SourceLoc, + pub srcloc: SourceLoc, /// The call's opcode. - opcode: Opcode, + pub opcode: Opcode, } /// A source-location mapping resulting from a compilation. @@ -2073,7 +2075,6 @@ mod test { struct TestCodeSink { offset: CodeOffset, traps: Vec<(CodeOffset, TrapCode)>, - callsites: Vec<(CodeOffset, Opcode)>, relocs: Vec<(CodeOffset, Reloc)>, } impl CodeSink for TestCodeSink { @@ -2086,9 +2087,6 @@ mod test { fn trap(&mut self, t: TrapCode, _: SourceLoc) { self.traps.push((self.offset, t)); } - fn add_call_site(&mut self, op: Opcode, _: SourceLoc) { - self.callsites.push((self.offset, op)); - } } let mut sink = TestCodeSink::default(); @@ -2103,7 +2101,13 @@ mod test { (2, TrapCode::IntegerDivisionByZero) ] ); - assert_eq!(sink.callsites, vec![(2, Opcode::Call),]); + assert_eq!( + buf.call_sites() + .iter() + .map(|call_site| (call_site.ret_addr, call_site.opcode)) + .collect::>(), + vec![(2, Opcode::Call)] + ); assert_eq!(sink.relocs, vec![(2, Reloc::Abs4), (3, Reloc::Abs8)]); } } From 63e23603461b8d8010caecfb1cfa63e180a6912c Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Tue, 11 Jan 2022 16:42:52 +0100 Subject: [PATCH 06/11] Remove trap from CodeSink And introduce MachBufferFinalized::traps() in the place. --- cranelift/codegen/src/binemit/memorysink.rs | 8 ------ cranelift/codegen/src/binemit/mod.rs | 5 +--- cranelift/codegen/src/context.rs | 15 +++++++---- cranelift/codegen/src/lib.rs | 2 +- cranelift/codegen/src/machinst/buffer.rs | 29 +++++++++------------ cranelift/filetests/src/test_compile.rs | 1 - 6 files changed, 25 insertions(+), 35 deletions(-) diff --git a/cranelift/codegen/src/binemit/memorysink.rs b/cranelift/codegen/src/binemit/memorysink.rs index 8b5e02e738..d462dd7000 100644 --- a/cranelift/codegen/src/binemit/memorysink.rs +++ b/cranelift/codegen/src/binemit/memorysink.rs @@ -35,7 +35,6 @@ pub struct MemoryCodeSink<'a> { /// Offset is isize because its major consumer needs it in that form. offset: isize, relocs: &'a mut dyn RelocSink, - traps: &'a mut dyn TrapSink, } impl<'a> MemoryCodeSink<'a> { @@ -48,13 +47,11 @@ impl<'a> MemoryCodeSink<'a> { pub unsafe fn new( data: *mut u8, relocs: &'a mut dyn RelocSink, - traps: &'a mut dyn TrapSink, ) -> Self { Self { data, offset: 0, relocs, - traps, } } @@ -106,11 +103,6 @@ impl<'a> CodeSink for MemoryCodeSink<'a> { let ofs = self.offset as CodeOffset; self.relocs.reloc_external(ofs, srcloc, rel, name, addend); } - - fn trap(&mut self, code: TrapCode, srcloc: SourceLoc) { - let ofs = self.offset as CodeOffset; - self.traps.trap(ofs, srcloc, code); - } } /// A `RelocSink` implementation that does nothing, which is convenient when diff --git a/cranelift/codegen/src/binemit/mod.rs b/cranelift/codegen/src/binemit/mod.rs index b2c345a37a..8fe228789f 100644 --- a/cranelift/codegen/src/binemit/mod.rs +++ b/cranelift/codegen/src/binemit/mod.rs @@ -11,7 +11,7 @@ pub use self::memorysink::{ TrapSink, }; pub use self::stack_map::StackMap; -use crate::ir::{ExternalName, SourceLoc, TrapCode}; +use crate::ir::{ExternalName, SourceLoc}; use core::fmt; #[cfg(feature = "enable-serde")] use serde::{Deserialize, Serialize}; @@ -110,7 +110,4 @@ pub trait CodeSink { /// Add a relocation referencing an external symbol plus the addend at the current offset. fn reloc_external(&mut self, _: SourceLoc, _: Reloc, _: &ExternalName, _: Addend); - - /// Add trap information for the current offset. - fn trap(&mut self, _: TrapCode, _: SourceLoc); } diff --git a/cranelift/codegen/src/context.rs b/cranelift/codegen/src/context.rs index d5adc868ec..fecf0a852b 100644 --- a/cranelift/codegen/src/context.rs +++ b/cranelift/codegen/src/context.rs @@ -18,7 +18,7 @@ use crate::isa::TargetIsa; use crate::legalizer::simple_legalize; use crate::licm::do_licm; use crate::loop_analysis::LoopAnalysis; -use crate::machinst::{MachCompileResult, MachStackMap}; +use crate::machinst::{MachCompileResult, MachStackMap, MachTrap}; use crate::nan_canonicalization::do_nan_canonicalization; use crate::remove_constant_phis::do_remove_constant_phis; use crate::result::CodegenResult; @@ -194,16 +194,21 @@ impl Context { stack_maps: &mut dyn StackMapSink, ) -> CodeInfo { let _tt = timing::binemit(); - let mut sink = MemoryCodeSink::new(mem, relocs, traps); + let mut sink = MemoryCodeSink::new(mem, relocs); let result = self .mach_compile_result .as_ref() .expect("only using mach backend now"); result.buffer.emit(&mut sink); let info = sink.info(); - // New backends do not emit StackMaps through the `CodeSink` because its interface - // requires `Value`s; instead, the `StackMap` objects are directly accessible via - // `result.buffer.stack_maps()`. + for &MachTrap { + offset, + srcloc, + code, + } in result.buffer.traps() + { + traps.trap(offset, srcloc, code); + } for &MachStackMap { offset_end, ref stack_map, diff --git a/cranelift/codegen/src/lib.rs b/cranelift/codegen/src/lib.rs index 182de0a359..7e14b5380c 100644 --- a/cranelift/codegen/src/lib.rs +++ b/cranelift/codegen/src/lib.rs @@ -88,7 +88,7 @@ pub mod verifier; pub mod write; pub use crate::entity::packed_option; -pub use crate::machinst::buffer::{MachCallSite, MachSrcLoc}; +pub use crate::machinst::buffer::{MachCallSite, MachSrcLoc, MachTrap}; pub use crate::machinst::TextSectionBuilder; mod bitset; diff --git a/cranelift/codegen/src/machinst/buffer.rs b/cranelift/codegen/src/machinst/buffer.rs index 302a81f630..6f73649ff4 100644 --- a/cranelift/codegen/src/machinst/buffer.rs +++ b/cranelift/codegen/src/machinst/buffer.rs @@ -1449,7 +1449,6 @@ impl MachBufferFinalized { // to add the appropriate relocations in this case. let mut next_reloc = 0; - let mut next_trap = 0; for (idx, byte) in self.data.iter().enumerate() { while next_reloc < self.relocs.len() && self.relocs[next_reloc].offset == idx as CodeOffset @@ -1458,16 +1457,15 @@ impl MachBufferFinalized { sink.reloc_external(reloc.srcloc, reloc.kind, &reloc.name, reloc.addend); next_reloc += 1; } - while next_trap < self.traps.len() && self.traps[next_trap].offset == idx as CodeOffset - { - let trap = &self.traps[next_trap]; - sink.trap(trap.code, trap.srcloc); - next_trap += 1; - } sink.put1(*byte); } } + /// Get the list of trap records for this code. + pub fn traps(&self) -> &[MachTrap] { + &self.traps[..] + } + /// Get the stack map metadata for this code. pub fn stack_maps(&self) -> &[MachStackMap] { &self.stack_maps[..] @@ -1521,14 +1519,14 @@ struct MachReloc { } /// A trap record resulting from a compilation. -struct MachTrap { +pub struct MachTrap { /// The offset at which the trap instruction occurs, *relative to the /// containing section*. - offset: CodeOffset, + pub offset: CodeOffset, /// The original source location. - srcloc: SourceLoc, + pub srcloc: SourceLoc, /// The trap code. - code: TrapCode, + pub code: TrapCode, } /// A call site record resulting from a compilation. @@ -2074,7 +2072,6 @@ mod test { #[derive(Default)] struct TestCodeSink { offset: CodeOffset, - traps: Vec<(CodeOffset, TrapCode)>, relocs: Vec<(CodeOffset, Reloc)>, } impl CodeSink for TestCodeSink { @@ -2084,9 +2081,6 @@ mod test { fn reloc_external(&mut self, _: SourceLoc, r: Reloc, _: &ExternalName, _: Addend) { self.relocs.push((self.offset, r)); } - fn trap(&mut self, t: TrapCode, _: SourceLoc) { - self.traps.push((self.offset, t)); - } } let mut sink = TestCodeSink::default(); @@ -2094,7 +2088,10 @@ mod test { assert_eq!(sink.offset, 4); assert_eq!( - sink.traps, + buf.traps() + .iter() + .map(|trap| (trap.offset, trap.code)) + .collect::>(), vec![ (1, TrapCode::HeapOutOfBounds), (2, TrapCode::IntegerOverflow), diff --git a/cranelift/filetests/src/test_compile.rs b/cranelift/filetests/src/test_compile.rs index 5c03e96bec..c059f60d7d 100644 --- a/cranelift/filetests/src/test_compile.rs +++ b/cranelift/filetests/src/test_compile.rs @@ -93,7 +93,6 @@ impl binemit::CodeSink for SizeSink { _addend: binemit::Addend, ) { } - fn trap(&mut self, _code: ir::TrapCode, _srcloc: ir::SourceLoc) {} } fn check_precise_output(text: &str, context: &Context) -> Result<()> { From a48a60f958316d15d8c58ef6dff72a1f884230fc Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Tue, 11 Jan 2022 16:54:27 +0100 Subject: [PATCH 07/11] Remove reloc_external from CodeSink And introduce MachBufferFinalized::relocs() in the place. --- cranelift/codegen/src/binemit/memorysink.rs | 29 +++------------ cranelift/codegen/src/binemit/mod.rs | 4 -- cranelift/codegen/src/context.rs | 14 ++++++- cranelift/codegen/src/lib.rs | 2 +- cranelift/codegen/src/machinst/buffer.rs | 41 ++++++++++----------- cranelift/filetests/src/test_compile.rs | 9 ----- 6 files changed, 38 insertions(+), 61 deletions(-) diff --git a/cranelift/codegen/src/binemit/memorysink.rs b/cranelift/codegen/src/binemit/memorysink.rs index d462dd7000..651921609f 100644 --- a/cranelift/codegen/src/binemit/memorysink.rs +++ b/cranelift/codegen/src/binemit/memorysink.rs @@ -29,30 +29,22 @@ use core::ptr::write_unaligned; /// /// Note that `MemoryCodeSink` writes multi-byte values in the native byte order of the host. This /// is not the right thing to do for cross compilation. -pub struct MemoryCodeSink<'a> { +pub struct MemoryCodeSink { /// Pointer to start of sink's preallocated memory. data: *mut u8, /// Offset is isize because its major consumer needs it in that form. offset: isize, - relocs: &'a mut dyn RelocSink, } -impl<'a> MemoryCodeSink<'a> { +impl<'a> MemoryCodeSink { /// Create a new memory code sink that writes a function to the memory pointed to by `data`. /// /// # Safety /// /// This function is unsafe since `MemoryCodeSink` does not perform bounds checking on the /// memory buffer, and it can't guarantee that the `data` pointer is valid. - pub unsafe fn new( - data: *mut u8, - relocs: &'a mut dyn RelocSink, - ) -> Self { - Self { - data, - offset: 0, - relocs, - } + pub unsafe fn new(data: *mut u8) -> Self { + Self { data, offset: 0 } } /// Information about the generated code and read-only data. @@ -85,24 +77,13 @@ pub trait TrapSink { fn trap(&mut self, _: CodeOffset, _: SourceLoc, _: TrapCode); } -impl<'a> CodeSink for MemoryCodeSink<'a> { +impl CodeSink for MemoryCodeSink { fn put1(&mut self, x: u8) { unsafe { write_unaligned(self.data.offset(self.offset), x); self.offset += 1; } } - - fn reloc_external( - &mut self, - srcloc: SourceLoc, - rel: Reloc, - name: &ExternalName, - addend: Addend, - ) { - let ofs = self.offset as CodeOffset; - self.relocs.reloc_external(ofs, srcloc, rel, name, addend); - } } /// A `RelocSink` implementation that does nothing, which is convenient when diff --git a/cranelift/codegen/src/binemit/mod.rs b/cranelift/codegen/src/binemit/mod.rs index 8fe228789f..2bb800976f 100644 --- a/cranelift/codegen/src/binemit/mod.rs +++ b/cranelift/codegen/src/binemit/mod.rs @@ -11,7 +11,6 @@ pub use self::memorysink::{ TrapSink, }; pub use self::stack_map::StackMap; -use crate::ir::{ExternalName, SourceLoc}; use core::fmt; #[cfg(feature = "enable-serde")] use serde::{Deserialize, Serialize}; @@ -107,7 +106,4 @@ pub struct CodeInfo { pub trait CodeSink { /// Add 1 byte to the code section. fn put1(&mut self, _: u8); - - /// Add a relocation referencing an external symbol plus the addend at the current offset. - fn reloc_external(&mut self, _: SourceLoc, _: Reloc, _: &ExternalName, _: Addend); } diff --git a/cranelift/codegen/src/context.rs b/cranelift/codegen/src/context.rs index fecf0a852b..c896c8e737 100644 --- a/cranelift/codegen/src/context.rs +++ b/cranelift/codegen/src/context.rs @@ -18,7 +18,7 @@ use crate::isa::TargetIsa; use crate::legalizer::simple_legalize; use crate::licm::do_licm; use crate::loop_analysis::LoopAnalysis; -use crate::machinst::{MachCompileResult, MachStackMap, MachTrap}; +use crate::machinst::{MachCompileResult, MachReloc, MachStackMap, MachTrap}; use crate::nan_canonicalization::do_nan_canonicalization; use crate::remove_constant_phis::do_remove_constant_phis; use crate::result::CodegenResult; @@ -194,13 +194,23 @@ impl Context { stack_maps: &mut dyn StackMapSink, ) -> CodeInfo { let _tt = timing::binemit(); - let mut sink = MemoryCodeSink::new(mem, relocs); + let mut sink = MemoryCodeSink::new(mem); let result = self .mach_compile_result .as_ref() .expect("only using mach backend now"); result.buffer.emit(&mut sink); let info = sink.info(); + for &MachReloc { + offset, + srcloc, + kind, + ref name, + addend, + } in result.buffer.relocs() + { + relocs.reloc_external(offset, srcloc, kind, name, addend); + } for &MachTrap { offset, srcloc, diff --git a/cranelift/codegen/src/lib.rs b/cranelift/codegen/src/lib.rs index 7e14b5380c..e860990372 100644 --- a/cranelift/codegen/src/lib.rs +++ b/cranelift/codegen/src/lib.rs @@ -88,7 +88,7 @@ pub mod verifier; pub mod write; pub use crate::entity::packed_option; -pub use crate::machinst::buffer::{MachCallSite, MachSrcLoc, MachTrap}; +pub use crate::machinst::buffer::{MachCallSite, MachReloc, MachSrcLoc, MachTrap}; pub use crate::machinst::TextSectionBuilder; mod bitset; diff --git a/cranelift/codegen/src/machinst/buffer.rs b/cranelift/codegen/src/machinst/buffer.rs index 6f73649ff4..6c38297a33 100644 --- a/cranelift/codegen/src/machinst/buffer.rs +++ b/cranelift/codegen/src/machinst/buffer.rs @@ -1448,19 +1448,16 @@ impl MachBufferFinalized { // add this designation and segregate the output; take care, however, // to add the appropriate relocations in this case. - let mut next_reloc = 0; - for (idx, byte) in self.data.iter().enumerate() { - while next_reloc < self.relocs.len() - && self.relocs[next_reloc].offset == idx as CodeOffset - { - let reloc = &self.relocs[next_reloc]; - sink.reloc_external(reloc.srcloc, reloc.kind, &reloc.name, reloc.addend); - next_reloc += 1; - } - sink.put1(*byte); + for &byte in self.data.iter() { + sink.put1(byte); } } + /// Get the list of external relocations for this code. + pub fn relocs(&self) -> &[MachReloc] { + &self.relocs[..] + } + /// Get the list of trap records for this code. pub fn traps(&self) -> &[MachTrap] { &self.traps[..] @@ -1504,18 +1501,18 @@ struct MachLabelFixup { } /// A relocation resulting from a compilation. -struct MachReloc { +pub struct MachReloc { /// The offset at which the relocation applies, *relative to the /// containing section*. - offset: CodeOffset, + pub offset: CodeOffset, /// The original source location. - srcloc: SourceLoc, + pub srcloc: SourceLoc, /// The kind of relocation. - kind: Reloc, + pub kind: Reloc, /// The external symbol / name to which this relocation refers. - name: ExternalName, + pub name: ExternalName, /// The addend to add to the symbol value. - addend: i64, + pub addend: i64, } /// A trap record resulting from a compilation. @@ -2072,15 +2069,11 @@ mod test { #[derive(Default)] struct TestCodeSink { offset: CodeOffset, - relocs: Vec<(CodeOffset, Reloc)>, } impl CodeSink for TestCodeSink { fn put1(&mut self, _: u8) { self.offset += 1; } - fn reloc_external(&mut self, _: SourceLoc, r: Reloc, _: &ExternalName, _: Addend) { - self.relocs.push((self.offset, r)); - } } let mut sink = TestCodeSink::default(); @@ -2105,6 +2098,12 @@ mod test { .collect::>(), vec![(2, Opcode::Call)] ); - assert_eq!(sink.relocs, vec![(2, Reloc::Abs4), (3, Reloc::Abs8)]); + assert_eq!( + buf.relocs() + .iter() + .map(|reloc| (reloc.offset, reloc.kind)) + .collect::>(), + vec![(2, Reloc::Abs4), (3, Reloc::Abs8)] + ); } } diff --git a/cranelift/filetests/src/test_compile.rs b/cranelift/filetests/src/test_compile.rs index c059f60d7d..cadc016dc1 100644 --- a/cranelift/filetests/src/test_compile.rs +++ b/cranelift/filetests/src/test_compile.rs @@ -84,15 +84,6 @@ impl binemit::CodeSink for SizeSink { fn put1(&mut self, _: u8) { self.offset += 1; } - - fn reloc_external( - &mut self, - _srcloc: ir::SourceLoc, - _reloc: binemit::Reloc, - _name: &ir::ExternalName, - _addend: binemit::Addend, - ) { - } } fn check_precise_output(text: &str, context: &Context) -> Result<()> { From 55d722db05da65621eb17c95050d475d2d632565 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Tue, 11 Jan 2022 17:10:37 +0100 Subject: [PATCH 08/11] Remove CodeSink --- cranelift/codegen/src/binemit/memorysink.rs | 49 +-------------------- cranelift/codegen/src/binemit/mod.rs | 12 +---- cranelift/codegen/src/context.rs | 11 +++-- cranelift/codegen/src/isa/aarch64/mod.rs | 6 +-- cranelift/codegen/src/isa/s390x/mod.rs | 6 +-- cranelift/codegen/src/isa/x64/mod.rs | 2 +- cranelift/codegen/src/machinst/buffer.rs | 27 +++--------- cranelift/filetests/src/test_compile.rs | 13 +----- 8 files changed, 23 insertions(+), 103 deletions(-) diff --git a/cranelift/codegen/src/binemit/memorysink.rs b/cranelift/codegen/src/binemit/memorysink.rs index 651921609f..149fed67bb 100644 --- a/cranelift/codegen/src/binemit/memorysink.rs +++ b/cranelift/codegen/src/binemit/memorysink.rs @@ -13,47 +13,9 @@ //! that a `MemoryCodeSink` will always write binary machine code to raw memory. It forwards any //! relocations to a `RelocSink` trait object. Relocations are less frequent than the //! `CodeSink::put*` methods, so the performance impact of the virtual callbacks is less severe. -use super::{Addend, CodeInfo, CodeOffset, CodeSink, Reloc}; +use super::{Addend, CodeOffset, Reloc}; use crate::binemit::stack_map::StackMap; use crate::ir::{ExternalName, SourceLoc, TrapCode}; -use core::ptr::write_unaligned; - -/// A `CodeSink` that writes binary machine code directly into memory. -/// -/// A `MemoryCodeSink` object should be used when emitting a Cranelift IR function into executable -/// memory. It writes machine code directly to a raw pointer without any bounds checking, so make -/// sure to allocate enough memory for the whole function. The number of bytes required is returned -/// by the `Context::compile()` function. -/// -/// Any relocations in the function are forwarded to the `RelocSink` trait object. -/// -/// Note that `MemoryCodeSink` writes multi-byte values in the native byte order of the host. This -/// is not the right thing to do for cross compilation. -pub struct MemoryCodeSink { - /// Pointer to start of sink's preallocated memory. - data: *mut u8, - /// Offset is isize because its major consumer needs it in that form. - offset: isize, -} - -impl<'a> MemoryCodeSink { - /// Create a new memory code sink that writes a function to the memory pointed to by `data`. - /// - /// # Safety - /// - /// This function is unsafe since `MemoryCodeSink` does not perform bounds checking on the - /// memory buffer, and it can't guarantee that the `data` pointer is valid. - pub unsafe fn new(data: *mut u8) -> Self { - Self { data, offset: 0 } - } - - /// Information about the generated code and read-only data. - pub fn info(&self) -> CodeInfo { - CodeInfo { - total_size: self.offset as CodeOffset, - } - } -} /// A trait for receiving relocations for code that is emitted directly into memory. pub trait RelocSink { @@ -77,15 +39,6 @@ pub trait TrapSink { fn trap(&mut self, _: CodeOffset, _: SourceLoc, _: TrapCode); } -impl CodeSink for MemoryCodeSink { - fn put1(&mut self, x: u8) { - unsafe { - write_unaligned(self.data.offset(self.offset), x); - self.offset += 1; - } - } -} - /// A `RelocSink` implementation that does nothing, which is convenient when /// compiling code that does not relocate anything. #[derive(Default)] diff --git a/cranelift/codegen/src/binemit/mod.rs b/cranelift/codegen/src/binemit/mod.rs index 2bb800976f..0c634b27af 100644 --- a/cranelift/codegen/src/binemit/mod.rs +++ b/cranelift/codegen/src/binemit/mod.rs @@ -7,8 +7,7 @@ mod memorysink; mod stack_map; pub use self::memorysink::{ - MemoryCodeSink, NullRelocSink, NullStackMapSink, NullTrapSink, RelocSink, StackMapSink, - TrapSink, + NullRelocSink, NullStackMapSink, NullTrapSink, RelocSink, StackMapSink, TrapSink, }; pub use self::stack_map::StackMap; use core::fmt; @@ -98,12 +97,3 @@ pub struct CodeInfo { /// Number of bytes in total. pub total_size: CodeOffset, } - -/// Abstract interface for adding bytes to the code segment. -/// -/// A `CodeSink` will receive all of the machine code for a function. It also accepts relocations -/// which are locations in the code section that need to be fixed up when linking. -pub trait CodeSink { - /// Add 1 byte to the code section. - fn put1(&mut self, _: u8); -} diff --git a/cranelift/codegen/src/context.rs b/cranelift/codegen/src/context.rs index c896c8e737..2cebe0d3c3 100644 --- a/cranelift/codegen/src/context.rs +++ b/cranelift/codegen/src/context.rs @@ -9,7 +9,7 @@ //! contexts concurrently. Typically, you would have one context per compilation thread and only a //! single ISA instance. -use crate::binemit::{CodeInfo, MemoryCodeSink, RelocSink, StackMapSink, TrapSink}; +use crate::binemit::{CodeInfo, RelocSink, StackMapSink, TrapSink}; use crate::dce::do_dce; use crate::dominator_tree::DominatorTree; use crate::flowgraph::ControlFlowGraph; @@ -186,6 +186,7 @@ impl Context { /// and it can't guarantee that the `mem` pointer is valid. /// /// Returns information about the emitted code and data. + #[deny(unsafe_op_in_unsafe_fn)] pub unsafe fn emit_to_memory( &self, mem: *mut u8, @@ -194,13 +195,15 @@ impl Context { stack_maps: &mut dyn StackMapSink, ) -> CodeInfo { let _tt = timing::binemit(); - let mut sink = MemoryCodeSink::new(mem); let result = self .mach_compile_result .as_ref() .expect("only using mach backend now"); - result.buffer.emit(&mut sink); - let info = sink.info(); + let info = result.code_info(); + + let mem = unsafe { std::slice::from_raw_parts_mut(mem, info.total_size as usize) }; + mem.copy_from_slice(result.buffer.data()); + for &MachReloc { offset, srcloc, diff --git a/cranelift/codegen/src/isa/aarch64/mod.rs b/cranelift/codegen/src/isa/aarch64/mod.rs index ae19034dd9..16e45cba00 100644 --- a/cranelift/codegen/src/isa/aarch64/mod.rs +++ b/cranelift/codegen/src/isa/aarch64/mod.rs @@ -130,7 +130,7 @@ impl TargetIsa for AArch64Backend { Some(UnwindInfo::SystemV( crate::isa::unwind::systemv::create_unwind_info_from_insts( &result.buffer.unwind_info[..], - result.buffer.data.len(), + result.buffer.data().len(), &mapper, )?, )) @@ -216,7 +216,7 @@ mod test { isa_flags, ); let buffer = backend.compile_function(&mut func, false).unwrap().buffer; - let code = &buffer.data[..]; + let code = buffer.data(); // mov x1, #0x1234 // add w0, w0, w1 @@ -271,7 +271,7 @@ mod test { let result = backend .compile_function(&mut func, /* want_disasm = */ false) .unwrap(); - let code = &result.buffer.data[..]; + let code = result.buffer.data(); // mov x1, #0x1234 // #4660 // add w0, w0, w1 diff --git a/cranelift/codegen/src/isa/s390x/mod.rs b/cranelift/codegen/src/isa/s390x/mod.rs index dbf8679924..046e077fcd 100644 --- a/cranelift/codegen/src/isa/s390x/mod.rs +++ b/cranelift/codegen/src/isa/s390x/mod.rs @@ -136,7 +136,7 @@ impl TargetIsa for S390xBackend { Some(UnwindInfo::SystemV( crate::isa::unwind::systemv::create_unwind_info_from_insts( &result.buffer.unwind_info[..], - result.buffer.data.len(), + result.buffer.data().len(), &mapper, )?, )) @@ -225,7 +225,7 @@ mod test { let result = backend .compile_function(&mut func, /* want_disasm = */ false) .unwrap(); - let code = &result.buffer.data[..]; + let code = result.buffer.data(); // ahi %r2, 0x1234 // br %r14 @@ -277,7 +277,7 @@ mod test { let result = backend .compile_function(&mut func, /* want_disasm = */ false) .unwrap(); - let code = &result.buffer.data[..]; + let code = result.buffer.data(); // FIXME: the branching logic should be optimized more diff --git a/cranelift/codegen/src/isa/x64/mod.rs b/cranelift/codegen/src/isa/x64/mod.rs index 7e4c4f277b..5fe00926a0 100644 --- a/cranelift/codegen/src/isa/x64/mod.rs +++ b/cranelift/codegen/src/isa/x64/mod.rs @@ -122,7 +122,7 @@ impl TargetIsa for X64Backend { Some(UnwindInfo::SystemV( crate::isa::unwind::systemv::create_unwind_info_from_insts( &result.buffer.unwind_info[..], - result.buffer.data.len(), + result.buffer.data().len(), &mapper, )?, )) diff --git a/cranelift/codegen/src/machinst/buffer.rs b/cranelift/codegen/src/machinst/buffer.rs index 6c38297a33..2eeab23f33 100644 --- a/cranelift/codegen/src/machinst/buffer.rs +++ b/cranelift/codegen/src/machinst/buffer.rs @@ -140,7 +140,7 @@ //! Given these invariants, we argue why each optimization preserves execution //! semantics below (grep for "Preserves execution semantics"). -use crate::binemit::{Addend, CodeOffset, CodeSink, Reloc, StackMap}; +use crate::binemit::{Addend, CodeOffset, Reloc, StackMap}; use crate::ir::{ExternalName, Opcode, SourceLoc, TrapCode}; use crate::isa::unwind::UnwindInst; use crate::machinst::{ @@ -233,7 +233,7 @@ pub struct MachBuffer { /// without fixups. This allows the type to be independent of the backend. pub struct MachBufferFinalized { /// The buffer contents, as raw bytes. - pub data: SmallVec<[u8; 1024]>, + data: SmallVec<[u8; 1024]>, /// Any relocations referring to this code. Note that only *external* /// relocations are tracked here; references to labels within the buffer are /// resolved before emission. @@ -1435,8 +1435,8 @@ impl MachBufferFinalized { s } - /// Emit this buffer to the given CodeSink. - pub fn emit(&self, sink: &mut CS) { + /// Get the code bytes. + pub fn data(&self) -> &[u8] { // N.B.: we emit every section into the .text section as far as // the `CodeSink` is concerned; we do not bother to segregate // the contents into the actual program text, the jumptable and the @@ -1448,9 +1448,7 @@ impl MachBufferFinalized { // add this designation and segregate the output; take care, however, // to add the appropriate relocations in this case. - for &byte in self.data.iter() { - sink.put1(byte); - } + &self.data[..] } /// Get the list of external relocations for this code. @@ -2066,20 +2064,7 @@ mod test { let buf = buf.finish(); - #[derive(Default)] - struct TestCodeSink { - offset: CodeOffset, - } - impl CodeSink for TestCodeSink { - fn put1(&mut self, _: u8) { - self.offset += 1; - } - } - - let mut sink = TestCodeSink::default(); - buf.emit(&mut sink); - - assert_eq!(sink.offset, 4); + assert_eq!(buf.data(), &[1, 2, 3, 4]); assert_eq!( buf.traps() .iter() diff --git a/cranelift/filetests/src/test_compile.rs b/cranelift/filetests/src/test_compile.rs index cadc016dc1..265d6c02be 100644 --- a/cranelift/filetests/src/test_compile.rs +++ b/cranelift/filetests/src/test_compile.rs @@ -4,7 +4,7 @@ use crate::subtest::{run_filecheck, Context, SubTest}; use anyhow::{bail, Result}; -use cranelift_codegen::binemit::{self, CodeInfo}; +use cranelift_codegen::binemit::CodeInfo; use cranelift_codegen::ir; use cranelift_reader::{TestCommand, TestOption}; use log::info; @@ -75,17 +75,6 @@ impl SubTest for TestCompile { } } -/// Code sink that simply counts bytes. -struct SizeSink { - offset: binemit::CodeOffset, -} - -impl binemit::CodeSink for SizeSink { - fn put1(&mut self, _: u8) { - self.offset += 1; - } -} - fn check_precise_output(text: &str, context: &Context) -> Result<()> { let actual = text.lines().collect::>(); From b803514d55e265094d2c8cae26798a6318e97d02 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Tue, 11 Jan 2022 18:17:29 +0100 Subject: [PATCH 09/11] Remove sink arguments from compile_and_emit The data can be accessed after the fact using context.mach_compile_result --- cranelift/codegen/src/context.rs | 45 ++------------- cranelift/codegen/src/lib.rs | 2 +- cranelift/filetests/src/function_runner.rs | 6 +- cranelift/jit/examples/jit-minimal.rs | 11 +--- cranelift/jit/src/backend.rs | 18 ++++-- cranelift/jit/tests/basic.rs | 13 +---- cranelift/module/src/module.rs | 6 +- cranelift/object/src/backend.rs | 25 ++++---- cranelift/object/tests/basic.rs | 17 +----- cranelift/src/bugpoint.rs | 15 +---- cranelift/src/compile.rs | 34 ++++++++++- cranelift/src/wasm.rs | 34 ++++++++++- crates/cranelift/src/compiler.rs | 67 ++++++++++++++++------ 13 files changed, 155 insertions(+), 138 deletions(-) diff --git a/cranelift/codegen/src/context.rs b/cranelift/codegen/src/context.rs index 2cebe0d3c3..91781d1895 100644 --- a/cranelift/codegen/src/context.rs +++ b/cranelift/codegen/src/context.rs @@ -9,7 +9,7 @@ //! contexts concurrently. Typically, you would have one context per compilation thread and only a //! single ISA instance. -use crate::binemit::{CodeInfo, RelocSink, StackMapSink, TrapSink}; +use crate::binemit::CodeInfo; use crate::dce::do_dce; use crate::dominator_tree::DominatorTree; use crate::flowgraph::ControlFlowGraph; @@ -18,7 +18,7 @@ use crate::isa::TargetIsa; use crate::legalizer::simple_legalize; use crate::licm::do_licm; use crate::loop_analysis::LoopAnalysis; -use crate::machinst::{MachCompileResult, MachReloc, MachStackMap, MachTrap}; +use crate::machinst::MachCompileResult; use crate::nan_canonicalization::do_nan_canonicalization; use crate::remove_constant_phis::do_remove_constant_phis; use crate::result::CodegenResult; @@ -111,16 +111,11 @@ impl Context { &mut self, isa: &dyn TargetIsa, mem: &mut Vec, - relocs: &mut dyn RelocSink, - traps: &mut dyn TrapSink, - stack_maps: &mut dyn StackMapSink, ) -> CodegenResult<()> { let info = self.compile(isa)?; let old_len = mem.len(); mem.resize(old_len + info.total_size as usize, 0); - let new_info = unsafe { - self.emit_to_memory(mem.as_mut_ptr().add(old_len), relocs, traps, stack_maps) - }; + let new_info = unsafe { self.emit_to_memory(mem.as_mut_ptr().add(old_len)) }; debug_assert!(new_info == info); Ok(()) } @@ -187,13 +182,7 @@ impl Context { /// /// Returns information about the emitted code and data. #[deny(unsafe_op_in_unsafe_fn)] - pub unsafe fn emit_to_memory( - &self, - mem: *mut u8, - relocs: &mut dyn RelocSink, - traps: &mut dyn TrapSink, - stack_maps: &mut dyn StackMapSink, - ) -> CodeInfo { + pub unsafe fn emit_to_memory(&self, mem: *mut u8) -> CodeInfo { let _tt = timing::binemit(); let result = self .mach_compile_result @@ -204,32 +193,6 @@ impl Context { let mem = unsafe { std::slice::from_raw_parts_mut(mem, info.total_size as usize) }; mem.copy_from_slice(result.buffer.data()); - for &MachReloc { - offset, - srcloc, - kind, - ref name, - addend, - } in result.buffer.relocs() - { - relocs.reloc_external(offset, srcloc, kind, name, addend); - } - for &MachTrap { - offset, - srcloc, - code, - } in result.buffer.traps() - { - traps.trap(offset, srcloc, code); - } - for &MachStackMap { - offset_end, - ref stack_map, - .. - } in result.buffer.stack_maps() - { - stack_maps.add_stack_map(offset_end, stack_map.clone()); - } info } diff --git a/cranelift/codegen/src/lib.rs b/cranelift/codegen/src/lib.rs index e860990372..3d5ca20035 100644 --- a/cranelift/codegen/src/lib.rs +++ b/cranelift/codegen/src/lib.rs @@ -88,7 +88,7 @@ pub mod verifier; pub mod write; pub use crate::entity::packed_option; -pub use crate::machinst::buffer::{MachCallSite, MachReloc, MachSrcLoc, MachTrap}; +pub use crate::machinst::buffer::{MachCallSite, MachReloc, MachSrcLoc, MachStackMap, MachTrap}; pub use crate::machinst::TextSectionBuilder; mod bitset; diff --git a/cranelift/filetests/src/function_runner.rs b/cranelift/filetests/src/function_runner.rs index 4046072a23..116dd73774 100644 --- a/cranelift/filetests/src/function_runner.rs +++ b/cranelift/filetests/src/function_runner.rs @@ -1,6 +1,5 @@ //! Provides functionality for compiling and running CLIF IR for `run` tests. use core::mem; -use cranelift_codegen::binemit::{NullRelocSink, NullStackMapSink, NullTrapSink}; use cranelift_codegen::data_value::DataValue; use cranelift_codegen::ir::{condcodes::IntCC, Function, InstBuilder, Signature}; use cranelift_codegen::isa::TargetIsa; @@ -241,14 +240,11 @@ fn compile(function: Function, isa: &dyn TargetIsa) -> Result ModuleResult { info!("defining function {}: {}", id, ctx.func.display()); let decl = self.declarations.get_function_decl(id); @@ -674,7 +672,17 @@ impl Module for JITModule { .expect("TODO: handle OOM etc."); let mut reloc_sink = JITRelocSink::default(); - unsafe { ctx.emit_to_memory(ptr, &mut reloc_sink, trap_sink, stack_map_sink) }; + unsafe { ctx.emit_to_memory(ptr) }; + for &MachReloc { + offset, + srcloc, + kind, + ref name, + addend, + } in ctx.mach_compile_result.as_ref().unwrap().buffer.relocs() + { + reloc_sink.reloc_external(offset, srcloc, kind, name, addend); + } self.record_function_for_perf(ptr, size, &decl.name); self.compiled_functions[id] = Some(CompiledBlob { diff --git a/cranelift/jit/tests/basic.rs b/cranelift/jit/tests/basic.rs index f53480702d..a1a038f6fe 100644 --- a/cranelift/jit/tests/basic.rs +++ b/cranelift/jit/tests/basic.rs @@ -1,4 +1,3 @@ -use cranelift_codegen::binemit::{NullStackMapSink, NullTrapSink}; use cranelift_codegen::ir::*; use cranelift_codegen::isa::CallConv; use cranelift_codegen::settings::{self, Configurable}; @@ -56,11 +55,7 @@ fn define_simple_function(module: &mut JITModule) -> FuncId { bcx.ins().return_(&[]); } - let mut trap_sink = NullTrapSink {}; - let mut stack_map_sink = NullStackMapSink {}; - module - .define_function(func_id, &mut ctx, &mut trap_sink, &mut stack_map_sink) - .unwrap(); + module.define_function(func_id, &mut ctx).unwrap(); func_id } @@ -205,11 +200,7 @@ fn libcall_function() { bcx.ins().return_(&[]); } - let mut trap_sink = NullTrapSink {}; - let mut stack_map_sink = NullStackMapSink {}; - module - .define_function(func_id, &mut ctx, &mut trap_sink, &mut stack_map_sink) - .unwrap(); + module.define_function(func_id, &mut ctx).unwrap(); module.finalize_definitions(); } diff --git a/cranelift/module/src/module.rs b/cranelift/module/src/module.rs index 191d468ed7..3d5816973f 100644 --- a/cranelift/module/src/module.rs +++ b/cranelift/module/src/module.rs @@ -554,8 +554,6 @@ pub trait Module { &mut self, func: FuncId, ctx: &mut Context, - trap_sink: &mut dyn binemit::TrapSink, - stack_map_sink: &mut dyn binemit::StackMapSink, ) -> ModuleResult; /// Define a function, taking the function body from the given `bytes`. @@ -656,10 +654,8 @@ impl Module for &mut M { &mut self, func: FuncId, ctx: &mut Context, - trap_sink: &mut dyn binemit::TrapSink, - stack_map_sink: &mut dyn binemit::StackMapSink, ) -> ModuleResult { - (**self).define_function(func, ctx, trap_sink, stack_map_sink) + (**self).define_function(func, ctx) } fn define_function_bytes( diff --git a/cranelift/object/src/backend.rs b/cranelift/object/src/backend.rs index 57c450de49..d3eea47fae 100644 --- a/cranelift/object/src/backend.rs +++ b/cranelift/object/src/backend.rs @@ -3,9 +3,9 @@ use anyhow::anyhow; use cranelift_codegen::entity::SecondaryMap; use cranelift_codegen::isa::TargetIsa; -use cranelift_codegen::{self, ir}; +use cranelift_codegen::{self, ir, MachReloc}; use cranelift_codegen::{ - binemit::{Addend, CodeOffset, Reloc, RelocSink, StackMapSink, TrapSink}, + binemit::{Addend, CodeOffset, Reloc, RelocSink}, CodegenError, }; use cranelift_module::{ @@ -307,20 +307,23 @@ impl Module for ObjectModule { &mut self, func_id: FuncId, ctx: &mut cranelift_codegen::Context, - trap_sink: &mut dyn TrapSink, - stack_map_sink: &mut dyn StackMapSink, ) -> ModuleResult { info!("defining function {}: {}", func_id, ctx.func.display()); let mut code: Vec = Vec::new(); let mut reloc_sink = ObjectRelocSink::default(); - ctx.compile_and_emit( - self.isa(), - &mut code, - &mut reloc_sink, - trap_sink, - stack_map_sink, - )?; + ctx.compile_and_emit(self.isa(), &mut code)?; + + for &MachReloc { + offset, + srcloc, + kind, + ref name, + addend, + } in ctx.mach_compile_result.as_ref().unwrap().buffer.relocs() + { + reloc_sink.reloc_external(offset, srcloc, kind, name, addend); + } self.define_function_bytes(func_id, &code, &reloc_sink.relocs) } diff --git a/cranelift/object/tests/basic.rs b/cranelift/object/tests/basic.rs index a5add02295..8a3764f9b8 100644 --- a/cranelift/object/tests/basic.rs +++ b/cranelift/object/tests/basic.rs @@ -1,9 +1,6 @@ use cranelift_codegen::ir::*; use cranelift_codegen::isa::CallConv; -use cranelift_codegen::{ - binemit::{NullStackMapSink, NullTrapSink}, - settings, -}; +use cranelift_codegen::settings; use cranelift_codegen::{ir::types::I16, Context}; use cranelift_entity::EntityRef; use cranelift_frontend::*; @@ -53,11 +50,7 @@ fn define_simple_function(module: &mut ObjectModule) -> FuncId { bcx.ins().return_(&[]); } - let mut trap_sink = NullTrapSink {}; - let mut stack_map_sink = NullStackMapSink {}; - module - .define_function(func_id, &mut ctx, &mut trap_sink, &mut stack_map_sink) - .unwrap(); + module.define_function(func_id, &mut ctx).unwrap(); func_id } @@ -194,11 +187,7 @@ fn libcall_function() { bcx.ins().return_(&[]); } - let mut trap_sink = NullTrapSink {}; - let mut stack_map_sink = NullStackMapSink {}; - module - .define_function(func_id, &mut ctx, &mut trap_sink, &mut stack_map_sink) - .unwrap(); + module.define_function(func_id, &mut ctx).unwrap(); module.finish(); } diff --git a/cranelift/src/bugpoint.rs b/cranelift/src/bugpoint.rs index e16e91ae40..995e416764 100644 --- a/cranelift/src/bugpoint.rs +++ b/cranelift/src/bugpoint.rs @@ -1,6 +1,5 @@ //! CLI tool to reduce Cranelift IR files crashing during compilation. -use crate::disasm::{PrintRelocs, PrintStackMaps, PrintTraps}; use crate::utils::{parse_sets_and_triple, read_to_string}; use anyhow::{Context as _, Result}; use cranelift_codegen::cursor::{Cursor, FuncCursor}; @@ -1029,17 +1028,9 @@ impl<'a> CrashCheckContext<'a> { std::panic::set_hook(Box::new(|_| {})); // silence panics let res = match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - let mut relocs = PrintRelocs::new(false); - let mut traps = PrintTraps::new(false); - let mut stack_maps = PrintStackMaps::new(false); - - let _ = self.context.compile_and_emit( - self.isa, - &mut self.code_memory, - &mut relocs, - &mut traps, - &mut stack_maps, - ); + let _ = self + .context + .compile_and_emit(self.isa, &mut self.code_memory); })) { Ok(()) => CheckResult::Succeed, Err(err) => CheckResult::Crash(get_panic_string(err)), diff --git a/cranelift/src/compile.rs b/cranelift/src/compile.rs index c1993e8b65..abada4ea34 100644 --- a/cranelift/src/compile.rs +++ b/cranelift/src/compile.rs @@ -3,10 +3,11 @@ use crate::disasm::{print_all, PrintRelocs, PrintStackMaps, PrintTraps}; use crate::utils::{parse_sets_and_triple, read_to_string}; use anyhow::{Context as _, Result}; +use cranelift_codegen::binemit::{RelocSink, StackMapSink, TrapSink}; use cranelift_codegen::print_errors::pretty_error; use cranelift_codegen::settings::FlagsOrIsa; -use cranelift_codegen::timing; use cranelift_codegen::Context; +use cranelift_codegen::{timing, MachReloc, MachStackMap, MachTrap}; use cranelift_reader::{parse_test, ParseOptions}; use std::path::Path; use std::path::PathBuf; @@ -79,9 +80,36 @@ fn handle_module(options: &Options, path: &Path, name: &str, fisa: FlagsOrIsa) - // Compile and encode the result to machine code. context - .compile_and_emit(isa, &mut mem, &mut relocs, &mut traps, &mut stack_maps) + .compile_and_emit(isa, &mut mem) .map_err(|err| anyhow::anyhow!("{}", pretty_error(&context.func, err)))?; - let code_info = context.mach_compile_result.as_ref().unwrap().code_info(); + let result = context.mach_compile_result.as_ref().unwrap(); + let code_info = result.code_info(); + for &MachReloc { + offset, + srcloc, + kind, + ref name, + addend, + } in result.buffer.relocs() + { + relocs.reloc_external(offset, srcloc, kind, name, addend); + } + for &MachTrap { + offset, + srcloc, + code, + } in result.buffer.traps() + { + traps.trap(offset, srcloc, code); + } + for &MachStackMap { + offset_end, + ref stack_map, + .. + } in result.buffer.stack_maps() + { + stack_maps.add_stack_map(offset_end, stack_map.clone()); + } if options.print { println!("{}", context.func.display()); diff --git a/cranelift/src/wasm.rs b/cranelift/src/wasm.rs index bb3b93b098..d377858f5b 100644 --- a/cranelift/src/wasm.rs +++ b/cranelift/src/wasm.rs @@ -10,11 +10,12 @@ use crate::disasm::{print_all, PrintRelocs, PrintStackMaps, PrintTraps}; use crate::utils::parse_sets_and_triple; use anyhow::{Context as _, Result}; +use cranelift_codegen::binemit::{RelocSink, StackMapSink, TrapSink}; use cranelift_codegen::ir::DisplayFunctionAnnotations; use cranelift_codegen::print_errors::{pretty_error, pretty_verifier_error}; use cranelift_codegen::settings::FlagsOrIsa; -use cranelift_codegen::timing; use cranelift_codegen::Context; +use cranelift_codegen::{timing, MachReloc, MachStackMap, MachTrap}; use cranelift_entity::EntityRef; use cranelift_wasm::{translate_module, DummyEnvironment, FuncIndex, ReturnMode}; use std::io::Read; @@ -267,9 +268,36 @@ fn handle_module(options: &Options, path: &Path, name: &str, fisa: FlagsOrIsa) - } } else { context - .compile_and_emit(isa, &mut mem, &mut relocs, &mut traps, &mut stack_maps) + .compile_and_emit(isa, &mut mem) .map_err(|err| anyhow::anyhow!("{}", pretty_error(&context.func, err)))?; - let code_info = context.mach_compile_result.as_ref().unwrap().code_info(); + let result = context.mach_compile_result.as_ref().unwrap(); + let code_info = result.code_info(); + for &MachReloc { + offset, + srcloc, + kind, + ref name, + addend, + } in result.buffer.relocs() + { + relocs.reloc_external(offset, srcloc, kind, name, addend); + } + for &MachTrap { + offset, + srcloc, + code, + } in result.buffer.traps() + { + traps.trap(offset, srcloc, code); + } + for &MachStackMap { + offset_end, + ref stack_map, + .. + } in result.buffer.stack_maps() + { + stack_maps.add_stack_map(offset_end, stack_map.clone()); + } if options.print_size { println!( diff --git a/crates/cranelift/src/compiler.rs b/crates/cranelift/src/compiler.rs index f02fee01d2..278aeabbed 100644 --- a/crates/cranelift/src/compiler.rs +++ b/crates/cranelift/src/compiler.rs @@ -7,12 +7,13 @@ use crate::{ CompiledFunction, FunctionAddressMap, Relocation, RelocationTarget, }; use anyhow::{Context as _, Result}; +use cranelift_codegen::binemit::{RelocSink as _, StackMapSink as _, TrapSink as _}; use cranelift_codegen::ir::{self, ExternalName, InstBuilder, MemFlags}; use cranelift_codegen::isa::TargetIsa; use cranelift_codegen::print_errors::pretty_error; -use cranelift_codegen::settings; -use cranelift_codegen::MachSrcLoc; use cranelift_codegen::{binemit, Context}; +use cranelift_codegen::{settings, MachReloc, MachTrap}; +use cranelift_codegen::{MachSrcLoc, MachStackMap}; use cranelift_entity::{EntityRef, PrimaryMap}; use cranelift_frontend::FunctionBuilder; use cranelift_wasm::{ @@ -170,15 +171,37 @@ impl wasmtime_environ::Compiler for Compiler { let mut trap_sink = TrapSink::new(); let mut stack_map_sink = StackMapSink::default(); context - .compile_and_emit( - isa, - &mut code_buf, - &mut reloc_sink, - &mut trap_sink, - &mut stack_map_sink, - ) + .compile_and_emit(isa, &mut code_buf) .map_err(|error| CompileError::Codegen(pretty_error(&context.func, error)))?; + let result = context.mach_compile_result.as_ref().unwrap(); + for &MachReloc { + offset, + srcloc, + kind, + ref name, + addend, + } in result.buffer.relocs() + { + reloc_sink.reloc_external(offset, srcloc, kind, name, addend); + } + for &MachTrap { + offset, + srcloc, + code, + } in result.buffer.traps() + { + trap_sink.trap(offset, srcloc, code); + } + for &MachStackMap { + offset_end, + ref stack_map, + .. + } in result.buffer.stack_maps() + { + stack_map_sink.add_stack_map(offset_end, stack_map.clone()); + } + let unwind_info = context .create_unwind_info(isa) .map_err(|error| CompileError::Codegen(pretty_error(&context.func, error)))?; @@ -535,18 +558,26 @@ impl Compiler { ) -> Result { let mut code_buf = Vec::new(); let mut reloc_sink = TrampolineRelocSink::default(); - let mut trap_sink = binemit::NullTrapSink {}; - let mut stack_map_sink = binemit::NullStackMapSink {}; context - .compile_and_emit( - isa, - &mut code_buf, - &mut reloc_sink, - &mut trap_sink, - &mut stack_map_sink, - ) + .compile_and_emit(isa, &mut code_buf) .map_err(|error| CompileError::Codegen(pretty_error(&context.func, error)))?; + for &MachReloc { + offset, + srcloc, + kind, + ref name, + addend, + } in context + .mach_compile_result + .as_ref() + .unwrap() + .buffer + .relocs() + { + reloc_sink.reloc_external(offset, srcloc, kind, name, addend); + } + let unwind_info = context .create_unwind_info(isa) .map_err(|error| CompileError::Codegen(pretty_error(&context.func, error)))?; From f0e821b9e014f48be935d2d20a66165c03237ee3 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Tue, 11 Jan 2022 19:03:10 +0100 Subject: [PATCH 10/11] Remove all Sink traits --- cranelift/codegen/src/binemit/memorysink.rs | 79 ------- cranelift/codegen/src/binemit/mod.rs | 4 - cranelift/codegen/src/machinst/buffer.rs | 2 + cranelift/jit/src/backend.rs | 53 +---- cranelift/jit/src/compiled_blob.rs | 11 +- cranelift/module/src/data_context.rs | 20 +- cranelift/module/src/lib.rs | 2 +- cranelift/module/src/module.rs | 19 +- cranelift/object/src/backend.rs | 51 +---- cranelift/src/compile.rs | 42 +--- cranelift/src/disasm.rs | 124 +++++------ cranelift/src/wasm.rs | 54 ++--- crates/cranelift/src/compiler.rs | 231 ++++++-------------- 13 files changed, 188 insertions(+), 504 deletions(-) delete mode 100644 cranelift/codegen/src/binemit/memorysink.rs diff --git a/cranelift/codegen/src/binemit/memorysink.rs b/cranelift/codegen/src/binemit/memorysink.rs deleted file mode 100644 index 149fed67bb..0000000000 --- a/cranelift/codegen/src/binemit/memorysink.rs +++ /dev/null @@ -1,79 +0,0 @@ -//! Code sink that writes binary machine code into contiguous memory. -//! -//! The `CodeSink` trait is the most general way of extracting binary machine code from Cranelift, -//! and it is implemented by things like the `test binemit` file test driver to generate -//! hexadecimal machine code. The `CodeSink` has some undesirable performance properties because of -//! the dual abstraction: `TargetIsa` is a trait object implemented by each supported ISA, so it -//! can't have any generic functions that could be specialized for each `CodeSink` implementation. -//! This results in many virtual function callbacks (one per `put*` call) when -//! `TargetIsa::emit_inst()` is used. -//! -//! The `MemoryCodeSink` type fixes the performance problem because it is a type known to -//! `TargetIsa` so it can specialize its machine code generation for the type. The trade-off is -//! that a `MemoryCodeSink` will always write binary machine code to raw memory. It forwards any -//! relocations to a `RelocSink` trait object. Relocations are less frequent than the -//! `CodeSink::put*` methods, so the performance impact of the virtual callbacks is less severe. -use super::{Addend, CodeOffset, Reloc}; -use crate::binemit::stack_map::StackMap; -use crate::ir::{ExternalName, SourceLoc, TrapCode}; - -/// A trait for receiving relocations for code that is emitted directly into memory. -pub trait RelocSink { - /// Add a relocation referencing an external symbol at the current offset. - fn reloc_external( - &mut self, - _: CodeOffset, - _: SourceLoc, - _: Reloc, - _: &ExternalName, - _: Addend, - ); -} - -/// A trait for receiving trap codes and offsets. -/// -/// If you don't need information about possible traps, you can use the -/// [`NullTrapSink`](NullTrapSink) implementation. -pub trait TrapSink { - /// Add trap information for a specific offset. - fn trap(&mut self, _: CodeOffset, _: SourceLoc, _: TrapCode); -} - -/// A `RelocSink` implementation that does nothing, which is convenient when -/// compiling code that does not relocate anything. -#[derive(Default)] -pub struct NullRelocSink {} - -impl RelocSink for NullRelocSink { - fn reloc_external( - &mut self, - _: CodeOffset, - _: SourceLoc, - _: Reloc, - _: &ExternalName, - _: Addend, - ) { - } -} - -/// A `TrapSink` implementation that does nothing, which is convenient when -/// compiling code that does not rely on trapping semantics. -#[derive(Default)] -pub struct NullTrapSink {} - -impl TrapSink for NullTrapSink { - fn trap(&mut self, _offset: CodeOffset, _srcloc: SourceLoc, _code: TrapCode) {} -} - -/// A trait for emitting stack maps. -pub trait StackMapSink { - /// Output a bitmap of the stack representing the live reference variables at this code offset. - fn add_stack_map(&mut self, _: CodeOffset, _: StackMap); -} - -/// Placeholder StackMapSink that does nothing. -pub struct NullStackMapSink {} - -impl StackMapSink for NullStackMapSink { - fn add_stack_map(&mut self, _: CodeOffset, _: StackMap) {} -} diff --git a/cranelift/codegen/src/binemit/mod.rs b/cranelift/codegen/src/binemit/mod.rs index 0c634b27af..750eaa2a21 100644 --- a/cranelift/codegen/src/binemit/mod.rs +++ b/cranelift/codegen/src/binemit/mod.rs @@ -3,12 +3,8 @@ //! The `binemit` module contains code for translating Cranelift's intermediate representation into //! binary machine code. -mod memorysink; mod stack_map; -pub use self::memorysink::{ - NullRelocSink, NullStackMapSink, NullTrapSink, RelocSink, StackMapSink, TrapSink, -}; pub use self::stack_map::StackMap; use core::fmt; #[cfg(feature = "enable-serde")] diff --git a/cranelift/codegen/src/machinst/buffer.rs b/cranelift/codegen/src/machinst/buffer.rs index 2eeab23f33..7091d880dc 100644 --- a/cranelift/codegen/src/machinst/buffer.rs +++ b/cranelift/codegen/src/machinst/buffer.rs @@ -1499,6 +1499,7 @@ struct MachLabelFixup { } /// A relocation resulting from a compilation. +#[derive(Clone, Debug)] pub struct MachReloc { /// The offset at which the relocation applies, *relative to the /// containing section*. @@ -1514,6 +1515,7 @@ pub struct MachReloc { } /// A trap record resulting from a compilation. +#[derive(Clone, Debug)] pub struct MachTrap { /// The offset at which the trap instruction occurs, *relative to the /// containing section*. diff --git a/cranelift/jit/src/backend.rs b/cranelift/jit/src/backend.rs index 660758b25b..ed45734136 100644 --- a/cranelift/jit/src/backend.rs +++ b/cranelift/jit/src/backend.rs @@ -5,13 +5,13 @@ use cranelift_codegen::isa::TargetIsa; use cranelift_codegen::settings::Configurable; use cranelift_codegen::{self, ir, settings, MachReloc}; use cranelift_codegen::{ - binemit::{Addend, CodeInfo, CodeOffset, Reloc, RelocSink}, + binemit::{CodeInfo, Reloc}, CodegenError, }; use cranelift_entity::SecondaryMap; use cranelift_module::{ DataContext, DataDescription, DataId, FuncId, Init, Linkage, Module, ModuleCompiledFunction, - ModuleDeclarations, ModuleError, ModuleResult, RelocRecord, + ModuleDeclarations, ModuleError, ModuleResult, }; use log::info; use std::collections::HashMap; @@ -671,25 +671,17 @@ impl Module for JITModule { .allocate(size, EXECUTABLE_DATA_ALIGNMENT) .expect("TODO: handle OOM etc."); - let mut reloc_sink = JITRelocSink::default(); unsafe { ctx.emit_to_memory(ptr) }; - for &MachReloc { - offset, - srcloc, - kind, - ref name, - addend, - } in ctx.mach_compile_result.as_ref().unwrap().buffer.relocs() - { - reloc_sink.reloc_external(offset, srcloc, kind, name, addend); - } + let relocs = ctx + .mach_compile_result + .as_ref() + .unwrap() + .buffer + .relocs() + .to_vec(); self.record_function_for_perf(ptr, size, &decl.name); - self.compiled_functions[id] = Some(CompiledBlob { - ptr, - size, - relocs: reloc_sink.relocs, - }); + self.compiled_functions[id] = Some(CompiledBlob { ptr, size, relocs }); if self.isa.flags().is_pic() { self.pending_got_updates.push(GotUpdate { @@ -729,7 +721,7 @@ impl Module for JITModule { &mut self, id: FuncId, bytes: &[u8], - relocs: &[RelocRecord], + relocs: &[MachReloc], ) -> ModuleResult { info!("defining function {} with bytes", id); let total_size: u32 = match bytes.len().try_into() { @@ -896,26 +888,3 @@ fn lookup_with_dlsym(name: &str) -> Option<*const u8> { None } } - -#[derive(Default)] -struct JITRelocSink { - relocs: Vec, -} - -impl RelocSink for JITRelocSink { - fn reloc_external( - &mut self, - offset: CodeOffset, - _srcloc: ir::SourceLoc, - reloc: Reloc, - name: &ir::ExternalName, - addend: Addend, - ) { - self.relocs.push(RelocRecord { - offset, - reloc, - name: name.clone(), - addend, - }); - } -} diff --git a/cranelift/jit/src/compiled_blob.rs b/cranelift/jit/src/compiled_blob.rs index 5af0067422..c5a62701f8 100644 --- a/cranelift/jit/src/compiled_blob.rs +++ b/cranelift/jit/src/compiled_blob.rs @@ -1,13 +1,13 @@ use cranelift_codegen::binemit::Reloc; use cranelift_codegen::ir::ExternalName; -use cranelift_module::RelocRecord; +use cranelift_codegen::MachReloc; use std::convert::TryFrom; #[derive(Clone)] pub(crate) struct CompiledBlob { pub(crate) ptr: *mut u8, pub(crate) size: usize, - pub(crate) relocs: Vec, + pub(crate) relocs: Vec, } impl CompiledBlob { @@ -19,16 +19,17 @@ impl CompiledBlob { ) { use std::ptr::write_unaligned; - for &RelocRecord { - reloc, + for &MachReloc { + kind, offset, + srcloc: _, ref name, addend, } in &self.relocs { debug_assert!((offset as usize) < self.size); let at = unsafe { self.ptr.offset(isize::try_from(offset).unwrap()) }; - match reloc { + match kind { Reloc::Abs4 => { let base = get_address(name); let what = unsafe { base.offset(isize::try_from(addend).unwrap()) }; diff --git a/cranelift/module/src/data_context.rs b/cranelift/module/src/data_context.rs index ce9536b1b0..d872819b9e 100644 --- a/cranelift/module/src/data_context.rs +++ b/cranelift/module/src/data_context.rs @@ -2,14 +2,13 @@ use cranelift_codegen::binemit::{Addend, CodeOffset, Reloc}; use cranelift_codegen::entity::PrimaryMap; -use cranelift_codegen::ir; +use cranelift_codegen::ir::{self, SourceLoc}; +use cranelift_codegen::MachReloc; use std::borrow::ToOwned; use std::boxed::Box; use std::string::String; use std::vec::Vec; -use crate::RelocRecord; - /// This specifies how data is to be initialized. #[derive(PartialEq, Eq, Debug)] pub enum Init { @@ -59,25 +58,24 @@ pub struct DataDescription { impl DataDescription { /// An iterator over all relocations of the data object. - pub fn all_relocs<'a>( - &'a self, - pointer_reloc: Reloc, - ) -> impl Iterator + 'a { + pub fn all_relocs<'a>(&'a self, pointer_reloc: Reloc) -> impl Iterator + 'a { let func_relocs = self .function_relocs .iter() - .map(move |&(offset, id)| RelocRecord { - reloc: pointer_reloc, + .map(move |&(offset, id)| MachReloc { + kind: pointer_reloc, offset, + srcloc: SourceLoc::default(), name: self.function_decls[id].clone(), addend: 0, }); let data_relocs = self .data_relocs .iter() - .map(move |&(offset, id, addend)| RelocRecord { - reloc: pointer_reloc, + .map(move |&(offset, id, addend)| MachReloc { + kind: pointer_reloc, offset, + srcloc: SourceLoc::default(), name: self.data_decls[id].clone(), addend, }); diff --git a/cranelift/module/src/lib.rs b/cranelift/module/src/lib.rs index 21d1c9df27..82562ae5df 100644 --- a/cranelift/module/src/lib.rs +++ b/cranelift/module/src/lib.rs @@ -43,7 +43,7 @@ mod traps; pub use crate::data_context::{DataContext, DataDescription, Init}; pub use crate::module::{ DataId, FuncId, FuncOrDataId, Linkage, Module, ModuleCompiledFunction, ModuleDeclarations, - ModuleError, ModuleResult, RelocRecord, + ModuleError, ModuleResult, }; pub use crate::traps::TrapSite; diff --git a/cranelift/module/src/module.rs b/cranelift/module/src/module.rs index 3d5816973f..336307a3a2 100644 --- a/cranelift/module/src/module.rs +++ b/cranelift/module/src/module.rs @@ -7,8 +7,8 @@ use super::HashMap; use crate::data_context::DataContext; -use cranelift_codegen::binemit; use cranelift_codegen::entity::{entity_impl, PrimaryMap}; +use cranelift_codegen::{binemit, MachReloc}; use cranelift_codegen::{ir, isa, CodegenError, Context}; use std::borrow::ToOwned; use std::string::String; @@ -416,19 +416,6 @@ pub struct ModuleCompiledFunction { pub size: binemit::CodeOffset, } -/// A record of a relocation to perform. -#[derive(Clone)] -pub struct RelocRecord { - /// Where in the generated code this relocation is to be applied. - pub offset: binemit::CodeOffset, - /// The kind of relocation this represents. - pub reloc: binemit::Reloc, - /// What symbol we're relocating against. - pub name: ir::ExternalName, - /// The offset to add to the relocation. - pub addend: binemit::Addend, -} - /// A `Module` is a utility for collecting functions and data objects, and linking them together. pub trait Module { /// Return the `TargetIsa` to compile for. @@ -567,7 +554,7 @@ pub trait Module { &mut self, func: FuncId, bytes: &[u8], - relocs: &[RelocRecord], + relocs: &[MachReloc], ) -> ModuleResult; /// Define a data object, producing the data contents from the given `DataContext`. @@ -662,7 +649,7 @@ impl Module for &mut M { &mut self, func: FuncId, bytes: &[u8], - relocs: &[RelocRecord], + relocs: &[MachReloc], ) -> ModuleResult { (**self).define_function_bytes(func, bytes, relocs) } diff --git a/cranelift/object/src/backend.rs b/cranelift/object/src/backend.rs index d3eea47fae..0998d829ee 100644 --- a/cranelift/object/src/backend.rs +++ b/cranelift/object/src/backend.rs @@ -5,12 +5,12 @@ use cranelift_codegen::entity::SecondaryMap; use cranelift_codegen::isa::TargetIsa; use cranelift_codegen::{self, ir, MachReloc}; use cranelift_codegen::{ - binemit::{Addend, CodeOffset, Reloc, RelocSink}, + binemit::{Addend, CodeOffset, Reloc}, CodegenError, }; use cranelift_module::{ DataContext, DataDescription, DataId, FuncId, Init, Linkage, Module, ModuleCompiledFunction, - ModuleDeclarations, ModuleError, ModuleResult, RelocRecord, + ModuleDeclarations, ModuleError, ModuleResult, }; use log::info; use object::write::{ @@ -310,29 +310,21 @@ impl Module for ObjectModule { ) -> ModuleResult { info!("defining function {}: {}", func_id, ctx.func.display()); let mut code: Vec = Vec::new(); - let mut reloc_sink = ObjectRelocSink::default(); ctx.compile_and_emit(self.isa(), &mut code)?; - for &MachReloc { - offset, - srcloc, - kind, - ref name, - addend, - } in ctx.mach_compile_result.as_ref().unwrap().buffer.relocs() - { - reloc_sink.reloc_external(offset, srcloc, kind, name, addend); - } - - self.define_function_bytes(func_id, &code, &reloc_sink.relocs) + self.define_function_bytes( + func_id, + &code, + ctx.mach_compile_result.as_ref().unwrap().buffer.relocs(), + ) } fn define_function_bytes( &mut self, func_id: FuncId, bytes: &[u8], - relocs: &[RelocRecord], + relocs: &[MachReloc], ) -> ModuleResult { info!("defining function {} with bytes", func_id); let total_size: u32 = match bytes.len().try_into() { @@ -563,9 +555,9 @@ impl ObjectModule { } } - fn process_reloc(&self, record: &RelocRecord) -> ObjectRelocRecord { + fn process_reloc(&self, record: &MachReloc) -> ObjectRelocRecord { let mut addend = record.addend; - let (kind, encoding, size) = match record.reloc { + let (kind, encoding, size) = match record.kind { Reloc::Abs4 => (RelocationKind::Absolute, RelocationEncoding::Generic, 32), Reloc::Abs8 => (RelocationKind::Absolute, RelocationEncoding::Generic, 64), Reloc::X86PCRel4 => (RelocationKind::Relative, RelocationEncoding::Generic, 32), @@ -710,26 +702,3 @@ struct ObjectRelocRecord { size: u8, addend: Addend, } - -#[derive(Default)] -struct ObjectRelocSink { - relocs: Vec, -} - -impl RelocSink for ObjectRelocSink { - fn reloc_external( - &mut self, - offset: CodeOffset, - _srcloc: ir::SourceLoc, - reloc: Reloc, - name: &ir::ExternalName, - addend: Addend, - ) { - self.relocs.push(RelocRecord { - offset, - reloc, - addend, - name: name.clone(), - }) - } -} diff --git a/cranelift/src/compile.rs b/cranelift/src/compile.rs index abada4ea34..be1d1f3d51 100644 --- a/cranelift/src/compile.rs +++ b/cranelift/src/compile.rs @@ -1,13 +1,12 @@ //! CLI tool to read Cranelift IR files and compile them into native code. -use crate::disasm::{print_all, PrintRelocs, PrintStackMaps, PrintTraps}; +use crate::disasm::print_all; use crate::utils::{parse_sets_and_triple, read_to_string}; use anyhow::{Context as _, Result}; -use cranelift_codegen::binemit::{RelocSink, StackMapSink, TrapSink}; use cranelift_codegen::print_errors::pretty_error; use cranelift_codegen::settings::FlagsOrIsa; +use cranelift_codegen::timing; use cranelift_codegen::Context; -use cranelift_codegen::{timing, MachReloc, MachStackMap, MachTrap}; use cranelift_reader::{parse_test, ParseOptions}; use std::path::Path; use std::path::PathBuf; @@ -69,10 +68,6 @@ fn handle_module(options: &Options, path: &Path, name: &str, fisa: FlagsOrIsa) - }; for (func, _) in test_file.functions { - let mut relocs = PrintRelocs::new(options.print); - let mut traps = PrintTraps::new(options.print); - let mut stack_maps = PrintStackMaps::new(options.print); - if let Some(isa) = isa { let mut context = Context::new(); context.func = func; @@ -84,32 +79,6 @@ fn handle_module(options: &Options, path: &Path, name: &str, fisa: FlagsOrIsa) - .map_err(|err| anyhow::anyhow!("{}", pretty_error(&context.func, err)))?; let result = context.mach_compile_result.as_ref().unwrap(); let code_info = result.code_info(); - for &MachReloc { - offset, - srcloc, - kind, - ref name, - addend, - } in result.buffer.relocs() - { - relocs.reloc_external(offset, srcloc, kind, name, addend); - } - for &MachTrap { - offset, - srcloc, - code, - } in result.buffer.traps() - { - traps.trap(offset, srcloc, code); - } - for &MachStackMap { - offset_end, - ref stack_map, - .. - } in result.buffer.stack_maps() - { - stack_maps.add_stack_map(offset_end, stack_map.clone()); - } if options.print { println!("{}", context.func.display()); @@ -120,9 +89,10 @@ fn handle_module(options: &Options, path: &Path, name: &str, fisa: FlagsOrIsa) - isa, &mem, code_info.total_size, - &relocs, - &traps, - &stack_maps, + options.print, + result.buffer.relocs(), + result.buffer.traps(), + result.buffer.stack_maps(), )?; } } diff --git a/cranelift/src/disasm.rs b/cranelift/src/disasm.rs index 7a108c5d80..174fb88651 100644 --- a/cranelift/src/disasm.rs +++ b/cranelift/src/disasm.rs @@ -1,85 +1,53 @@ use anyhow::Result; use cfg_if::cfg_if; use cranelift_codegen::isa::TargetIsa; -use cranelift_codegen::{binemit, ir}; +use cranelift_codegen::{MachReloc, MachStackMap, MachTrap}; use std::fmt::Write; -pub struct PrintRelocs { - pub flag_print: bool, - pub text: String, -} - -impl PrintRelocs { - pub fn new(flag_print: bool) -> Self { - Self { - flag_print, - text: String::new(), - } +pub fn print_relocs(relocs: &[MachReloc]) -> String { + let mut text = String::new(); + for &MachReloc { + kind, + offset, + srcloc: _, + ref name, + addend, + } in relocs + { + writeln!( + text, + "reloc_external: {} {} {} at {}", + kind, name, addend, offset + ) + .unwrap(); } + text } -impl binemit::RelocSink for PrintRelocs { - fn reloc_external( - &mut self, - where_: binemit::CodeOffset, - _srcloc: ir::SourceLoc, - r: binemit::Reloc, - name: &ir::ExternalName, - addend: binemit::Addend, - ) { - if self.flag_print { - writeln!( - &mut self.text, - "reloc_external: {} {} {} at {}", - r, name, addend, where_ - ) - .unwrap(); - } +pub fn print_traps(traps: &[MachTrap]) -> String { + let mut text = String::new(); + for &MachTrap { + offset, + srcloc: _, + code, + } in traps + { + writeln!(text, "trap: {} at {}", code, offset).unwrap(); } + text } -pub struct PrintTraps { - pub flag_print: bool, - pub text: String, -} - -impl PrintTraps { - pub fn new(flag_print: bool) -> Self { - Self { - flag_print, - text: String::new(), - } - } -} - -impl binemit::TrapSink for PrintTraps { - fn trap(&mut self, offset: binemit::CodeOffset, _srcloc: ir::SourceLoc, code: ir::TrapCode) { - if self.flag_print { - writeln!(&mut self.text, "trap: {} at {}", code, offset).unwrap(); - } - } -} - -pub struct PrintStackMaps { - pub flag_print: bool, - pub text: String, -} - -impl PrintStackMaps { - pub fn new(flag_print: bool) -> Self { - Self { - flag_print, - text: String::new(), - } - } -} - -impl binemit::StackMapSink for PrintStackMaps { - fn add_stack_map(&mut self, offset: binemit::CodeOffset, _: binemit::StackMap) { - if self.flag_print { - writeln!(&mut self.text, "add_stack_map at {}", offset).unwrap(); - } +pub fn print_stack_maps(traps: &[MachStackMap]) -> String { + let mut text = String::new(); + for &MachStackMap { + offset, + offset_end: _, + stack_map: _, + } in traps + { + writeln!(text, "add_stack_map at {}", offset).unwrap(); } + text } cfg_if! { @@ -193,13 +161,21 @@ pub fn print_all( isa: &dyn TargetIsa, mem: &[u8], code_size: u32, - relocs: &PrintRelocs, - traps: &PrintTraps, - stack_maps: &PrintStackMaps, + print: bool, + relocs: &[MachReloc], + traps: &[MachTrap], + stack_maps: &[MachStackMap], ) -> Result<()> { print_bytes(&mem); print_disassembly(isa, &mem[0..code_size as usize])?; - println!("\n{}\n{}\n{}", &relocs.text, &traps.text, &stack_maps.text); + if print { + println!( + "\n{}\n{}\n{}", + print_relocs(relocs), + print_traps(traps), + print_stack_maps(stack_maps) + ); + } Ok(()) } diff --git a/cranelift/src/wasm.rs b/cranelift/src/wasm.rs index d377858f5b..4fd83992b7 100644 --- a/cranelift/src/wasm.rs +++ b/cranelift/src/wasm.rs @@ -7,15 +7,14 @@ allow(clippy::too_many_arguments, clippy::cognitive_complexity) )] -use crate::disasm::{print_all, PrintRelocs, PrintStackMaps, PrintTraps}; +use crate::disasm::print_all; use crate::utils::parse_sets_and_triple; use anyhow::{Context as _, Result}; -use cranelift_codegen::binemit::{RelocSink, StackMapSink, TrapSink}; use cranelift_codegen::ir::DisplayFunctionAnnotations; use cranelift_codegen::print_errors::{pretty_error, pretty_verifier_error}; use cranelift_codegen::settings::FlagsOrIsa; +use cranelift_codegen::timing; use cranelift_codegen::Context; -use cranelift_codegen::{timing, MachReloc, MachStackMap, MachTrap}; use cranelift_entity::EntityRef; use cranelift_wasm::{translate_module, DummyEnvironment, FuncIndex, ReturnMode}; use std::io::Read; @@ -259,45 +258,17 @@ fn handle_module(options: &Options, path: &Path, name: &str, fisa: FlagsOrIsa) - let mut saved_size = None; let func_index = num_func_imports + def_index.index(); let mut mem = vec![]; - let mut relocs = PrintRelocs::new(options.print); - let mut traps = PrintTraps::new(options.print); - let mut stack_maps = PrintStackMaps::new(options.print); - if options.check_translation { + let (relocs, traps, stack_maps) = if options.check_translation { if let Err(errors) = context.verify(fisa) { anyhow::bail!("{}", pretty_verifier_error(&context.func, None, errors)); } + (vec![], vec![], vec![]) } else { context .compile_and_emit(isa, &mut mem) .map_err(|err| anyhow::anyhow!("{}", pretty_error(&context.func, err)))?; let result = context.mach_compile_result.as_ref().unwrap(); let code_info = result.code_info(); - for &MachReloc { - offset, - srcloc, - kind, - ref name, - addend, - } in result.buffer.relocs() - { - relocs.reloc_external(offset, srcloc, kind, name, addend); - } - for &MachTrap { - offset, - srcloc, - code, - } in result.buffer.traps() - { - traps.trap(offset, srcloc, code); - } - for &MachStackMap { - offset_end, - ref stack_map, - .. - } in result.buffer.stack_maps() - { - stack_maps.add_stack_map(offset_end, stack_map.clone()); - } if options.print_size { println!( @@ -315,7 +286,12 @@ fn handle_module(options: &Options, path: &Path, name: &str, fisa: FlagsOrIsa) - if options.disasm { saved_size = Some(code_info.total_size); } - } + ( + result.buffer.relocs().to_vec(), + result.buffer.traps().to_vec(), + result.buffer.stack_maps().to_vec(), + ) + }; if options.print { vprintln!(options.verbose, ""); @@ -351,7 +327,15 @@ fn handle_module(options: &Options, path: &Path, name: &str, fisa: FlagsOrIsa) - } if let Some(total_size) = saved_size { - print_all(isa, &mem, total_size, &relocs, &traps, &stack_maps)?; + print_all( + isa, + &mem, + total_size, + options.print, + &relocs, + &traps, + &stack_maps, + )?; } context.clear(); diff --git a/crates/cranelift/src/compiler.rs b/crates/cranelift/src/compiler.rs index 278aeabbed..5adcce21e2 100644 --- a/crates/cranelift/src/compiler.rs +++ b/crates/cranelift/src/compiler.rs @@ -7,11 +7,10 @@ use crate::{ CompiledFunction, FunctionAddressMap, Relocation, RelocationTarget, }; use anyhow::{Context as _, Result}; -use cranelift_codegen::binemit::{RelocSink as _, StackMapSink as _, TrapSink as _}; use cranelift_codegen::ir::{self, ExternalName, InstBuilder, MemFlags}; use cranelift_codegen::isa::TargetIsa; use cranelift_codegen::print_errors::pretty_error; -use cranelift_codegen::{binemit, Context}; +use cranelift_codegen::Context; use cranelift_codegen::{settings, MachReloc, MachTrap}; use cranelift_codegen::{MachSrcLoc, MachStackMap}; use cranelift_entity::{EntityRef, PrimaryMap}; @@ -167,40 +166,85 @@ impl wasmtime_environ::Compiler for Compiler { self.save_translator(func_translator); let mut code_buf: Vec = Vec::new(); - let mut reloc_sink = RelocSink::new(); - let mut trap_sink = TrapSink::new(); - let mut stack_map_sink = StackMapSink::default(); context .compile_and_emit(isa, &mut code_buf) .map_err(|error| CompileError::Codegen(pretty_error(&context.func, error)))?; let result = context.mach_compile_result.as_ref().unwrap(); + + let mut func_relocs = Vec::new(); for &MachReloc { offset, - srcloc, + srcloc: _, kind, ref name, addend, } in result.buffer.relocs() { - reloc_sink.reloc_external(offset, srcloc, kind, name, addend); + let reloc_target = if let ExternalName::User { namespace, index } = *name { + debug_assert_eq!(namespace, 0); + RelocationTarget::UserFunc(FuncIndex::from_u32(index)) + } else if let ExternalName::LibCall(libcall) = *name { + RelocationTarget::LibCall(libcall) + } else { + panic!("unrecognized external name") + }; + func_relocs.push(Relocation { + reloc: kind, + reloc_target, + offset, + addend, + }); } + + let mut traps = Vec::new(); for &MachTrap { offset, - srcloc, + srcloc: _, code, } in result.buffer.traps() { - trap_sink.trap(offset, srcloc, code); + traps.push(TrapInformation { + code_offset: offset, + trap_code: match code { + ir::TrapCode::StackOverflow => TrapCode::StackOverflow, + ir::TrapCode::HeapOutOfBounds => TrapCode::HeapOutOfBounds, + ir::TrapCode::HeapMisaligned => TrapCode::HeapMisaligned, + ir::TrapCode::TableOutOfBounds => TrapCode::TableOutOfBounds, + ir::TrapCode::IndirectCallToNull => TrapCode::IndirectCallToNull, + ir::TrapCode::BadSignature => TrapCode::BadSignature, + ir::TrapCode::IntegerOverflow => TrapCode::IntegerOverflow, + ir::TrapCode::IntegerDivisionByZero => TrapCode::IntegerDivisionByZero, + ir::TrapCode::BadConversionToInteger => TrapCode::BadConversionToInteger, + ir::TrapCode::UnreachableCodeReached => TrapCode::UnreachableCodeReached, + ir::TrapCode::Interrupt => TrapCode::Interrupt, + + // these should never be emitted by wasmtime-cranelift + ir::TrapCode::User(_) => unreachable!(), + }, + }); } + + // This is converting from Cranelift's representation of a stack map to + // Wasmtime's representation. They happen to align today but that may + // not always be true in the future. + let mut stack_maps = Vec::new(); for &MachStackMap { offset_end, ref stack_map, .. } in result.buffer.stack_maps() { - stack_map_sink.add_stack_map(offset_end, stack_map.clone()); + let stack_map = wasmtime_environ::StackMap::new( + stack_map.mapped_words(), + stack_map.as_slice().iter().map(|a| a.0), + ); + stack_maps.push(StackMapInformation { + code_offset: offset_end, + stack_map, + }); } + stack_maps.sort_unstable_by_key(|info| info.code_offset); let unwind_info = context .create_unwind_info(isa) @@ -229,14 +273,14 @@ impl wasmtime_environ::Compiler for Compiler { let length = u32::try_from(code_buf.len()).unwrap(); Ok(Box::new(CompiledFunction { body: code_buf, - relocations: reloc_sink.func_relocs, + relocations: func_relocs, value_labels_ranges: ranges.unwrap_or(Default::default()), stack_slots: context.func.stack_slots, unwind_info, - traps: trap_sink.traps, + traps, info: FunctionInfo { start_srcloc: address_transform.start_srcloc, - stack_maps: stack_map_sink.finish(), + stack_maps, start: 0, length, }, @@ -557,14 +601,14 @@ impl Compiler { isa: &dyn TargetIsa, ) -> Result { let mut code_buf = Vec::new(); - let mut reloc_sink = TrampolineRelocSink::default(); + let mut relocs = Vec::new(); context .compile_and_emit(isa, &mut code_buf) .map_err(|error| CompileError::Codegen(pretty_error(&context.func, error)))?; for &MachReloc { offset, - srcloc, + srcloc: _, kind, ref name, addend, @@ -575,7 +619,17 @@ impl Compiler { .buffer .relocs() { - reloc_sink.reloc_external(offset, srcloc, kind, name, addend); + let reloc_target = if let ir::ExternalName::LibCall(libcall) = *name { + RelocationTarget::LibCall(libcall) + } else { + panic!("unrecognized external name") + }; + relocs.push(Relocation { + reloc: kind, + reloc_target, + offset, + addend, + }); } let unwind_info = context @@ -585,7 +639,7 @@ impl Compiler { Ok(CompiledFunction { body: code_buf, unwind_info, - relocations: reloc_sink.relocs, + relocations: relocs, stack_slots: Default::default(), value_labels_ranges: Default::default(), info: Default::default(), @@ -657,146 +711,3 @@ fn collect_address_maps( } } } - -/// Implementation of a relocation sink that just saves all the information for later -struct RelocSink { - /// Relocations recorded for the function. - func_relocs: Vec, -} - -impl binemit::RelocSink for RelocSink { - fn reloc_external( - &mut self, - offset: binemit::CodeOffset, - _srcloc: ir::SourceLoc, - reloc: binemit::Reloc, - name: &ExternalName, - addend: binemit::Addend, - ) { - let reloc_target = if let ExternalName::User { namespace, index } = *name { - debug_assert_eq!(namespace, 0); - RelocationTarget::UserFunc(FuncIndex::from_u32(index)) - } else if let ExternalName::LibCall(libcall) = *name { - RelocationTarget::LibCall(libcall) - } else { - panic!("unrecognized external name") - }; - self.func_relocs.push(Relocation { - reloc, - reloc_target, - offset, - addend, - }); - } -} - -impl RelocSink { - /// Return a new `RelocSink` instance. - fn new() -> Self { - Self { - func_relocs: Vec::new(), - } - } -} - -/// Implementation of a trap sink that simply stores all trap info in-memory -#[derive(Default)] -struct TrapSink { - /// The in-memory vector of trap info - traps: Vec, -} - -impl TrapSink { - /// Create a new `TrapSink` - fn new() -> Self { - Self::default() - } -} - -impl binemit::TrapSink for TrapSink { - fn trap( - &mut self, - code_offset: binemit::CodeOffset, - _source_loc: ir::SourceLoc, - trap_code: ir::TrapCode, - ) { - self.traps.push(TrapInformation { - code_offset, - trap_code: match trap_code { - ir::TrapCode::StackOverflow => TrapCode::StackOverflow, - ir::TrapCode::HeapOutOfBounds => TrapCode::HeapOutOfBounds, - ir::TrapCode::HeapMisaligned => TrapCode::HeapMisaligned, - ir::TrapCode::TableOutOfBounds => TrapCode::TableOutOfBounds, - ir::TrapCode::IndirectCallToNull => TrapCode::IndirectCallToNull, - ir::TrapCode::BadSignature => TrapCode::BadSignature, - ir::TrapCode::IntegerOverflow => TrapCode::IntegerOverflow, - ir::TrapCode::IntegerDivisionByZero => TrapCode::IntegerDivisionByZero, - ir::TrapCode::BadConversionToInteger => TrapCode::BadConversionToInteger, - ir::TrapCode::UnreachableCodeReached => TrapCode::UnreachableCodeReached, - ir::TrapCode::Interrupt => TrapCode::Interrupt, - - // these should never be emitted by wasmtime-cranelift - ir::TrapCode::User(_) => unreachable!(), - }, - }); - } -} - -#[derive(Default)] -struct StackMapSink { - infos: Vec, -} - -impl binemit::StackMapSink for StackMapSink { - fn add_stack_map(&mut self, code_offset: binemit::CodeOffset, stack_map: binemit::StackMap) { - // This is converting from Cranelift's representation of a stack map to - // Wasmtime's representation. They happen to align today but that may - // not always be true in the future. - let stack_map = wasmtime_environ::StackMap::new( - stack_map.mapped_words(), - stack_map.as_slice().iter().map(|a| a.0), - ); - self.infos.push(StackMapInformation { - code_offset, - stack_map, - }); - } -} - -impl StackMapSink { - fn finish(mut self) -> Vec { - self.infos.sort_unstable_by_key(|info| info.code_offset); - self.infos - } -} - -/// We don't expect trampoline compilation to produce many relocations, so -/// this `RelocSink` just asserts that it doesn't recieve most of them, but -/// handles libcall ones. -#[derive(Default)] -struct TrampolineRelocSink { - relocs: Vec, -} - -impl binemit::RelocSink for TrampolineRelocSink { - fn reloc_external( - &mut self, - offset: binemit::CodeOffset, - _srcloc: ir::SourceLoc, - reloc: binemit::Reloc, - name: &ir::ExternalName, - addend: binemit::Addend, - ) { - let reloc_target = if let ir::ExternalName::LibCall(libcall) = *name { - RelocationTarget::LibCall(libcall) - } else { - panic!("unrecognized external name") - }; - self.relocs.push(Relocation { - reloc, - reloc_target, - offset, - addend, - }); - } -} From 17021bc77ae0ac9486eac8476b9a0ebca8ece715 Mon Sep 17 00:00:00 2001 From: bjorn3 Date: Wed, 12 Jan 2022 17:19:34 +0100 Subject: [PATCH 11/11] Extract helper functions --- crates/cranelift/src/compiler.rs | 159 +++++++++++++++++-------------- 1 file changed, 88 insertions(+), 71 deletions(-) diff --git a/crates/cranelift/src/compiler.rs b/crates/cranelift/src/compiler.rs index 5adcce21e2..05da81ff50 100644 --- a/crates/cranelift/src/compiler.rs +++ b/crates/cranelift/src/compiler.rs @@ -172,79 +172,21 @@ impl wasmtime_environ::Compiler for Compiler { let result = context.mach_compile_result.as_ref().unwrap(); - let mut func_relocs = Vec::new(); - for &MachReloc { - offset, - srcloc: _, - kind, - ref name, - addend, - } in result.buffer.relocs() - { - let reloc_target = if let ExternalName::User { namespace, index } = *name { - debug_assert_eq!(namespace, 0); - RelocationTarget::UserFunc(FuncIndex::from_u32(index)) - } else if let ExternalName::LibCall(libcall) = *name { - RelocationTarget::LibCall(libcall) - } else { - panic!("unrecognized external name") - }; - func_relocs.push(Relocation { - reloc: kind, - reloc_target, - offset, - addend, - }); - } + let func_relocs = result + .buffer + .relocs() + .into_iter() + .map(mach_reloc_to_reloc) + .collect::>(); - let mut traps = Vec::new(); - for &MachTrap { - offset, - srcloc: _, - code, - } in result.buffer.traps() - { - traps.push(TrapInformation { - code_offset: offset, - trap_code: match code { - ir::TrapCode::StackOverflow => TrapCode::StackOverflow, - ir::TrapCode::HeapOutOfBounds => TrapCode::HeapOutOfBounds, - ir::TrapCode::HeapMisaligned => TrapCode::HeapMisaligned, - ir::TrapCode::TableOutOfBounds => TrapCode::TableOutOfBounds, - ir::TrapCode::IndirectCallToNull => TrapCode::IndirectCallToNull, - ir::TrapCode::BadSignature => TrapCode::BadSignature, - ir::TrapCode::IntegerOverflow => TrapCode::IntegerOverflow, - ir::TrapCode::IntegerDivisionByZero => TrapCode::IntegerDivisionByZero, - ir::TrapCode::BadConversionToInteger => TrapCode::BadConversionToInteger, - ir::TrapCode::UnreachableCodeReached => TrapCode::UnreachableCodeReached, - ir::TrapCode::Interrupt => TrapCode::Interrupt, + let traps = result + .buffer + .traps() + .into_iter() + .map(mach_trap_to_trap) + .collect::>(); - // these should never be emitted by wasmtime-cranelift - ir::TrapCode::User(_) => unreachable!(), - }, - }); - } - - // This is converting from Cranelift's representation of a stack map to - // Wasmtime's representation. They happen to align today but that may - // not always be true in the future. - let mut stack_maps = Vec::new(); - for &MachStackMap { - offset_end, - ref stack_map, - .. - } in result.buffer.stack_maps() - { - let stack_map = wasmtime_environ::StackMap::new( - stack_map.mapped_words(), - stack_map.as_slice().iter().map(|a| a.0), - ); - stack_maps.push(StackMapInformation { - code_offset: offset_end, - stack_map, - }); - } - stack_maps.sort_unstable_by_key(|info| info.code_offset); + let stack_maps = mach_stack_maps_to_stack_maps(result.buffer.stack_maps()); let unwind_info = context .create_unwind_info(isa) @@ -711,3 +653,78 @@ fn collect_address_maps( } } } + +fn mach_reloc_to_reloc(reloc: &MachReloc) -> Relocation { + let &MachReloc { + offset, + srcloc: _, + kind, + ref name, + addend, + } = reloc; + let reloc_target = if let ExternalName::User { namespace, index } = *name { + debug_assert_eq!(namespace, 0); + RelocationTarget::UserFunc(FuncIndex::from_u32(index)) + } else if let ExternalName::LibCall(libcall) = *name { + RelocationTarget::LibCall(libcall) + } else { + panic!("unrecognized external name") + }; + Relocation { + reloc: kind, + reloc_target, + offset, + addend, + } +} + +fn mach_trap_to_trap(trap: &MachTrap) -> TrapInformation { + let &MachTrap { + offset, + srcloc: _, + code, + } = trap; + TrapInformation { + code_offset: offset, + trap_code: match code { + ir::TrapCode::StackOverflow => TrapCode::StackOverflow, + ir::TrapCode::HeapOutOfBounds => TrapCode::HeapOutOfBounds, + ir::TrapCode::HeapMisaligned => TrapCode::HeapMisaligned, + ir::TrapCode::TableOutOfBounds => TrapCode::TableOutOfBounds, + ir::TrapCode::IndirectCallToNull => TrapCode::IndirectCallToNull, + ir::TrapCode::BadSignature => TrapCode::BadSignature, + ir::TrapCode::IntegerOverflow => TrapCode::IntegerOverflow, + ir::TrapCode::IntegerDivisionByZero => TrapCode::IntegerDivisionByZero, + ir::TrapCode::BadConversionToInteger => TrapCode::BadConversionToInteger, + ir::TrapCode::UnreachableCodeReached => TrapCode::UnreachableCodeReached, + ir::TrapCode::Interrupt => TrapCode::Interrupt, + + // these should never be emitted by wasmtime-cranelift + ir::TrapCode::User(_) => unreachable!(), + }, + } +} + +fn mach_stack_maps_to_stack_maps(mach_stack_maps: &[MachStackMap]) -> Vec { + // This is converting from Cranelift's representation of a stack map to + // Wasmtime's representation. They happen to align today but that may + // not always be true in the future. + let mut stack_maps = Vec::new(); + for &MachStackMap { + offset_end, + ref stack_map, + .. + } in mach_stack_maps + { + let stack_map = wasmtime_environ::StackMap::new( + stack_map.mapped_words(), + stack_map.as_slice().iter().map(|a| a.0), + ); + stack_maps.push(StackMapInformation { + code_offset: offset_end, + stack_map, + }); + } + stack_maps.sort_unstable_by_key(|info| info.code_offset); + stack_maps +}