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

Try to fix issue #633 #672

Merged
merged 10 commits into from
Oct 24, 2019
Merged

Try to fix issue #633 #672

merged 10 commits into from
Oct 24, 2019

Conversation

xkubov
Copy link
Contributor

@xkubov xkubov commented Oct 21, 2019

Redesigns loadOp methods that are responsible for the loading of operands of instructions.
Before this PR it was possible to load operands of incompatible type for the instruction which did
result in faulty decompilation. This PR tries to bring changes that will ensure that operands are
converted with a proper method before usage.

Peter Kubov added 4 commits October 21, 2019 14:47
Currently there is not a good implementation/design for `storeOp`
methods, that store LLVM Values into other operands.
As there may occur errors f.e. on the side of capstone, we have to
make sure that operands are converted correctly.

This commit brings only temprorary solution for arm64
as better refactorization of store operands method is required.
This hovewer affects all architectures and design shuld be better
rethought.
Currently there is not a good implementation/design for `storeOp`
methods, that store LLVM Values into other operands.
As there may occur errors f.e. on the side of capstone, we have to
make sure that operands are converted correctly.

This commit brings only temprorary solution for arm
as better refactorization of store operands method is required.
This hovewer affects all architectures and design shuld be better
rethought.
@xkubov
Copy link
Contributor Author

xkubov commented Oct 21, 2019

I guess let's run TeamCity.

@PeterMatula PeterMatula self-requested a review October 22, 2019 08:15
@PeterMatula
Copy link
Collaborator

Well, I guess you are not in that user list that can trigger the TC builds :-)

@PeterMatula
Copy link
Collaborator

Run TeamCity builds.

@PeterMatula
Copy link
Collaborator

PeterMatula commented Oct 22, 2019

Ok, @s3rvac says you are in the list, but the magic regex is run (tc|teamcity) (builds|tests) (case-insensitive)
ROFL

src/capstone2llvmir/capstone2llvmir_impl.h Outdated Show resolved Hide resolved
src/capstone2llvmir/capstone2llvmir_impl.h Outdated Show resolved Hide resolved
Peter Kubov added 2 commits October 23, 2019 14:08
The commit 825efca brought unwanted change of ranaming `loadOp` method
from `loadOpUnary` to `loadOpsUnary` which is not consistent
with naming of other methods. This commit brings fix for this
change and therefore the old name of the method.
Provides marking of the method `checkTypeConvertion` as internal
by giving it underscore prefix (`_checkTypeConvertion`). This
will indicate that this method is ment only to be used as a specific
helper method and not to be used with other methods.

Also provides documentation for this method as the name of the
method may suggest different functionality.
@xkubov
Copy link
Contributor Author

xkubov commented Oct 23, 2019

So I'll try to run TeamCity tests correctly now.

@PeterMatula
Copy link
Collaborator

@xkubov, two more things:

  1. Are there some tests for this? I know that it is a broad refactorization that aims to make the code that is already working more safe -- less error prone. But it was triggered by Unsuccessful MIPS decompilation #633, so are there some test at least for the original issue reported there? If yes, then great. If no, add something please. Ideally, if possible, in a form of an unit test - it is the most straightforward and fastest running - you should be able to put in an exact instruction (in text or binary form) that caused the problem.
  2. Add entry for the Unsuccessful MIPS decompilation #633 fix, and this Try to fix issue #633 #672 PR to CHANGELOG.md - the same way as other entries there.

Sometimes, Keystone refuses to assembly the given instruction. Then, we can not translate and emulate it. This adds a possibility to directly emulate binary data - skipping the assembly process.
@PeterMatula PeterMatula merged commit 8d0fe7b into master Oct 24, 2019
@PeterMatula
Copy link
Collaborator

I wanted to finish this, so I did both of those things.

@PeterMatula PeterMatula deleted the issue-633 branch October 24, 2019 12:45
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