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

Auto-Sync Mips #2410

Merged
merged 7 commits into from
Sep 7, 2024
Merged

Auto-Sync Mips #2410

merged 7 commits into from
Sep 7, 2024

Conversation

wargio
Copy link
Contributor

@wargio wargio commented Jul 28, 2024

Your checklist for this pull request

  • I've documented or updated the documentation of every API function and struct this PR changes.
  • I've added tests that prove my fix is effective or that my feature works (if possible)

Detailed description
Adds options for:

  • CS_OPT_SYNTAX_NOREGNAME on Mips
  • CS_OPT_SYNTAX_NO_DOLLAR (new, removes $ from the register)

Adds support for:

  • microMips32r3
  • microMips32r6
  • nanoMIPS (Generic, NMS1, i7200)
  • Mips16
  • Mips I ISA
  • Mips II ISA
  • Mips32 r2 ISA
  • Mips32 r3 ISA
  • Mips32 r5 ISA
  • Mips32 r6 ISA
  • Mips III ISA
  • Mips IV ISA
  • Mips V ISA
  • Mips64 r2 ISA
  • Mips64 r3 ISA
  • Mips64 r5 ISA
  • Mips64 r6 ISA
  • Octeon (cnMIPS)
  • Octeon+ (cnMIPS+)

cstool has been refactored for better UX: now by adding +<feature> you can directly modify the output of capstone using all options.

Arch specific options:
        +att             ATT syntax (only: x86)
        +intel           Intel syntax (only: x86)
        +masm            Intel MASM syntax (only: x86)
        +noregname       Number only registers (only: Arm64, ARM, LoongArch, Mips, PowerPC)
        +moto            Use $ as hex prefix (only: MOS65XX)
        +regalias        Use register aliases, like r9 > sb (only: ARM, Arm64)
        +percentage      Adds % in front of the registers (only: PowerPC)
        +nodollar        Removes $ in front of the registers (only: Mips)
        +nofloat         Disables floating point support (only: Mips)
        +ptr64           Enables 64-bit pointers support (only: Mips)

For example:

$ ./build/cstool -s mipsel64+nofloat "32 C0 38 46 32 02 20 46 32 03 20 "
 0  32 c0 38 46  .byte  0x32, 0xc0, 0x38, 0x46
 4  32 02 20 46  .byte  0x32, 0x02, 0x20, 0x46
$ ./build/cstool -s mipsel64 "32 C0 38 46 32 02 20 46 32 03 20 "
 0  32 c0 38 46  c.eq.d $f24, $f24
 4  32 02 20 46  c.eq.d $fcc2, $f0, $f0

Also fixes the following issues:

$ ./build/cstool mipsel 0400000c
 0  04 00 00 0c  jal    0x10
$ ./build/cstool mips3 0120102d0080e82d40ab500000a0402d0080502d01a0702d40ac1000
 0  01 20 10 2d  move   $v0, $t1
 4  00 80 e8 2d  move   $sp, $a0
 8  40 ab 50 00  dmtc0  $t3, $10, 0
 c  00 a0 40 2d  move   $t0, $a1
10  00 80 50 2d  move   $t2, $a0
14  01 a0 70 2d  move   $t6, $t5
18  40 ac 10 00  dmtc0  $t4, $2, 0
$ ./build/cstool mipsel32r6 b506ff7d
 0  b5 06 ff 7d  pref   0x1f, 0xd($t7)
$ ./build/cstool mips3 00c0502d
 0  00 c0 50 2d  move   $t2, $a2
$ ./build/cstool mips64r2 40021000
 0  40 02 10 00  mfc0   $v0, $2, 0
$ ./build/cstool mips3 46200937
 0  46 20 09 37  c.ule.d        $fcc1, $f1, $f0
$ ./build/cstool mipsel64 03800c4080008c35
 0  03 80 0c 40  mfc0   $t4, $16, 3
 4  80 00 8c 35  ori    $t4, $t4, 0x80
$ ./build/cstool -d mipsel 4000000c0800e003
 0  40 00 00 0c  jal    0x100
        ID: 34 (jal)
        op_count: 1
                operands[0].type: IMM = 0x100
        Groups: call HasStdEnc NotInMicroMips 

 4  08 00 e0 03  jr     $ra
        ID: 773 (jr)
        op_count: 1
                operands[0].type: REG = ra
        Groups: return jump HasStdEnc NotMips32r6 NotMips64r6 NotInMicroMips 
# the test in the example is always decoded as xori, i checked also with other disassemblers.
$ ./build/cstool mips 7c03e83b
 0  7c 03 e8 3b  rdhwr  $v1, $29
$ ./build/cstool mipsel64 "32 C0 38 46 32 02 20 46 32 03 20 46"
 0  32 c0 38 46  c.eq.d $f24, $f24
 4  32 02 20 46  c.eq.d $fcc2, $f0, $f0
 8  32 03 20 46  c.eq.d $fcc3, $f0, $f0
$ ./build/cstool mipsel32r6 "000019f8"
 0  00 00 19 f8  jalrc  $t9

@wargio wargio force-pushed the auto-sync-mips branch 2 times, most recently from 509ae13 to 0ff1af5 Compare August 28, 2024 07:23
@wargio wargio changed the title Auto sync mips Auto-Sync Mips Aug 28, 2024
@wargio wargio marked this pull request as ready for review August 28, 2024 07:25
@github-actions github-actions bot added the Github-files Github related files label Aug 28, 2024
@Rot127 Rot127 added this to the v6 - Alpha milestone Aug 28, 2024
@Rot127
Copy link
Collaborator

