[WIP] implement enable_sbpf_v3_deployment_and_execution#9028
[WIP] implement enable_sbpf_v3_deployment_and_execution#9028topointon-jump wants to merge 6 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This WIP PR refactors the sBPF interpreter jump table construction to make opcode differences between sBPF versions more explicit, as groundwork for enabling sBPF v3 deployment/execution in the Firedancer VM.
Changes:
- Replaced the per-version “ALL_OPCODE/ALL_ILLEGAL + CONDITIONAL” initializer scheme with a full v0 base table plus version-specific override initializers.
- Introduced
V0_BASEandVERSION_OVERRIDES(v)macros to centralize and group version-dependent opcode behavior (by SIMD spec). - Expanded the jump table initialization to explicitly cover all defined sBPF versions (v0–v4).
| /* interp_jump_table holds the sBPF interpreter jump table. It is an | ||
| array where each index is an opcode that can be jumped to be | ||
| executed. Invalid opcodes branch to the sigill label. */ | ||
| executed. Invalid opcodes branch to the sigill label.*/ |
There was a problem hiding this comment.
The header block comment ends with label.*/ (missing a space before the closing */). This reads like a typo and is inconsistent with the surrounding comment formatting; add the space so the comment is properly separated from the terminator.
| executed. Invalid opcodes branch to the sigill label.*/ | |
| executed. Invalid opcodes branch to the sigill label. */ |
| /* Suppress warnings for intentional designated initializer overrides. | ||
| VERSION_OVERRIDES re-initializes entries from V0_BASE; the last | ||
| value wins per C semantics. */ | ||
| #if defined(__GNUC__) | ||
| #pragma GCC diagnostic ignored "-Woverride-init" | ||
| #endif |
There was a problem hiding this comment.
The #pragma GCC diagnostic ignored "-Woverride-init" block is guarded by __GNUC__, which is also defined by Clang. With -Werror builds, Clang can fail on unknown warning groups and/or still emit the duplicate-designator warning under its own flag name. Consider using compiler-specific diagnostic pragmas (GCC: -Woverride-init, Clang: -Winitializer-overrides) with push/pop to keep the suppression narrowly scoped and portable.
Performance Measurements ⏳
|
Performance Measurements ⏳
|
topointon-jump
force-pushed
the
firedancer-enable-sbpf-v3-deployment-and-execution
branch
2 times, most recently
from
30c5ce9 to
5effd04
Compare
Performance Measurements ⏳
|
| #define STRICT_EXPECTED_PHDR_CNT (2U) | ||
| ulong expected_p_vaddr[ STRICT_EXPECTED_PHDR_CNT ] = { FD_SBPF_MM_BYTECODE_ADDR, FD_SBPF_MM_RODATA_ADDR }; /* { MM_RODATA_START(0), MM_BYTECODE_START(0x100000000) } */ | ||
| uint expected_p_flags[ STRICT_EXPECTED_PHDR_CNT ] = { FD_SBPF_PF_R, FD_SBPF_PF_X }; | ||
|
|
There was a problem hiding this comment.
In the strict ELF PHDR expectations, expected_p_flags[0] implies the first segment is rodata (PF_R), but expected_p_vaddr[0] uses FD_SBPF_MM_BYTECODE_ADDR (defined as 0 with a "bytecode" comment). This mismatch makes the strict-layout intent hard to follow. Consider introducing strict-parser-specific constants or updating the local FD_SBPF_MM_*_ADDR naming/comments to clearly reflect the v3 strict layout.
src/flamenco/vm/fd_vm_interp_core.c
Outdated
| /* Internal call | ||
| https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/interpreter.rs#L555-L573 */ | ||
| if( FD_VM_SBPF_STATIC_SYSCALLS( sbpf_version ) ) { | ||
| /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/interpreter.rs#L556-L563 */ | ||
|
|
There was a problem hiding this comment.
This adds v3 CALL_IMM behavior (static syscalls + internal relative calls via src/imm). There are existing interpreter unit tests (e.g. test_vm_interp.c covers syscalls), but none that assert the v3-specific src==0 syscall vs src==1 internal-call semantics. Consider adding unit tests covering those v3 cases and an invalid combination that must fault.
src/flamenco/vm/fd_vm_interp_core.c
Outdated
| if( !FD_VM_SBPF_STATIC_SYSCALLS( sbpf_version ) || src == 0 ) { | ||
| fd_sbpf_syscalls_t const * syscall = fd_sbpf_syscalls_query_const( syscalls, (ulong)imm, NULL ); | ||
| if( FD_LIKELY( syscall ) ) { | ||
| FD_VM_INTERP_SYSCALL_EXEC; | ||
| resolved = 1; |
There was a problem hiding this comment.
For sbpf_version < v3, this block can set resolved = 1 (syscall found) but execution still proceeds into the non-static internal-call handling below, which may push a stack frame / update pc if the key also maps to a valid calldest (or hits the entrypoint special-case). Consider only attempting the internal-call path when the syscall lookup fails (e.g. wrap the internal-call logic with if( !resolved )).
Performance Measurements ⏳
|
topointon-jump
force-pushed
the
firedancer-enable-sbpf-v3-deployment-and-execution
branch
from
555cfb0 to
fbac824
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
src/flamenco/vm/fd_vm_private.h:472
- The stack-gap address translation is currently gated on !fd_sbpf_manual_stack_frame_bump_enabled(vm->sbpf_version), which is also true for SBPF v3+. Per the new feature flags (fd_sbpf_stack_frame_gaps_enabled / FD_VM_SBPF_STACK_FRAME_GAPS), gaps only exist for v0, so this will incorrectly treat v3 stack addresses as having 4k gaps (and may turn valid v3 stack accesses into access violations / wrong offsets). Gate this logic on the dedicated stack-frame-gaps feature (v==FD_SBPF_V0) instead.
/* Stack memory regions have 4kB unmapped "gaps" in-between each frame, which only exist if...
- dynamic stack frames are not enabled (!(SBPF version >= SBPF_V1))
https://github.com/anza-xyz/agave/blob/v2.2.12/programs/bpf_loader/src/lib.rs#L344-L351
*/
if( FD_UNLIKELY( region==FD_VM_STACK_REGION &&
!fd_sbpf_manual_stack_frame_bump_enabled( vm->sbpf_version ) ) ) {
/* If an access starts in a gap region, that is an access violation */
if( FD_UNLIKELY( !!(vaddr & 0x1000) ) ) {
return sentinel;
}
/* To account for the fact that we have gaps in the virtual address space but not in the
physical address space, we need to subtract from the offset the size of all the virtual
gap frames underneath it.
https://github.com/solana-labs/rbpf/blob/b503a1867a9cfa13f93b4d99679a17fe219831de/src/memory_region.rs#L147-L149 */
ulong gap_mask = 0xFFFFFFFFFFFFF000;
offset = ( ( offset & gap_mask ) >> 1 ) | ( offset & ~gap_mask );
}
src/flamenco/progcache/fd_prog_load.c:115
- fd_prog_versions assigns v.min_sbpf_version twice: first to (enable_v0 ? V0 : V3) and then unconditionally overwrites it with (enable_v0 ? V0 : min(V2, v.max_sbpf_version)). The second assignment makes the new V3-based rule ineffective and can yield a min version < V3 even when v0 is disabled. Remove or reconcile the duplicate assignment so min/max version selection matches the intended Agave logic.
fd_prog_versions_t v = {0};
/* https://github.com/anza-xyz/agave/blob/v4.0.0-beta.4/syscalls/src/lib.rs#L314-L319 */
v.min_sbpf_version = enable_v0 ? FD_SBPF_V0 : FD_SBPF_V3;
/* https://github.com/anza-xyz/agave/blob/v4.0.0-beta.4/syscalls/src/lib.rs#L320-L328 */
if( enable_v3 ) {
v.max_sbpf_version = FD_SBPF_V3;
} else if( enable_v2 ) {
v.max_sbpf_version = FD_SBPF_V2;
} else if( enable_v1 ) {
v.max_sbpf_version = FD_SBPF_V1;
} else {
v.max_sbpf_version = FD_SBPF_V0;
}
v.min_sbpf_version = enable_v0 ? FD_SBPF_V0 : fd_uint_min( FD_SBPF_V2, v.max_sbpf_version );
return v;
| /* Parse program headers (expecting up to 2 segments: rodata + bytecode) | ||
| https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/elf.rs#L448-L484 */ | ||
|
|
||
| #define STRICT_EXPECTED_PHDR_CNT (2U) | ||
| ulong expected_p_vaddr[ STRICT_EXPECTED_PHDR_CNT ] = { FD_SBPF_MM_BYTECODE_ADDR, FD_SBPF_MM_RODATA_ADDR }; /* { MM_RODATA_START(0), MM_BYTECODE_START(0x100000000) } */ | ||
| uint expected_p_flags[ STRICT_EXPECTED_PHDR_CNT ] = { FD_SBPF_PF_R, FD_SBPF_PF_X }; | ||
|
|
||
| /* https://github.com/anza-xyz/sbpf/blob/v0.12.2/src/elf.rs#L455-L487 | ||
| Note: Agave iterates with a zip, i.e. it cuts the loop to 4, even | ||
| though the number of phdrs is allowed to be higher. */ | ||
| ulong expected_p_vaddr[ EXPECTED_PHDR_CNT ] = { FD_SBPF_MM_BYTECODE_ADDR, FD_SBPF_MM_RODATA_ADDR, FD_SBPF_MM_STACK_ADDR, FD_SBPF_MM_HEAP_ADDR }; | ||
| uint expected_p_flags[ EXPECTED_PHDR_CNT ] = { FD_SBPF_PF_X, FD_SBPF_PF_R, FD_SBPF_PF_RW, FD_SBPF_PF_RW }; | ||
| fd_elf64_phdr phdr[ EXPECTED_PHDR_CNT ]; | ||
| for( uint i=0; i<EXPECTED_PHDR_CNT; i++ ) { | ||
| ulong phdr_off = sizeof(fd_elf64_ehdr) + i*sizeof(fd_elf64_phdr); | ||
| phdr[ i ] = FD_LOAD( fd_elf64_phdr, bin+phdr_off ); | ||
| /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/elf.rs#L455-L463 | ||
| If the first PH is not marked as readonly, expect the rodata | ||
| segment to be skipped. */ | ||
| fd_elf64_phdr phdr0 = FD_LOAD( fd_elf64_phdr, bin + sizeof(fd_elf64_ehdr) ); | ||
| int skip_rodata = ( phdr0.p_flags != expected_p_flags[ 0 ] ); | ||
| uint ph_start = skip_rodata ? 1U : 0U; | ||
|
|
||
| if( FD_UNLIKELY( !skip_rodata && ehdr.e_phnum < 2 ) ) { | ||
| /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/elf.rs#L461-L463 */ | ||
| return FD_SBPF_ELF_PARSER_ERR_INVALID_FILE_HEADER; | ||
| } | ||
|
|
||
| ulong p_filesz = ( expected_p_flags[ i ] & FD_SBPF_PF_W ) ? 0UL : phdr[ i ].p_memsz; | ||
| /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/elf.rs#L464 */ | ||
| ulong expected_offset = program_header_table_end; | ||
| fd_elf64_phdr bytecode_phdr = {0}; | ||
|
|
||
| uint ph_count = fd_uint_min( ehdr.e_phnum, STRICT_EXPECTED_PHDR_CNT ); | ||
| for( uint ei=ph_start, pi=0; ei<STRICT_EXPECTED_PHDR_CNT && pi<ph_count; ei++, pi++ ) { | ||
| fd_elf64_phdr phdr_i = FD_LOAD( fd_elf64_phdr, bin + sizeof(fd_elf64_ehdr) + pi*sizeof(fd_elf64_phdr) ); | ||
|
|
||
| /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/elf.rs#L468-L479 */ | ||
| int parse_phdr_err = | ||
| ( phdr[ i ].p_type != FD_ELF_PT_LOAD ) | ||
| | ( phdr[ i ].p_flags != expected_p_flags[ i ] ) | ||
| | ( phdr[ i ].p_offset < program_header_table_end ) | ||
| | ( phdr[ i ].p_offset >= bin_sz ) | ||
| | ( phdr[ i ].p_offset % 8UL != 0UL ) | ||
| | ( phdr[ i ].p_vaddr != expected_p_vaddr[ i ] ) | ||
| | ( phdr[ i ].p_paddr != expected_p_vaddr[ i ] ) | ||
| | ( phdr[ i ].p_filesz != p_filesz ) | ||
| | ( phdr[ i ].p_filesz > bin_sz - phdr[ i ].p_offset ) | ||
| | ( phdr[ i ].p_filesz % 8UL != 0UL ) | ||
| | ( phdr[ i ].p_memsz >= FD_SBPF_MM_REGION_SZ ) | ||
| ( phdr_i.p_type != FD_ELF_PT_LOAD ) | ||
| | ( phdr_i.p_flags != expected_p_flags[ ei ] ) | ||
| | ( phdr_i.p_offset != expected_offset ) /* exact sequential: https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/elf.rs#L470 */ | ||
| | ( phdr_i.p_offset >= bin_sz ) | ||
| | ( phdr_i.p_offset % 8UL != 0UL ) | ||
| | ( phdr_i.p_vaddr != expected_p_vaddr[ ei ] ) | ||
| | ( phdr_i.p_paddr != expected_p_vaddr[ ei ] ) | ||
| | ( phdr_i.p_filesz != phdr_i.p_memsz ) /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/elf.rs#L475 */ | ||
| | ( phdr_i.p_filesz > bin_sz - phdr_i.p_offset ) | ||
| | ( phdr_i.p_filesz % 8UL != 0UL ) | ||
| | ( phdr_i.p_memsz >= FD_SBPF_MM_REGION_SZ ) | ||
| ; | ||
| if( FD_UNLIKELY( parse_phdr_err ) ) { | ||
| return FD_SBPF_ELF_PARSER_ERR_INVALID_PROGRAM_HEADER; | ||
| } | ||
|
|
||
| /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/elf.rs#L483 */ | ||
| expected_offset = fd_ulong_sat_add( expected_offset, phdr_i.p_filesz ); | ||
| if( ei == 1 ) { bytecode_phdr = phdr_i; } | ||
| } |
There was a problem hiding this comment.
The strict ELF parser behavior changed substantially (now expects up to 2 PT_LOAD segments with exact sequential offsets, plus optional skip_rodata). Please update/add sbpf loader unit tests/fixtures to cover the new strict layout (rodata@0 + bytecode@0x100000000, and the skip_rodata variant), since existing strict-hex tests are likely no longer representative.
src/flamenco/vm/fd_vm_interp_core.c
Outdated
| /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/interpreter.rs#L556-L563 */ | ||
|
|
||
| /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/program.rs#L97-L103 */ | ||
| long target_pc_l = fd_long_sat_add( (long)pc, fd_long_sat_add( (long)(int)imm, 1L ) ); |
There was a problem hiding this comment.
For SBPF v3+ internal CALL_IMM handling, target_pc_l is computed with fd_long_sat_add. The upstream reference linked in the comment uses wrapping arithmetic; saturating here can change control-flow for large (imm,pc) combinations and diverge from Agave semantics. Use wrapping add semantics for the PC computation (and then bounds-check) to match the spec.
| long target_pc_l = fd_long_sat_add( (long)pc, fd_long_sat_add( (long)(int)imm, 1L ) ); | |
| /* Use wrapping add semantics for PC computation, then bounds-check. */ | |
| ulong offset_u = (ulong)(long)(int)imm + 1UL; | |
| long offset_l = (long)offset_u; | |
| ulong target_pc_u = (ulong)pc + (ulong)offset_l; | |
| long target_pc_l = (long)target_pc_u; |
| FD_VM_INTERP_BRANCH_BEGIN(0x16_jmp32) /* FD_SBPF_OP_JEQ32_IMM */ | ||
| pc += fd_ulong_if( (uint)reg_dst==(uint)imm, offset, 0UL ); | ||
| FD_VM_INTERP_BRANCH_END; | ||
|
|
||
| FD_VM_INTERP_BRANCH_BEGIN(0x1e_jmp32) /* FD_SBPF_OP_JEQ32_REG */ | ||
| pc += fd_ulong_if( (uint)reg_dst==(uint)reg_src, offset, 0UL ); | ||
| FD_VM_INTERP_BRANCH_END; | ||
|
|
||
| FD_VM_INTERP_BRANCH_BEGIN(0x26_jmp32) /* FD_SBPF_OP_JGT32_IMM */ | ||
| pc += fd_ulong_if( (uint)reg_dst>(uint)imm, offset, 0UL ); | ||
| FD_VM_INTERP_BRANCH_END; | ||
|
|
||
| FD_VM_INTERP_BRANCH_BEGIN(0x2e_jmp32) /* FD_SBPF_OP_JGT32_REG */ | ||
| pc += fd_ulong_if( (uint)reg_dst>(uint)reg_src, offset, 0UL ); | ||
| FD_VM_INTERP_BRANCH_END; | ||
|
|
||
| FD_VM_INTERP_BRANCH_BEGIN(0x36_jmp32) /* FD_SBPF_OP_JGE32_IMM */ |
There was a problem hiding this comment.
SIMD-0377 JMP32 execution is newly implemented here, but there don’t appear to be corresponding VM unit tests exercising these opcodes (immediate/reg variants, signed vs unsigned, and version gating to v3+). Add interpreter/validation tests to cover the JMP32 instruction set so regressions are caught early.
| FD_VM_INTERP_BRANCH_BEGIN(0x16_jmp32) /* FD_SBPF_OP_JEQ32_IMM */ | |
| pc += fd_ulong_if( (uint)reg_dst==(uint)imm, offset, 0UL ); | |
| FD_VM_INTERP_BRANCH_END; | |
| FD_VM_INTERP_BRANCH_BEGIN(0x1e_jmp32) /* FD_SBPF_OP_JEQ32_REG */ | |
| pc += fd_ulong_if( (uint)reg_dst==(uint)reg_src, offset, 0UL ); | |
| FD_VM_INTERP_BRANCH_END; | |
| FD_VM_INTERP_BRANCH_BEGIN(0x26_jmp32) /* FD_SBPF_OP_JGT32_IMM */ | |
| pc += fd_ulong_if( (uint)reg_dst>(uint)imm, offset, 0UL ); | |
| FD_VM_INTERP_BRANCH_END; | |
| FD_VM_INTERP_BRANCH_BEGIN(0x2e_jmp32) /* FD_SBPF_OP_JGT32_REG */ | |
| pc += fd_ulong_if( (uint)reg_dst>(uint)reg_src, offset, 0UL ); | |
| FD_VM_INTERP_BRANCH_END; | |
| FD_VM_INTERP_BRANCH_BEGIN(0x36_jmp32) /* FD_SBPF_OP_JGE32_IMM */ | |
| FD_VM_INTERP_BRANCH_BEGIN(0x16_jmp32) /* FD_SBPF_OP_JEQ32_IMM */ | |
| if( FD_UNLIKELY( sbpf_version<3UL ) ) goto sigill; | |
| pc += fd_ulong_if( (uint)reg_dst==(uint)imm, offset, 0UL ); | |
| FD_VM_INTERP_BRANCH_END; | |
| FD_VM_INTERP_BRANCH_BEGIN(0x1e_jmp32) /* FD_SBPF_OP_JEQ32_REG */ | |
| if( FD_UNLIKELY( sbpf_version<3UL ) ) goto sigill; | |
| pc += fd_ulong_if( (uint)reg_dst==(uint)reg_src, offset, 0UL ); | |
| FD_VM_INTERP_BRANCH_END; | |
| FD_VM_INTERP_BRANCH_BEGIN(0x26_jmp32) /* FD_SBPF_OP_JGT32_IMM */ | |
| if( FD_UNLIKELY( sbpf_version<3UL ) ) goto sigill; | |
| pc += fd_ulong_if( (uint)reg_dst>(uint)imm, offset, 0UL ); | |
| FD_VM_INTERP_BRANCH_END; | |
| FD_VM_INTERP_BRANCH_BEGIN(0x2e_jmp32) /* FD_SBPF_OP_JGT32_REG */ | |
| if( FD_UNLIKELY( sbpf_version<3UL ) ) goto sigill; | |
| pc += fd_ulong_if( (uint)reg_dst>(uint)reg_src, offset, 0UL ); | |
| FD_VM_INTERP_BRANCH_END; | |
| FD_VM_INTERP_BRANCH_BEGIN(0x36_jmp32) /* FD_SBPF_OP_JGE32_IMM */ | |
| if( FD_UNLIKELY( sbpf_version<3UL ) ) goto sigill; |
topointon-jump
force-pushed
the
firedancer-enable-sbpf-v3-deployment-and-execution
branch
from
fbac824 to
fdbdabd
Compare
topointon-jump
force-pushed
the
firedancer-enable-sbpf-v3-deployment-and-execution
branch
from
fdbdabd to
45908e8
Compare
topointon-jump
force-pushed
the
firedancer-enable-sbpf-v3-deployment-and-execution
branch
from
45908e8 to
452b099
Compare
topointon-jump
force-pushed
the
firedancer-enable-sbpf-v3-deployment-and-execution
branch
from
452b099 to
ee56ad9
Compare
topointon-jump
force-pushed
the
firedancer-enable-sbpf-v3-deployment-and-execution
branch
from
1bad971 to
5398869
Compare
topointon-jump
force-pushed
the
firedancer-enable-sbpf-v3-deployment-and-execution
branch
from
5398869 to
12ecdf3
Compare
topointon-jump
force-pushed
the
firedancer-enable-sbpf-v3-deployment-and-execution
branch
from
12ecdf3 to
2a113a5
Compare
src/flamenco/vm/fd_vm_interp_core.c
Outdated
| /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/interpreter.rs#L542-L577 */ | ||
| FD_VM_INTERP_BRANCH_BEGIN(0x85) { /* FD_SBPF_OP_CALL_IMM */ | ||
|
|
||
| fd_sbpf_syscalls_t const * syscall = imm!=fd_sbpf_syscalls_key_null() ? fd_sbpf_syscalls_query_const( syscalls, (ulong)imm, NULL ) : NULL; | ||
| if( FD_UNLIKELY( !syscall ) ) { /* Optimize for the syscall case */ | ||
| int resolved = 0; | ||
|
|
||
| /* External syscall | ||
| https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/interpreter.rs#L545-L553 */ | ||
| if( !FD_VM_SBPF_STATIC_SYSCALLS( sbpf_version ) || src == 0 ) { | ||
| fd_sbpf_syscalls_t const * syscall = imm!=fd_sbpf_syscalls_key_null() | ||
| ? fd_sbpf_syscalls_query_const( syscalls, (ulong)imm, NULL ) : NULL; | ||
| if( FD_LIKELY( syscall ) ) { | ||
| FD_VM_INTERP_SYSCALL_EXEC; | ||
| resolved = 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
New SBPF v3 behavior is introduced here (static syscall resolution via src and the new JMP32 opcode semantics), but the existing VM interpreter unit tests appear to default to SBPF v0 only (TEST_VM_DEFAULT_SBPF_VERSION). Add/extend unit tests that execute small programs under SBPF v3 to cover: (1) CALL_IMM external vs internal resolution rules, and (2) at least one JMP32 opcode per signed/unsigned variant to ensure dispatch + validation match the spec.
src/flamenco/vm/fd_vm_interp_core.c
Outdated
| if( !FD_VM_SBPF_STATIC_SYSCALLS( sbpf_version ) || src == 0 ) { | ||
| fd_sbpf_syscalls_t const * syscall = imm!=fd_sbpf_syscalls_key_null() | ||
| ? fd_sbpf_syscalls_query_const( syscalls, (ulong)imm, NULL ) : NULL; | ||
| if( FD_LIKELY( syscall ) ) { | ||
| FD_VM_INTERP_SYSCALL_EXEC; | ||
| resolved = 1; | ||
| } | ||
| } | ||
|
|
||
| /* Internal call | ||
| https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/interpreter.rs#L555-L573 */ | ||
| if( FD_VM_SBPF_STATIC_SYSCALLS( sbpf_version ) ) { | ||
| /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/interpreter.rs#L556-L563 */ | ||
|
|
||
| /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/program.rs#L97-L103 */ | ||
| long target_pc_l = fd_long_sat_add( (long)pc, fd_long_sat_add( (long)(int)imm, 1L ) ); | ||
| if( target_pc_l>=0L && (ulong)target_pc_l<text_cnt && src == 1 ) { | ||
| FD_VM_INTERP_STACK_PUSH; | ||
| pc = (ulong)target_pc_l - 1; | ||
| resolved = 1; | ||
| } | ||
| } else { | ||
| /* Note we do the stack push before updating the pc(*). This implies | ||
| that the call stack frame gets allocated _before_ checking if the | ||
| call target is valid. It would be fine to switch the order | ||
| though such would change the precise faulting semantics of | ||
| sigtextbr and sigstack. | ||
| that the call stack frame gets allocated _before_ checking if the | ||
| call target is valid. It would be fine to switch the order |
There was a problem hiding this comment.
In the CALL_IMM implementation, the external-syscall path can execute a syscall and set resolved=1, but the non-static-syscalls internal-call path (the else starting at line 717) still runs unconditionally afterward. This can incorrectly attempt to treat syscall IDs as call destinations and jump to sigillbr (or push a frame / change pc) after a successful syscall. Gate the internal-call logic on !resolved (or restore the old if( !syscall ) { internal } else { syscall } structure for non-static-syscalls) so syscalls don't fall through into internal call handling.
topointon-jump
force-pushed
the
firedancer-enable-sbpf-v3-deployment-and-execution
branch
from
2a113a5 to
8c99176
Compare
Performance Measurements ⏳
|
Performance Measurements ⏳
|
| /* SIMD-0178: Static syscalls (SBPF V3+) | ||
| https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/interpreter.rs#L542-L577 */ | ||
| FD_VM_INTERP_BRANCH_BEGIN(0x85_static) { /* FD_SBPF_OP_CALL_IMM (static syscalls) */ | ||
|
|
||
| if( src == 0 ) { | ||
| /* External syscall | ||
| https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/interpreter.rs#L545-L553 */ | ||
| fd_sbpf_syscalls_t const * syscall = imm!=fd_sbpf_syscalls_key_null() ? fd_sbpf_syscalls_query_const( syscalls, (ulong)imm, NULL ) : NULL; | ||
| if( FD_UNLIKELY( !syscall ) ) goto sigillbr; | ||
| FD_VM_INTERP_SYSCALL_EXEC; | ||
| } else if( src == 1 ) { | ||
| /* Internal call | ||
| https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/interpreter.rs#L555-L563 | ||
| https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/program.rs#L97-L103 */ | ||
| long target_pc_l = fd_long_sat_add( (long)pc, fd_long_sat_add( (long)(int)imm, 1L ) ); | ||
| if( FD_UNLIKELY( target_pc_l<0L || (ulong)target_pc_l>=text_cnt ) ) goto sigillbr; | ||
| FD_VM_INTERP_STACK_PUSH; | ||
| pc = (ulong)target_pc_l - 1; | ||
| } else { | ||
| /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/interpreter.rs#L574-L576 */ | ||
| goto sigillbr; | ||
| } | ||
|
|
||
| } FD_VM_INTERP_BRANCH_END; |
There was a problem hiding this comment.
New SBPF v3+ behavior is introduced here (static syscall dispatch via opcode 0x85_static and internal-call semantics) but there are no corresponding unit tests validating the new opcode paths (e.g., src==0 external syscall lookup, src==1 relative internal call, and invalid src values). Adding targeted interpreter tests for SBPF v3 would help prevent regressions.
| /* SIMD-0377: JMP32 | ||
| https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/interpreter.rs#L480-L501 | ||
|
|
||
| 0x16 - 0xde ******************************************************/ | ||
|
|
||
| FD_VM_INTERP_BRANCH_BEGIN(0x16_jmp32) /* FD_SBPF_OP_JEQ32_IMM */ | ||
| pc += fd_ulong_if( (uint)reg_dst==(uint)imm, offset, 0UL ); | ||
| FD_VM_INTERP_BRANCH_END; | ||
|
|
||
| FD_VM_INTERP_BRANCH_BEGIN(0x1e_jmp32) /* FD_SBPF_OP_JEQ32_REG */ | ||
| pc += fd_ulong_if( (uint)reg_dst==(uint)reg_src, offset, 0UL ); | ||
| FD_VM_INTERP_BRANCH_END; |
There was a problem hiding this comment.
JMP32 instructions (SIMD-0377) are newly implemented here, but there are no unit tests covering their branching semantics (signed vs unsigned comparisons, imm vs reg variants, and correct pc update using the offset). Adding test cases for a few representative JMP32 ops would make it much safer to enable SBPF v3 execution.
| /* Strict ELF loading (for SBPF V3+ programs). | ||
|
|
||
| SBPF V3+ programs do not require relocations or calldests, so this | ||
| function is much cheaper than fd_sbpf_program_load_lenient. | ||
|
|
||
| https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/elf.rs#L406-L590 */ | ||
| static int | ||
| fd_sbpf_program_load_strict( fd_sbpf_program_t * prog, | ||
| void const * bin ) { | ||
| fd_elf64_ehdr ehdr = FD_LOAD( fd_elf64_ehdr, bin ); | ||
| fd_elf64_phdr phdr_0 = FD_LOAD( fd_elf64_phdr, bin+sizeof(fd_elf64_ehdr) ); | ||
| int skip_rodata = phdr_0.p_flags != FD_SBPF_PF_R; | ||
|
|
||
| /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/elf.rs#L486-L496 */ | ||
| fd_elf64_phdr bytecode_phdr; | ||
| if( FD_UNLIKELY( skip_rodata ) ) { | ||
| prog->rodata_sz = 0UL; | ||
| bytecode_phdr = phdr_0; | ||
| } else { | ||
| prog->rodata_sz = phdr_0.p_memsz; | ||
| bytecode_phdr = FD_LOAD( fd_elf64_phdr, bin+sizeof(fd_elf64_ehdr)+sizeof(fd_elf64_phdr) ); | ||
| } | ||
|
|
||
| /* https://github.com/anza-xyz/sbpf/blob/v0.14.4/src/elf.rs#L510-L514 */ | ||
| prog->entry_pc = fd_ulong_sat_sub( ehdr.e_entry, bytecode_phdr.p_vaddr ) / 8UL; | ||
| return FD_SBPF_ELF_SUCCESS; |
There was a problem hiding this comment.
fd_sbpf_program_load_strict() currently only updates rodata_sz and entry_pc, but never copies the PT_LOAD segment contents from the ELF into prog->rodata (nor adjusts prog->text). This will leave rodata/text backing memory uninitialized for SBPF v3+ programs, breaking execution and validator behavior. The strict loader should populate prog->rodata with the loaded rodata segment (and bytecode segment if it is stored in the same buffer) in the same layout expected by fd_vm_mem_cfg / prog->text, similar to how fd_sbpf_program_relocate() copies the bin for the lenient path.
| info->bin_sz = bin_sz; | ||
| info->text_off = (uint)phdr[ 0 ].p_offset; | ||
| info->text_sz = (uint)phdr[ 0 ].p_memsz; | ||
| info->text_cnt = (uint)( phdr[ 0 ].p_memsz / 8UL ); | ||
| info->text_off = (uint)bytecode_phdr.p_offset; | ||
| info->text_sz = (uint)bytecode_phdr.p_memsz; | ||
| info->text_cnt = (uint)( bytecode_phdr.p_memsz / 8UL ); |
There was a problem hiding this comment.
fd_sbpf_elf_peek_strict() sets info->text_off to bytecode_phdr.p_offset (a file offset). If strict loading maps segments by vaddr (e.g., copying the rodata PT_LOAD so that vaddr 0 maps to rodata[0]), then using the file offset here will make prog->text point at the wrong location. Consider setting text_off to the bytecode offset within the loaded in-memory image (e.g., 0 when skip_rodata, else rodata_memsz) and ensuring fd_sbpf_program_load_strict loads the bytecode accordingly.
| #if defined(__GNUC__) | ||
| #pragma GCC diagnostic ignored "-Woverride-init" | ||
| #endif |
There was a problem hiding this comment.
The GCC diagnostic suppression for -Woverride-init is applied without a push/pop scope, which can mask other override-init warnings for the rest of this translation unit. Consider wrapping it in a diagnostic push/pop around just the jump table initializer so only the intentional overrides are suppressed.
topointon-jump
force-pushed
the
firedancer-enable-sbpf-v3-deployment-and-execution
branch
from
11381fa to
1225a35
Compare
Performance Measurements ⏳
|
topointon-jump
force-pushed
the
firedancer-enable-sbpf-v3-deployment-and-execution
branch
from
1225a35 to
b4c87c8
Compare
| /* SIMD-0174: PQR (SBPF V2) / SIMD-0377: JMP32 (SBPF V3) | ||
| These opcodes conflict in SBPF V2 and SBPF V3, so we use | ||
| OVERRIDE_WITH_FALLBACK to avoid last-write-wins bugs. | ||
|
|
||
| /* SIMD-0174: PQR */ | ||
| /* 27: 11 */ CONDITIONAL( 0x36, FD_VM_SBPF_ENABLE_PQR, &&interp_0x36, &&sigill), | ||
| /* 28: 14 */ CONDITIONAL( 0x3e, FD_VM_SBPF_ENABLE_PQR, &&interp_0x3e, &&sigill), | ||
| /* 29: 16 */ CONDITIONAL( 0x46, FD_VM_SBPF_ENABLE_PQR, &&interp_0x46, &&sigill), | ||
| /* 30: 17 */ CONDITIONAL( 0x4e, FD_VM_SBPF_ENABLE_PQR, &&interp_0x4e, &&sigill), | ||
| /* 31: 18 */ CONDITIONAL( 0x56, FD_VM_SBPF_ENABLE_PQR, &&interp_0x56, &&sigill), | ||
| /* 32: 19 */ CONDITIONAL( 0x5e, FD_VM_SBPF_ENABLE_PQR, &&interp_0x5e, &&sigill), | ||
| /* 33: 23 */ CONDITIONAL( 0x66, FD_VM_SBPF_ENABLE_PQR, &&interp_0x66, &&sigill), | ||
| /* 34: 27 */ CONDITIONAL( 0x6e, FD_VM_SBPF_ENABLE_PQR, &&interp_0x6e, &&sigill), | ||
| /* 35: 31 */ CONDITIONAL( 0x76, FD_VM_SBPF_ENABLE_PQR, &&interp_0x76, &&sigill), | ||
| /* 36: 35 */ CONDITIONAL( 0x7e, FD_VM_SBPF_ENABLE_PQR, &&interp_0x7e, &&sigill), | ||
| /* 37: 38 */ CONDITIONAL( 0x86, FD_VM_SBPF_ENABLE_PQR, &&interp_0x86, &&sigill), | ||
| /* 38: 42 */ CONDITIONAL( 0x8e, FD_VM_SBPF_ENABLE_PQR, &&interp_0x8e, &&sigill), | ||
| /* 39: 46 */ CONDITIONAL( 0x96, FD_VM_SBPF_ENABLE_PQR, &&interp_0x96, &&sigill), | ||
| /* 40: 50 */ CONDITIONAL( 0x9e, FD_VM_SBPF_ENABLE_PQR, &&interp_0x9e, &&sigill), | ||
| /* 41: 52 */ CONDITIONAL( 0xb6, FD_VM_SBPF_ENABLE_PQR, &&interp_0xb6, &&sigill), | ||
| /* 42: 54 */ CONDITIONAL( 0xbe, FD_VM_SBPF_ENABLE_PQR, &&interp_0xbe, &&sigill), | ||
| /* 43: 55 */ CONDITIONAL( 0xc6, FD_VM_SBPF_ENABLE_PQR, &&interp_0xc6, &&sigill), | ||
| /* 44: 56 */ CONDITIONAL( 0xce, FD_VM_SBPF_ENABLE_PQR, &&interp_0xce, &&sigill), | ||
| /* 45: 58 */ CONDITIONAL( 0xd6, FD_VM_SBPF_ENABLE_PQR, &&interp_0xd6, &&sigill), | ||
| /* 46: 59 */ CONDITIONAL( 0xde, FD_VM_SBPF_ENABLE_PQR, &&interp_0xde, &&sigill), | ||
| /* 47: 60 */ CONDITIONAL( 0xe6, FD_VM_SBPF_ENABLE_PQR, &&interp_0xe6, &&sigill), | ||
| /* 48: 61 */ CONDITIONAL( 0xee, FD_VM_SBPF_ENABLE_PQR, &&interp_0xee, &&sigill), | ||
| /* 49: 62 */ CONDITIONAL( 0xf6, FD_VM_SBPF_ENABLE_PQR, &&interp_0xf6, &&sigill), | ||
| /* 50: 64 */ CONDITIONAL( 0xfe, FD_VM_SBPF_ENABLE_PQR, &&interp_0xfe, &&sigill), | ||
| FD_VM_SBPF_ENABLE_JMP32 and FD_VM_SBPF_ENABLE_PQR are mutually | ||
| exclusive - see static asserts below. */ \ |
There was a problem hiding this comment.
VERSION_OVERRIDES is a multi-line macro, but the comment block starting at the PQR/JMP32 section does not end each line with a \. This will terminate the macro early and leave the subsequent OVERRIDE_WITH_FALLBACK(...) lines at file scope, which should fail to compile. Add trailing \ to every line inside the macro (including comment-only lines) or convert the block comment to a single physical line.
Performance Measurements ⏳
|
topointon-jump
force-pushed
the
firedancer-enable-sbpf-v3-deployment-and-execution
branch
from
b4c87c8 to
08d138f
Compare
Performance Measurements ⏳
|
topointon-jump
force-pushed
the
firedancer-enable-sbpf-v3-deployment-and-execution
branch
from
08d138f to
ef2434a
Compare
Performance Measurements ⏳
|
WIP NOT READY FOR REVIEW