Remove deprecated accessed_flags flags field from ZydisDecodedInstruction#262
Remove deprecated accessed_flags flags field from ZydisDecodedInstruction#262
accessed_flags flags field from ZydisDecodedInstruction#262Conversation
ea27991 to
5580ced
Compare
|
Hmm. I seem to remember that we decided that there is precisely zero value in being able to distinguish |
|
The only drawback including the extra info would be a slightly larger binary (maybe 1KiB). I personally would be fine keeping only the cpu_flags_read/written, but at the same time I'm wondering if any user possibly uses the advanced info for anything. |
|
I personally think having more granular detail would be desired, I was a bit surprised that it got reduced to the two fields only. |
|
I think there were some instructions for which the affected flags change depending on semantics. But these were only like 1-2 exceptions for which we defined the "worst case" set. |
Interesting. Could you perhaps provide some insights in how you'd profit from that in any real world project? Note that there are only 81 instructions that use On the other hand, with the new design, users will now have to inspect (or OR together) the 4 different write flags in order to determine if any sort of write occurred. Corresponding instruction DB queries$ jq '.[] | select(.affected_flags and any(.affected_flags[] | values; . == "0")) | .mnemonic' instructions.json | sort | uniq | wc -l
81$ jq '.[] | select(.affected_flags and any(.affected_flags[] | values; . == "1")) | .mnemonic' instructions.json | sort | uniq | wc -l
1 |
With the old design before the flags_read/written masks, it was even worse tho 😃 Maybe we can go for a compromise and keep the flags_read/written while adding the other mask as additional info. |
Well I would go with modified, consumed, undefined. As for the undefined flags there are quite interesting uses in obfuscations, so allowing the analysis to spot use of undefined flags without specifically checking for the instruction makes it a bit easier. The interesting thing about those undefined flags is that they are usually concrete per CPU and just undefined in a general sense. |
5580ced to
b9717e4
Compare
|
I started a poll about this on Twitter yesterday and also asked around in some Discord communities. While nobody I talked to seemed to have a particularly strong opinion about it, it seems like quite a few people have previously profited from having this distinction in one way or another. Looking at this data, I'm going to pull an 180 on my previous position and suggest that we just keep things as implemented in this PR. :) |
b9717e4 to
ad037b5
Compare
Thanks for taking care 👍 |
5226272 to
29fce53
Compare
athre0z
left a comment
There was a problem hiding this comment.
LGTM! Sorry for taking so long to review.
No worries at all! Thanks. |
Removes a bunch of deprecated code related to accessed cpu flags.
In the current state we lost fine granular information about FPU flag actions:
TESTED,TESTED_MODIFIED->READMODIFIED,TESTED_MODIFIEDSET_0,SET_1,UNDEFINED->WRITTENI'm planning to bring back this functionality in a follow up commit.