Rot127 commented Aug 28, 2024

The auto-sync test is fixed in the modern_testing PR.

Copy link
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

As discussed move details to MipsMapping.c

arch/Mips/MipsInstPrinter.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Awesome job! This was a big one. And it looks great.
Please address the comments.

Also, in the long run we can think about adding the instruction formats as additional information.

LLVM defines them well on a shallow look (the ISA doesn't though). But I like to have them provide them, because if Capstone fails badly to provide certain details, people can extract them by themselves until there is a fix.
Of course referencing MipsInstrFormats.td.

Also please rebase onto #2456 and generate the yaml tests. The current MC tests can stay as they are. Don't delete them. They are still used by the fuzzer.

arch/Mips/MipsInstPrinter.c Outdated Show resolved Hide resolved
arch/Mips/MipsInstPrinter.c Show resolved Hide resolved
arch/Mips/MipsInstPrinter.c Show resolved Hide resolved
arch/Mips/MipsMapping.c Outdated Show resolved Hide resolved
arch/Mips/MipsMapping.c Outdated Show resolved Hide resolved
arch/Mips/MipsMapping.c Show resolved Hide resolved
arch/Mips/MipsMapping.c Show resolved Hide resolved
arch/Mips/MipsMapping.h Outdated Show resolved Hide resolved
cstool/cstool.c Outdated Show resolved Hide resolved
cstool/cstool.c Outdated Show resolved Hide resolved
@wargio wargio force-pushed the auto-sync-mips branch 2 times, most recently from 9a603cc to 1256c4e Compare September 2, 2024 06:44
@wargio wargio requested a review from Rot127 September 2, 2024 06:57
* Move patch constraints into the config file and add a test
* Handle some Mips operand printer.
@wargio wargio force-pushed the auto-sync-mips branch 2 times, most recently from a52c901 to 3ea4e9f Compare September 3, 2024 02:46
@github-actions github-actions bot added python bindings java bindings ocaml bindings labels Sep 3, 2024
@wargio wargio force-pushed the auto-sync-mips branch 3 times, most recently from a07829b to 5e2d6f4 Compare September 3, 2024 07:44
Copy link
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

Ok nice. Almost done. Some more nitpicks:

  • Please add the resolved issues (from the PR descriptions above) as tests to tests/issues.yaml.
  • Please add the newly added features (access, is_reglist etc.) and new restrictions (checks features more strictly) for Mips in docs/cs_v6_release_guide.md.
  • Also document new option (CS_OPT_SYNTAX_NO_DOLLAR) and cstool behavior there.

suite/cstest/src/test_run.c Outdated Show resolved Hide resolved
suite/cstest/src/main.c Outdated Show resolved Hide resolved
include/capstone/mips.h Show resolved Hide resolved
arch/Mips/MipsMapping.c Outdated Show resolved Hide resolved
arch/Mips/MipsInstPrinter.c Show resolved Hide resolved
include/capstone/mips.h Outdated Show resolved Hide resolved
@wargio wargio force-pushed the auto-sync-mips branch 2 times, most recently from 531e913 to 0d08be8 Compare September 3, 2024 11:37
@Rot127
Copy link
Collaborator

Rot127 commented Sep 3, 2024

diff --git a/suite/auto-sync/src/autosync/cpptranslator/patches/FieldFromInstr.py b/suite/auto-sync/src/autosync/cpptranslator/patches/FieldFromInstr.py
index 67f7faae..58092ea0 100644
--- a/suite/auto-sync/src/autosync/cpptranslator/patches/FieldFromInstr.py
+++ b/suite/auto-sync/src/autosync/cpptranslator/patches/FieldFromInstr.py
@@ -41,9 +41,9 @@ class FieldFromInstr(Patch):
 
         # Determine width of instruction by the variable name.
         if ffi_first_arg_text[-2:] == "32":
-            inst_width = 4
+            inst_width = b"4"
         elif ffi_first_arg_text[-2:] == "16":
-            inst_width = 2
+            inst_width = b"2"
         else:
             # Get the Val/Inst parameter.
             # Its type determines the instruction width.
diff --git a/.github/workflows/auto-sync.yml b/.github/workflows/auto-sync.yml
index 9d26470d..d1428d06 100644
--- a/.github/workflows/auto-sync.yml
+++ b/.github/workflows/auto-sync.yml
@@ -101,4 +101,3 @@ jobs:
           ./src/autosync/cpptranslator/Differ.py -a ARM --check_saved
           ./src/autosync/cpptranslator/Differ.py -a PPC --check_saved
           ./src/autosync/cpptranslator/Differ.py -a LoongArch --check_saved
-          ./src/autosync/cpptranslator/Differ.py -a Mips --check_saved

@wargio
Copy link
Contributor Author

wargio commented Sep 3, 2024

@Rot127 fixed

Copy link
Collaborator

@Rot127 Rot127 left a comment

Choose a reason for hiding this comment

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

Excellent!

@wargio
Copy link
Contributor Author

wargio commented Sep 6, 2024

@kabeor

@XVilka
Copy link
Contributor

XVilka commented Sep 7, 2024

Would be nice to get it merged, so Coverity would check this code too, also we could update it in Rizin @kabeor

@wargio wargio mentioned this pull request Sep 7, 2024
5 tasks
Copy link
Member

@kabeor kabeor left a comment

Choose a reason for hiding this comment

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

Really awesome, thank you!

@kabeor kabeor merged commit 6a7fef6 into capstone-engine:next Sep 7, 2024
19 checks passed
@wargio wargio deleted the auto-sync-mips branch September 7, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment