Add support for Sail unit tests #710

Timmmm · 2025-02-05T20:47:34Z

This adds basic support for Sail unit tests. These have some pros and cons over C tests:

Pros:

They don't require a RISC-V C compiler.
They can test internal code (hence why I've called them unit tests).
They can "cheat", e.g. to change privilege mode you can just magically cur_privilege = Supervisor. With C you have to implement a syscall, etc.

Cons:

They are tied to internal code which will make refactoring more tedious because it means you are likely to need to update the tests. We can minimise that by using "high level" functions like execute(), read_CSR(), etc.
They only run on the Sail model, so we can't e.g. verify them against SPIKE.

Given the sorry state of testing in this repo, and the fact that we don't have a way of writing C tests at all yet, I think this is probably a good medium term approach.

Currently the unit test executable reuses a load of code from the emulator, which is not ideal. When we have wrapped the model in a nice C++ library we can use that instead.

jordancarlin · 2025-02-06T00:40:58Z

Feels like these have the potential to be a lower barrier of entry for writing certain kinds of tests,(especially for tricky to reach edge cases of the system), so seems like a good idea to me. Once we get the C/assembly test situation sorted out we can think about what makes sense to run as a C test versus a unit test, but having both options seems like a net positive.

jordancarlin · 2025-04-03T20:13:16Z

@Timmmm Now that Sail 0.19 is released with unit test support, this is something that can be finished up if we still want it. Not sure that there are a whole lot of tests that should exist here instead of as first party assembly tests, but seems reasonable to include support for them.

github-actions · 2025-05-13T10:51:12Z

Test Results

401 tests +1 401 ✅ +1 1m 32s ⏱️ +3s
1 suites ±0 0 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 359c327. ± Comparison against base commit d8c3006.

♻️ This comment has been updated with latest results.

CMakeLists.txt

test/unit_tests/main_unit_tests.cpp

model/CMakeLists.txt

jordancarlin

Might be worth adding a README to the test/unit_tests directory explaining that the tests themselves are in the model/unit_tests directory or directly inline with the model. Otherwise, LGTM. Great to get more testing in!

Timmmm · 2025-06-13T12:09:09Z

Rebased & README added.

Timmmm · 2025-06-13T13:26:50Z

I'll see if I can sort out the Lean errors.

Timmmm · 2025-06-16T14:02:23Z

Rebased but it's broken now and awkward to fix due to using the Sail generated header. Will probably have to wait until runtime XLEN is merged.

Timmmm · 2025-07-01T13:17:04Z

Rebased & tweaked a little to handle the changes since. I'll give this a couple of days before merging in case anyone wants to take a look.

test/unit_tests/main_unit_tests.cpp

Timmmm · 2025-07-01T15:07:41Z

Ah yeah good call - also since the XLEN stuff was merged I can move the generated C into the library. I renamed it to riscv_model since it's not just the support code now.

jordancarlin · 2025-07-02T05:52:39Z

Seems to have broken Lean:

error: build failed
✖ [45/46] Building LeanRV64D
trace: .> LEAN_PATH=/home/runner/work/sail-riscv/sail-riscv/build/model/Lean_RV64D/.lake/build/lib/lean /home/runner/.elan/toolchains/leanprover--lean4-nightly---nightly-2025-06-21/bin/lean --tstack=400000 /home/runner/work/sail-riscv/sail-riscv/build/model/Lean_RV64D/LeanRV64D.lean -R /home/runner/work/sail-riscv/sail-riscv/build/model/Lean_RV64D -o /home/runner/work/sail-riscv/sail-riscv/build/model/Lean_RV64D/.lake/build/lib/lean/LeanRV64D.olean -i /home/runner/work/sail-riscv/sail-riscv/build/model/Lean_RV64D/.lake/build/lib/lean/LeanRV64D.ilean -c /home/runner/work/sail-riscv/sail-riscv/build/model/Lean_RV64D/.lake/build/ir/LeanRV64D.c --setup /home/runner/work/sail-riscv/sail-riscv/build/model/Lean_RV64D/.lake/build/ir/LeanRV64D.setup.json --json
error: LeanRV64D.lean:171:32: unknown identifier 'undefined_RVFI_DII_Instruction_Packet'
error: LeanRV64D.lean:172:30: unknown identifier 'undefined_RVFI_DII_Execution_Packet_InstMetaData'
error: LeanRV64D.lean:173:28: unknown identifier 'undefined_RVFI_DII_Execution_Packet_PC'
error: LeanRV64D.lean:174:29: unknown identifier 'undefined_RVFI_DII_Execution_Packet_Ext_Integer'
error: LeanRV64D.lean:176:29: unknown identifier 'undefined_RVFI_DII_Execution_Packet_Ext_MemAccess'
error: LeanRV64D.lean:355:91: unknown identifier 'sail_main'
error: Lean exited with code 1

This adds basic support for Sail unit tests. These have some pros and cons over C tests: Pros: * They don't require a RISC-V C compiler. * They can test internal code (hence why I've called them unit tests). * They can "cheat", e.g. to change privilege mode you can just magically `cur_privilege = Supervisor`. With C you have to implement a syscall, etc. Cons: * They are tied to internal code which will make refactoring more tedious because it means you are likely to need to update the tests. We can minimise that by using "high level" functions like `execute()`, `read_CSR()`, etc. * They only run on the Sail model, so we can't e.g. verify them against SPIKE. Currently the unit test executable reuses a load of code from the emulator, which is not ideal. When we have wrapped the model in a nice C++ library we can use that instead.

