From 8f713f61f887b2860b1ffcd546ae7ed75de0eed6 Mon Sep 17 00:00:00 2001 From: Ryan Jones-Ward Date: Thu, 25 Jan 2024 12:34:16 +0000 Subject: [PATCH] Fix a long standing bug with the way stack frame sizes are calculated, it's now no longer needed to have seperate debug and release blocks. Wee! --- redox-core/src/boot_rom.rs | 2 +- redox-core/src/cpu.rs | 22 ++++++-------- redox-core/src/mem/memory_handler.rs | 43 ++++++---------------------- redox-core/src/reg/registers.rs | 29 ++++++++----------- redox-parser/src/asm_parser.rs | 2 +- redox-terminal/src/main.rs | 10 +++++-- 6 files changed, 38 insertions(+), 70 deletions(-) diff --git a/redox-core/src/boot_rom.rs b/redox-core/src/boot_rom.rs index 025d3a2..6965821 100644 --- a/redox-core/src/boot_rom.rs +++ b/redox-core/src/boot_rom.rs @@ -33,7 +33,7 @@ impl BootRom { // Completely disable maskable interrupts. Instruction::MovU32ImmU32Reg(0x0, RegisterId::EIM), // Setup the stack. - Instruction::MovU32ImmU32Reg(mem.stack_segment_end as u32, RegisterId::EFP), + Instruction::MovU32ImmU32Reg(mem.stack_segment_end as u32, RegisterId::EBP), Instruction::MovU32ImmU32Reg(mem.stack_segment_end as u32, RegisterId::ESP), // Setup the segment registers. Instruction::MovU32ImmU32Reg(mem.stack_segment_start as u32, RegisterId::ESS), diff --git a/redox-core/src/cpu.rs b/redox-core/src/cpu.rs index 497f118..e32ea39 100644 --- a/redox-core/src/cpu.rs +++ b/redox-core/src/cpu.rs @@ -709,7 +709,7 @@ impl Cpu { // NOTE - remember that the states need to be popped in the reverse order! // First, get the current frame pointer. - let frame_pointer = self.registers.read_reg_u32_unchecked(&RegisterId::EFP); + let frame_pointer = self.registers.read_reg_u32_unchecked(&RegisterId::EBP); // Next, reset the stack pointer to the address of the frame pointer. // Any stack entries higher in the stack can be disregarded as they are out of scope @@ -717,11 +717,8 @@ impl Cpu { self.write_reg_u32_unchecked(&RegisterId::ESP, frame_pointer); // Next, pop the old stack frame size. - let old_stack_frame_size = mem.pop_u32(); - mem.stack_frame_size = old_stack_frame_size; - - // We will need to keep this for resetting the frame pointer position below. - let stack_frame_size = old_stack_frame_size; + let old_stack_frame_pointer = mem.pop_u32(); + mem.stack_base_pointer = old_stack_frame_pointer as usize; // Next, pop the f32 data registers from the stack. for reg in PRESERVE_REGISTERS_F32_REV { @@ -742,7 +739,7 @@ impl Cpu { // Finally, adjust our frame pointer position to the original frame pointer // address plus the stack frame size. - self.write_reg_u32_unchecked(&RegisterId::EFP, frame_pointer + stack_frame_size); + self.write_reg_u32_unchecked(&RegisterId::EBP, old_stack_frame_pointer); } /// Push the relevant processor register states to the stack. @@ -763,13 +760,10 @@ impl Cpu { } // Next, push the current stack frame size to the stack. - mem.push_u32(mem.stack_frame_size); + mem.push_u32(mem.stack_base_pointer as u32); // Next, update the stack frame pointer to be the current stack pointer location. - self.write_reg_u32_unchecked(&RegisterId::EFP, self.get_stack_pointer()); - - // Finally, reset the stack frame size so we can track the new frame. - mem.stack_frame_size = 0; + self.write_reg_u32_unchecked(&RegisterId::EBP, self.get_stack_pointer()); } /// Perform a hard reset on the CPU. @@ -2284,7 +2278,7 @@ mod tests_cpu { assert_eq!(v.com_bus.mem.pop_u32(), 3); // The stack should now be empty. - assert_eq!(v.com_bus.mem.stack_frame_size, 0); + assert_eq!(v.com_bus.mem.get_stack_frame_size(), 0); } else { panic!("failed to correctly execute test id {id}"); } @@ -2402,7 +2396,7 @@ mod tests_cpu { assert_eq!(v.com_bus.mem.pop_u32(), 3); // The stack should now be empty. - assert_eq!(v.com_bus.mem.stack_frame_size, 0); + assert_eq!(v.com_bus.mem.get_stack_frame_size(), 0); } else { panic!("failed to correctly execute test id {id}"); } diff --git a/redox-core/src/mem/memory_handler.rs b/redox-core/src/mem/memory_handler.rs index c3a8dce..0f1f79d 100644 --- a/redox-core/src/mem/memory_handler.rs +++ b/redox-core/src/mem/memory_handler.rs @@ -78,8 +78,8 @@ pub struct MemoryHandler { /// The current stack pointer. stack_pointer: usize, - /// The size of the current stack frame. - pub stack_frame_size: u32, + /// The base (frame) stack pointer. + pub stack_base_pointer: usize, } impl MemoryHandler { @@ -138,7 +138,7 @@ impl MemoryHandler { stack_segment_start, stack_segment_end, stack_pointer: stack_segment_end, - stack_frame_size: 0, + stack_base_pointer: stack_segment_end, }; // Insert the physical RAM memory-mapped segments that @@ -887,6 +887,11 @@ impl MemoryHandler { .expect("failed to get physical RAM memory segment") } + /// Get the size of the current stack frame. + pub fn get_stack_frame_size(&self) -> usize { + self.stack_base_pointer - self.stack_pointer + } + /// Attempt to read a f32 value from memory. /// /// # Arguments @@ -1064,18 +1069,6 @@ impl MemoryHandler { // Update the stack pointer. self.stack_pointer = value_start_pos + SIZE_OF_F32; - // Update the frame size indicator. - // A little hackery to ensure we don't assert in debug builds in - // instances where the stack size is zero, something that can occur during a subroutine call. - #[cfg(debug_assertions)] - { - self.stack_frame_size = self.stack_frame_size.wrapping_sub(SIZE_OF_F32 as u32); - } - #[cfg(not(debug_assertions))] - { - self.stack_frame_size -= SIZE_OF_F32 as u32; - } - result } @@ -1132,18 +1125,6 @@ impl MemoryHandler { // Update the stack pointer. self.stack_pointer = value_start_pos + SIZE_OF_U32; - // Update the frame size indicator. - // A little hackery to ensure we don't assert in debug builds in - // instances where the stack size is zero, something that can occur during a subroutine call. - #[cfg(debug_assertions)] - { - self.stack_frame_size = self.stack_frame_size.wrapping_sub(SIZE_OF_U32 as u32); - } - #[cfg(not(debug_assertions))] - { - self.stack_frame_size -= SIZE_OF_U32 as u32; - } - result } @@ -1173,9 +1154,6 @@ impl MemoryHandler { // Update the stack pointer. self.stack_pointer = value_start_pos; - - // Update the frame size indicator. - self.stack_frame_size += SIZE_OF_F32 as u32; } /// Attempt to push a [`StackTypeHint`] onto the stack hint list. @@ -1221,9 +1199,6 @@ impl MemoryHandler { // Update the stack pointer. self.stack_pointer = value_start_pos; - - // Update the frame size indicator. - self.stack_frame_size += SIZE_OF_U32 as u32; } /// Attempt to read a [`RegisterId`] from a memory slice. @@ -1359,8 +1334,8 @@ impl MemoryHandler { #[inline] pub fn reset_stack_configuration(&mut self) { self.clear_stack_type_hints(); - self.stack_frame_size = 0; self.stack_pointer = self.stack_segment_end; + self.stack_base_pointer = self.stack_pointer; } /// Write a u32 value into memory. diff --git a/redox-core/src/reg/registers.rs b/redox-core/src/reg/registers.rs index 22124cf..5adae81 100644 --- a/redox-core/src/reg/registers.rs +++ b/redox-core/src/reg/registers.rs @@ -53,8 +53,8 @@ pub enum RegisterId { EAC = 100, /// Instruction pointer register. EIP = 101, - /// Stack frame pointer. - EFP = 102, + /// Stack base pointer - the pointer to the base of the current stack frame. + EBP = 102, /// Stack pointer register. ESP = 103, /// CPU flags register. @@ -86,18 +86,15 @@ impl FromStr for RegisterId { "ER6" => Ok(RegisterId::ER6), "ER7" => Ok(RegisterId::ER7), "ER8" => Ok(RegisterId::ER8), - "FR1" => Ok(RegisterId::FR1), "FR2" => Ok(RegisterId::FR2), - "EAC" => Ok(RegisterId::EAC), "EIP" => Ok(RegisterId::EIP), - "EFP" => Ok(RegisterId::EFP), + "EBP" => Ok(RegisterId::EBP), "ESP" => Ok(RegisterId::ESP), "EFL" => Ok(RegisterId::EFL), "EIM" => Ok(RegisterId::EIM), "IDTR" => Ok(RegisterId::IDTR), - "ESS" => Ok(RegisterId::ESS), "ECS" => Ok(RegisterId::ECS), "EDS" => Ok(RegisterId::EDS), @@ -122,7 +119,7 @@ impl From for RegisterId { 30 => RegisterId::FR2, 100 => RegisterId::EAC, 101 => RegisterId::EIP, - 102 => RegisterId::EFP, + 102 => RegisterId::EBP, 103 => RegisterId::ESP, 104 => RegisterId::EFL, 105 => RegisterId::EIM, @@ -149,14 +146,12 @@ impl Display for RegisterId { RegisterId::FR1 => "FR1", RegisterId::FR2 => "FR2", RegisterId::EAC => "EAC", - RegisterId::EIP => "EIP", - RegisterId::EFP => "EFP", + RegisterId::EBP => "EFP", RegisterId::ESP => "ESP", RegisterId::EFL => "EFL", RegisterId::EIM => "EIM", RegisterId::IDTR => "IDTR", - RegisterId::ESS => "ESS", RegisterId::ECS => "ECS", RegisterId::EDS => "EDS", @@ -186,7 +181,7 @@ pub struct Registers { // [ System Registers ] // eac: RegisterU32, eip: RegisterU32, - efp: RegisterU32, + ebp: RegisterU32, esp: RegisterU32, efl: RegisterU32, eim: RegisterU32, @@ -223,7 +218,7 @@ impl Registers { // [ System Registers ] // eac: RegisterU32::new(RegisterId::EAC, rw, 0), eip: RegisterU32::new(RegisterId::EIP, rw, BOOT_MEMORY_START as u32), - efp: RegisterU32::new(RegisterId::EFP, rpw, 0), + ebp: RegisterU32::new(RegisterId::EBP, rpw, 0), esp: RegisterU32::new(RegisterId::ESP, rpw, 0), efl: RegisterU32::new(RegisterId::EFL, rpw, 0), eim: RegisterU32::new(RegisterId::EIM, rw, 0), @@ -262,7 +257,7 @@ impl Registers { RegisterId::ER8 => u32_vec.push(&self.er8), RegisterId::EAC => u32_vec.push(&self.eac), RegisterId::EIP => u32_vec.push(&self.eip), - RegisterId::EFP => u32_vec.push(&self.efp), + RegisterId::EBP => u32_vec.push(&self.ebp), RegisterId::ESP => u32_vec.push(&self.esp), RegisterId::EFL => u32_vec.push(&self.efl), RegisterId::EIM => u32_vec.push(&self.eim), @@ -302,7 +297,7 @@ impl Registers { | RegisterId::ER8 | RegisterId::EAC | RegisterId::EIP - | RegisterId::EFP + | RegisterId::EBP | RegisterId::ESP | RegisterId::EFL | RegisterId::EIM @@ -335,7 +330,7 @@ impl Registers { RegisterId::ER8 => &self.er8, RegisterId::EAC => &self.eac, RegisterId::EIP => &self.eip, - RegisterId::EFP => &self.efp, + RegisterId::EBP => &self.ebp, RegisterId::ESP => &self.esp, RegisterId::EFL => &self.efl, RegisterId::EIM => &self.eim, @@ -371,7 +366,7 @@ impl Registers { | RegisterId::ER8 | RegisterId::EAC | RegisterId::EIP - | RegisterId::EFP + | RegisterId::EBP | RegisterId::ESP | RegisterId::EFL | RegisterId::EIM @@ -404,7 +399,7 @@ impl Registers { RegisterId::ER8 => &mut self.er8, RegisterId::EAC => &mut self.eac, RegisterId::EIP => &mut self.eip, - RegisterId::EFP => &mut self.efp, + RegisterId::EBP => &mut self.ebp, RegisterId::ESP => &mut self.esp, RegisterId::EFL => &mut self.efl, RegisterId::EIM => &mut self.eim, diff --git a/redox-parser/src/asm_parser.rs b/redox-parser/src/asm_parser.rs index 34f2fd5..172483e 100644 --- a/redox-parser/src/asm_parser.rs +++ b/redox-parser/src/asm_parser.rs @@ -30,7 +30,7 @@ const U32_REGISTERS: [RegisterId; 18] = [ RegisterId::ER8, RegisterId::EAC, RegisterId::EIP, - RegisterId::EFP, + RegisterId::EBP, RegisterId::ESP, RegisterId::EFL, RegisterId::EIM, diff --git a/redox-terminal/src/main.rs b/redox-terminal/src/main.rs index 523a40f..01dac6c 100644 --- a/redox-terminal/src/main.rs +++ b/redox-terminal/src/main.rs @@ -69,12 +69,12 @@ fn main() { let code = "call :LABEL_1"; - let mut parser = AsmParser::new(); + /*let mut parser = AsmParser::new(); parser.parse_code(code); println!("parsed = {:?}", parser.parsed_instructions); println!("labels = {:?}", parser.labeled_instructions); - return; + return;*/ let instructions = &[ // Indicate that we want to make a seeded random number generator. @@ -83,6 +83,7 @@ fn main() { Instruction::OutU32Imm(0xdeadbeef, 0x0), // Read a PRNG from the device. Instruction::InU32Reg(0x0, RegisterId::ER1), + Instruction::PushU32Imm(0xdeadbeef), /*// Write the handler addresses into the IVT. // Handler for 0x00. This is a non-maskable interrupt. // We are essentially replacing the default handler for the division by zero interrupt. @@ -130,7 +131,10 @@ fn main() { println!("Code successfully executed in {elapsed} ns!"); println!(); println!("Machine Mode? {}", vm.cpu.is_machine_mode); - println!("Stack Frame Size: {}", vm.com_bus.mem.stack_frame_size); + println!( + "Stack Frame Size: {}", + vm.com_bus.mem.get_stack_frame_size() + ); println!(); println!("----------[Registers]----------");