Fix: Auidit report changes#19
Merged
Merged
Conversation
DecodeA64: .D atomics used decimal funct5 + rs2=rs1; add 0b prefix and use rs2
DecodeI: RV64 shamt now 6-bit (inst[25:20]); ExecuteM64: DIVUW/REMUW use low-32 uint32
Add edge-case tests for .D atomics, shamt 32/63, DIVUW/REMUW high bits
ExecuteA/A64: LR/SC reservation so SC can fail, 32-bit AMO.W rs2, trap misaligned atomics Run.fs: load all PT_LOAD segments (code/data/zero-filled bss) for 32- and 64-bit ELF Add tests: LR/SC, AMO.W width, alignment, ELF32/64 load, runCycle integration
…test coverage Robustness: run-loop step-limit (StepLimit trap), ELF/run exception handling, RV32 DIV/REM overflow check, -v trace Fixes: CLI long-key value arg-index, immutable setRegister (Array.copy); hygiene (module rename, typo, StartsWith, doc comments) Version: <Version>0.5.0 in fsproj as single source, read from assembly + dynamic year; DecodeM tuple match -> 100% line coverage
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughImplements LR/SC reservation semantics and alignment checks for A-extension, fixes decoding and arithmetic bugs, switches ELF loading to PT_LOAD, adds bounded execution with a StepLimit, updates CLI/versioning and CI to collect/upload coverage, and adds extensive unit and integration tests validating these behaviors. ChangesRISC-V Simulator Atomics and Corrections
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Tests/run/run.fs (1)
105-113: 💤 Low valueConsider using
Assert.Throwsfor clearer exception testing.The current approach works but is less idiomatic for xUnit. Using
Assert.Throws<exn>would make the test intent clearer and provide better failure messages.♻️ Suggested refactor
[<Fact>] let ``readElfFile throws on a non-ELF file`` () = - let path = Path.GetTempFileName() - try - File.WriteAllBytes(path, [| 0uy; 1uy; 2uy; 3uy |]) - let threw = try (Run.readElfFile path |> ignore; false) with _ -> true - Assert.True(threw) - finally - File.Delete path + withTempElf [| 0uy; 1uy; 2uy; 3uy |] (fun path -> + Assert.ThrowsAny<exn>(fun () -> Run.readElfFile path |> ignore) |> ignore)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/run/run.fs` around lines 105 - 113, Replace the manual try/with boolean flag with xUnit's Assert.Throws to make the test clearer: in the test ``readElfFile throws on a non-ELF file`` call Assert.Throws<exn>(fun () -> Run.readElfFile path |> ignore) (keeping the File.WriteAllBytes setup and File.Delete cleanup in the finally block) so the assertion directly checks that Run.readElfFile throws an exception.risc-v.fsproj (1)
21-24: ⚡ Quick winConsider removing coverlet.collector from the main project.
coverlet.collectoris a test-time dependency and is already included inTests/Tests.fsproj(lines 11-14). Adding it to the main executable project (risc-v.fsproj) is unconventional and may be unnecessary duplication. Coverage collection is typically handled by the test project alone.♻️ Proposed fix to remove the duplicate dependency
<ItemGroup> - <PackageReference Include="coverlet.collector" Version="10.0.1"> - <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> - <PrivateAssets>all</PrivateAssets> - </PackageReference> <PackageReference Include="ELFSharp" Version="2.17.3" /> <PackageReference Update="FSharp.Core" Version="10.0.107" /> </ItemGroup>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@risc-v.fsproj` around lines 21 - 24, Remove the test-only package reference from the main project: delete the PackageReference element for "coverlet.collector" in the risc-v.fsproj file (the block containing Include="coverlet.collector" Version="10.0.1"), since coverage is already declared in Tests/Tests.fsproj and this duplication is unnecessary; after removal, restore/verify builds and test coverage run from the test project only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/dotnet.yml:
- Around line 29-32: The GitHub Actions step named "Upload coverage reports to
Codecov" currently pins the action by tag (`uses: codecov/codecov-action@v5`);
replace that tag with the corresponding commit SHA for the desired release
(e.g., `uses: codecov/codecov-action@<commit-sha>`) to prevent supply-chain
drift, leaving the token input (`token: ${{ secrets.CODECOV_TOKEN }}`)
unchanged; obtain the exact SHA from the codecov/codecov-action releases page
and update the `uses:` value in the same step.
In `@CLI.fs`:
- Around line 6-10: The current version computation uses
GetExecutingAssembly().GetName().Version which can be null; update the let
version binding to handle a null Version by checking the returned value (symbol:
version and local v) and falling back to a safe default string like "v0.0.0" (or
use individual null-coalesced numbers), e.g. match v with null -> fallback | _
-> sprintf "v%d.%d.%d" v.Major v.Minor v.Build; ensure you only access
v.Major/v.Minor/v.Build when v is non-null to avoid NullReferenceException.
In `@Program.fs`:
- Around line 16-20: The error handler accesses x.Files.Value.[0] without
ensuring the array is non-empty, which can raise IndexOutOfRangeException;
update the with ex -> block (the exception handling for Run.Run x) to safely
obtain a first-file string by checking x.Files (and the inner array length) or
pattern-matching the array and falling back to a placeholder like "<no files>"
before using it in the printfn error message so the logger never indexes an
empty array.
In `@README.md`:
- Line 130: The Markdown heading jumps two levels (from h2 to "#### Supported
`.NET 10`"); change the heading level to increment by one by replacing "####
Supported `.NET 10`" with "### Supported `.NET 10`" so it follows the previous
h2/h3 sequence and adheres to proper heading hierarchy.
---
Nitpick comments:
In `@risc-v.fsproj`:
- Around line 21-24: Remove the test-only package reference from the main
project: delete the PackageReference element for "coverlet.collector" in the
risc-v.fsproj file (the block containing Include="coverlet.collector"
Version="10.0.1"), since coverage is already declared in Tests/Tests.fsproj and
this duplication is unnecessary; after removal, restore/verify builds and test
coverage run from the test project only.
In `@Tests/run/run.fs`:
- Around line 105-113: Replace the manual try/with boolean flag with xUnit's
Assert.Throws to make the test clearer: in the test ``readElfFile throws on a
non-ELF file`` call Assert.Throws<exn>(fun () -> Run.readElfFile path |> ignore)
(keeping the File.WriteAllBytes setup and File.Delete cleanup in the finally
block) so the assertion directly checks that Run.readElfFile throws an
exception.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58a40d32-707f-4b00-9195-a32d02b89aff
📒 Files selected for processing (29)
.github/workflows/dotnet.yml.gitignore.travis.ymlArch.fsCLI.fsDecodeA64.fsDecodeI.fsDecodeM.fsExecuteA.fsExecuteA64.fsExecuteM.fsExecuteM64.fsMachineState.fsProgram.fsREADME.mdRun.fsTests/Tests.fsprojTests/run/run.fsTests/rv64a/amo.fsTests/rv64a/amoD.fsTests/rv64a/amoSem.fsTests/rv64i/alui.fsTests/rv64m/alu.fsTests/unit/app.fsTests/unit/cli.fsTests/unit/decode.fsTests/unit/execute.fsTests/unit/units.fsrisc-v.fsproj
💤 Files with no reviewable changes (1)
- .travis.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores