Skip to content

[PVM] Enhance djump & branch pc handling and check post pc & gas in PVM tests #150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
0xjunha merged 3 commits into from
Jun 17, 2025
Merged

[PVM] Enhance djump & branch pc handling and check post pc & gas in PVM tests #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f7e1c52
Update pc & gas counter values on page-fault
0xjunha Jun 17, 2025
747bd6a
Update `djump` & `branch`: pc progress conditions
0xjunha Jun 17, 2025
73837e3
Test pc values
0xjunha Jun 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions conformance-tests/src/pvm_harness.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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},
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
8 changes: 6 additions & 2 deletions pvm/pvm-core/src/interpreter/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use fr_pvm_types::{
exit_reason::ExitReason,
};

#[derive(Debug)]
pub struct SingleStepResult {
pub exit_reason: ExitReason,
pub state_change: VMStateChange,
Expand Down Expand Up @@ -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 {
Expand All @@ -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),
Expand Down
76 changes: 33 additions & 43 deletions pvm/pvm-core/src/program/instruction/set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)),
}
}

Expand All @@ -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)
Expand All @@ -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))
}
}

Expand All @@ -137,12 +125,14 @@ impl InstructionSet {
/// Opcode: 0
pub fn trap(
vm_state: &VMState,
program_state: &ProgramState,
_program_state: &ProgramState,
) -> Result<SingleStepResult, VMCoreError> {
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()
},
})
Expand Down Expand Up @@ -324,7 +314,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand Down Expand Up @@ -354,7 +344,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand Down Expand Up @@ -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()
},
})
Expand All @@ -749,7 +739,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -772,7 +762,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -795,7 +785,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -818,7 +808,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -841,7 +831,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -864,7 +854,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -889,7 +879,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -915,7 +905,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -940,7 +930,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -965,7 +955,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand Down Expand Up @@ -2194,7 +2184,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -2219,7 +2209,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -2243,7 +2233,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -2269,7 +2259,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -2294,7 +2284,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -2319,7 +2309,7 @@ impl InstructionSet {
Ok(SingleStepResult {
exit_reason,
state_change: VMStateChange {
new_pc: target as RegValue,
new_pc: target,
..Default::default()
},
})
Expand All @@ -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()
},
})
Expand Down
10 changes: 6 additions & 4 deletions pvm/pvm-core/src/state/state_change.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@ impl VMStateMutator {
vm_state: &mut VMState,
change: &VMStateChange,
) -> Result<SignedGas, VMCoreError> {
// 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 {
Expand All @@ -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)
}

Expand Down