Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MIPS -- add fp register; support for lwl, beqz and sltiu instructions #4513

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

matteius
Copy link
Contributor

Your checklist for this pull request

  • I've read the guidelines for contributing to this repository
  • I made sure to follow the project's coding style
  • I've documented or updated the documentation of every function and struct this PR changes. If not so I've explained why.
  • I've added tests that prove my fix is effective or that my feature works (if possible)
  • I've updated the rizin book with the relevant information (if needed)

Detailed description

  • In MIPS assembly language, fp stands for "frame pointer." It is a register used for stack frame management, particularly in functions that need to allocate space on the stack for local variables or save the state of other registers.
  • Add support for instructions:
  • lwl - load word left, The lwl instruction loads a word (4 bytes) from memory into a register. It loads the left-most (most significant) bytes of the word from memory, starting at a specified address and continuing to the next aligned word boundary.
  • beqz - branch if equal zero, The beqz instruction is a pseudo-instruction in MIPS assembly, which branches to a specified address if the value in a given register is zero. It is equivalent to beq $zero, rs, offset.
  • sltiu - Set Less Than Immediate Unsigne, The sltiu instruction sets a register to 1 if the value in a source register is less than an immediate value, when both are treated as unsigned integers. Otherwise, it sets the register to 0.

Also two quality of life improvements:

  • When the exported lw command (from another program) is missing the offset, we can assume offset of 0 and this lets us read that back in without manually adding a 0 offset in the assembly.
  • The purpose of adding the condition || (p[0] == '$' && !strcmp(p + 1, regs[n])) in the getreg function is to handle both formats of register names that are common in assembly language. In MIPS assembly, registers can be referred to either by their name (e.g., v0, s3) or by their name prefixed with a dollar sign (e.g., $v0, $s3).

Test plan

I am using this code to insert instructions via cutter, and so I get console errors whenever I encounter something that cannot be assembled.

Closing issues

Maybe closes #4509 assuming the tests get run by default by existing in that directory.

Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

If you want to check both disassembly and assembly, you will need to use da in asm tests

test/db/asm/mips32 Outdated Show resolved Hide resolved
@matteius
Copy link
Contributor Author

Thanks for taking a look @wargio and @XVilka -- I didn't quite understand:

If you want to check both disassembly and assembly, you will need to use da in asm tests

@XVilka
Copy link
Member

XVilka commented May 23, 2024

Lines should start not from d instruction but from da instruction, then it will check the both ways - disassembly and assembly.

@wargio
Copy link
Member

wargio commented May 23, 2024

For clarity, please check also https://github.com/rizinorg/rizin/tree/dev/test#writing-assembly-tests

@matteius
Copy link
Contributor Author

I will look into the test failures more later on when I have more time, thanks!

@wargio
Copy link
Member

wargio commented May 23, 2024

looks like an endianness issue.

Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

Many tests fail, mostly disassembly. But @wargio is working on rewriting MIPS support from scratch in capstone (see the auto-sync project #4007, #3684), I wonder if it makes sense trying to fix it in the same PR? Maybe can test only assembly for now?

[XX] db/asm/mips_32 <asm> mfhi t0
-- <asm> mfhi t0 <--> 10400000
-- assembly
--- expected
+++ actual
@@ -1,1 +1,1 @@
-10400000
+10000001


�[A�[2K
[XX] db/asm/mips_32 <asm> mflo t0
-- <asm> mflo t0 <--> 12400000
-- assembly
--- expected
+++ actual
@@ -1,1 +1,1 @@
-12400000
+12000001


�[A�[2K
[XX] db/asm/mips_32 <asm> jalr t0, t1
-- <asm> jalr t0, t1 <--> 09f82001
-- disassembly
--- expected
+++ actual
@@ -1,1 +1,1 @@
-jalr t0, t1
+jalr t1

-- assembly
--- expected
+++ actual
@@ -1,1 +1,1 @@
-09f82001
+09f8e003


�[A�[2K
[XX] db/asm/mips_32 <asm> jal 0x123456
-- <asm> jal 0x123456 <--> 5634120c
-- disassembly
--- expected
+++ actual
@@ -1,1 +1,1 @@
-jal 0x123456
+jal 0x48d158

-- assembly
--- expected
+++ actual
@@ -1,1 +1,1 @@
-5634120c
+158d040c


�[A�[2K
[XX] db/asm/mips_32 <asm> j 0x123456
-- <asm> j 0x123456 <--> 56341208
-- disassembly
--- expected
+++ actual
@@ -1,1 +1,1 @@
-j 0x123456
+j 0x48d158

-- assembly
--- expected
+++ actual
@@ -1,1 +1,1 @@
-56341208
+158d0408

[XX] db/asm/mips_32                                  26 OK         0 BR       92 XX        0 FX

@wargio
Copy link
Member

wargio commented May 25, 2024

i'm ok for the assembly

@XVilka
Copy link
Member

XVilka commented May 30, 2024

@matteius could you please convert "da" to just "a", so that only assembly is checked, and we will merge if tests pass?Note though, some assembly tests also produce errors, please take a look at these failure as well.

@matteius
Copy link
Contributor Author

matteius commented Jun 2, 2024

I expect the tests will fail again; I only changed to test just assembly, but I spent a bit of time trying to run the tests locally and couldn't. I was able to build rizin, and then I did:

(base) matteius@matteius-3-0:~/rizin/test$ ../build/binrz/rz-test/rz-test db/esil/mips_32 
...
exec: Permission denied

I tried also as sudo, but that gets that exec is not found.

Copy link
Member

@XVilka XVilka left a comment

Choose a reason for hiding this comment

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

There is probably one small mistake somewhere in the assembler:

[XX] db/asm/mips_32 <asm> and t0, t1, t2
-- <asm> and t0, t1, t2 ---> 012b4824
-- assembly
--- expected
+++ actual
@@ -1,1 +1,1 @@
-012b4824
+012a4024



[XX] db/asm/mips_32 <asm> xor t0, t1, t2
-- <asm> xor t0, t1, t2 ---> 012b4826
-- assembly
--- expected
+++ actual
@@ -1,1 +1,1 @@
-012b4826
+012a4026

@wargio
Copy link
Member

wargio commented Sep 18, 2024

since i'm updating the mips code, i can also integrate these changes into rizin.

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

Successfully merging this pull request may close these issues.

No mips32 asm tests.
3 participants