Fix x86 return pattern detection

[mortimer]

Summary

Replace 0xc9 (LEAVE) with 0xcb (RETF) in ret_pattern_p(). Also put 0xc3 first, since it is the most common form and will match first.

[labath]

Could you please write a test for this? You can take a look at unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp for other x86AssemblyInspectionEngine tests.

Also, I'm curious how you found this bug. (i.e. which functionality was broken with the old implementation).

[mortimer]

For sure I can add a test.

I found this only because I was looking to fix something else (prologue detection on OpenBSD with -fret-protector) and noticed that the return pattern detect function was wrong. So nothing was broken that I found, though the inclusion of 0xc9 as a return pattern could conceivably cause problems under some circumstances. In any event, I figured it would be easy enough to submit a fix, since it’s a tiny change.

[mortimer]

Added a test to verify that the unwind state is reset after each kind of return.

[jasonmolenda]

Thanks for fixing this and adding a testcase. I don't know why 0xc9 leave was here; it's handled over in x86AssemblyInspectionEngine::leave_pattern_p. Do you have commit access?

[mortimer]

Happy to help out. :-) I do not have commit access.

[labath]

Thanks for the fix and the test.

source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp

if (*p == 0x89) {
 
     regno = ((*(p + 1) >> 3) & 0x7) | src_reg_prefix_bit;
     rbp_offset = offset > 0 ? offset : -offset;
     return true;
   }
   return false;
 }
 
-// ret [0xc9] or [0xc2 imm8] or [0xca imm8]
+// ret [0xc3] or [0xcb] or [0xc2 imm16] or [0xca imm16]
 bool x86AssemblyInspectionEngine::ret_pattern_p() {
   uint8_t *p = m_cur_insn;
-  return *p == 0xc9 || *p == 0xc2 || *p == 0xca || *p == 0xc3;
+  return *p == 0xc3 || *p == 0xc2 || *p == 0xca || *p == 0xcb;
 }
 
 uint32_t x86AssemblyInspectionEngine::extract_4(uint8_t *b) {
   uint32_t v = 0;
   for (int i = 3; i >= 0; i--)
     v = (v << 8) | b[i];
   return v;
 }

unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp

TEST_F(Testx86AssemblyInspectionEngine, TestDisassemblyJunkBytes) {
 
   unwind_plan.Clear();
 
   EXPECT_TRUE(engine64->GetNonCallSiteUnwindPlanFromAssembly(
       data, sizeof(data), sample_range, unwind_plan));
 
 }
 
