Skip to content

Fix instruction length calculation. #311

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

Open
igankevich wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix instruction length calculation. #311

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
094ec8d
Fix instruction length calculation.
igankevich Jun 26, 2025
8fbc286
Add instruction length calculation test.
igankevich Jun 26, 2025
5a25d28
Merge branch 'master' into ivan/fix-instruction-length-calculation
igankevich Jul 11, 2025
5db45a2
Move `if` to the end of `after_instruction`.
igankevich Jul 11, 2025
c70ad4a
Move `if` back. Increase max. instruction length by 10.
igankevich Jul 11, 2025
d22fae3
Disable `wasm3` feature of `benchtool`.
igankevich Jul 11, 2025
63c1fff
Merge branch 'master' into ivan/fix-instruction-length-calculation
igankevich Jul 15, 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: 7 additions & 3 deletions crates/polkavm/src/compiler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ where
memset_trampoline_start: usize,
memset_trampoline_end: usize,
custom_codegen: Option<Arc<dyn CustomCodegen>>,
asm_len_before: usize,

_phantom: PhantomData<(S, B)>,
}
Expand Down Expand Up @@ -260,6 +261,7 @@ where
memset_trampoline_start: 0,
memset_trampoline_end: 0,
custom_codegen: config.custom_codegen.clone(),
asm_len_before: 0,
_phantom: PhantomData,
};

Expand Down Expand Up @@ -454,18 +456,20 @@ where
}
}

