x86: fix decoding of mandatory prefixes#2856
Draft
jxors wants to merge 4 commits intocapstone-engine:nextfrom
Draft
x86: fix decoding of mandatory prefixes#2856jxors wants to merge 4 commits intocapstone-engine:nextfrom
jxors wants to merge 4 commits intocapstone-engine:nextfrom
Conversation
Contributor
Author
|
I've marked this as a draft, because It used to decode as I will fix this issue and do more testing to make sure everything works as intended. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Your checklist for this pull request
Detailed description
As mentioned in #2853, the current decoding of mandatory prefixes is not correct. It expects a hard-coded ordering of prefixes that is not required by instruction reference manuals, and also does not handle repeated
66prefixes.Initially, I tried to modify the existing function with additional checks. However, it is not possible in general to determine whether a prefix should function as a mandatory prefix without inspecting the opcode, so these checks need to occur later in the decoding process.
To fix this, I have removed the
mandatoryPrefixfield and replaced it with a function that resolves conflicting mandatory prefixes in thegetIDfunction.Test plan
I have added
232764 tests, comprised of instructions that were originally decoded incorrectly by capstone as well as unexpected edge-cases. For example, instructions with opcode0FB8..0FBFtreat only the REPE/REPNE prefixes as mandatory, and still expect the data size override to function normally.In general, this patch should only affect instructions in the secondary opcode map with both a REP (
F2/F3) and a data size override (66) prefix.Closing issues
closes #2853