diff --git a/conformance-tests/src/pvm_harness.rs b/conformance-tests/src/pvm_harness.rs index 1aed560a..6ce79c66 100644 --- a/conformance-tests/src/pvm_harness.rs +++ b/conformance-tests/src/pvm_harness.rs @@ -1,3 +1,4 @@ +use fr_common::utils::tracing::setup_timed_tracing; use fr_pvm_core::{ interpreter::Interpreter, program::{loader::ProgramLoader, types::program_state::ProgramState}, @@ -213,6 +214,9 @@ impl PVMHarness { } pub fn run_test_case(filename: &str) { + // Config tracing subscriber + setup_timed_tracing(); + // load test case let filename = PathBuf::from(filename); let test_case = PVMHarness::load_test_case(&filename); @@ -247,11 +251,9 @@ pub fn run_test_case(filename: &str) { _ => panic!("Unexpected exit reason"), }; - // Bypass gas_counter for now, since it is not finalized yet // assert_eq!(pvm.state, expected_vm); - // TODO: test gas counter and pc values - // assert_eq!(pvm.state.gas_counter, expected_vm.gas_counter); - // assert_eq!(pvm.state.pc, expected_vm.pc); + // assert_eq!(pvm.state.gas_counter, expected_vm.gas_counter); // FIXME: Skipped due to issues in page-fault test cases + assert_eq!(pvm.state.pc, expected_vm.pc); assert_eq!(pvm.state.regs, expected_vm.regs); assert_eq!(pvm.state.memory, expected_vm.memory); assert_eq!(actual_status, expected_status); diff --git a/pvm/pvm-core/src/interpreter/mod.rs b/pvm/pvm-core/src/interpreter/mod.rs index afe7a5fb..13f929b0 100644 --- a/pvm/pvm-core/src/interpreter/mod.rs +++ b/pvm/pvm-core/src/interpreter/mod.rs @@ -17,6 +17,7 @@ use fr_pvm_types::{ exit_reason::ExitReason, }; +#[derive(Debug)] pub struct SingleStepResult { pub exit_reason: ExitReason, pub state_change: VMStateChange, @@ -111,7 +112,10 @@ impl Interpreter { ) { Ok(post_gas) => post_gas, Err(VMCoreError::InvalidMemZone) => return Ok(ExitReason::Panic), - Err(VMCoreError::PageFault(address)) => return Ok(ExitReason::PageFault(address)), + Err(VMCoreError::PageFault(address)) => { + vm_state.pc = 0; // TODO: Revisit (test vectors assume page-fault resets pc value but not in GP) + return Ok(ExitReason::PageFault(address)); + } Err(e) => return Err(e), }; if post_gas < 0 { @@ -131,7 +135,7 @@ impl Interpreter { ExitReason::Continue => continue, termination @ (ExitReason::Panic | ExitReason::RegularHalt) => { // Reset the program counter - vm_state.pc = 0; + // vm_state.pc = 0; // TODO: Revisit (test vectors assume panic/halt doesn't reset pc value but not in GP) return Ok(termination); } other => return Ok(other), diff --git a/pvm/pvm-core/src/program/instruction/set.rs b/pvm/pvm-core/src/program/instruction/set.rs index ea5091dc..60b92969 100644 --- a/pvm/pvm-core/src/program/instruction/set.rs +++ b/pvm/pvm-core/src/program/instruction/set.rs @@ -58,7 +58,7 @@ impl InstructionSet { program_state: &ProgramState, target: MemAddress, condition: bool, - ) -> Result<(ExitReason, MemAddress), VMCoreError> { + ) -> Result<(ExitReason, RegValue), VMCoreError> { match ( condition, program_state @@ -67,13 +67,10 @@ impl InstructionSet { ) { (false, _) => Ok(( ExitReason::Continue, - reg_to_mem_address(Interpreter::next_pc(vm_state, program_state)), - )), - (true, true) => Ok((ExitReason::Continue, target)), - (true, false) => Ok(( - ExitReason::Panic, - reg_to_mem_address(Interpreter::next_pc(vm_state, program_state)), + Interpreter::next_pc(vm_state, program_state), )), + (true, true) => Ok((ExitReason::Continue, target as RegValue)), + (true, false) => Ok((ExitReason::Panic, vm_state.pc)), } } @@ -87,24 +84,18 @@ impl InstructionSet { vm_state: &VMState, program_state: &ProgramState, a: usize, - ) -> Result<(ExitReason, MemAddress), VMCoreError> { + ) -> Result<(ExitReason, RegValue), VMCoreError> { const SPECIAL_HALT_VALUE: usize = (1 << 32) - (1 << 16); if a == SPECIAL_HALT_VALUE { - return Ok(( - ExitReason::RegularHalt, - reg_to_mem_address(Interpreter::next_pc(vm_state, program_state)), - )); + return Ok((ExitReason::RegularHalt, vm_state.pc)); } let jump_table_len = program_state.jump_table.len(); // Check if the argument `a` is valid and compute the target if a == 0 || a > jump_table_len * JUMP_ALIGNMENT || a % JUMP_ALIGNMENT != 0 { - return Ok(( - ExitReason::Panic, - reg_to_mem_address(Interpreter::next_pc(vm_state, program_state)), - )); + return Ok((ExitReason::Panic, vm_state.pc)); } let aligned_index = (a / JUMP_ALIGNMENT) @@ -119,12 +110,9 @@ impl InstructionSet { .basic_block_start_indices .contains(&(target as usize)) { - Ok((ExitReason::Continue, target)) + Ok((ExitReason::Continue, target as RegValue)) } else { - Ok(( - ExitReason::Panic, - reg_to_mem_address(Interpreter::next_pc(vm_state, program_state)), - )) + Ok((ExitReason::Panic, vm_state.pc)) } } @@ -137,12 +125,14 @@ impl InstructionSet { /// Opcode: 0 pub fn trap( vm_state: &VMState, - program_state: &ProgramState, + _program_state: &ProgramState, ) -> Result { Ok(SingleStepResult { exit_reason: ExitReason::Panic, state_change: VMStateChange { - new_pc: Interpreter::next_pc(vm_state, program_state), + // TODO: Revisit (Test vectors assume `TRAP` doesn't alter pc values) + // new_pc: Interpreter::next_pc(vm_state, program_state), + new_pc: vm_state.pc, ..Default::default() }, }) @@ -324,7 +314,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -354,7 +344,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -726,7 +716,7 @@ impl InstructionSet { exit_reason, state_change: VMStateChange { register_write: Some((ins.rs1()?, ins.imm1()?)), - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -749,7 +739,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -772,7 +762,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -795,7 +785,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -818,7 +808,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -841,7 +831,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -864,7 +854,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -889,7 +879,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -915,7 +905,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -940,7 +930,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -965,7 +955,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -2194,7 +2184,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -2219,7 +2209,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -2243,7 +2233,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -2269,7 +2259,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -2294,7 +2284,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -2319,7 +2309,7 @@ impl InstructionSet { Ok(SingleStepResult { exit_reason, state_change: VMStateChange { - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) @@ -2346,7 +2336,7 @@ impl InstructionSet { exit_reason, state_change: VMStateChange { register_write: Some((ins.rs1()?, ins.imm1()?)), - new_pc: target as RegValue, + new_pc: target, ..Default::default() }, }) diff --git a/pvm/pvm-core/src/state/state_change.rs b/pvm/pvm-core/src/state/state_change.rs index 0c8032b4..0d6384f3 100644 --- a/pvm/pvm-core/src/state/state_change.rs +++ b/pvm/pvm-core/src/state/state_change.rs @@ -75,6 +75,12 @@ impl VMStateMutator { vm_state: &mut VMState, change: &VMStateChange, ) -> Result { + // Apply PC change + vm_state.pc = change.new_pc; + + // Check gas counter and apply gas change + let post_gas = GasCharger::apply_gas_cost(vm_state, change.gas_charge)?; + // Apply register changes if let Some((reg_index, new_val)) = change.register_write { if reg_index >= REGISTERS_COUNT { @@ -101,11 +107,7 @@ impl VMStateMutator { Err(e) => return Err(e.into()), } } - // Apply PC change - vm_state.pc = change.new_pc; - // Check gas counter and apply gas change - let post_gas = GasCharger::apply_gas_cost(vm_state, change.gas_charge)?; Ok(post_gas) }