From a776f27c7c9adc0512e1f0e2c329212c218085e3 Mon Sep 17 00:00:00 2001 From: martyall Date: Mon, 6 Jan 2025 08:36:41 -0800 Subject: [PATCH 1/5] add basic openmips tests using pattern found in cannon --- o1vm/src/cannon.rs | 20 +++++++ o1vm/src/cli/cannon.rs | 11 ++++ o1vm/src/interpreters/mips/witness.rs | 9 +++- o1vm/tests/test_mips_elf.rs | 76 +++++++++++++++++++++++++++ 4 files changed, 115 insertions(+), 1 deletion(-) create mode 100644 o1vm/tests/test_mips_elf.rs diff --git a/o1vm/src/cannon.rs b/o1vm/src/cannon.rs index a87e5e92a4..a888bf898d 100644 --- a/o1vm/src/cannon.rs +++ b/o1vm/src/cannon.rs @@ -217,9 +217,29 @@ pub struct VmConfiguration { pub proof_fmt: String, pub snapshot_fmt: String, pub pprof_cpu: bool, + pub halt_address: Option, pub host: Option, } +impl Default for VmConfiguration { + fn default() -> Self { + VmConfiguration { + input_state_file: "state.json".to_string(), + output_state_file: "out.json".to_string(), + metadata_file: None, + proof_at: StepFrequency::Never, + stop_at: StepFrequency::Never, + snapshot_state_at: StepFrequency::Never, + info_at: StepFrequency::Never, + proof_fmt: "proof-%d.json".to_string(), + snapshot_fmt: "state-%d.json".to_string(), + pprof_cpu: false, + halt_address: None, + host: None, + } + } +} + #[derive(Debug, Clone)] pub struct Start { pub time: std::time::Instant, diff --git a/o1vm/src/cli/cannon.rs b/o1vm/src/cli/cannon.rs index 9445be62d0..a8f8277550 100644 --- a/o1vm/src/cli/cannon.rs +++ b/o1vm/src/cli/cannon.rs @@ -61,6 +61,13 @@ pub struct MipsVmConfigurationArgs { )] snapshot_state_at: StepFrequency, + #[arg( + long = "halt-address", + value_name = "ADDR", + help = "halt address (in hexadecimal). Jumping to this address will halt the program." + )] + halt_address: Option, + #[arg(name = "host", value_name = "HOST", help = "host program specification [host program arguments]", num_args = 1.., last = true)] host: Vec, } @@ -78,6 +85,10 @@ impl From for VmConfiguration { proof_fmt: cfg.proof_fmt, snapshot_fmt: cfg.snapshot_fmt, pprof_cpu: cfg.pprof_cpu, + halt_address: cfg.halt_address.map(|s| { + u32::from_str_radix(s.trim_start_matches("0x"), 16) + .expect("Failed to parse halt address as hex") + }), host: if cfg.host.is_empty() { None } else { diff --git a/o1vm/src/interpreters/mips/witness.rs b/o1vm/src/interpreters/mips/witness.rs index b5fa8161a0..eb7db1b967 100644 --- a/o1vm/src/interpreters/mips/witness.rs +++ b/o1vm/src/interpreters/mips/witness.rs @@ -1168,9 +1168,16 @@ impl Env { self.instruction_counter = self.next_instruction_counter(); + config.halt_address.iter().for_each(|halt_address: &u32| { + if self.get_instruction_pointer() == (*halt_address as u64) { + debug!("Program jumped to halt address: {:#X}", halt_address); + self.halt = true; + } + }); + // Integer division by MAX_ACC to obtain the actual instruction count if self.halt { - println!( + debug!( "Halted at step={} instruction={:?}", self.normalized_instruction_counter(), opcode diff --git a/o1vm/tests/test_mips_elf.rs b/o1vm/tests/test_mips_elf.rs new file mode 100644 index 0000000000..64c08034f2 --- /dev/null +++ b/o1vm/tests/test_mips_elf.rs @@ -0,0 +1,76 @@ +use ark_ff::Field; +use mina_curves::pasta::Fp; +use o1vm::{ + cannon::{self, State, VmConfiguration}, + elf_loader::Architecture, + interpreters::mips::witness::{self}, + preimage_oracle::{NullPreImageOracle, PreImageOracleT}, +}; + +fn parse_state(fp: String) -> State { + let curr_dir = std::env::current_dir().unwrap(); + let path = curr_dir.join(std::path::PathBuf::from(fp)); + o1vm::elf_loader::parse_elf(Architecture::Mips, &path).unwrap() +} + +fn read_word(env: &mut witness::Env, addr: u32) -> u32 { + let bytes: [u8; 4] = [ + env.get_memory_direct(addr), + env.get_memory_direct(addr + 1), + env.get_memory_direct(addr + 2), + env.get_memory_direct(addr + 3), + ]; + u32::from_be_bytes(bytes) +} + +fn test_single_register_result(prgoram_name: &str, expected: u32) { + let base_dir = "resources/programs/mips/bin/"; + // this choice was taken from the cannon state_test.go, it is arbitrary + let halt_address = 0xa7ef00d0_u32; + let bin_file = format!("{}{}", base_dir, prgoram_name); + let mut state = parse_state(bin_file); + // each of the open mips tests end execution with the instruciton 'jr $ra'. As in cannon, we can + // set $ra to a specific value and test the program counter to signal the program has terminated. + state.registers[31] = halt_address; + let start = cannon::Start::create(state.step as usize); + let configuration = VmConfiguration { + halt_address: Some(halt_address), + ..Default::default() + }; + let mut witness = witness::Env::>::create( + cannon::PAGE_SIZE as usize, + state, + Box::new(NullPreImageOracle), + ); + while !witness.halt { + witness.step(&configuration, &None, &start); + } + // If you look at the tests, this is always used as a base address to find the registers for + // signaling the program exit flag and the return result + let return_register = 0xbffffff0_u32; + + let done_register = return_register + 4; + assert_eq!( + read_word(&mut witness, return_register + 4), + 1, + "Expected done register to be set to 1, got {}", + done_register + ); + + let result_register = return_register + 8; + assert_eq!( + read_word(&mut witness, result_register), + expected, + "Program {} failure: expected result register to be {}, got {}", + prgoram_name, + expected, + result_register + ); + println!("Program {} passed", prgoram_name); +} + +#[test] +fn open_mips_tests_execution() { + test_single_register_result("nor", 0x1); + test_single_register_result("jr", 0x1); +} From 367517d1f7e1f3eaabcd100ad7e6ca0b0f3d07c0 Mon Sep 17 00:00:00 2001 From: martyall Date: Mon, 6 Jan 2025 13:28:59 -0800 Subject: [PATCH 2/5] run the open mips tests that we can manage --- .github/workflows/mips-build.yml | 13 ++- o1vm/Cargo.toml | 3 + o1vm/test-gen-state-json.sh | 14 --- o1vm/tests/test_mips_elf.rs | 148 +++++++++++++++++++------------ 4 files changed, 103 insertions(+), 75 deletions(-) delete mode 100755 o1vm/test-gen-state-json.sh diff --git a/.github/workflows/mips-build.yml b/.github/workflows/mips-build.yml index 264a485ee4..61f72aae0f 100644 --- a/.github/workflows/mips-build.yml +++ b/.github/workflows/mips-build.yml @@ -1,7 +1,11 @@ -name: MIPS Build and Package +name: MIPS Build, Test, and Package on: workflow_dispatch: + pull_request: + push: + branches: + - master jobs: build: @@ -36,8 +40,9 @@ jobs: with: rust_toolchain_version: ${{ matrix.rust_toolchain_version }} - - name: Test elf_loader against mips programs - run: ./o1vm/test-gen-state-json.sh + - name: Test interpreter against mips programs + run: | + cargo test --features open_mips --test test_mips_elf -- --nocapture - name: Create tar archive run: | @@ -48,4 +53,4 @@ jobs: uses: actions/upload-artifact@v4 with: name: mips-binaries - path: o1vm/resources/programs/mips/mips-binaries.tar.gz + path: o1vm/resources/programs/mips/mips-binaries.tar.gz \ No newline at end of file diff --git a/o1vm/Cargo.toml b/o1vm/Cargo.toml index a91b75d54c..ac2a7016d4 100644 --- a/o1vm/Cargo.toml +++ b/o1vm/Cargo.toml @@ -20,6 +20,9 @@ path = "src/legacy/main.rs" name = "pickles_o1vm" path = "src/pickles/main.rs" +[features] +open_mips = [] + [dependencies] # FIXME: Only activate this when legacy_o1vm is built ark-bn254.workspace = true diff --git a/o1vm/test-gen-state-json.sh b/o1vm/test-gen-state-json.sh deleted file mode 100755 index 61a4387c7c..0000000000 --- a/o1vm/test-gen-state-json.sh +++ /dev/null @@ -1,14 +0,0 @@ -#!/bin/bash - -BIN_DIR=${1:-${BIN_DIR:-o1vm/resources/programs/mips/bin}} - -# Ensure the directory exists -if [[ ! -d "$BIN_DIR" ]]; then - echo "Error: Directory '$BIN_DIR' does not exist." - exit 1 -fi - -find "$BIN_DIR" -type f ! -name "*.o" | while read -r file; do - echo "Processing: $file" - cargo run --bin pickles_o1vm -- cannon gen-state-json -i "$file" -done diff --git a/o1vm/tests/test_mips_elf.rs b/o1vm/tests/test_mips_elf.rs index 64c08034f2..9d8c2fc5c5 100644 --- a/o1vm/tests/test_mips_elf.rs +++ b/o1vm/tests/test_mips_elf.rs @@ -6,71 +6,105 @@ use o1vm::{ interpreters::mips::witness::{self}, preimage_oracle::{NullPreImageOracle, PreImageOracleT}, }; +use std::{ + fs, + path::{Path, PathBuf}, +}; -fn parse_state(fp: String) -> State { - let curr_dir = std::env::current_dir().unwrap(); - let path = curr_dir.join(std::path::PathBuf::from(fp)); - o1vm::elf_loader::parse_elf(Architecture::Mips, &path).unwrap() +struct MipsTest { + bin_file: PathBuf, } -fn read_word(env: &mut witness::Env, addr: u32) -> u32 { - let bytes: [u8; 4] = [ - env.get_memory_direct(addr), - env.get_memory_direct(addr + 1), - env.get_memory_direct(addr + 2), - env.get_memory_direct(addr + 3), - ]; - u32::from_be_bytes(bytes) +// currently excluding any oracle based tests and a select group of tests that are failing +fn is_test_excluded(bin_file: &Path) -> bool { + let file_name = bin_file.file_name().unwrap().to_str().unwrap(); + let untested_programs = ["exit_group", "jal", "mul", "jalr"]; + file_name.starts_with("oracle") || untested_programs.contains(&file_name) } -fn test_single_register_result(prgoram_name: &str, expected: u32) { - let base_dir = "resources/programs/mips/bin/"; - // this choice was taken from the cannon state_test.go, it is arbitrary - let halt_address = 0xa7ef00d0_u32; - let bin_file = format!("{}{}", base_dir, prgoram_name); - let mut state = parse_state(bin_file); - // each of the open mips tests end execution with the instruciton 'jr $ra'. As in cannon, we can - // set $ra to a specific value and test the program counter to signal the program has terminated. - state.registers[31] = halt_address; - let start = cannon::Start::create(state.step as usize); - let configuration = VmConfiguration { - halt_address: Some(halt_address), - ..Default::default() - }; - let mut witness = witness::Env::>::create( - cannon::PAGE_SIZE as usize, - state, - Box::new(NullPreImageOracle), - ); - while !witness.halt { - witness.step(&configuration, &None, &start); +impl MipsTest { + fn parse_state(&self) -> State { + let curr_dir = std::env::current_dir().unwrap(); + let path = curr_dir.join(&self.bin_file); + o1vm::elf_loader::parse_elf(Architecture::Mips, &path).unwrap() + } + + fn read_word(env: &mut witness::Env, addr: u32) -> u32 { + let bytes: [u8; 4] = [ + env.get_memory_direct(addr), + env.get_memory_direct(addr + 1), + env.get_memory_direct(addr + 2), + env.get_memory_direct(addr + 3), + ]; + u32::from_be_bytes(bytes) } - // If you look at the tests, this is always used as a base address to find the registers for - // signaling the program exit flag and the return result - let return_register = 0xbffffff0_u32; - let done_register = return_register + 4; - assert_eq!( - read_word(&mut witness, return_register + 4), - 1, - "Expected done register to be set to 1, got {}", - done_register - ); + fn run(&self) -> Result<(), String> { + let mut state = self.parse_state(); + let halt_address = 0xa7ef00d0_u32; + state.registers[31] = halt_address; + + let start = cannon::Start::create(state.step as usize); + let configuration = VmConfiguration { + halt_address: Some(halt_address), + ..Default::default() + }; + + let mut witness = witness::Env::>::create( + cannon::PAGE_SIZE as usize, + state, + Box::new(NullPreImageOracle), + ); + + while !witness.halt { + witness.step(&configuration, &None, &start); + } + + let return_register = 0xbffffff0_u32; + let done_register = return_register + 4; + let result_register = return_register + 8; - let result_register = return_register + 8; - assert_eq!( - read_word(&mut witness, result_register), - expected, - "Program {} failure: expected result register to be {}, got {}", - prgoram_name, - expected, - result_register - ); - println!("Program {} passed", prgoram_name); + let done_value = Self::read_word(&mut witness, done_register); + if done_value != 1 { + return Err(format!( + "Expected done register to be set to 1, got {:#x}", + done_value + )); + } + + let result_value = Self::read_word(&mut witness, result_register); + if result_value != 1 { + return Err(format!( + "Program {:?} failure: expected result register to contain 1, got {:#x}", + self.bin_file, result_value + )); + } + + Ok(()) + } } -#[test] -fn open_mips_tests_execution() { - test_single_register_result("nor", 0x1); - test_single_register_result("jr", 0x1); +#[cfg(test)] +mod tests { + use super::*; + + #[test] + #[cfg_attr(not(feature = "open_mips"), ignore)] + fn open_mips_tests() { + let test_dir = "resources/programs/mips/bin"; + let test_files: Vec = fs::read_dir(test_dir) + .unwrap_or_else(|_| panic!("Error reading directory {}", test_dir)) + .filter_map(|entry| entry.ok()) + .map(|entry| entry.path()) + .filter(|f| f.is_file() && f.extension().is_none() && !is_test_excluded(f)) + .map(|f| MipsTest { bin_file: f }) + .collect(); + + for test in test_files { + let test_name = test.bin_file.file_name().unwrap().to_str().unwrap(); + if let Err(err) = test.run() { + panic!("Test '{}' failed: {}", test_name, err); + } + } + } } From 849d9ae40b41b92c1fad317271224182464654c2 Mon Sep 17 00:00:00 2001 From: martyall Date: Mon, 6 Jan 2025 13:46:44 -0800 Subject: [PATCH 3/5] relable o1vm mips ci --- .github/workflows/{mips-build.yml => o1vm-mips-ci.yml} | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) rename .github/workflows/{mips-build.yml => o1vm-mips-ci.yml} (93%) diff --git a/.github/workflows/mips-build.yml b/.github/workflows/o1vm-mips-ci.yml similarity index 93% rename from .github/workflows/mips-build.yml rename to .github/workflows/o1vm-mips-ci.yml index 61f72aae0f..83b52a055a 100644 --- a/.github/workflows/mips-build.yml +++ b/.github/workflows/o1vm-mips-ci.yml @@ -1,4 +1,4 @@ -name: MIPS Build, Test, and Package +name: ov1m CI on: workflow_dispatch: @@ -8,7 +8,8 @@ on: - master jobs: - build: + build_test_package: + name: Build, test, and package simple MIPS programs runs-on: ubuntu-latest strategy: matrix: From a7f71e2e62213bd1624332d3d0439e83f7e6a30b Mon Sep 17 00:00:00 2001 From: martyall Date: Tue, 7 Jan 2025 08:11:51 -0800 Subject: [PATCH 4/5] fix Makefile problem with jal and jalr tests --- Makefile | 2 +- o1vm/tests/test_mips_elf.rs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 43f57c5021..1e6ff2f289 100644 --- a/Makefile +++ b/Makefile @@ -181,7 +181,7 @@ ${O1VM_MIPS_SOURCE_DIR}/%.asm: ${OPTIMISM_MIPS_SOURCE_DIR}/%.asm @echo "Transforming $< to $@, making it compatible for o1vm" @sed \ -e '/\.balign 4/d' \ - -e '/\.set\s*noreorder/d' \ + -e 's/^\s*\.set\s*noreorder/.set noreorder/' \ -e '/\.ent\s*test/d' \ -e '/\.end test/d' \ -e 's/\.section .test, "x"/.section .text/' \ diff --git a/o1vm/tests/test_mips_elf.rs b/o1vm/tests/test_mips_elf.rs index 9d8c2fc5c5..04ec89d336 100644 --- a/o1vm/tests/test_mips_elf.rs +++ b/o1vm/tests/test_mips_elf.rs @@ -18,7 +18,7 @@ struct MipsTest { // currently excluding any oracle based tests and a select group of tests that are failing fn is_test_excluded(bin_file: &Path) -> bool { let file_name = bin_file.file_name().unwrap().to_str().unwrap(); - let untested_programs = ["exit_group", "jal", "mul", "jalr"]; + let untested_programs = ["exit_group", "mul"]; file_name.starts_with("oracle") || untested_programs.contains(&file_name) } @@ -40,6 +40,7 @@ impl MipsTest { } fn run(&self) -> Result<(), String> { + println!("Running test: {:?}", self.bin_file); let mut state = self.parse_state(); let halt_address = 0xa7ef00d0_u32; state.registers[31] = halt_address; @@ -47,6 +48,7 @@ impl MipsTest { let start = cannon::Start::create(state.step as usize); let configuration = VmConfiguration { halt_address: Some(halt_address), + stop_at: cannon::StepFrequency::Exactly(1000), ..Default::default() }; From 4fa1f2641735147b7cac6f5ce99fa0fb41bed38e Mon Sep 17 00:00:00 2001 From: martyall Date: Tue, 7 Jan 2025 08:53:05 -0800 Subject: [PATCH 5/5] make mips build an action so can be reused --- .github/actions/build-mips/action.yaml | 22 ++++++++ .github/workflows/ci-nightly.yml | 3 ++ .github/workflows/ci.yml | 3 ++ .github/workflows/o1vm-mips-ci.yml | 57 -------------------- .github/workflows/o1vm-upload-mips-build.yml | 28 ++++++++++ 5 files changed, 56 insertions(+), 57 deletions(-) create mode 100644 .github/actions/build-mips/action.yaml delete mode 100644 .github/workflows/o1vm-mips-ci.yml create mode 100644 .github/workflows/o1vm-upload-mips-build.yml diff --git a/.github/actions/build-mips/action.yaml b/.github/actions/build-mips/action.yaml new file mode 100644 index 0000000000..26dda82f20 --- /dev/null +++ b/.github/actions/build-mips/action.yaml @@ -0,0 +1,22 @@ +name: 'Build MIPS Programs' +description: 'Builds MIPS programs for testing' + +runs: + using: "composite" + steps: + - name: Cache apt packages + uses: actions/cache@v4 + with: + path: | + /var/cache/apt/archives/*.deb + key: ${{ runner.os }}-apt-${{ hashFiles('.github/workflows/o1vm-mips-build.yml') }} + + - name: Install MIPS tools + shell: bash + run: | + sudo apt-get update + sudo apt-get install -y binutils-mips-linux-gnu + + - name: Build MIPS programs + shell: bash + run: make build-mips-programs \ No newline at end of file diff --git a/.github/workflows/ci-nightly.yml b/.github/workflows/ci-nightly.yml index 8da92c5de5..3d4434b538 100644 --- a/.github/workflows/ci-nightly.yml +++ b/.github/workflows/ci-nightly.yml @@ -52,6 +52,9 @@ jobs: run: | make install-test-deps + - name: Build the MIPS binaries + uses: ./.github/actions/build-mips + - name: Run all tests with the code coverage run: | eval $(opam env) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 09b91f2ab9..97502dd7a1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -162,6 +162,9 @@ jobs: run: | make install-test-deps + - name: Build the MIPS binaries + uses: ./.github/actions/build-mips + - name: Doc tests if: ${{ matrix.rust_toolchain_version != env.RUST_TOOLCHAIN_COVERAGE_VERSION }} run: | diff --git a/.github/workflows/o1vm-mips-ci.yml b/.github/workflows/o1vm-mips-ci.yml deleted file mode 100644 index 83b52a055a..0000000000 --- a/.github/workflows/o1vm-mips-ci.yml +++ /dev/null @@ -1,57 +0,0 @@ -name: ov1m CI - -on: - workflow_dispatch: - pull_request: - push: - branches: - - master - -jobs: - build_test_package: - name: Build, test, and package simple MIPS programs - runs-on: ubuntu-latest - strategy: - matrix: - rust_toolchain_version: ["1.74"] - - steps: - - uses: actions/checkout@v4 - with: - submodules: 'recursive' - - - name: Cache apt packages - id: apt-cache - uses: actions/cache@v4 - with: - path: | - /var/cache/apt/archives/*.deb - key: ${{ runner.os }}-apt-${{ hashFiles('.github/workflows/mips-build.yml') }} - - - name: Install MIPS tools - run: | - sudo apt-get update - sudo apt-get install -y binutils-mips-linux-gnu - - - name: Build MIPS programs - run: make build-mips-programs - - - name: Use shared Rust toolchain setting up steps - uses: ./.github/actions/toolchain-shared - with: - rust_toolchain_version: ${{ matrix.rust_toolchain_version }} - - - name: Test interpreter against mips programs - run: | - cargo test --features open_mips --test test_mips_elf -- --nocapture - - - name: Create tar archive - run: | - cd o1vm/resources/programs/mips - tar -czf mips-binaries.tar.gz bin/ - - - name: Upload artifacts - uses: actions/upload-artifact@v4 - with: - name: mips-binaries - path: o1vm/resources/programs/mips/mips-binaries.tar.gz \ No newline at end of file diff --git a/.github/workflows/o1vm-upload-mips-build.yml b/.github/workflows/o1vm-upload-mips-build.yml new file mode 100644 index 0000000000..72e3e77cb6 --- /dev/null +++ b/.github/workflows/o1vm-upload-mips-build.yml @@ -0,0 +1,28 @@ +name: Upload MIPS Binaries + +on: + workflow_dispatch: + +jobs: + build_and_upload: + name: Build and Upload MIPS Programs + runs-on: ubuntu-latest + steps: + + - uses: actions/checkout@v4 + with: + submodules: 'recursive' + + - name: Build MIPS binaries + uses: ./.github/actions/build-mips + + - name: Create tar archive + run: | + cd o1vm/resources/programs/mips + tar -czf mips-binaries.tar.gz bin/ + + - name: Upload artifacts + uses: actions/upload-artifact@v4 + with: + name: mips-binaries + path: o1vm/resources/programs/mips/mips-binaries.tar.gz \ No newline at end of file