Skip to content

test: integrate riscv64im-unknown-openvm-elf target into build system#2765

Draft
Tuanlinh12312 wants to merge 1 commit intodevelop-v2.1.0-rv64from
rv64-openvm-target-integration
Draft

test: integrate riscv64im-unknown-openvm-elf target into build system#2765
Tuanlinh12312 wants to merge 1 commit intodevelop-v2.1.0-rv64from
rv64-openvm-target-integration

Conversation

@Tuanlinh12312
Copy link
Copy Markdown

@Tuanlinh12312 Tuanlinh12312 commented Apr 30, 2026

Summary

Integrate the new built-in Rust target riscv64im-unknown-openvm-elf (target_os = "openvm") into the OpenVM build system and guest crates. This replaces the custom JSON target spec (riscv64im-openvm-none-elf, target_os = "none") and enables std support for guest programs.

The target itself was added to the OpenVM Rust fork in openvm-org/rust#3.

Changes

  • Build system (crates/toolchain/build/src/lib.rs):

    • Switch RUSTC_TARGET from riscv64im-openvm-none-elf to riscv64im-unknown-openvm-elf
    • Switch default toolchain to openvm-rv64 (custom-built rustc from the fork)
    • Add std to -Zbuild-std crate list when std feature is requested
    • Skip rustup component checks for linked (non-rustup-managed) toolchains
  • Guest runtime (crates/toolchain/openvm/):

    • New pal_abi.rs: provides sys_write, sys_panic, sys_rand, sys_halt, sys_alloc_words, and other extern "C" symbols that the std PAL declares. Bridges them to OpenVM's custom RISC-V instructions via openvm_riscv_guest.
    • Fix entry! macro to generate #[no_mangle] fn main() when both std and openvm_intrinsics are active (previously only generated it for no_std).
  • Platform (crates/toolchain/platform/Cargo.toml):

    • Expand dependency cfg gate from target_os = "none" to any(target_os = "none", target_os = "openvm") so libm resolves for the new target.
  • Guest programs (~100 files):

    • Update no_main cfg gate from cfg_attr(target_os = "none", no_main) to cfg_attr(any(target_os = "none", target_os = "openvm"), no_main) across all guest programs.

Test plan

Local test setup

