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

Aarch64 #1366

Closed
wants to merge 251 commits into from
Closed

Aarch64 #1366

wants to merge 251 commits into from

Conversation

nkaretnikov
Copy link
Contributor

@nkaretnikov nkaretnikov commented Jan 23, 2019

This change is Reviewable

Cherry-picked (with minor adjustments) from
disconnect3d/aarch64/skeleton.
The `uname -m` on a Huawei Nova 2i phone returns `aarch64`.

Cherry-picked (with minor adjustments) from
disconnect3d/aarch64/skeleton.
Cherry-picked from disconnect3d/aarch64/skeleton.
Cherry-picked from disconnect3d/aarch64/skeleton.
Cherry-picked (with minor adjustments) from
disconnect3d/aarch64/skeleton.
Cherry-picked (with minor adjustments) from
disconnect3d/aarch64/skeleton.
Cherry-picked from disconnect3d/aarch64/skeleton.
Cherry-picked (with minor adjustments) from
disconnect3d/aarch64/skeleton.
Cherry-picked (with minor adjustments) from
disconnect3d/aarch64/skeleton.
Via ARMv8 Reference Manual, §A1.3.2 (p. A1-35), as pointed out on the SO linked below:
> AArch64 state supports only a single instruction set, called A64.
> This is a fixed-length instruction set that uses 32-bit instruction encodings.

https://stackoverflow.com/questions/46086329/can-i-use-thumb-instructions-in-an-arm64-binary

Cherry-picked from disconnect3d/aarch64/skeleton.
Cherry-picked from disconnect3d/aarch64/skeleton.
@nkaretnikov
Copy link
Contributor Author

My Aarch64 work will be happening here. Currently this just includes code by @disconnect3d (with minor changes and updated for current master) and is completely untested. Things might be changing a lot while this is still in development, but feel free to review and comment if you feel like it.

@nkaretnikov
Copy link
Contributor Author

CC @yan (just in case)

@nkaretnikov
Copy link
Contributor Author

Fixed codeclimate issues in all files affected by this branch, not just my changes, including ignored paths.
If it's too much, I can fix only those introduced by me, but the above was easier to track locally.

There are some things left, but:

  • they are usually ignored ("Consider refactoring"), if I understood @ehennenfent correctly
  • fixes would lead to even uglier (IMO) code ("Visually indented line with same indent as next logical line")
  • or those issues are just hard to fix ("Consider simplifying this complex logical expression").

Maybe it's worth ignoring some issues in the config: https://pep8.readthedocs.io/en/latest/intro.html#configuration

(Note that the number of issues is different below from what GitHub reports, but that's because I provide paths myself.)

% codeclimate analyze $(git diff --name-only master)
Starting analysis
Running structure: Done!
Running duplication: Done!
Running pep8: Done!

== manticore/native/cpu/aarch64.py (8 issues) ==
271-5318: `Aarch64Cpu` has 170 functions (exceeds 35 allowed). Consider refactoring. [structure]
302-331: Consider simplifying this complex logical expression. [structure]
305: Visually indented line with same indent as next logical line [pep8]
838: Visually indented line with same indent as next logical line [pep8]
1220-1234: Consider simplifying this complex logical expression. [structure]
3032-3060: Consider simplifying this complex logical expression. [structure]
3035: Visually indented line with same indent as next logical line [pep8]
4680-4694: Consider simplifying this complex logical expression. [structure]

== manticore/platforms/linux.py (1 issue) ==
873-1071: Function `load` has a Cognitive Complexity of 74 (exceeds 40 allowed). Consider refactoring. [structure]

== tests/native/test_aarch64cpu.py (2 issues) ==
1-14120: File `test_aarch64cpu.py` has 11416 lines of code (exceeds 9000 allowed). Consider refactoring. [structure]
172-14027: `Aarch64Instructions` has 1,412 functions (exceeds 35 allowed). Consider refactoring. [structure]

== tests/native/test_armv7unicorn.py (1 issue) ==
96-1328: `Armv7UnicornInstructions` has 161 functions (exceeds 35 allowed). Consider refactoring. [structure]

Analysis complete! Found 12 issues.

@nkaretnikov
Copy link
Contributor Author

Just to demonstrate accepted fixes (that I know of), I'll let you judge:

# 3: Visually indented line with same indent as next logical line [pep8]
if (True or
    False):
    pass

# Accepted.
if (
    True or
    False
):
    pass

# Accepted.
if (True or
   False):
    pass

# Accepted.
if (True or
        False):
    pass

@nkaretnikov
Copy link
Contributor Author

Squashed and rebased on master:
https://github.com/nkaretnikov/manticore/commits/aarch64-squash-rebase

Verified that there are no major differences by diffing my changes on this branch to my changes on the new one.

Will be splitting the new branch into smaller PRs next.

@nkaretnikov
Copy link
Contributor Author

@ehennenfent Split nkaretnikov/aarch64-squash-rebase (this PR squashed and rebased on master) into multiple PRs:

The idea here is to merge them in this particular order as each PR builds on the previous one. If git can't deal with commits on a feature branch that are already present on master, one will have to cherry-pick or rebase first.

After applying all of them, there should be no changes against nkaretnikov/aarch64-squash-rebase.

The advantage of this split method is that it doesn't introduce or remove any code in-between.
The disadvantage is that you'll need to merge some commits to master without tests. But you can always test locally first, and the tests will end up in master anyway when the last commit is merged. So this shouldn't be a problem.

@nkaretnikov
Copy link
Contributor Author

I'm closing this one to avoid confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Manticore Releases
  
Included in 0.3.1!
Development

Successfully merging this pull request may close these issues.

None yet

3 participants