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

Add ARC auto-sync architecture #2570

Open
wants to merge 30 commits into
base: next
Choose a base branch
from

Conversation

R33v0LT
Copy link
Contributor

@R33v0LT R33v0LT commented Dec 5, 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 the necessary ARC files for the disassembler to work properly
  • I've added ARC to python bindings
  • I've added ARC to auto-sync scripts
  • Dev fuzzing is done without errors
  • I've added tests that prove my fix is effective or that my feature works (if possible)

Detailed description

This PR adds support for the ARC architecture in capstone. It is worth noting that this architecture is experimental in lvm, that is, full support for all instructions is not fully implemented. Also, MC tests in llvm (like llvm_root/llvm/test/MC/Disassembler/ARC/ldst.txt ) have a format different from other architectures, which is why tests auto-generated by MCUpdater.py does not add instruction bytes to yaml. I think this format will change when ARC is fully supported
To successfully pass the autosync test, a merge of the following PR is required: capstone-engine/llvm-capstone#69

Test plan

There are no differences in the way capstone is built with the ARC architecture
MC tests were generated by MCUpdater. Only instruction bytes are manually added. All other tests are added manually
At the moment, all tests are passing successfully, there are CI/CD results to confirm

Closing issues

None

@R33v0LT R33v0LT marked this pull request as draft December 5, 2024 14:47
@R33v0LT
Copy link
Contributor Author

R33v0LT commented Dec 11, 2024

@Rot127, fuzzing, as I expected, shows that those instructions that are not yet supported for ARC in llvm lead to incorrect decoding and program crash. llvm-mc from the official github also fails with an error. Should we do something now or ignore it until llvm updates?

@Rot127
Copy link
Collaborator

Rot127 commented Dec 11, 2024

You can fix it in our fork of LLVM. And open an issue in LLVM with a reference to the fix commit. They can cherry-pick it. If you have the time, you can also open a PR with the fix yourself.

@R33v0LT
Copy link
Contributor Author

R33v0LT commented Dec 11, 2024

The problem is solved now. The DecodeGPR32RegisterClass return value was not checked (in llvm disassembler also)
I must reran fuzzing again to check if all is good now. If so, I'll marked this PR as ready for review

@R33v0LT R33v0LT marked this pull request as ready for review December 13, 2024 09:44
arch/ARC/ARCDisassembler.c Outdated Show resolved Hide resolved
@Rot127
Copy link
Collaborator

Rot127 commented Dec 17, 2024

Do you consider this done? Then I would review it this week.

@R33v0LT R33v0LT closed this Dec 17, 2024
@R33v0LT R33v0LT reopened this Dec 17, 2024
@R33v0LT
Copy link
Contributor Author

R33v0LT commented Dec 17, 2024

Do you consider this done? Then I would review it this week.

Yes. I think it is done now

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.

Continue later (at arc.h). But so far looks very good!

arch/ARC/ARCDisassembler.c Show resolved Hide resolved
arch/ARC/ARCGenCSFeatureName.inc Outdated Show resolved Hide resolved
arch/ARC/ARCMapping.c Outdated Show resolved Hide resolved
arch/ARC/ARCMapping.c Outdated Show resolved Hide resolved
arch/ARC/ARCMapping.h Outdated Show resolved Hide resolved
cstool/cstool.c Outdated Show resolved Hide resolved
bindings/python/capstone/arc_const.py Outdated Show resolved Hide resolved
bindings/python/capstone/arc.py Outdated Show resolved Hide resolved
bindings/python/capstone/arc.py Outdated Show resolved Hide resolved
bindings/python/capstone/arc.py 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.

This was a very straight forward review! Excellent job, well done!

Please add ARC in cs_v6_release_guide.md und New features.
Simple two liner is enough. Just please mention about the memory operands.

Otherwise, please address the other small requests.

Regarding the not yet implemented memory operand type: Do you want to add them? It complicates the details a bit. But on the other hand there seems only the MemOperandRI type. So not too much work I think? Or was there another problem?

Nonetheless, we can merge it like this. But we should have them before v6 Gold.
cc @XVilka @kabeor

suite/auto-sync/src/autosync/Targets.py Outdated Show resolved Hide resolved
suite/cstest/include/test_detail_arc.h Outdated Show resolved Hide resolved
suite/cstest/src/test_detail_arc.c Outdated Show resolved Hide resolved
suite/test_corpus3.py Outdated Show resolved Hide resolved
tests/details/arc.yaml Outdated Show resolved Hide resolved
@R33v0LT R33v0LT requested a review from Rot127 December 25, 2024 12:55
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.

Nice! Almost done. Just those few nitpicks.

Also, please mark the suggestions as resolved when done.

arch/ARC/ARCMapping.c Outdated Show resolved Hide resolved
ARC_INS_ADD_,
ARC_INS_ADD_F,
ARC_INS_ADD,
ARC_INS_AND_,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't forget about those ones.

Copy link
Contributor Author

@R33v0LT R33v0LT Jan 5, 2025

Choose a reason for hiding this comment

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

Created pull request for this to capstone-llvm: capstone-engine/llvm-capstone#72

@R33v0LT R33v0LT requested a review from Rot127 January 5, 2025 12:17
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.

2 participants