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

[aleksandr.urakov]

Summary

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.

[labath]

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.

[aleksandr.urakov]

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

[labath]

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. :)

[jasonmolenda]

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.

unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp

1953

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 2.2.1.2 "More on REX Prefix Fields").

[aleksandr.urakov]

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

[aleksandr.urakov]

Thanks all for the review!

source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp

 }
 
 // pushq %rbx
 // pushl %ebx
 bool x86AssemblyInspectionEngine::push_reg_p(int &regno) {
   uint8_t *p = m_cur_insn;
   int regno_prefix_bit = 0;
   // If we have a rex prefix byte, check to see if a B bit is set
-  if (m_wordsize == 8 && *p == 0x41) {
-    regno_prefix_bit = 1 << 3;
+  if (m_wordsize == 8 && (*p & 0xfe) == 0x40) {
+    regno_prefix_bit = (*p & 1) << 3;
     p++;
   }
   if (*p >= 0x50 && *p <= 0x57) {
     regno = (*p - 0x50) | regno_prefix_bit;
     return true;
   }
   return false;
 }

 }
 
 // popq %rbx
 // popl %ebx
 bool x86AssemblyInspectionEngine::pop_reg_p(int &regno) {
   uint8_t *p = m_cur_insn;
   int regno_prefix_bit = 0;
   // If we have a rex prefix byte, check to see if a B bit is set
-  if (m_wordsize == 8 && *p == 0x41) {
-    regno_prefix_bit = 1 << 3;
+  if (m_wordsize == 8 && (*p & 0xfe) == 0x40) {
+    regno_prefix_bit = (*p & 1) << 3;
     p++;
   }
   if (*p >= 0x58 && *p <= 0x5f) {
     regno = (*p - 0x58) | regno_prefix_bit;
     return true;
   }
   return false;
 }

unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp

TEST_F(Testx86AssemblyInspectionEngine, TestPushEBP) {
   EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
   EXPECT_EQ(8, row_sp->GetCFAValue().GetOffset());
 
   EXPECT_TRUE(row_sp->GetRegisterInfo(k_ebp, regloc));
   EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
   EXPECT_EQ(-8, regloc.GetOffset());
 }
 
+TEST_F(Testx86AssemblyInspectionEngine, TestPushRBPWithREX) {
+  UnwindPlan::Row::RegisterLocation regloc;
+  UnwindPlan::RowSP row_sp;
+
+  uint8_t data[] = {
+      0x40, 0x55, // pushq %rbp
+      0x90  // nop
+  };
+
+  AddressRange sample_range(0x1000, sizeof(data));
+  UnwindPlan unwind_plan(eRegisterKindLLDB);
+
+  std::unique_ptr<x86AssemblyInspectionEngine> engine64 = Getx86_64Inspector();
+  EXPECT_TRUE(engine64->GetNonCallSiteUnwindPlanFromAssembly(
+      data, sizeof(data), sample_range, unwind_plan));
+
+  row_sp = unwind_plan.GetRowForFunctionOffset(2);
+
+  EXPECT_EQ(2ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_rsp);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(16, row_sp->GetCFAValue().GetOffset());
+
+  EXPECT_TRUE(row_sp->GetRegisterInfo(k_rbp, regloc));
+  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
+  EXPECT_EQ(-16, regloc.GetOffset());
+}
+
 TEST_F(Testx86AssemblyInspectionEngine, TestPushESI) {
   UnwindPlan::Row::RegisterLocation regloc;
   UnwindPlan::RowSP row_sp;
   AddressRange sample_range;
   UnwindPlan unwind_plan(eRegisterKindLLDB);
   std::unique_ptr<x86AssemblyInspectionEngine> engine32 = Geti386Inspector();
 
   uint8_t data[] = {
TEST_F(Testx86AssemblyInspectionEngine, TestPopEBP) {
   row_sp = unwind_plan.GetRowForFunctionOffset(2);
   EXPECT_EQ(2ull, row_sp->GetOffset());
   EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_rsp);
   EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
   EXPECT_EQ(4, row_sp->GetCFAValue().GetOffset());
   EXPECT_FALSE(row_sp->GetRegisterInfo(k_ebp, regloc));
 }
 
+TEST_F(Testx86AssemblyInspectionEngine, TestPopRBPWithREX) {
+  UnwindPlan::Row::RegisterLocation regloc;
+  UnwindPlan::RowSP row_sp;
+  AddressRange sample_range;
+  UnwindPlan unwind_plan(eRegisterKindLLDB);
+  std::unique_ptr<x86AssemblyInspectionEngine> engine = Getx86_64Inspector();
+
+  uint8_t data[] = {
+      0x40, 0x55, // pushq %rbp
+      0x40, 0x5d, // popq %rbp
+      0x90        // nop
+  };
+
+  sample_range = AddressRange(0x1000, sizeof(data));
+
+  EXPECT_TRUE(engine->GetNonCallSiteUnwindPlanFromAssembly(
+      data, sizeof(data), sample_range, unwind_plan));
+
+  row_sp = unwind_plan.GetRowForFunctionOffset(4);
+  EXPECT_EQ(4ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_rsp);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(8, row_sp->GetCFAValue().GetOffset());
+  EXPECT_FALSE(row_sp->GetRegisterInfo(k_rbp, regloc));
+}
+
 TEST_F(Testx86AssemblyInspectionEngine, TestPopESI) {
   UnwindPlan::Row::RegisterLocation regloc;
   UnwindPlan::RowSP row_sp;
   AddressRange sample_range;
   UnwindPlan unwind_plan(eRegisterKindLLDB);
   std::unique_ptr<x86AssemblyInspectionEngine> engine = Geti386Inspector();
 
   uint8_t data[] = {