Skip to content

Commit

Permalink
Fix a long standing bug with the way stack frame sizes are calculated…
Browse files Browse the repository at this point in the history
…, it's now no longer needed to have seperate debug and release blocks. Wee!
  • Loading branch information
sciguyryan committed Jan 25, 2024
1 parent b527adf commit 8f713f6
Show file tree
Hide file tree
Showing 6 changed files with 38 additions and 70 deletions.
2 changes: 1 addition & 1 deletion redox-core/src/boot_rom.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
22 changes: 8 additions & 14 deletions redox-core/src/cpu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -709,19 +709,16 @@ 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
// from this point forward.
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 {
Expand All @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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}");
}
Expand Down Expand Up @@ -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}");
}
Expand Down
43 changes: 9 additions & 34 deletions redox-core/src/mem/memory_handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
29 changes: 12 additions & 17 deletions redox-core/src/reg/registers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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),
Expand All @@ -122,7 +119,7 @@ impl From<u8> 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,
Expand All @@ -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",
Expand Down Expand Up @@ -186,7 +181,7 @@ pub struct Registers {
// [ System Registers ] //
eac: RegisterU32,
eip: RegisterU32,
efp: RegisterU32,
ebp: RegisterU32,
esp: RegisterU32,
efl: RegisterU32,
eim: RegisterU32,
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -302,7 +297,7 @@ impl Registers {
| RegisterId::ER8
| RegisterId::EAC
| RegisterId::EIP
| RegisterId::EFP
| RegisterId::EBP
| RegisterId::ESP
| RegisterId::EFL
| RegisterId::EIM
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -371,7 +366,7 @@ impl Registers {
| RegisterId::ER8
| RegisterId::EAC
| RegisterId::EIP
| RegisterId::EFP
| RegisterId::EBP
| RegisterId::ESP
| RegisterId::EFL
| RegisterId::EIM
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion redox-parser/src/asm_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
10 changes: 7 additions & 3 deletions redox-terminal/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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]----------");
Expand Down

0 comments on commit 8f713f6

Please sign in to comment.