Alasdair · 2025-07-02T14:23:43Z

That's the same issue I've been having with my modules PR, so maybe the issue is unrelated if it's here too

Timmmm · 2025-07-03T11:18:14Z

Ok I had a look into this and the issue is that Lean is treating the very last function in the code as special. On master the LeanRV64D.lean file starts with

import LeanRV64D.Main

but on this branch it starts with

import LeanRV64D.TestMstatus

So the problem is I added a file after main.sail. I guess your PR does the same thing?

I'll reorder the files in this PR since it doesn't make much difference but that seems like something that should be fixed.

jordancarlin · 2025-07-07T14:06:08Z

test/unit_tests/main_unit_tests.cpp

+
+  sail_config_set_string(DEFAULT_JSON);
+  sail_set_abstract_xlen();
+  sail_set_abstract_ext_d_supported();


Might eventually be worth factoring these abstract type initializations out so they don't get out of sync with the main simulator as we add new abstract types (vlen, etc.). Doesn't need to happen in this PR though.

Same with the config_* and trace_log.

Yeah I still don't quite understand why they aren't called automatically in model_init().

Maybe they could be? If we ensured calling them is idempotent moving them into model_init shouldn't be a breaking change. I may have been overthinking some ordering issue between setting up abstract types and initialising letbindings and registers.

Slightly different topic: is there a reason for model_init() not being included in the generated C header?

pmundkur

It would be great to finally get this in!

pmundkur · 2025-07-08T16:26:02Z

test/unit_tests/main_unit_tests.cpp

+
+  sail_config_set_string(DEFAULT_JSON);
+  sail_set_abstract_xlen();
+  sail_set_abstract_ext_d_supported();


Same with the config_* and trace_log.

Timmmm force-pushed the user/timh/unit_tests branch from 39c5491 to 6598087 Compare May 13, 2025 09:52

Timmmm marked this pull request as ready for review May 13, 2025 09:53

Timmmm force-pushed the user/timh/unit_tests branch from 6598087 to e73b81d Compare May 13, 2025 10:31

jordancarlin reviewed May 13, 2025

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

test/unit_tests/main_unit_tests.cpp Outdated Show resolved Hide resolved

model/CMakeLists.txt Outdated Show resolved Hide resolved

Timmmm force-pushed the user/timh/unit_tests branch 3 times, most recently from 8a55801 to a1d9603 Compare May 23, 2025 10:00

jordancarlin approved these changes May 24, 2025

View reviewed changes

Timmmm force-pushed the user/timh/unit_tests branch from a1d9603 to d4e8acd Compare June 13, 2025 12:08

Timmmm added will be merged Scheduled to be merged soon if nobody objects and removed will be merged Scheduled to be merged soon if nobody objects labels Jun 13, 2025

Timmmm force-pushed the user/timh/unit_tests branch from d4e8acd to 272ea4a Compare June 16, 2025 14:01

Timmmm force-pushed the user/timh/unit_tests branch from 272ea4a to bea6229 Compare July 1, 2025 13:15

Timmmm added the will be merged Scheduled to be merged soon if nobody objects label Jul 1, 2025

jordancarlin requested changes Jul 1, 2025

View reviewed changes

test/unit_tests/main_unit_tests.cpp Outdated Show resolved Hide resolved

Timmmm removed the will be merged Scheduled to be merged soon if nobody objects label Jul 1, 2025

Timmmm force-pushed the user/timh/unit_tests branch from 1a02bb3 to 80ec84e Compare July 2, 2025 08:00

Reorder files to work around Lean issue

359c327

Timmmm force-pushed the user/timh/unit_tests branch from 80ec84e to 359c327 Compare July 3, 2025 11:17

Timmmm requested a review from jordancarlin July 7, 2025 12:07

jordancarlin approved these changes Jul 7, 2025

View reviewed changes

pmundkur approved these changes Jul 8, 2025

View reviewed changes

pmundkur added the will be merged Scheduled to be merged soon if nobody objects label Jul 8, 2025

Timmmm added this pull request to the merge queue Jul 9, 2025

Merged via the queue into riscv:master with commit 576ce07 Jul 9, 2025
7 checks passed

Timmmm deleted the user/timh/unit_tests branch July 14, 2025 09:55

Add support for Sail unit tests #710

Add support for Sail unit tests #710

Uh oh!

Conversation

Timmmm commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jordancarlin commented Feb 6, 2025

Uh oh!

jordancarlin commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jordancarlin left a comment

Choose a reason for hiding this comment

Uh oh!

Timmmm commented Jun 13, 2025

Uh oh!

Timmmm commented Jun 13, 2025

Uh oh!

Timmmm commented Jun 16, 2025

Uh oh!

Timmmm commented Jul 1, 2025

Uh oh!

Uh oh!

Timmmm commented Jul 1, 2025

Uh oh!

jordancarlin commented Jul 2, 2025

Uh oh!

Alasdair commented Jul 2, 2025

Uh oh!

Timmmm commented Jul 3, 2025

Uh oh!

jordancarlin Jul 7, 2025

Choose a reason for hiding this comment

Uh oh!

pmundkur Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Timmmm Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Alasdair Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

pmundkur Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

pmundkur left a comment

Choose a reason for hiding this comment

Uh oh!

pmundkur Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Timmmm commented Feb 5, 2025 •

edited

Loading

jordancarlin commented Apr 3, 2025 •

edited

Loading

github-actions bot commented May 13, 2025 •

edited

Loading