Tests require a custom rustc toolchain built from the fork branch feat/riscv64im-unknown-openvm-elf (openvm-org/rust#3):

# In the openvm-org/rust clone:
BOOTSTRAP_SKIP_TARGET_SANITY=1 python3 x.py build library --target riscv64im-unknown-openvm-elf
python3 x.py build library  # host library
rustup toolchain link openvm-rv64 build/host/stage1

Test results

cargo nextest run --cargo-profile=fast -p openvm-riscv-integration-tests --run-ignored all

18/19 pass. test_tiny_mem_test fails because embedded-alloc 0.6.0 calls Layout::dangling() which was renamed to Layout::dangling_ptr() in the newer Rust nightly our fork is built from. This is an upstream crate incompatibility — latest embedded-alloc (0.7.0) has the same issue.

CI note

CI does not have the custom openvm-rv64 toolchain. A toolchain distribution workflow (building and releasing custom rustc binaries, similar to zisk and sp1) is needed as follow-up work.

🤖 Generated with Claude Code

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2026

Claude finished @Tuanlinh12312's task in 3m 40s —— View job


Code Review: PR #2765 — Integrate riscv64im-unknown-openvm-elf target into build system

  • Gather PR diff and understand scope of changes
  • Review crates/toolchain/build/src/lib.rs — target switch and build-std changes
  • Review crates/toolchain/openvm/src/lib.rs — entry macro changes
  • Review crates/toolchain/openvm/src/pal_abi.rs — new PAL ABI bridge
  • Review crates/toolchain/platform/Cargo.toml — cfg gate update
  • Review guest program no_main cfg gate changes (spot-check)
  • Check for missed target_os = "none" references across codebase
  • Post comprehensive review

Overview

This PR switches the OpenVM guest target from a custom JSON spec (riscv64im-openvm-none-elf) to a built-in Rust target (riscv64im-unknown-openvm-elf with target_os = "openvm"), adds the PAL ABI bridge for std support, updates the entry! macro, and mechanically updates no_main cfg gates across ~100 guest programs. The scope is well-organized and the mechanical changes are consistent.


Issues

1. Potential buffer overflow in sys_rand — verify ABI contract

crates/toolchain/openvm/src/pal_abi.rs:28-31

pub unsafe extern "C" fn sys_rand(recv_buf: *mut u32, words: usize) {
    hint_random(words);
    hint_buffer_chunked(recv_buf as *mut u8, words);
}

hint_random and hint_buffer_chunked both operate in 8-byte (dword) units (see extensions/riscv/guest/src/io.rs:40). The parameter name words with a *mut u32 buffer type suggests 4-byte word semantics. If the Rust fork's PAL calls sys_rand(&mut buf, buf.len()) where buf: [u32; N], this would write N * 8 bytes into an N * 4-byte buffer — a 2x overwrite.

Please verify that the PAL ABI contract matches: if words is intended to mean u64 dwords, the function signature is misleading. If it means u32 words, the hint_random/hint_buffer_chunked calls need adjustment (e.g., (words + 1) / 2).

2. Remaining target_os = "none" cfg gates in Cargo.toml files not updated

Several Cargo.toml files still use target_os = "none" which will be incorrect for the new target_os = "openvm":

Guest-only deps that will be missed (gate evaluates to false for openvm target):

  • guest-libs/keccak256/Cargo.toml:16cfg(all(target_arch = "riscv64", target_os = "none")) gates spin dependency

Host-only deps that will leak into guest builds (gate evaluates to true for openvm target):

  • extensions/pairing/guest/Cargo.toml:29cfg(not(target_os = "none")) gates num-bigint, num-traits, lazy_static
  • extensions/algebra/guest/Cargo.toml:19cfg(not(target_os = "none")) gates num-bigint, halo2curves-axiom
  • guest-libs/p256/Cargo.toml:77, guest-libs/k256/Cargo.toml:29, guest-libs/pairing/Cargo.toml:29
  • crates/toolchain/tests/Cargo.toml:33
  • extensions/ecc/tests/programs/Cargo.toml:39

These should be updated to use any(target_os = "none", target_os = "openvm") (or not(any(...)) for host-only gates). If these are intentionally deferred to a follow-up PR, please note that in the description.

Fix this →

3. Stale JSON target spec file not removed

crates/toolchain/build/riscv64im-openvm-none-elf.json — This file is no longer used since the target is now built-in, but it still exists. Should be deleted to avoid confusion.

4. Stale RUST_TARGET_PATH and rustc_target_spec_dir()

crates/toolchain/build/src/lib.rs:25-28, 287 — The docstring still says "Directory containing the target JSON" and RUST_TARGET_PATH is still set in cargo_command(). With a built-in target, this env var and the function are unused. Consider removing them (or at least updating the doc).


Suggestions (non-blocking)

5. Linked toolchain detection heuristic is fragile

crates/toolchain/build/src/lib.rs:404

let is_linked_toolchain = !toolchain_name.contains("nightly") 
    && !toolchain_name.contains("stable") 
    && !toolchain_name.contains("beta");

This negative heuristic could misclassify custom toolchain names that happen to contain "nightly" (e.g., my-nightly-fork) or miss linked toolchains with those substrings. A more robust approach would be to check rustup toolchain list output for the (linked) suffix, or to attempt rustup component list and handle the failure gracefully rather than guessing.

6. cargo_command public API breaking change

crates/toolchain/build/src/lib.rs:254 — The with_std: bool parameter was added to the public cargo_command function, which is re-exported by openvm-sdk. This is a breaking change for any external callers. Not a problem if this branch is not yet released, but worth noting for semver if it is.

7. exit_code::SUCCESS is defined but unused

crates/toolchain/openvm/src/pal_abi.rs:11 — Minor: SUCCESS = 0 is never referenced. Remove it or mark with #[allow(dead_code)] if it's part of the ABI contract.

8. sys_write diverges on unsupported FDs without annotation

crates/toolchain/openvm/src/pal_abi.rs:64-73 — The function signature returns () but terminates the VM for non-stdout/stderr FDs. This is correct zkVM behavior, but the caller (std) expects it to return. If the Rust fork's PAL contract allows panic/abort here, this is fine; just verify it matches.


Positive observations

  • The mechanical no_main cfg gate update across 100 guest files is comprehensive — zero old-style cfg_attr(target_os = "none", no_main) references remain in .rs files.
  • The entry! macro simplification (removing the std-feature conditional split) is clean — with target_os = "openvm", openvm_intrinsics is the single controlling axis.
  • The with_std build-std integration is minimal and well-placed.
  • The Cargo.lock changes are just transitive itertools version bumps — no concern.

Summary: The core changes are sound. The main concern is issue #2 — the Cargo.toml cfg gates for target_os = "none" in several crates were not updated, which could cause incorrect dependency resolution when building with the new target. Issue #1 (potential sys_rand buffer overflow) should be verified against the Rust fork's PAL contract.

@github-actions

This comment has been minimized.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can you use the develop v2 rc1 implementation for this and adapt to rv64. it's better if the diff is minimal and only contains the necessary changes and any fixes
https://github.com/openvm-org/openvm/blob/69373ff1d06b613025fe4e7a02d735a1609412c4/crates/toolchain/openvm/src/pal_abi.rs

Update the OpenVM build system and guest crates to support the new
built-in Rust target `riscv64im-unknown-openvm-elf` (target_os = "openvm").

Changes:
- Switch RUSTC_TARGET from JSON spec to built-in target
- Add pal_abi.rs bridging std PAL extern symbols to OpenVM custom instructions
- Fix entry! macro to work with std + openvm_intrinsics
- Add target_os = "openvm" to platform Cargo.toml dependency cfg gate
- Add std to build-std when std feature is requested
- Skip rustup component checks for linked toolchains
- Update no_main cfg gate across all guest programs

Tested: 18/19 riscv integration tests pass (including std tests).
One failure (test_tiny_mem_test) is an upstream embedded-alloc crate
incompatibility with newer Rust nightly.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Tuanlinh12312 Tuanlinh12312 force-pushed the rv64-openvm-target-integration branch from aafca0a to e8e2e8f Compare April 30, 2026 22:18
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 30, 2026

Claude finished @Tuanlinh12312's task in 3m 31s —— View job


Code Review: PR #2765 — Integrate riscv64im-unknown-openvm-elf target into build system

  • Gather PR diff and understand scope of changes
  • Review crates/toolchain/build/src/lib.rs — target switch and build-std changes
  • Review crates/toolchain/openvm/src/lib.rs — entry macro changes
  • Review crates/toolchain/openvm/src/pal_abi.rs — new PAL ABI bridge
  • Review crates/toolchain/platform/Cargo.toml — cfg gate update
  • Check for missed target_os = "none" references in Cargo.toml files
  • Review guest program no_main cfg gate changes (spot-check)
  • Check against develop-v2.1.0-rv64 base for reference implementation
  • Post comprehensive review

Overview

This PR switches the OpenVM guest target from a custom JSON spec (riscv64im-openvm-none-elf, target_os = "none") to a built-in Rust target (riscv64im-unknown-openvm-elf, target_os = "openvm"), adds the PAL ABI bridge for std support, updates the entry! macro, and mechanically updates no_main cfg gates across ~100 guest programs. The mechanical .rs changes are comprehensive — zero old-style cfg_attr(target_os = "none", no_main) references remain.


Issues

1. [must-fix] Reviewer request: pal_abi.rs should be based on the develop v2 rc1 implementation

Per @shuklaayush's review comment, this file should use the develop v2 rc1 implementation as the base and adapt only the RV64-specific changes. This minimizes the diff and ensures consistency with the reviewed/merged implementation. The current version appears to be independently written with different patterns (e.g., different exit code values, different error handling approaches).

2. [bug] Potential buffer overflow in sys_rand

crates/toolchain/openvm/src/pal_abi.rs:71-73

pub unsafe extern "C" fn sys_rand(recv_buf: *mut u32, words: usize) {
    hint_random(words);
    hint_buffer_chunked(recv_buf as *mut u8, words);
}

hint_random(len) generates len random u64 values (8 bytes each), and hint_buffer_chunked(ptr, num_dwords) reads num_dwords dwords (8 bytes each) — see extensions/riscv/guest/src/io.rs:40-46. The parameter name words with a *mut u32 buffer type suggests 4-byte word semantics. If the PAL contract passes buf.len() where buf: [u32; N], this writes N * 8 bytes into an N * 4-byte buffer — a 2x overwrite.

Verify the Rust fork's PAL ABI contract. If words means u32 words, the calls need (words + 1) / 2 dwords. If it means u64 dwords, the function signature is misleading.

3. [bug] Remaining target_os = "none" cfg gates in Cargo.toml files not updated

Several Cargo.toml files still use target_os = "none" which will resolve incorrectly for the new target_os = "openvm":

Guest-only deps that will be missed (gate evaluates to false for openvm target):

  • guest-libs/keccak256/Cargo.toml:16cfg(all(target_arch = "riscv64", target_os = "none")) gates spin dependency

Host-only deps that will leak into guest builds (gate evaluates to true for openvm target):

  • extensions/pairing/guest/Cargo.toml:29,35cfg(not(target_os = "none")) gates num-bigint, num-traits, lazy_static
  • extensions/algebra/guest/Cargo.toml:19cfg(not(target_os = "none")) gates num-bigint, halo2curves-axiom
  • guest-libs/p256/Cargo.toml:77, guest-libs/k256/Cargo.toml:29, guest-libs/pairing/Cargo.toml:29
  • crates/toolchain/tests/Cargo.toml:33
  • extensions/ecc/tests/programs/Cargo.toml:39

These should be updated to use any(target_os = "none", target_os = "openvm") (or not(any(...)) for host-only gates). If intentionally deferred, note it in the description.

Fix this →

4. [cleanup] Stale JSON target spec file not removed

crates/toolchain/build/riscv64im-openvm-none-elf.json still exists. Since the target is now built-in, this file is dead code and should be deleted to avoid confusion.

5. [cleanup] Stale RUST_TARGET_PATH and rustc_target_spec_dir()

crates/toolchain/build/src/lib.rs:25-28,287 — The docstring still says "Directory containing the target JSON" and RUST_TARGET_PATH is still set in cargo_command(). With a built-in target, this env var and function are no longer needed. Consider removing them or updating the doc.


Suggestions (non-blocking)

6. Linked toolchain detection heuristic is fragile

crates/toolchain/build/src/lib.rs:404

let is_linked_toolchain = !toolchain_name.contains("nightly") 
    && !toolchain_name.contains("stable") 
    && !toolchain_name.contains("beta");

This negative heuristic could misclassify custom toolchain names containing "nightly" (e.g., my-nightly-fork). A more robust approach: attempt rustup component list --toolchain <name> and handle failure gracefully, or check rustup toolchain list output for the (linked) suffix.

7. cargo_command public API breaking change

crates/toolchain/build/src/lib.rs:254 — The with_std: bool parameter was added to the public cargo_command function, which is re-exported via openvm-sdk (see crates/sdk/src/lib.rs:18). This is a breaking change for external callers. Consider whether this should be a GuestOptions field instead, or if the breaking change is acceptable for this branch.

8. sys_cycle_count and other TODO stubs

crates/toolchain/openvm/src/pal_abi.rs:96-99 — Several functions (sys_cycle_count, sys_read, sys_read_words, sys_getenv, sys_argc, sys_argv) print TODO messages and terminate. These are reasonable stubs, but note that sys_getenv returns 0 instead of usize::MAX (the documented "not set" return value per its own doc comment at line 175), which means callers will interpret "not set" as "set with 0-byte value" and try to read the value.


Positive observations

  • The mechanical no_main cfg gate update across all guest .rs files is comprehensive — zero old-style references remain.
  • The entry! macro simplification (removing the std-feature conditional split, using openvm_intrinsics as the single axis) is clean and correct.
  • The with_std build-std integration is minimal and well-placed.
  • No references to the old target name riscv64im-openvm-none-elf remain in .rs or .toml files.

Summary: The core approach is sound. The blocking items are: (1) rebasing pal_abi.rs on the develop v2 rc1 implementation per reviewer request, (2) verifying the sys_rand buffer size semantics, and (3) updating the remaining target_os = "none" cfg gates in Cargo.toml files to avoid incorrect dependency resolution with the new target.

@github-actions
Copy link
Copy Markdown

group app.proof_time_ms app.cycles leaf.proof_time_ms
fibonacci 1,894 4,000,051 536
keccak 13,608 14,365,133 2,220
sha2_bench 9,406 11,167,961 1,266
regex 1,574 4,090,656 376
ecrecover 637 112,210 287
pairing 744 592,827 280
kitchen_sink 2,069 1,979,971 435

Note: cells_used metrics omitted because CUDA tracegen does not expose unpadded trace heights.

Commit: e8e2e8f

Benchmark Workflow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants