Address code review feedback.

This commit addresses code review feedback:

* Remove unnecessary unsafe code.
* Emit the unwind information always as little endian.
* Fix comments.

A dependency from cranelift-codegen to the byteorder crate was added.
The byteorder crate is a no-dependencies crate with a reasonable
abstraction for writing binary data for a specific endianness.

cranelift-codegen/Cargo.toml

@@ -23,6 +23,7 @@ target-lexicon = "0.8.1"
 log = { version = "0.4.6", default-features = false }
 serde = { version = "1.0.94", features = ["derive"], optional = true }
 smallvec = { version = "0.6.10" }
+byteorder = "1.3.2"
 # It is a goal of the cranelift-codegen crate to have minimal external dependencies.
 # Please don't add any unless they are essential to the task of creating binary
 # machine code. Integration tests that need external dependencies can be

cranelift-codegen/src/isa/x86/abi.rs

@@ -665,6 +665,6 @@ pub fn emit_unwind_info(func: &ir::Function, isa: &dyn TargetIsa, mem: &mut Vec<
     // Assumption: RBP is being used as the frame pointer
     // In the future, Windows fastcall codegen should usually omit the frame pointer
     if let Some(info) = UnwindInfo::try_from_func(func, isa, Some(RU::rbp.into())) {
-        info.emit(mem);
+        info.emit(mem).expect("failed to emit unwind information");
     }
 }

cranelift-codegen/src/isa/x86/unwind.rs

@@ -1,45 +1,21 @@
 //! Unwind information for x64 Windows.
+
 use super::registers::RU;
 use crate::ir::{Function, InstructionData, Opcode};
 use crate::isa::{CallConv, RegUnit, TargetIsa};
 use alloc::vec::Vec;
+use byteorder::{LittleEndian, WriteBytesExt};
 
-// Maximum (inclusive) size of a "small" stack allocation
+/// Maximum (inclusive) size of a "small" stack allocation
 const SMALL_ALLOC_MAX_SIZE: u32 = 128;
-// Maximum (inclusive) size of a "large" stack allocation that can represented in 16-bits
+/// Maximum (inclusive) size of a "large" stack allocation that can represented in 16-bits
 const LARGE_ALLOC_16BIT_MAX_SIZE: u32 = 524280;
 
-struct MemoryWriter {
-    ptr: *mut u8,
-    offset: usize,
-}
-
-impl MemoryWriter {
-    pub fn write_u8(&mut self, v: u8) {
-        self.write(v);
-    }
-
-    pub fn write_u16(&mut self, v: u16) {
-        self.write(v);
-    }
-
-    pub fn write_u32(&mut self, v: u32) {
-        self.write(v);
-    }
-
-    fn write<T>(&mut self, x: T) {
-        unsafe {
-            #[cfg_attr(feature = "cargo-clippy", allow(clippy::cast_ptr_alignment))]
-            core::ptr::write_unaligned(self.ptr.add(self.offset) as *mut T, x);
-            self.offset += core::mem::size_of::<T>();
-        }
-    }
-}
-
-// The supported unwind codes for the x64 Windows ABI.
-// See: https://docs.microsoft.com/en-us/cpp/build/exception-handling-x64
-// Only what is needed to describe the prologues generated by the Cranelift x86 ISA are represented here.
-// Note: the Cranelift x86 ISA RU enum matches the Windows unwind GPR encoding values.
+/// The supported unwind codes for the x64 Windows ABI.
+///
+/// See: https://docs.microsoft.com/en-us/cpp/build/exception-handling-x64
+/// Only what is needed to describe the prologues generated by the Cranelift x86 ISA are represented here.
+/// Note: the Cranelift x86 ISA RU enum matches the Windows unwind GPR encoding values.
 #[derive(Debug, PartialEq, Eq)]
 enum UnwindCode {
     PushRegister { offset: u8, reg: RegUnit },
@@ -48,7 +24,7 @@ enum UnwindCode {
 }
 
 impl UnwindCode {
-    fn emit(&self, writer: &mut MemoryWriter) {
+    fn emit(&self, mem: &mut Vec<u8>) -> std::io::Result<()> {
         enum UnwindOperation {
             PushNonvolatileRegister,
             LargeStackAlloc,
@@ -58,34 +34,36 @@ impl UnwindCode {
 
         match self {
             Self::PushRegister { offset, reg } => {
-                writer.write_u8(*offset);
-                writer.write_u8(
+                mem.write_u8(*offset)?;
+                mem.write_u8(
                     ((*reg as u8) << 4) | (UnwindOperation::PushNonvolatileRegister as u8),
-                );
+                )?;
             }
             Self::StackAlloc { offset, size } => {
                 // Stack allocations on Windows must be a multiple of 8 and be at least 1 slot
                 assert!(*size >= 8);
                 assert!((*size % 8) == 0);
 
-                writer.write_u8(*offset);
+                mem.write_u8(*offset)?;
                 if *size <= SMALL_ALLOC_MAX_SIZE {
-                    writer.write_u8(
+                    mem.write_u8(
                         ((((*size - 8) / 8) as u8) << 4) | UnwindOperation::SmallStackAlloc as u8,
-                    );
+                    )?;
                 } else if *size <= LARGE_ALLOC_16BIT_MAX_SIZE {
-                    writer.write_u8(UnwindOperation::LargeStackAlloc as u8);
-                    writer.write_u16((*size / 8) as u16);
+                    mem.write_u8(UnwindOperation::LargeStackAlloc as u8)?;
+                    mem.write_u16::<LittleEndian>((*size / 8) as u16)?;
                 } else {
-                    writer.write_u8((1 << 4) | (UnwindOperation::LargeStackAlloc as u8));
-                    writer.write_u32(*size);
+                    mem.write_u8((1 << 4) | (UnwindOperation::LargeStackAlloc as u8))?;
+                    mem.write_u32::<LittleEndian>(*size)?;
                 }
             }
             Self::SetFramePointer { offset, sp_offset } => {
-                writer.write_u8(*offset);
-                writer.write_u8((*sp_offset << 4) | (UnwindOperation::SetFramePointer as u8));
+                mem.write_u8(*offset)?;
+                mem.write_u8((*sp_offset << 4) | (UnwindOperation::SetFramePointer as u8))?;
             }
-        }
+        };
+
+        Ok(())
     }
 
     fn node_count(&self) -> usize {
@@ -104,8 +82,10 @@ impl UnwindCode {
     }
 }
 
-// For information about Windows x64 unwind info, see:
-// https://docs.microsoft.com/en-us/cpp/build/exception-handling-x64
+/// Represents Windows x64 unwind information.
+///
+/// For information about Windows x64 unwind info, see:
+/// https://docs.microsoft.com/en-us/cpp/build/exception-handling-x64
 #[derive(Debug, PartialEq, Eq)]
 pub struct UnwindInfo {
     flags: u8,
@@ -243,7 +223,7 @@ impl UnwindInfo {
             .fold(0, |nodes, c| nodes + c.node_count())
     }
 
-    pub fn emit(&self, mem: &mut Vec<u8>) {
+    pub fn emit(&self, mem: &mut Vec<u8>) -> std::io::Result<()> {
         const UNWIND_INFO_VERSION: u8 = 1;
 
         let size = self.size();
@@ -252,38 +232,35 @@ impl UnwindInfo {
         // Ensure the memory is 32-bit aligned
         assert_eq!(offset % 4, 0);
 
-        mem.resize(offset + size, 0);
-        unsafe {
-            let mut writer = MemoryWriter {
-                ptr: mem.as_mut_ptr().add(offset),
-                offset: 0,
-            };
-            let node_count = self.node_count();
-            assert!(node_count <= 256);
+        mem.reserve(offset + size);
 
-            writer.write_u8((self.flags << 3) | UNWIND_INFO_VERSION);
-            writer.write_u8(self.prologue_size);
-            writer.write_u8(node_count as u8);
+        let node_count = self.node_count();
+        assert!(node_count <= 256);
 
-            if let Some(reg) = self.frame_register {
-                writer.write_u8((self.frame_register_offset << 4) | reg as u8);
-            } else {
-                writer.write_u8(0);
-            }
+        mem.write_u8((self.flags << 3) | UNWIND_INFO_VERSION)?;
+        mem.write_u8(self.prologue_size)?;
+        mem.write_u8(node_count as u8)?;
 
-            // Unwind codes are written in reverse order (prologue offset descending)
-            for code in self.unwind_codes.iter().rev() {
-                code.emit(&mut writer);
-            }
+        if let Some(reg) = self.frame_register {
+            mem.write_u8((self.frame_register_offset << 4) | reg as u8)?;
+        } else {
+            mem.write_u8(0)?;
+        }
 
-            // To keep a 32-bit alignment, emit 2 bytes of padding if there's an odd number of 16-bit nodes
-            if (node_count & 1) == 1 {
-                writer.write_u16(0);
-            }
+        // Unwind codes are written in reverse order (prologue offset descending)
+        for code in self.unwind_codes.iter().rev() {
+            code.emit(mem)?;
+        }
 
-            // Ensure the correct number of bytes was emitted
-            assert_eq!((writer.offset - offset), size);
-        };
+        // To keep a 32-bit alignment, emit 2 bytes of padding if there's an odd number of 16-bit nodes
+        if (node_count & 1) == 1 {
+            mem.write_u16::<LittleEndian>(0)?;
+        }
+
+        // Ensure the correct number of bytes was emitted
+        assert_eq!(mem.len() - offset, size);
+
+        Ok(())
     }
 }
 
@@ -354,7 +331,9 @@ mod tests {
         assert_eq!(unwind.size(), 12);
 
         let mut mem = Vec::new();
-        unwind.emit(&mut mem);
+        unwind
+            .emit(&mut mem)
+            .expect("failed to emit unwind information");
 
         assert_eq!(
             mem,
@@ -418,7 +397,9 @@ mod tests {
         assert_eq!(unwind.size(), 12);
 
         let mut mem = Vec::new();
-        unwind.emit(&mut mem);
+        unwind
+            .emit(&mut mem)
+            .expect("failed to emit unwind information");;
 
         assert_eq!(
             mem,
@@ -482,7 +463,9 @@ mod tests {
         assert_eq!(unwind.size(), 16);
 
         let mut mem = Vec::new();
-        unwind.emit(&mut mem);
+        unwind
+            .emit(&mut mem)
+            .expect("failed to emit unwind information");;
 
         assert_eq!(
             mem,

cranelift-filetests/Cargo.toml

@@ -20,6 +20,7 @@ log = "0.4.6"
 memmap = "0.7.0"
 num_cpus = "1.8.0"
 region = "2.1.2"
+byteorder = "1.3.2"
 
 [features]
 basic-blocks = []