fn before_instruction(&self, program_counter: u32) {
fn before_instruction(&mut self, program_counter: u32) {
if log::log_enabled!(log::Level::Trace) {
self.trace_compiled_instruction(program_counter);
}
if cfg!(debug_assertions) && !self.step_tracing && self.custom_codegen.is_none() {
self.asm_len_before = self.asm.len();
}
}

fn after_instruction<const KIND: usize>(&mut self, program_counter: u32, args_length: u32) {
assert!(KIND == CONTINUE_BASIC_BLOCK || KIND == END_BASIC_BLOCK || KIND == END_BASIC_BLOCK_INVALID);

if cfg!(debug_assertions) && !self.step_tracing && self.custom_codegen.is_none() {
let offset = self.program_counter_to_machine_code_offset_list.last().unwrap().1 as usize;
let instruction_length = self.asm.len() - offset;
let instruction_length = self.asm.len() - self.asm_len_before;
Copy link
Copy Markdown
Collaborator

@koute koute Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this isn't necessarily correct.

In general the point of this check is as follows: we want to estimate how many bytes of native assembly each byte of PVM bytecode can emit at maximum, and then allocate the appropriate amount of address space so that it will all fit even in the worst case. So we need to include the size of the gas metering stub in these calculations somehow. If we apply this change then the size of the gas metering stub will effectively be ignored.

However, I think it should be fine if we move this whole if to the end of this function so that it will include the size of the gas metering stub.

Copy link
Copy Markdown
Author

@igankevich igankevich Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I reverted my changes and instead increased max. instruction length by 10 (the size of the stub). I hope this is correct. The former if already included gas metering stub length.

if instruction_length > VM_COMPILER_MAXIMUM_INSTRUCTION_LENGTH as usize {
self.panic_on_too_long_instruction(program_counter, instruction_length)
}
Expand Down
74 changes: 46 additions & 28 deletions crates/polkavm/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::mutex::Mutex;
use crate::{
BackendKind, CallError, Caller, Config, Engine, GasMeteringKind, InterruptKind, Linker, MemoryAccessError, Module, ModuleConfig,
BackendKind, CallError, Caller, Config, Engine, Gas, GasMeteringKind, InterruptKind, Linker, MemoryAccessError, Module, ModuleConfig,
ProgramBlob, ProgramCounter, Reg, Segfault,
};
use alloc::collections::BTreeMap;
Expand Down Expand Up @@ -2648,12 +2648,12 @@ struct TestInstance {
}

impl TestInstance {
fn new(config: &Config, elf: &'static [u8], optimize: bool) -> Self {
fn new(config: &Config, module_config: &ModuleConfig, elf: &'static [u8], optimize: bool) -> Self {
let _ = env_logger::try_init();
let blob = get_blob_impl(optimize, false, elf);

let engine = Engine::new(config).unwrap();
let module = Module::from_blob(&engine, &Default::default(), blob).unwrap();
let module = Module::from_blob(&engine, module_config, blob).unwrap();
let mut linker = Linker::new();
linker
.define_typed("multiply_by_2", |_caller: Caller<()>, value: u32| -> u32 { value * 2 })
Expand Down Expand Up @@ -2750,15 +2750,15 @@ impl TestBlobArgs {

fn test_blob_basic_test(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
assert_eq!(i.call::<(), u32>("push_one_to_global_vec", ()).unwrap(), 1);
assert_eq!(i.call::<(), u32>("push_one_to_global_vec", ()).unwrap(), 2);
assert_eq!(i.call::<(), u32>("push_one_to_global_vec", ()).unwrap(), 3);
}

fn test_blob_atomic_fetch_add(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
assert_eq!(i.call::<(u32,), u32>("atomic_fetch_add", (1,)).unwrap(), 0);
assert_eq!(i.call::<(u32,), u32>("atomic_fetch_add", (1,)).unwrap(), 1);
assert_eq!(i.call::<(u32,), u32>("atomic_fetch_add", (1,)).unwrap(), 2);
Expand All @@ -2770,7 +2770,7 @@ fn test_blob_atomic_fetch_add(args: TestBlobArgs) {

fn test_blob_atomic_fetch_swap(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
assert_eq!(i.call::<(u32,), u32>("atomic_fetch_swap", (10,)).unwrap(), 0);
assert_eq!(i.call::<(u32,), u32>("atomic_fetch_swap", (100,)).unwrap(), 10);
assert_eq!(i.call::<(u32,), u32>("atomic_fetch_swap", (1000,)).unwrap(), 100);
Expand Down Expand Up @@ -2799,7 +2799,7 @@ fn test_blob_atomic_fetch_minmax(args: TestBlobArgs) {
];

let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
for (name, cb) in list {
for a in [-10, 0, 10] {
for b in [-10, 0, 10] {
Expand All @@ -2814,19 +2814,19 @@ fn test_blob_atomic_fetch_minmax(args: TestBlobArgs) {

fn test_blob_hostcall(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
assert_eq!(i.call::<(u32,), u32>("test_multiply_by_6", (10,)).unwrap(), 60);
}

fn test_blob_define_abi(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
assert!(i.call::<(), ()>("test_define_abi", ()).is_ok());
}

fn test_blob_input_registers(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
assert!(i.call::<(), ()>("test_input_registers", ()).is_ok());
}

Expand All @@ -2844,7 +2844,7 @@ fn test_blob_call_sbrk_from_host_function(args: TestBlobArgs) {

fn test_blob_program_memory_can_be_reused_and_cleared(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
let address = i.call::<(), u32>("get_global_address", ()).unwrap();

assert_eq!(i.instance.read_memory(address, 4).unwrap(), [0x00, 0x00, 0x00, 0x00]);
Expand All @@ -2864,7 +2864,7 @@ fn test_blob_program_memory_can_be_reused_and_cleared(args: TestBlobArgs) {

fn test_blob_out_of_bounds_memory_access_generates_a_trap(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
let address = i.call::<(), u32>("get_global_address", ()).unwrap();
assert_eq!(i.call::<(u32,), u32>("read_u32", (address,)).unwrap(), 0);
i.call::<(), ()>("increment_global", ()).unwrap();
Expand All @@ -2878,7 +2878,7 @@ fn test_blob_out_of_bounds_memory_access_generates_a_trap(args: TestBlobArgs) {

fn test_blob_call_sbrk_impl(args: TestBlobArgs, mut call_sbrk: impl FnMut(&mut TestInstance, u32) -> u32) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
let memory_map = i.module.memory_map().clone();
let heap_base = memory_map.heap_base();
let page_size = memory_map.page_size();
Expand Down Expand Up @@ -2934,7 +2934,7 @@ fn test_blob_call_sbrk_impl(args: TestBlobArgs, mut call_sbrk: impl FnMut(&mut T

fn test_blob_add_u32(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
assert_eq!(i.call::<(u32, u32), u32>("add_u32", (1, 2,)).unwrap(), 3);
assert_eq!(i.instance.reg(Reg::A0), 3);

Expand All @@ -2952,7 +2952,7 @@ fn test_blob_add_u32(args: TestBlobArgs) {

fn test_blob_add_u64(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
assert_eq!(i.call::<(u64, u64), u64>("add_u64", (1, 2,)).unwrap(), 3);
assert_eq!(i.instance.reg(Reg::A0), 3);
assert_eq!(
Expand All @@ -2963,21 +2963,21 @@ fn test_blob_add_u64(args: TestBlobArgs) {

fn test_blob_xor_imm_u32(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
for value in [0, 0xaaaaaaaa, 0x55555555, 0x12345678, 0xffffffff] {
assert_eq!(i.call::<(u32,), u32>("xor_imm_u32", (value,)).unwrap(), value ^ 0xfb8f5c1e);
}
}

fn test_blob_branch_less_than_zero(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
i.call::<(), ()>("test_branch_less_than_zero", ()).unwrap();
}

fn test_blob_fetch_add_atomic_u64(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
assert_eq!(i.call::<(u64,), u64>("fetch_add_atomic_u64", (1,)).unwrap(), 0);
assert_eq!(i.call::<(u64,), u64>("fetch_add_atomic_u64", (0,)).unwrap(), 1);
assert_eq!(i.call::<(u64,), u64>("fetch_add_atomic_u64", (0,)).unwrap(), 1);
Expand All @@ -2987,25 +2987,25 @@ fn test_blob_fetch_add_atomic_u64(args: TestBlobArgs) {

fn test_blob_cmov_if_zero_with_zero_reg(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
i.call::<(), ()>("cmov_if_zero_with_zero_reg", ()).unwrap();
}

fn test_blob_cmov_if_not_zero_with_zero_reg(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
i.call::<(), ()>("cmov_if_not_zero_with_zero_reg", ()).unwrap();
}

fn test_blob_min_stack_size(args: TestBlobArgs) {
let elf = args.get_test_program();
let i = TestInstance::new(&args.config, elf, args.optimize);
let i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
assert_eq!(i.instance.module().memory_map().stack_size(), 65536);
}

fn test_blob_negate_and_add(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
if !args.is_64_bit {
assert_eq!(i.call::<(u32, u32), u32>("negate_and_add", (123, 1,)).unwrap(), 15);
} else {
Expand All @@ -3015,13 +3015,13 @@ fn test_blob_negate_and_add(args: TestBlobArgs) {

fn test_blob_return_tuple_from_import(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
i.call::<(), ()>("test_return_tuple", ()).unwrap();
}

fn test_blob_return_tuple_from_export(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
if args.is_64_bit {
let a0 = 0x123456789abcdefe_u64;
let a1 = 0x1122334455667788_u64;
Expand Down Expand Up @@ -3051,23 +3051,40 @@ fn test_blob_return_tuple_from_export(args: TestBlobArgs) {

fn test_blob_get_heap_base(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
let heap_base = i.call::<(), u32>("get_heap_base", ()).unwrap();
assert_eq!(heap_base, i.instance.module().memory_map().heap_base());
}

fn test_blob_get_self_address(args: TestBlobArgs) {
let elf = args.get_test_program();
let mut i = TestInstance::new(&args.config, elf, args.optimize);
let mut i = TestInstance::new(&args.config, &Default::default(), elf, args.optimize);
let addr = i.call::<(), u32>("get_self_address", ()).unwrap();
assert_ne!(addr, 0);
}

fn test_blob_instruction_length(args: TestBlobArgs) {
// Test that the instruction length is properly calculated when gas metering is enabled and a
// long instruction appears at the start of the block (right after gas metering stub).
let elf = args.get_test_program();
let mut module_config = ModuleConfig::default();
module_config.set_gas_metering(Some(GasMeteringKind::Sync));
let mut i = TestInstance::new(&args.config, &module_config, elf, args.optimize);
i.instance.set_gas(Gas::MAX);
if args.is_64_bit {
assert_eq!(i.call::<(u64, u64), u64>("div_asm", (10, 2,)).unwrap(), 5);
assert_eq!(i.instance.reg(Reg::A0), 5);
} else {
assert_eq!(i.call::<(u32, u32), u32>("div_asm", (10, 2,)).unwrap(), 5);
assert_eq!(i.instance.reg(Reg::A0), 5);
}
}

fn test_asm_reloc_add_sub(config: Config, optimize: bool) {
const BLOB_64: &[u8] = include_bytes!("../../../guest-programs/asm-tests/output/reloc_add_sub_64.elf");

let elf = BLOB_64;
let mut i = TestInstance::new(&config, elf, optimize);
let mut i = TestInstance::new(&config, &Default::default(), elf, optimize);

let address = i.call::<(u32,), u32>("get_string", (0,)).unwrap();
assert_eq!(i.instance.read_u32(address).unwrap(), 0x01010101);
Expand All @@ -3083,7 +3100,7 @@ fn test_asm_reloc_hi_lo(config: Config, optimize: bool) {
const BLOB_64: &[u8] = include_bytes!("../../../guest-programs/asm-tests/output/reloc_hi_lo_64.elf");

let elf = BLOB_64;
let mut i = TestInstance::new(&config, elf, optimize);
let mut i = TestInstance::new(&config, &Default::default(), elf, optimize);

let address = i.call::<(u32,), u32>("get_string", (0,)).unwrap();
assert_eq!(i.instance.read_u32(address).unwrap(), 0xA1010101);
Expand Down Expand Up @@ -3897,6 +3914,7 @@ run_test_blob_tests! {
test_blob_return_tuple_from_export
test_blob_get_heap_base
test_blob_get_self_address
test_blob_instruction_length
}

run_asm_tests! {
Expand Down
18 changes: 17 additions & 1 deletion guest-programs/test-blob/src/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,11 +428,27 @@ extern "C" fn get_heap_base() -> u32 {

#[inline(never)]
fn get_self_address_impl() -> usize {
unsafe { GLOBAL += 1; }
unsafe {
GLOBAL += 1;
}
get_self_address_impl as usize
}

#[polkavm_derive::polkavm_export]
extern "C" fn get_self_address() -> u32 {
get_self_address_impl() as u32
}

#[polkavm_derive::polkavm_export]
extern "C" fn div_asm(a0: usize, a1: usize) -> usize {
unsafe {
let output;
core::arch::asm!(
"div a0, a0, a1",
in("a0") a0,
in("a1") a1,
lateout("a0") output,
);
output
}
}