Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2841863003:
Improve the decoding of the VEX encoded instructions. (Closed)
|
Created:
3 years, 7 months ago by Sébastien Marchand Modified:
3 years, 7 months ago CC:
syzygy-changes_googlegroups.com Target Ref:
refs/heads/master Project:
syzygy Visibility:
Public. |
DescriptionImprove the decoding of the VEX encoded instructions.
Currently our decoding assumes a constant instruction size (based on the
opcode) but this doesn't really work because these instructions use a
Mod R/M operand (see http://www.c-jump.com/CIS77/CPU/x86/X77_0090_addressing_modes.htm).
This add support for the different sizes that these instructions can have,
as well as some restrictions on which variants are supported.
This has been tested on the latest official build of Chrome.
Review-Url: https://codereview.chromium.org/2841863003
Committed: https://github.com/google/syzygy/commit/5970d392dbf2d3746fe13212eea1dbc9af9cd46c
Patch Set 1 #Patch Set 2 : Use a constant. #
Total comments: 4
Patch Set 3 : Add support for the Mod R/M byte. #Patch Set 4 : Add support for the Mod R/M byte. #
Total comments: 28
Patch Set 5 : Address Sam's comment, support SIB.base = 5 #
Total comments: 26
Patch Set 6 : Address Sam's second round of comments. #
Total comments: 4
Patch Set 7 : Nits. #
Total comments: 2
Patch Set 8 : nit #
Messages
Total messages: 25 (12 generated)
Description was changed from ========== Add some restrictions to the list of supported VEX instructions. ========== to ========== Add some restrictions to the list of supported VEX instructions. Our support for these instructions is really fragile right now and doesn't take into account the fact that they can have a variable length (because they're sometime using the ModRM:r/m encoding), this add some restrictions to our wrapper to make sure that we fail if we encounter a variant of an instruction that we haven't seen before. ==========
Description was changed from ========== Add some restrictions to the list of supported VEX instructions. Our support for these instructions is really fragile right now and doesn't take into account the fact that they can have a variable length (because they're sometime using the ModRM:r/m encoding), this add some restrictions to our wrapper to make sure that we fail if we encounter a variant of an instruction that we haven't seen before. ========== to ========== Add some restrictions to the list of supported VEX instructions. Our support for these instructions is really fragile right now and doesn't take into account the fact that they can have a variable length (because they're sometime using the ModRM:r/m encoding), this add some restrictions to our wrapper to make sure that we fail if we encounter a variant of an instruction that we haven't seen before. This also adds support for the 5 and 6 bytes variant of the vpbroadcast[b|d] instructions. ==========
huangs@chromium.org changed reviewers: + huangs@chromium.org
Some comments. https://codereview.chromium.org/2841863003/diff/20001/syzygy/core/disassemble... File syzygy/core/disassembler_util.cc (right): https://codereview.chromium.org/2841863003/diff/20001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:63: CHECK((ci->code[4] & kModRMMask) != 0) << Perhaps this should be CHECK((ci->code[4] & kModRMMask) != kModRMMask) ? Note that 0x00 = [r/m] with possible SIB 0x40 = [r/m + disp8] with possible SIB 0x80 = [r/m + disp32] with possible SIB 0xC0 = r/m <= No SIB, so we can count on # bytes being fixed. Same below. https://codereview.chromium.org/2841863003/diff/20001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:75: return 6; Cannot guarantee 6, due to possibility of SIB. For example: c4 e2 7d 58 44 00 00 => vpbroadcastd ymm0,DWORD ptr [rax+rax*1+0x0]
chrisha@chromium.org changed reviewers: + chrisha@chromium.org
https://codereview.chromium.org/2841863003/diff/20001/syzygy/core/disassemble... File syzygy/core/disassembler_util.cc (right): https://codereview.chromium.org/2841863003/diff/20001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:43: // - Fourth byte: The opcode for this instruction. Update the comment? Or is this still up to date? https://codereview.chromium.org/2841863003/diff/20001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:53: size_t Get3ByteVexEncodedInstructionSize(_CodeInfo* ci) { This is no longer just 3byte VEX instructions? Or is it?
Description was changed from ========== Add some restrictions to the list of supported VEX instructions. Our support for these instructions is really fragile right now and doesn't take into account the fact that they can have a variable length (because they're sometime using the ModRM:r/m encoding), this add some restrictions to our wrapper to make sure that we fail if we encounter a variant of an instruction that we haven't seen before. This also adds support for the 5 and 6 bytes variant of the vpbroadcast[b|d] instructions. ========== to ========== Improve the decoding of the VEX encoded instructions. Currently our decoding assumes a constant instruction size (based on the opcode) but this doesn't really work because these instructions use a Mod R/M operand (see http://www.c-jump.com/CIS77/CPU/x86/X77_0090_addressing_modes.htm). This add support for the different sizes that these instructions can have, as well as some restrictions on which variants are supported. ==========
My previous approach was still too simplistic and I had to add support for Mod R/M! I've updated the CL description and this CL is now bigger that it originally was :), PTAnL.
Haven't looked through everything, but here are some more items. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... File syzygy/core/disassembler_util.cc (right): https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:73: // If the SIB (Scale*Index+Base) bit is set then the operand uses an NIT: SIB is not a bit, but a special value. Maybe // If SIB (Scale*Index+Base) is specified... ? https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:75: const uint8_t kSIBMask = 0b100; NIT: SIB is not a mask; 0b101, 0b110, 0b111 are unrelated to SIB. Maybe kSIBValue? https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:81: // vpbroadcastb xmm2, BYTE PTR [edx+eax*2] NIT: Inconsistent tabbing for disassembly examples (below). https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:82: return 2; Depending how far you want to go down the rabbit hole... For mod == 0 only, another special case is that you'd need to read the SIB byte: 7 0 +---+---+---+---+---+---+---+---+ | scale | index | base | +---+---+---+---+---+---+---+---+ If |base| == 5 then you have +immd32 (instead of BP), i.e. need 4 more bytes. Try the Online disassembler with the following (second line is the anomalie) c4 e2 7d 58 04 c0 c4 e2 7d 58 04 c5 10 32 54 76 c4 e2 7d 58 44 c0 10 c4 e2 7d 58 44 c5 10 c4 e2 7d 58 84 c0 10 32 54 76 c4 e2 7d 58 84 c5 10 32 54 76 ^ SIB byte This enables stuff like (for second line) DWORD PTR [eax*8+0x76543210], which replaces DWORD PTR [eax*8+ebp]. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:83: } else if (modRMByte.r_m == 0b101) { Don't need "else" if returning? Same below. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:188: uint8_t rxb : 3; |rxb| is inverted, and should be labelled as such, e.g., |inv_rxb|? https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:197: uint8_t vvvv : 4; |vvvv| is inverted, and should be labelled as such, e.g., |inv_vvvv|? https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:216: << static_cast<size_t>(value) << "."; Add << std::dec at end to restore LOG(ERROR)'s state? https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:235: return true; Shorter to do return CheckField(...) && CheckField(...) && CheckField(...) && CheckField(...); https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:272: if (instruction.MatchExpectations(0b111, 0, 1, 1, "vbroadcasti128")) { NIT: Inconsistent of {} for these if's. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:277: if (instruction.MatchExpectations(0b111, 0, 1, 1, "vpbroadcastb")) 0x58 Should be vpbroadcastd (last letter). https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:280: case 0x78: // vpbroadcastd 0x78 Should be vpbroadcastb (last letter). Also, please sort the "case" values. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:289: case 0x90: // vpgatherdd case 0x8E disappeared? This is another vpmaskmovd that swaps the operands. Please check online disassembler: c4 e2 7d 8c 04 c0 c4 e2 7d 8e 04 c0 https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:301: case 0x38: // vinserti128 Sort?
Patchset #5 (id:80001) has been deleted
Thanks for the thorough review Sam (as always)! PTAnL. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... File syzygy/core/disassembler_util.cc (right): https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:73: // If the SIB (Scale*Index+Base) bit is set then the operand uses an On 2017/04/28 21:58:04, huangs wrote: > NIT: SIB is not a bit, but a special value. Maybe > // If SIB (Scale*Index+Base) is specified... > ? Done. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:75: const uint8_t kSIBMask = 0b100; On 2017/04/28 21:58:04, huangs wrote: > NIT: SIB is not a mask; 0b101, 0b110, 0b111 are unrelated to SIB. Maybe > kSIBValue? Ha, good point :) https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:81: // vpbroadcastb xmm2, BYTE PTR [edx+eax*2] On 2017/04/28 21:58:04, huangs wrote: > NIT: Inconsistent tabbing for disassembly examples (below). Done. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:82: return 2; On 2017/04/28 21:58:04, huangs wrote: > Depending how far you want to go down the rabbit hole... > > For mod == 0 only, another special case is that you'd need to read the SIB byte: > 7 0 > +---+---+---+---+---+---+---+---+ > | scale | index | base | > +---+---+---+---+---+---+---+---+ > > If |base| == 5 then you have +immd32 (instead of BP), i.e. need 4 more bytes. > Try the Online disassembler with the following (second line is the anomalie) > > c4 e2 7d 58 04 c0 > c4 e2 7d 58 04 c5 10 32 54 76 > c4 e2 7d 58 44 c0 10 > c4 e2 7d 58 44 c5 10 > c4 e2 7d 58 84 c0 10 32 54 76 > c4 e2 7d 58 84 c5 10 32 54 76 > ^ SIB byte > > This enables stuff like (for second line) DWORD PTR [eax*8+0x76543210], which > replaces DWORD PTR [eax*8+ebp]. Thanks! This is a simple special case and it doesn't seem to go deeper than that (according to http://www.c-jump.com/CIS77/CPU/x86/X77_0100_sib_byte_layout.htm) so I'll support it. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:83: } else if (modRMByte.r_m == 0b101) { On 2017/04/28 21:58:04, huangs wrote: > Don't need "else" if returning? Same below. Done. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:188: uint8_t rxb : 3; On 2017/04/28 21:58:04, huangs wrote: > |rxb| is inverted, and should be labelled as such, e.g., |inv_rxb|? Done. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:197: uint8_t vvvv : 4; On 2017/04/28 21:58:04, huangs wrote: > |vvvv| is inverted, and should be labelled as such, e.g., |inv_vvvv|? Done. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:216: << static_cast<size_t>(value) << "."; On 2017/04/28 21:58:04, huangs wrote: > Add > > << std::dec > > at end to restore LOG(ERROR)'s state? Done. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:235: return true; On 2017/04/28 21:58:04, huangs wrote: > Shorter to do > return CheckField(...) && > CheckField(...) && > CheckField(...) && > CheckField(...); Splitting this into multiple checks make it easier to debug (you can set breakpoints), this is a common pattern in the Syzygy codebase. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:272: if (instruction.MatchExpectations(0b111, 0, 1, 1, "vbroadcasti128")) { On 2017/04/28 21:58:04, huangs wrote: > NIT: Inconsistent of {} for these if's. Done. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:277: if (instruction.MatchExpectations(0b111, 0, 1, 1, "vpbroadcastb")) On 2017/04/28 21:58:04, huangs wrote: > 0x58 Should be vpbroadcastd (last letter). Done, thanks for spotting this! https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:280: case 0x78: // vpbroadcastd On 2017/04/28 21:58:04, huangs wrote: > 0x78 Should be vpbroadcastb (last letter). > > Also, please sort the "case" values. Done. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:289: case 0x90: // vpgatherdd On 2017/04/28 21:58:04, huangs wrote: > case 0x8E disappeared? This is another vpmaskmovd that swaps the operands. > Please check online disassembler: > > c4 e2 7d 8c 04 c0 > c4 e2 7d 8e 04 c0 Yeah, it wasn't properly unittested and I don't see it in the recent builds of Chrome, I'll re-add it in a further CL if needed. https://codereview.chromium.org/2841863003/diff/60001/syzygy/core/disassemble... syzygy/core/disassembler_util.cc:301: case 0x38: // vinserti128 On 2017/04/28 21:58:04, huangs wrote: > Sort? Done.
Round 2. https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... File syzygy/core/disassembler_util.cc (right): https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:37: // - mod: combined with the r/m field, encodes either 8 registers or 2 3 addressing modes, not 2? https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:47: // (if R/M = 0b100) or displacement only addressing mode (if R/M = 0b101). NIT: "R/M" here should be "r/m"? https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:68: // @param no_register_addressing_mode Indicates if the instruction supports NIT: Style-wise it's better to avoid negatives, i.e., have |has_register_addressing_mod|. But this is an annoying change, so I'll leave it to your discretion. https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:92: const uint8_t kSIBImm32Mask = 0b111; NIT: kSIBBaseMask? https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:180: // Check if this instruction match the expectations that we have for it. NIT: Checks https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:219: // Check if |value| is equal to |expected| value and log verbosely if it's not NIT: Checks https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:251: // Return the size of a 3-byte VEX encoded instruction. NIT: Returns https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:258: // constant, the 3 VEX bytes and the opcode). The 3 VEX bytes already includes the opcode. You mean the Mode R/M byte? https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:267: // Switch case based on the opcode mp used by this instruction. Typo: mp? https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:330: operand_size = GetModRMOperandBytesSize(ci, false); constants_size = 1? According to http://www.felixcloutier.com/x86/VEXTRACTI128.html VEXTRACTI128 takes Imm8. Example: C4 E3 75 39 00 42 => vextracti128 XMMWORD PTR [eax],ymm0,0x42 https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... File syzygy/core/disassembler_util_unittest.cc (right): https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util_unittest.cc:365: for (const auto iter : unittests::kVexInstructionsModRMVariants) { Maybe const auto& ? https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... File syzygy/core/disassembler_util_unittest_vex_utils.cc (right): https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util_unittest_vex_utils.cc:33: // vpermq ymm0, YMMWORD PTR [esi+eax*4+0x12345678], 0x42 I think this should be // vpermq ymm0, YMMWORD PTR [esi+eax*1+0x12345678], 0x42 ^ Here the SIB byte is 0x14 => scale = 0. If you want "*4" it should be 0x94? https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... File syzygy/core/disassembler_util_unittest_vex_utils.h (right): https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util_unittest_vex_utils.h:17: #include <stdint.h> for uint8_t.
Patchset #6 (id:120001) has been deleted
Thanks! PTAnL. https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... File syzygy/core/disassembler_util.cc (right): https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:37: // - mod: combined with the r/m field, encodes either 8 registers or 2 On 2017/05/01 18:54:13, huangs wrote: > 3 addressing modes, not 2? It should be "24" addressing modes (according to Wikipedia), and I think that it's accurate (if you look at all the possible combinations). https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:47: // (if R/M = 0b100) or displacement only addressing mode (if R/M = 0b101). On 2017/05/01 18:54:13, huangs wrote: > NIT: "R/M" here should be "r/m"? Done. https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:68: // @param no_register_addressing_mode Indicates if the instruction supports On 2017/05/01 18:54:13, huangs wrote: > NIT: Style-wise it's better to avoid negatives, i.e., have > |has_register_addressing_mod|. But this is an annoying change, so I'll leave it > to your discretion. Done, you're right it's cleaner like this (and not so annoying :) https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:92: const uint8_t kSIBImm32Mask = 0b111; On 2017/05/01 18:54:14, huangs wrote: > NIT: kSIBBaseMask? Done. https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:180: // Check if this instruction match the expectations that we have for it. On 2017/05/01 18:54:14, huangs wrote: > NIT: Checks Thanks. https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:219: // Check if |value| is equal to |expected| value and log verbosely if it's not On 2017/05/01 18:54:13, huangs wrote: > NIT: Checks Thanks again :) https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:251: // Return the size of a 3-byte VEX encoded instruction. On 2017/05/01 18:54:13, huangs wrote: > NIT: Returns ... And again :) https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:258: // constant, the 3 VEX bytes and the opcode). On 2017/05/01 18:54:13, huangs wrote: > The 3 VEX bytes already includes the opcode. You mean the Mode R/M byte? Yep, thanks for spotting this. https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:267: // Switch case based on the opcode mp used by this instruction. On 2017/05/01 18:54:14, huangs wrote: > Typo: mp? Oops https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:330: operand_size = GetModRMOperandBytesSize(ci, false); On 2017/05/01 18:54:13, huangs wrote: > constants_size = 1? According to > > http://www.felixcloutier.com/x86/VEXTRACTI128.html > > VEXTRACTI128 takes Imm8. Example: > > C4 E3 75 39 00 42 => vextracti128 XMMWORD PTR [eax],ymm0,0x42 You're right, the unittest for this instruction was missing! Now all the instructions are covered. https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... File syzygy/core/disassembler_util_unittest.cc (right): https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util_unittest.cc:365: for (const auto iter : unittests::kVexInstructionsModRMVariants) { On 2017/05/01 18:54:14, huangs wrote: > Maybe const auto& ? Done. https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... File syzygy/core/disassembler_util_unittest_vex_utils.cc (right): https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util_unittest_vex_utils.cc:33: // vpermq ymm0, YMMWORD PTR [esi+eax*4+0x12345678], 0x42 On 2017/05/01 18:54:14, huangs wrote: > I think this should be > // vpermq ymm0, YMMWORD PTR [esi+eax*1+0x12345678], 0x42 > ^ > Here the SIB byte is 0x14 => scale = 0. If you want "*4" it should be 0x94? You really see everything, thanks! https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... File syzygy/core/disassembler_util_unittest_vex_utils.h (right): https://codereview.chromium.org/2841863003/diff/100001/syzygy/core/disassembl... syzygy/core/disassembler_util_unittest_vex_utils.h:17: On 2017/05/01 18:54:14, huangs wrote: > #include <stdint.h> for uint8_t. Done.
LGTM with NITs. Please run full production flow to ensure things still work. Thanks! https://codereview.chromium.org/2841863003/diff/140001/syzygy/core/disassembl... File syzygy/core/disassembler_util.cc (right): https://codereview.chromium.org/2841863003/diff/140001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:68: // @param has_register_addressing_mod Indicates if the instruction supports Oops I made a type; should be |has_register_addressing_mode| (e at end). Sorry about that. https://codereview.chromium.org/2841863003/diff/140001/syzygy/core/disassembl... File syzygy/core/disassembler_util_unittest_vex_utils.cc (right): https://codereview.chromium.org/2841863003/diff/140001/syzygy/core/disassembl... syzygy/core/disassembler_util_unittest_vex_utils.cc:65: // vextracti128 XMMWORD PTR [edi+ecx*1], ymm5,0x1 NIT: Inconsistent spacing re. ",0x1".
Thanks! Waiting for Chris' review before committing this. https://codereview.chromium.org/2841863003/diff/140001/syzygy/core/disassembl... File syzygy/core/disassembler_util.cc (right): https://codereview.chromium.org/2841863003/diff/140001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:68: // @param has_register_addressing_mod Indicates if the instruction supports On 2017/05/01 21:42:19, huangs wrote: > Oops I made a type; should be |has_register_addressing_mode| (e at end). Sorry > about that. Ha, dumb me that just copy paste things :). (the name of the field is |mod|, so it seemed ok but I'll add the "e"). https://codereview.chromium.org/2841863003/diff/140001/syzygy/core/disassembl... File syzygy/core/disassembler_util_unittest_vex_utils.cc (right): https://codereview.chromium.org/2841863003/diff/140001/syzygy/core/disassembl... syzygy/core/disassembler_util_unittest_vex_utils.cc:65: // vextracti128 XMMWORD PTR [edi+ecx*1], ymm5,0x1 On 2017/05/01 21:42:19, huangs wrote: > NIT: Inconsistent spacing re. ",0x1". Ha!
lgtm! Thanks for solving VEX once and for all :) https://codereview.chromium.org/2841863003/diff/160001/syzygy/core/disassembl... File syzygy/core/disassembler_util.cc (right): https://codereview.chromium.org/2841863003/diff/160001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:89: // If |base| = 5 then there's an additional 4-byte used to encode the 4 bytes*
Don't jinx it! There's still some strict limitation on what kind of instruction are supported, so we'll see about the "once and for all" part :) Thanks, committing. https://codereview.chromium.org/2841863003/diff/160001/syzygy/core/disassembl... File syzygy/core/disassembler_util.cc (right): https://codereview.chromium.org/2841863003/diff/160001/syzygy/core/disassembl... syzygy/core/disassembler_util.cc:89: // If |base| = 5 then there's an additional 4-byte used to encode the On 2017/05/02 15:02:51, chrisha wrote: > 4 bytes* Done.
Description was changed from ========== Improve the decoding of the VEX encoded instructions. Currently our decoding assumes a constant instruction size (based on the opcode) but this doesn't really work because these instructions use a Mod R/M operand (see http://www.c-jump.com/CIS77/CPU/x86/X77_0090_addressing_modes.htm). This add support for the different sizes that these instructions can have, as well as some restrictions on which variants are supported. ========== to ========== Improve the decoding of the VEX encoded instructions. Currently our decoding assumes a constant instruction size (based on the opcode) but this doesn't really work because these instructions use a Mod R/M operand (see http://www.c-jump.com/CIS77/CPU/x86/X77_0090_addressing_modes.htm). This add support for the different sizes that these instructions can have, as well as some restrictions on which variants are supported. This has been tested on the latest official build of Chrome. ==========
The CQ bit was checked by sebmarchand@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from huangs@chromium.org, chrisha@chromium.org Link to the patchset: https://codereview.chromium.org/2841863003/#ps180001 (title: "nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493739420873460,
"parent_rev": "b8aa6a6d09dadd385a1afed5e71c3bb3514a4c0f", "commit_rev":
"5970d392dbf2d3746fe13212eea1dbc9af9cd46c"}
Message was sent while issue was closed.
Description was changed from ========== Improve the decoding of the VEX encoded instructions. Currently our decoding assumes a constant instruction size (based on the opcode) but this doesn't really work because these instructions use a Mod R/M operand (see http://www.c-jump.com/CIS77/CPU/x86/X77_0090_addressing_modes.htm). This add support for the different sizes that these instructions can have, as well as some restrictions on which variants are supported. This has been tested on the latest official build of Chrome. ========== to ========== Improve the decoding of the VEX encoded instructions. Currently our decoding assumes a constant instruction size (based on the opcode) but this doesn't really work because these instructions use a Mod R/M operand (see http://www.c-jump.com/CIS77/CPU/x86/X77_0090_addressing_modes.htm). This add support for the different sizes that these instructions can have, as well as some restrictions on which variants are supported. This has been tested on the latest official build of Chrome. Review-Url: https://codereview.chromium.org/2841863003 Committed: https://github.com/google/syzygy/commit/5970d392dbf2d3746fe13212eea1dbc9af9cd46c ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://github.com/google/syzygy/commit/5970d392dbf2d3746fe13212eea1dbc9af9cd46c |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
