Change UnwindAssemblyInstEmulation to remove a register location instead of marking it as IsSame()

[jasonmolenda]

Summary

This is a minor change and maybe more of a personal preference, but UnwindAssemblyInstEmulation marks registers that have been restored to their original values as "IsSame" - which is equivalent to the UnwindPlan not mentioning a location for the register at all. When I look at an unwind plan while a function is executing it's epilogue, the output looks like

row[14]: 520: CFA=sp+320 => x8=[CFA-240] x19= <same> x20= <same> x21= <same> x22= <same> x23=[CFA-56] x24=[CFA-64] x25=[CFA-72] x26=[CFA-80] x27=[CFA-88] x28=[CFA-96] fp= <same> lr= <same>
row[15]: 524: CFA=sp+320 => x8=[CFA-240] x19= <same> x20= <same> x21= <same> x22= <same> x23= <same> x24= <same> x25=[CFA-72] x26=[CFA-80] x27=[CFA-88] x28=[CFA-96] fp= <same> lr= <same>
row[16]: 528: CFA=sp+320 => x8=[CFA-240] x19= <same> x20= <same> x21= <same> x22= <same> x23= <same> x24= <same> x25= <same> x26= <same> x27=[CFA-88] x28=[CFA-96] fp= <same> lr= <same>
row[17]: 532: CFA=sp +0 => x8=[CFA-240] x19= <same> x20= <same> x21= <same> x22= <same> x23= <same> x24= <same> x25= <same> x26= <same> x27= <same> x28= <same> fp= <same> lr= <same>

etc. If we remove the registers from the row, the unwind dump will read like

row[14]: 516: CFA=sp+96 => x8=[CFA-240] x21=[CFA-40] x22=[CFA-48] x23=[CFA-56] x24=[CFA-64] x25=[CFA-72] x26=[CFA-80] x27=[CFA-88] x28=[CFA-96]
row[15]: 520: CFA=sp+96 => x8=[CFA-240] x23=[CFA-56] x24=[CFA-64] x25=[CFA-72] x26=[CFA-80] x27=[CFA-88] x28=[CFA-96]
row[16]: 524: CFA=sp+96 => x8=[CFA-240] x25=[CFA-72] x26=[CFA-80] x27=[CFA-88] x28=[CFA-96]
row[17]: 528: CFA=sp+96 => x8=[CFA-240] x27=[CFA-88] x28=[CFA-96]

There's no functional difference between the two (there SHOULD be no functional difference) - but I think it's a bit easier to read without the IsSame's. Does anyone care one way or the other?

[tberghammer]

My understanding (can be wrong) is that there IS a difference between not specifying a register in the unwind info versus specifying it as "is same" for volatile (caller saved) registers. I think for volatile registers not specifying them means that we can't access their current value in the parent frame (because they are lost) while specifying them as "is same" means we know that their value in the parent frame is the same as in the current one so we can display them.

If "is same" and "not specified" would be equivalent then we should just completely get rid of the "is same" case (as it is useless) but as far as I know they have subtle differences.

[jasonmolenda]

Ah, interesting point, I didn't think of that. However, this touches on another thing I've been thinking about as I look at the assembly inspection unwind plan generators. In the x86 unwind inspector, I've hardcoded the SysV-x86_64 ABI and the unwind plan generator ignores any saves/restores of volatile registers. It's a poor choice and it's the kind of thing that surely won't be correct when a Windows port is up & running.

I'm thinking the unwind plan generators should treat all registers as non-volatile. When UnwindLLDB / RegisterContextLLDB run the UnwindPlan, they can ask the ABI if a register is volatile or not - and refuse to retrieve a volatile register for a stack frame in the middle of the stack. (it SHOULD be doing that already)

The problem with tracking a register that is volatile is that as soon as the function makes a call into another function, we have to assume the register value is overwritten. So if we have

0xfffffff021f7bd80 <+0>:    stp    x28, x27, [sp, #-0x60]!
0xfffffff021f7bd84 <+4>:    stp    x26, x25, [sp, #0x10]
0xfffffff021f7bd88 <+8>:    stp    x24, x23, [sp, #0x20]
0xfffffff021f7bd8c <+12>:   stp    x22, x21, [sp, #0x30]
0xfffffff021f7bd90 <+16>:   stp    x20, x19, [sp, #0x40]
0xfffffff021f7bd94 <+20>:   stp    x29, x30, [sp, #0x50]
0xfffffff021f7bd98 <+24>:   add    x29, sp, #0x50            ; =0x50 
0xfffffff021f7bd9c <+28>:   sub    sp, sp, #0xe0             ; =0xe0 
0xfffffff021f7bdd4 <+84>:   bl     0xfffffff021f8af70  

0xfffffff021f7c334 <+1460>: str    w9, [sp, #0x60]

x9 is volatile in the AAPCS64 ABI, so at this offset in the assembly the value has already been overwritten by the call instruction at +84. If I later see a load of x9 and mark the register as "IsSame", now we've got a problem because we're saying it has the original value.

If we were going to follow the IsSame-means-unmodified thinking through, we'd want to mark every register as IsSame on function entry and only remove that markup when the register is modified.

I guess I'm trying to say two things. (1) I'd really like to get rid of IsSame so that the unwind plan dumps are easier for me to read ;) and (2) I think the instruction profiler unwind plan generators should track all registers without knowledge of the ABI, and leave it to the runtime code to decide which registers we will allow to be looked up.

source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp

const uint32_t reg_num =
         reg_info->kinds[m_unwind_plan_ptr->GetRegisterKind()];
     const uint32_t generic_regnum = reg_info->kinds[eRegisterKindGeneric];
     if (reg_num != LLDB_INVALID_REGNUM &&
         generic_regnum != LLDB_REGNUM_GENERIC_SP) {
       switch (context.info_type) {
       case EmulateInstruction::eInfoTypeAddress:
         if (m_pushed_regs.find(reg_num) != m_pushed_regs.end() &&
             context.info.address == m_pushed_regs[reg_num]) {
-          m_curr_row->SetRegisterLocationToSame(reg_num,
-                                                false /*must_replace*/);
+          m_curr_row->RemoveRegisterInfo(reg_num);
           m_curr_row_modified = true;
         }
         break;
       case EmulateInstruction::eInfoTypeISA:
         assert(
             (generic_regnum == LLDB_REGNUM_GENERIC_PC ||
              generic_regnum == LLDB_REGNUM_GENERIC_FLAGS) &&
             "eInfoTypeISA used for poping a register other the the PC/FLAGS");
         if (generic_regnum != LLDB_REGNUM_GENERIC_FLAGS) {
-          m_curr_row->SetRegisterLocationToSame(reg_num,
-                                                false /*must_replace*/);
+          m_curr_row->RemoveRegisterInfo(reg_num);
           m_curr_row_modified = true;
         }
         break;
       default:
         assert(false && "unhandled case, add code to handle this!");
         break;
       }
     }

unittests/UnwindAssembly/InstEmulation/TestArm64InstEmulation.cpp

TEST_F(TestArm64InstEmulation, TestMediumDarwinFunction) {
   EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
   EXPECT_EQ(48, row_sp->GetCFAValue().GetOffset());
 
   // 32: CFA=sp+48 => x19=[CFA-24] x20=[CFA-32] x21=[CFA-40] x22=[CFA-48] fp=
   // <same> lr= <same>
   row_sp = unwind_plan.GetRowForFunctionOffset(32);
   EXPECT_EQ(32ull, row_sp->GetOffset());
 
-  // I'd prefer if these restored registers were cleared entirely instead of set
-  // to IsSame...
-  EXPECT_TRUE(row_sp->GetRegisterInfo(gpr_fp_arm64, regloc));
-  EXPECT_TRUE(regloc.IsSame());
-
-  EXPECT_TRUE(row_sp->GetRegisterInfo(gpr_lr_arm64, regloc));
-  EXPECT_TRUE(regloc.IsSame());
+  EXPECT_FALSE(row_sp->GetRegisterInfo(gpr_fp_arm64, regloc));
+  EXPECT_FALSE(row_sp->GetRegisterInfo(gpr_lr_arm64, regloc));
 
   // 36: CFA=sp+48 => x19= <same> x20= <same> x21=[CFA-40] x22=[CFA-48] fp=
   // <same> lr= <same>
   row_sp = unwind_plan.GetRowForFunctionOffset(36);
   EXPECT_EQ(36ull, row_sp->GetOffset());
 
-  EXPECT_TRUE(row_sp->GetRegisterInfo(gpr_x19_arm64, regloc));
-  EXPECT_TRUE(regloc.IsSame());
-
-  EXPECT_TRUE(row_sp->GetRegisterInfo(gpr_x20_arm64, regloc));
-  EXPECT_TRUE(regloc.IsSame());
+  EXPECT_FALSE(row_sp->GetRegisterInfo(gpr_x19_arm64, regloc));
+  EXPECT_FALSE(row_sp->GetRegisterInfo(gpr_x20_arm64, regloc));
 
   // 40: CFA=sp +0 => x19= <same> x20= <same> x21= <same> x22= <same> fp= <same>
   // lr= <same>
   row_sp = unwind_plan.GetRowForFunctionOffset(40);
   EXPECT_EQ(40ull, row_sp->GetOffset());
   EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == gpr_sp_arm64);
   EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
   EXPECT_EQ(0, row_sp->GetCFAValue().GetOffset());
 
-  EXPECT_TRUE(row_sp->GetRegisterInfo(gpr_x21_arm64, regloc));
-  EXPECT_TRUE(regloc.IsSame());
-
-  EXPECT_TRUE(row_sp->GetRegisterInfo(gpr_x22_arm64, regloc));
-  EXPECT_TRUE(regloc.IsSame());
+  EXPECT_FALSE(row_sp->GetRegisterInfo(gpr_x21_arm64, regloc));
+  EXPECT_FALSE(row_sp->GetRegisterInfo(gpr_x22_arm64, regloc));
 }
 
 TEST_F(TestArm64InstEmulation, TestFramelessThreeEpilogueFunction) {
   ArchSpec arch("arm64-apple-ios10", nullptr);
   UnwindAssemblyInstEmulation *engine =
       static_cast<UnwindAssemblyInstEmulation *>(
           UnwindAssemblyInstEmulation::CreateInstance(arch));
   ASSERT_NE(nullptr, engine);
TEST_F(TestArm64InstEmulation, TestRegisterDoubleSpills) {
 
   //  60: CFA=sp +0 =>
   row_sp = unwind_plan.GetRowForFunctionOffset(60);
   EXPECT_EQ(60ull, row_sp->GetOffset());
   EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == gpr_sp_arm64);
   EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
   EXPECT_EQ(0, row_sp->GetCFAValue().GetOffset());
 
-  if (row_sp->GetRegisterInfo(fpu_d8_arm64, regloc))
-    EXPECT_TRUE(regloc.IsSame());
-  if (row_sp->GetRegisterInfo(fpu_d9_arm64, regloc))
-    EXPECT_TRUE(regloc.IsSame());
-  if (row_sp->GetRegisterInfo(fpu_d10_arm64, regloc))
-    EXPECT_TRUE(regloc.IsSame());
-  if (row_sp->GetRegisterInfo(fpu_d11_arm64, regloc))
-    EXPECT_TRUE(regloc.IsSame());
-  if (row_sp->GetRegisterInfo(fpu_d12_arm64, regloc))
-    EXPECT_TRUE(regloc.IsSame());
-  if (row_sp->GetRegisterInfo(fpu_d13_arm64, regloc))
-    EXPECT_TRUE(regloc.IsSame());
-  if (row_sp->GetRegisterInfo(fpu_d14_arm64, regloc))
-    EXPECT_TRUE(regloc.IsSame());
-  if (row_sp->GetRegisterInfo(fpu_d15_arm64, regloc))
-    EXPECT_TRUE(regloc.IsSame());
-  if (row_sp->GetRegisterInfo(gpr_x27_arm64, regloc))
-    EXPECT_TRUE(regloc.IsSame());
-  if (row_sp->GetRegisterInfo(gpr_x28_arm64, regloc))
-    EXPECT_TRUE(regloc.IsSame());
+  EXPECT_FALSE(row_sp->GetRegisterInfo(fpu_d8_arm64, regloc));
+  EXPECT_FALSE(row_sp->GetRegisterInfo(fpu_d9_arm64, regloc));
+  EXPECT_FALSE(row_sp->GetRegisterInfo(fpu_d10_arm64, regloc));
+  EXPECT_FALSE(row_sp->GetRegisterInfo(fpu_d11_arm64, regloc));
+  EXPECT_FALSE(row_sp->GetRegisterInfo(fpu_d12_arm64, regloc));
+  EXPECT_FALSE(row_sp->GetRegisterInfo(fpu_d13_arm64, regloc));
+  EXPECT_FALSE(row_sp->GetRegisterInfo(fpu_d14_arm64, regloc));
+  EXPECT_FALSE(row_sp->GetRegisterInfo(fpu_d15_arm64, regloc));
+  EXPECT_FALSE(row_sp->GetRegisterInfo(gpr_x27_arm64, regloc));
+  EXPECT_FALSE(row_sp->GetRegisterInfo(gpr_x28_arm64, regloc));
 }