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: add instructions #1419

Merged

Conversation

nkaretnikov
Copy link
Contributor

@nkaretnikov nkaretnikov commented Apr 29, 2019

This change is Reviewable

@nkaretnikov nkaretnikov changed the title Aarch64 instructions aarch64: add instructions Apr 29, 2019
@nkaretnikov nkaretnikov mentioned this pull request Apr 29, 2019
@nkaretnikov
Copy link
Contributor Author

See #1366 (comment).

Otherwise, 'scripts/travis_test.sh' fails to work.
Copy link
Contributor

@ehennenfent ehennenfent left a comment

Choose a reason for hiding this comment

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

Reviewed 16 of 18 files at r1, 1 of 1 files at r2.
Reviewable status: 17 of 18 files reviewed, 1 unresolved discussion (waiting on @nkaretnikov)


manticore/native/cpu/aarch64.py, line 436 at r2 (raw file):

    def _adds_subs_immediate(cpu, res_op, reg_op, imm_op, mnem):
        assert res_op.type is cs.arm64.ARM64_OP_REG

The asserts here (and in following functions) should probably be removed. I'm open to arguing about it if anyone on the team feels differently, but as a general rule, use of asserts should be limited in production code. Logically, they check conditions that are never supposed to be false, so even the modest performance penalty typically isn't worth it in expressions that are evaluated often. We do make use of asserts elsewhere in Manticore, but (with the exception of expression.py, which we probably need to refactor) they're mostly used during initialization and teardown. Having 2-4 asserts per instruction is going to be have a much more significant impact on aarch64 performance.

For parameter sanity checks like the ones in this file, type hints are usually a better way to go because they're ignored by the default Python interpreter and only incur a performance penalty when the developer wants to enforce them.

Copy link
Contributor

@ehennenfent ehennenfent left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 18 files at r1, 3 of 3 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@ehennenfent
Copy link
Contributor

We're going to retract the request to remove the asserts for now. Since this is a big new branch, we'll accept the performance hit of having some extra sanity checks. Once this has been out in the real world for a while, we'll convert them to type hints ourselves.

@ehennenfent ehennenfent changed the base branch from master to merge-aarch64 April 30, 2019 16:09
@nkaretnikov
Copy link
Contributor Author

Okay, sure. I've caught a few issues this way, but this doesn't seem to justify performance implications. Also, the current style is really verbose, which might be a disadvantage for those reading the code.

Have you considered running with -O in production? This will disable all asserts, so asserts in critical places would need to be converted to exceptions first.

As for the hints, could you show me how to do it if you know offhand? I can't figure this out: one would need to match a constant against the type attribute.

Copy link
Contributor

@ehennenfent ehennenfent left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@ehennenfent ehennenfent merged commit a01593f into trailofbits:merge-aarch64 Apr 30, 2019
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.

None yet

2 participants