+TEST_F(Testx86AssemblyInspectionEngine, TestReturnDetect) {
+  std::unique_ptr<x86AssemblyInspectionEngine> engine = Getx86_64Inspector();
+
+  // Single fragment with all four return forms.
+  // We want to verify that the unwind state is reset after each ret.
+  uint8_t data[] = {
+      0x55,             // offset  0 -- pushq %rbp
+      0x48, 0x89, 0xe5, // offset  1 -- movq %rsp, %rbp
+      0x31, 0xc0,       // offset  4 -- xorl %eax, %eax
+      0x5d,             // offset  6 -- popq %rbp
+      0xc3,             // offset  7 -- retq
+      0x31, 0xc0,       // offset  8 -- xorl %eax, %eax
+      0x5d,             // offset 10 -- popq %rbp
+      0xcb,             // offset 11 -- retf
+      0x31, 0xc0,       // offset 12 -- xorl %eax, %eax
+      0x5d,             // offset 14 -- popq %rbp
+      0xc2, 0x22, 0x11, // offset 15 -- retq 0x1122
+      0x31, 0xc0,       // offset 18 -- xorl %eax, %eax
+      0x5d,             // offset 20 -- popq %rbp
+      0xca, 0x44, 0x33, // offset 21 -- retf 0x3344
+      0x31, 0xc0,       // offset 24 -- xorl %eax, %eax
+  };
+
+  AddressRange sample_range(0x1000, sizeof(data));
+
+  UnwindPlan unwind_plan(eRegisterKindLLDB);
+  EXPECT_TRUE(engine->GetNonCallSiteUnwindPlanFromAssembly(
+      data, sizeof(data), sample_range, unwind_plan));
+
+  // Expect following unwind rows:
+  //  0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
+  //  1: CFA=rsp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
+  //  4: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
+  //  7: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
+  //  8: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
+  // 11: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
+  // 12: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
+  // 15: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
+  // 18: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
+  // 21: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
+  // 24: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
+
+  EXPECT_TRUE(unwind_plan.GetInitialCFARegister() == k_rsp);
+  EXPECT_TRUE(unwind_plan.GetUnwindPlanValidAtAllInstructions() ==
+              eLazyBoolYes);
+  EXPECT_TRUE(unwind_plan.GetSourcedFromCompiler() == eLazyBoolNo);
+
+  UnwindPlan::Row::RegisterLocation regloc;
+
+  //  0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
+  UnwindPlan::RowSP row_sp = unwind_plan.GetRowForFunctionOffset(0);
+  EXPECT_EQ(0ull, 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_TRUE(row_sp->GetRegisterInfo(k_rip, regloc));
+  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
+  EXPECT_EQ(-8, regloc.GetOffset());
+
+  //  1: CFA=rsp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
+  row_sp = unwind_plan.GetRowForFunctionOffset(1);
+  EXPECT_EQ(1ull, 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_rip, regloc));
+  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
+  EXPECT_EQ(-8, regloc.GetOffset());
+
+  //  4: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
+  row_sp = unwind_plan.GetRowForFunctionOffset(4);
+  EXPECT_EQ(4ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_rbp);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(16, row_sp->GetCFAValue().GetOffset());
+
+  EXPECT_TRUE(row_sp->GetRegisterInfo(k_rip, regloc));
+  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
+  EXPECT_EQ(-8, regloc.GetOffset());
+
+  //  7: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
+  row_sp = unwind_plan.GetRowForFunctionOffset(7);
+  EXPECT_EQ(7ull, 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_TRUE(row_sp->GetRegisterInfo(k_rip, regloc));
+  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
+  EXPECT_EQ(-8, regloc.GetOffset());
+
+  //  8: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
+  row_sp = unwind_plan.GetRowForFunctionOffset(8);
+  EXPECT_EQ(8ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_rbp);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(16, row_sp->GetCFAValue().GetOffset());
+
+  EXPECT_TRUE(row_sp->GetRegisterInfo(k_rip, regloc));
+  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
+  EXPECT_EQ(-8, regloc.GetOffset());
+
+  // 11: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
+  row_sp = unwind_plan.GetRowForFunctionOffset(11);
+  EXPECT_EQ(11ull, 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_TRUE(row_sp->GetRegisterInfo(k_rip, regloc));
+  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
+  EXPECT_EQ(-8, regloc.GetOffset());
+
+  // 12: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
+  row_sp = unwind_plan.GetRowForFunctionOffset(12);
+  EXPECT_EQ(12ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_rbp);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(16, row_sp->GetCFAValue().GetOffset());
+
+  EXPECT_TRUE(row_sp->GetRegisterInfo(k_rip, regloc));
+  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
+  EXPECT_EQ(-8, regloc.GetOffset());
+
+  // 15: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
+  row_sp = unwind_plan.GetRowForFunctionOffset(15);
+  EXPECT_EQ(15ull, 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_TRUE(row_sp->GetRegisterInfo(k_rip, regloc));
+  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
+  EXPECT_EQ(-8, regloc.GetOffset());
+
+  // 18: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
+  row_sp = unwind_plan.GetRowForFunctionOffset(18);
+  EXPECT_EQ(18ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_rbp);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(16, row_sp->GetCFAValue().GetOffset());
+
+  EXPECT_TRUE(row_sp->GetRegisterInfo(k_rip, regloc));
+  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
+  EXPECT_EQ(-8, regloc.GetOffset());
+
+  // 21: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
+  row_sp = unwind_plan.GetRowForFunctionOffset(21);
+  EXPECT_EQ(21ull, 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_TRUE(row_sp->GetRegisterInfo(k_rip, regloc));
+  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
+  EXPECT_EQ(-8, regloc.GetOffset());
+
+  // 24: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8]
+  row_sp = unwind_plan.GetRowForFunctionOffset(24);
+  EXPECT_EQ(24ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_rbp);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(16, row_sp->GetCFAValue().GetOffset());
+
+  EXPECT_TRUE(row_sp->GetRegisterInfo(k_rip, regloc));
+  EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
+  EXPECT_EQ(-8, regloc.GetOffset());
+}