Page MenuHomePhabricator

[x64] Process the B field of the REX prefix correctly for the PUSH and POP instructions

Authored by aleksandr.urakov on Feb 5 2019, 4:28 AM.



This patch makes x86AssemblyInspectionEngine to process zero value of the B field of the REX prefix in a correct way for PUSH and POP instructions. MSVC sometimes emits pushq %rbp instruction as 0x40 0x55, and it was not parsed correctly before.

Diff Detail


Event Timeline

labath added a comment.Feb 5 2019, 5:39 AM

The change makes sense, but I'd just create a new test with the instructions you're interested in instead of modifying an existing one. The comment above the code claims this assembly was produced by clang, which is clearly not true after your modifications.

Yes, you are right, thank you! I've updated the patch.

labath accepted this revision.Feb 5 2019, 7:17 AM

Looks good to me.

I don't expect Jason to have any problems with this, but it might be nice to wait a day or two to give him a chance to prove me wrong. :)

This revision is now accepted and ready to land.Feb 5 2019, 7:17 AM
jasonmolenda accepted this revision.Feb 5 2019, 4:10 PM

The change looks good to me, thanks for fixing this. I'm not sure you're testing what you meant to test in TestPopRBPWithREX because you're pushing/popping r13. You could get the unwind state at byte offset 2 (after the push r13 has executed) and see that you can GetRegisterInfo() r13, and the unwind state at byte offset 4 and verify that you can't GetRegisterInfo() r13. That's a good test to make sure we handle the B bit correctly.


These are pushing/popping r13 aren't they? 0x40 0x55 gives us register 5 (rbp), but 0x41 0x55 gives us register 13 (r13) if I'm reading the intel manuals right (volume 2a, section "More on REX Prefix Fields").

Yes, sure, sorry about this, I occasionally have forgot to update the prefixes...

aleksandr.urakov marked an inline comment as done.Feb 6 2019, 12:46 AM

Thanks all for the review!

This revision was automatically updated to reflect the committed changes.