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

fixing `pshufb` insn #726

Merged
merged 11 commits into from Oct 10, 2017

Conversation

Projects
None yet
2 participants
@gitoleg
Contributor

gitoleg commented Oct 6, 2017

The pshufb instruction was implemented previously as one expression
that has lots of nested if-then-else expressions.
It's possible to write a more readable code using a loop.

And if we compare a previous implementation with a new one, a difference
is quite noticeable:

bap-mc "660f3800042542424242" --show-bil --arch=X86 | wc --lines
327678
bap-mc "660f3800042542424242" --show-bil --arch=X86 | wc --lines
165

Also, the previous implementation didn't check a memory boundary in case of 128-bit operands.

@ivg

Don't use while, expand everything since you know the sizes.

Show outdated Hide outdated plugins/x86/x86_lifter.ml Outdated
@ivg

It is hard for me to prove to myself, that your pseudocode adequately represents the instruction semantics. Mostly since the instruction itself is very complex. Since I've already found without going very deep a few bugs in your implementation, I would expect that there should be more.

So, we need a test that will check that our implementation is fine. Please, write a unit test (in lib_test) that uses Stmt.eval to evaluate the expressions and check that the results matches the expected output. Use rappel to find out what output is expected. In the comments to the tests, please provide the rappel session, so that it can be reproduced.

Show outdated Hide outdated plugins/x86/x86_lifter.ml Outdated
Show outdated Hide outdated plugins/x86/x86_lifter.ml Outdated
Show outdated Hide outdated plugins/x86/x86_lifter.ml Outdated
Show outdated Hide outdated plugins/x86/x86_lifter.ml Outdated
@ivg

ivg requested changes Oct 10, 2017 edited

The tests are excellent (now I started to understand myself how this instruction works). The pseudocode is fine (I don't think it is really possible to make it more readable, given the complexity of the instruction).

There is only one issue, we can't add the tests to the bap-std suite, as x86 is not a part of the bap-std. So, please move it to a separate test suite. You can use oasis/dwarf along with the lib_test/bap_dwarf for the inspiration.

@ivg

Please remove unnecessary inline tests invocation.

@ivg

ivg approved these changes Oct 10, 2017

@ivg ivg merged commit 698fde0 into BinaryAnalysisPlatform:master Oct 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gitoleg gitoleg deleted the gitoleg:pshufb branch Mar 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment