RISC-V: disassembler incorrectly decodes CSRRx instructions

[gregoral]

RISC-V: disassembler incorrectly decodes CSRRx instructions

[uxmal]

Hello @gregoral, and thanks for chasing down this bug! I've reviewed the code
and your comments, and your proposed fix is correct: the affected instructions
need to decode their GP register number from bit position 15, which corresponds
to the r1 mutator in the RiscV disassembler.

Could you please give me some hints, about the process to submit a patch for this.
I would also try to add new tests for these instructions.

Hints written below. I'm unaware of your familiarity with Git workflows: I apologize
in advance if I've dumbed it down too much.

First, start by adding unit test(s) to the src\UnitTests\Arch\RiscV\RiscVDisassemblerTests.cs
If you navigate to the bottom file, you'll see unit tests like this:

        [Test]
        public void RiscV_dasm_sllw()
        {
            AssertCode("sllw\ta1,a1,a4", "BB95E500");
        }

This tests that the disassembler correctly disassembles and renders the opcode 00E595BB. Note
that the opcode is expressed in little-endian format as a hexadecimal string. What you need to do is add one or more
unit tests in this file, following the pattern established.

Now make sure all unit tests pass. If you're working inside Visual Studio, just run the test from the
test runner. You can also run the unit tests by typing the following in the src/BuildTargets directory:

dotnet msbuild -p:Platform=x64 -p:Configuration=Release -v:m -t:run_unit_tests -m BuildTargets.csproj
# or
dotnet msbuild -p:Platform=x64 -p:Configuration=Debug -v:m -t:run_unit_tests -m BuildTargets.csproj

You will also want to run the regression tests by typing the following in the same directory:

dotnet msbuild -p:Platform=x64 -p:Configuration=Release -v:m -t:run_regressions -m BuildTargets.csproj
# or
dotnet msbuild -p:Platform=x64 -p:Configuration=Debug -v:m -t:run_regressions -m BuildTargets.csproj

If all tests pass you commit your changes to your local repository; but before you commit, make sure
you've added yourself to the AUTHORS file in the root directory!

To to generate a pull request (PR), you need to make your changes visible publicly so I can pull them
into the https://github.com/uxmal/reko repository. Ideally you have made your own fork of the Reko repository
at https://github.com/gregoral/reko. If not, create a fork now in the github user interface, and add
it as a remote to your local repository with git remote add <name> where <name> is some useful label like github.
Then push your commit to your github.com repository: git push github.

From github you can now create a pull request. Make sure in the text of the PR you mention this issue (you could use
the Markdown see issue #1299). When you create the PR, the CI workers will kick off to verify that all tests
are passing.

Feel free to add any questions or comments here, or on the discord server (link in the project's README.md).

[gregoral]

Hi,
thanks for reviewing the proposed changes.
I'm familiar with git but I have never contributed to other projects.

I'll create a fork and follow the steps you outlined.
When I have a fix ready I'll create a PR.

Thank you for creating and maintaining this interesting project.

[gregoral]

Hi,
I have added one test but I plan to write more.

Along the way I found a bad test:

public void RiscV_dasm_csrrc()
{
    AssertCode("csrrc\tzero,mstatus,zero", "73B00230");
}

This appears to be an error and should be changed to:

AssertCode("csrrc\tzero,mstatus,zero", "73300030");
// or
AssertCode("csrrc\tzero,mstatus,t0", "73B00230");

as 73B00230 is actually csrrc zero,mstatus,t0, which I double-checked using gdb disassembler as a reference.

This is how gcc compiles and gdb decompiles csrrs and csrrc:

73 30 00 30     csrc    mstatus,zero
73 b0 02 30     csrc    mstatus,t0
f3 22 40 f1     csrr    t0,mhartid

NOTE: gdb converts raw instructions to pseudo instructions which reko doesn't do.

Should I group all CSRR related tests in one place, or should I add new tests at the bottom?
I think it would be best if all tests for similar instructions are in one place, is that ok?

Also I would propose the use of parameters with "unique" bit patterns.
This way it would be impossible to read expected value from bad offset.

For example using registers mstatus which is 0x300 and zero or x0 which is 0x00 in binary is not a particularly good combination, because it is possible to "decode" x0 even from bits that belong to mstatus.

By using more "unique" values these sort of errors would be much more unlikely or even impossible.

For instance csr register mcontext with number 0x7A8 has a much more varied bit pattern.
And register x22 or s6 with number 0x16 or 10110 is probably a better choice than x0 with a bit pattern 00000.
Also it would be good if no test used the same register twice.

[uxmal]

You're more than welcome to add more tests to ensure correctness. If you want to randomize the bit patterns in the decompiled instructions, feel free to do so as well. When I've written disassembler tests on the various architectures supporteed by Reko, I've typically generated a few kilobytes of random bytes, pointed the relevant disassembler at those bytes, and created unit tests from any bit patterns that the disassembler in question calls DisassemberBase.NotYetImplemented(string) on. Sometimes those bit patterns are collected from actual real binaries, which naturally contain more 0x00 and 0xFF bytes than a uniform distribution of values would.

As for pseudo-instructions: these are common on Risc-like architectures. Adding support to show instructions as pseudo-mnemonics or as their underlying raw mnemonics, and toggling between these two modes, is not terribly hard; it's just that no-one has asked for it yet. If this is something you'd like to see in Reko, open a separate issue and we can discuss further there.

As far as organization of unit tests: they are typically, but not consistently, ordered alphabetically. I have no strong opinions about it, except that I feel that going back to the almost 10000 unit tests and sorting them will prove to be big task, with no concrete benefit, and proves to be a big churn in the git history. But if you feel strongly that the methods in RiscVDisassemblerTests need to be sorted alphabetically, by all means go ahead.

[gregoral]

Hi, I have added 5 and fixed one test for CSRR* instructions.
There are now tests for all 6 variants: CSRRW, CSRRWI, CSRRC, CSRRCI, CSRRS, CSRRSI

I had to fix three other tests in RiscVRewriterTests.
The hex/binary representation didn't match the instruction and after the fix to mutator r1 these tests were failing.
I have manually reviewed these three and the result from rewriter appears to be correct.
Therefore I modified the expected string accordingly.

There is also one issue with the formatting of the decompiled instructions.
This is how an instruction with an immediate operand is decompiled.

csrrci x30,mtvec,0000000B

At least gcc assembler does not like it. The immediate value should be prefixed with 0x or formatted as decimal:

csrrci x30,mtvec,0x0000000B
# or
csrrci x30,mtvec,11

The minor issue is that this is a 5 bit value and could always be represented by two hex digits.

All test are passing now:

Passed! - Failed: 0, Passed: 14717, Skipped: 198, Total: 14915, Duration: 41 s - Reko.UnitTests.dll (net6.0)

There is a warning at the end of regression tests:

warning MSB8065: Custom build for item "...\run_regressions.runtimestamp.rule" succeeded, but specified output "....skipregressions" has not been created. This may cause incremental build to work incorrectly.

If you are ok with the proposed changes, I will commit changes and create a pull request.

This is what I was thinking for the commit message:

Fix: RISC-V CSRR instruction definition

CSRR instructions use the rs1 source regiter.

[uxmal]

I'm closing this issue now, as the original problem was solved by your PR. Don't forget to open new issue(s) for other bugs you've discovered.