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 Alpha architecture #2071

Merged
merged 26 commits into from
Dec 28, 2023
Merged

Add Alpha architecture #2071

merged 26 commits into from
Dec 28, 2023

Conversation

R33v0LT
Copy link
Contributor

@R33v0LT R33v0LT commented Jul 2, 2023

This PR adds Alpha (auto-sync) architecture for capstone

New Features

  • capstone support for alpha architecture
  • alpha python bindings
  • new tests (c/python)
  • alpha support in Updater (auto-sync)

arch/Alpha/AlphaMapping.c Outdated Show resolved Hide resolved
include/capstone/alpha.h Outdated Show resolved Hide resolved
@Rot127
Copy link
Collaborator

Rot127 commented Jul 2, 2023

Looks good so far. Nice work!

Copy link
Contributor

@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.

Don't forget to add yourself in CREDITS.TXT

@XVilka
Copy link
Contributor

XVilka commented Jul 21, 2023

@R33v0LT since ARM was merged into next, and @Rot127 updated PPC, please update this PR too.

arch/Alpha/AlphaInstPrinter.c Outdated Show resolved Hide resolved
arch/Alpha/AlphaInstPrinter.c Outdated Show resolved Hide resolved
arch/Alpha/AlphaGenCSMappingInsnOp.inc Outdated Show resolved Hide resolved
@Rot127
Copy link
Collaborator

Rot127 commented Aug 2, 2023

Generally looks good so far. Please don't forget to set the details of each operand (read/write access and the like).
You can look at the PPC implementation for an example

@XVilka

This comment was marked as resolved.

@XVilka

This comment was marked as resolved.

@Rot127
Copy link
Collaborator

Rot127 commented Nov 21, 2023

@R33v0LT You need to rebase again. @kabeor purged the test_aarch64 binary from the commit history. So it was rewritten, this is why you've got +2k commits here

@XVilka

This comment was marked as resolved.

@XVilka
Copy link
Contributor

XVilka commented Dec 6, 2023

@kabeor could you please approve workflows to run?
@R33v0LT why removing it from fuzzing? Technically it should be crash-free too.

@R33v0LT
Copy link
Contributor Author

R33v0LT commented Dec 7, 2023

@kabeor could you please approve workflows to run? @R33v0LT why removing it from fuzzing? Technically it should be crash-free too.

I mistakenly thought that fuzzing tests were related to llvm-mc. Fixed it

@R33v0LT
Copy link
Contributor Author

R33v0LT commented Dec 7, 2023

@Rot127, please take a look again. @kabeor could you please allow workflows to run?

@R33v0LT What is still missing? Why this is still a draft?

I can't figure out why python bindings are broken. As soon as I figure it out, I'll remove it from the draft

@XVilka
Copy link
Contributor

XVilka commented Dec 19, 2023

@R33v0LT I suppose it's not a draft anymore. Please remove the mark also update the PR description. @Rot127, please take a look again.

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! It looks very good!

Besides the few comments from above, please do the following before it can be merged:

Fuzz it a little more:

Apply the patch from here and test every byte combination for segfaults by running:

./cstool -d tc162 0 0xffffffff  # With details enabled
./cstool -d tc162 0 0xffffffff  # Without details

This way our chances for nasty surprises is lower.

Update the new updater

I replaced the Update-Arch.sh script with a proper Python scripts.
You need to add Alpha there and can drop the Update-Arch.sh change.

In suite/auto-sync/Updater/ you need to:

  • Add Alpha to parse_args() in ASUpdater.py, CppTranslator.py and Differ.py
  • Add a configuration for Alpha in suite/auto-sync/CppTranslator/arch_config.json. The config file is documented in CppTranslator/README.md.
  • Run the complete update procedure. It will fail at some point and you need to add some things to the Python scripts (e.g. to CppTranslator/Patches/Includes.py). Don't worry though, there shouldn't be many.

Also, I think the docs are still a confusingly written. And I update them in #2217.

Please, if you have any suggestions or run against walls, let me know. So this stuff can be made easier.

@XVilka @kabeor This last point (updating the updater) could be also moved into a separated PR. Just in case it should be merged earlier.

.gitmodules Outdated Show resolved Hide resolved
arch/Alpha/AlphaGenCSFeatureName.inc Outdated Show resolved Hide resolved
include/capstone/alpha.h Outdated Show resolved Hide resolved
cstool/cstool.c Show resolved Hide resolved
bindings/python/test_alpha.py Outdated Show resolved Hide resolved
arch/Alpha/AlphaMapping.c Outdated Show resolved Hide resolved
arch/Alpha/AlphaGenCSFeatureName.inc Outdated Show resolved Hide resolved
suite/cstest/src/alpha_detail.c Show resolved Hide resolved
tests/test_alpha.c Show resolved Hide resolved
suite/auto-sync/Updater/Update-Arch.sh Outdated Show resolved Hide resolved
@XVilka
Copy link
Contributor

XVilka commented Dec 20, 2023

@kabeor could you please allow workflows to run again? Also, mark it as "ready for review"?

@kabeor kabeor marked this pull request as ready for review December 20, 2023 00:21
Copy link
Contributor

@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.

LGTM as is. @R33v0LT Have you run the fuzzing?

@R33v0LT
Copy link
Contributor Author

R33v0LT commented Dec 20, 2023

LGTM as is. @R33v0LT Have you run the fuzzing?

In progress

@R33v0LT
Copy link
Contributor Author

R33v0LT commented Dec 20, 2023

@XVilka, @Rot127, Fuzzing completed without fails

Rot127

This comment was marked as resolved.

@XVilka
Copy link
Contributor

XVilka commented Dec 20, 2023

Ok, @kabeor @aquynh if you have no objections, this one is ready to merge too.

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.

@kabeor Please take a look when you find time.

@XVilka
Copy link
Contributor

XVilka commented Dec 27, 2023

@kabeor @aquynh have you had a chance to look at it?

@kabeor
Copy link
Member

kabeor commented Dec 28, 2023

It is really awesome, merged, thank you!

@kabeor kabeor merged commit 89fec6e into capstone-engine:next Dec 28, 2023
11 checks passed
@R33v0LT R33v0LT deleted the auto-sync-alpha branch January 5, 2025 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants