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

Replace disassembler engine with Zydis #1730

Merged
merged 11 commits into from Oct 9, 2017

Conversation

Projects
None yet
3 participants
@athre0z
Contributor

athre0z commented Sep 25, 2017

Current status

  • Adds zydis_wrapper
  • Adds Zydis as a submodule
  • Replaces all usages of capstone_wrapper with zydis_wrapper
  • Added IsBranchType, replacing many manual if(mnem == ...) occurrences
  • Copied previous QBeaEngine to CSQBeaEngine and CapstoneTokenizer to CsCapstoneTokenizer, for diffing
    • Reworked CsCapstoneTokenizer to use CapstoneTokenizer's data classes for easy comparision
  • Added CS<->Zydis tokenizer output diff code to Disassembly.cpp (commented out)

Future work required

  • Remove diff code and legacy tokenizer
  • Possibly more abstractions over the disassembler engine
  • Possibly refactor class names to use disassembler engine independent names
  • Possibly update x64dbg Zydis fork and use it for the submodule

If there's anything you don't like that you'd like me to change, just say the word.

athre0z added some commits Sep 20, 2017

Replace Capstone with Zydis
- While at it, added branch info logic to disassembler class
  - Thus reduce direct checks by mnemonic in GUI and analysis code
- Replaced direct disassembler struct access with disassembler class calls where trivially possible
- Removed workarounds for empty segment registers
- Temp. disabled `cbInstrCapstone` command
- Temp. disabled flag stuff in `QBeaEngine`
Renamed `Capstone` -> `Zydis`
- Prevents name clashes with actual capstone disassembler implementation
Added CS vs Zydis diff code & various fixes
- Fixed various porting bugs in the Zydis `CapstoneTokenizer`
- Added Capstone vs Zydis tokenizing diff and various exceptions for known issues
Print “far” token, support RTM instructions
- Also, more whitelist entries for the CS-Zydis diff
Moved “zydis_wrapper” into root repo
- Instead, we directly use Zydis as a submodule now
zydis_wrapper: Better compliance with style-guide
- Removed underscores
- Removed redundant “zy” prefix
- Executed `AStyleWhore` (sorreh, I use git on my macOS host, can’t put it into pre-commit-hook)
zydis_wrapper: Final touch
- Comment out diff code in GUI
- Enable optimization
- A few more whitelist entries in the diff code
- A few fixes in the old tokenizer to be consistent with the new one in diffs
- Remove LICENSE and README now that the wrapper is part of the x64dbg core repo
zydis_wrapper: Cleaned up branch types
- Remove unused semantic groups
- Improve handling of “far” in tokenizer
{
//set the branch destinations
node.brtrue = mCp.BranchDestination();
if(mCp.GetId() != X86_INS_JMP && mCp.GetId() != X86_INS_LJMP) //unconditional jumps dont have a brfalse
if(mCp.GetId() != ZYDIS_MNEMONIC_JMP) //unconditional jumps dont have a brfalse

This comment has been minimized.

@nooperation

nooperation Sep 29, 2017

Should this also exclude ZYDIS_MNEMONIC_JMP_FAR? The previous logic excluded both unconditional jumps

@nooperation

nooperation Sep 29, 2017

Should this also exclude ZYDIS_MNEMONIC_JMP_FAR? The previous logic excluded both unconditional jumps

This comment has been minimized.

@athre0z

athre0z Sep 29, 2017

Contributor

Zydis has the same mnemonic constant for both far and near jumps.

@athre0z

athre0z Sep 29, 2017

Contributor

Zydis has the same mnemonic constant for both far and near jumps.

This comment has been minimized.

@athre0z

athre0z Sep 29, 2017

Contributor

In case you're wondering why this is the case: we stick to Intel SDM as close as possible and Intel doesn't treat them as separate instructions either.

@athre0z

athre0z Sep 29, 2017

Contributor

In case you're wondering why this is the case: we stick to Intel SDM as close as possible and Intel doesn't treat them as separate instructions either.

@athre0z

This comment has been minimized.

Show comment
Hide comment
@athre0z

athre0z Oct 9, 2017

Contributor

The Mergings project card mentions that @blaquee found problems with this PR. Could you elaborate?

Contributor

athre0z commented Oct 9, 2017

The Mergings project card mentions that @blaquee found problems with this PR. Could you elaborate?

@mrexodia

This comment has been minimized.

Show comment
Hide comment
@mrexodia

mrexodia Oct 9, 2017

Member
Member

mrexodia commented Oct 9, 2017

@mrexodia mrexodia merged commit 77c6e95 into x64dbg:development Oct 9, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@athre0z athre0z deleted the athre0z:zydis branch Oct 9, 2017

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