[lldb] Handle backwards branches in UnwindAssemblyInstEmulation

[felipepiovezan]

This allows the unwinder to handle code with mid-function epilogues
where the subsequent code is reachable through a backwards branch.

Two changes are required to accomplish this:

1. Do not enqueue the subsequent instruction if the current instruction
   is a barrier(*).
2. When processing an instruction, stop ignoring branches with negative
   offsets.

(*) As per the definition in LLVM's MC layer, a barrier is any
instruction that "stops control flow from executing the instruction
immediately following it". See MCInstrDesc::isBarrier in MCInstrDesc.h

Part of a sequence of PRs:
[lldb][NFCI] Rewrite UnwindAssemblyInstEmulation in terms of a CFG visit #169630
[lldb][NFC] Rename forward_branch_offset to branch_offset in UnwindAssemblyInstEmulation #169631
[lldb] Add DisassemblerLLVMC::IsBarrier API #169632
[lldb] Handle backwards branches in UnwindAssemblyInstEmulation #169633

commit-id:fd266c13

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

This allows the unwinder to handle code with mid-function epilogues
where the subsequent code is reachable through a backwards branch.

Two changes are required to accomplish this:

1. Do not enqueue the subsequent instruction if the current instruction
   is a barrier(*).
2. When processing an instruction, stop ignoring branches with negative
   offsets.

(*) As per the definition in LLVM's MC layer, a barrier is any
instruction that "stops control flow from executing the instruction
immediately following it". See MCInstrDesc::isBarrier in MCInstrDesc.h

commit-id:fd266c13

---

Full diff: https://github.com/llvm/llvm-project/pull/169633.diff

3 Files Affected:

• (modified) lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp (+8-4)
• (modified) lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.h (+1-1)
• (modified) lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp (+18-3)

diff --git a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
index 21a01cc9126a0..c911b98539aec 100644
--- a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
+++ b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
@@ -227,6 +227,10 @@ bool UnwindAssemblyInstEmulation::GetNonCallSiteUnwindPlanFromAssembly(
       }
     }
 
+    // If inst is a barrier, do not propagate state to the next instruction.
+    if (inst.IsBarrier())
+      continue;
+
     // Were there any changes to the CFI while evaluating this instruction?
     if (m_curr_row_modified) {
       // Save the modified row if we don't already have a CFI row in the
@@ -530,20 +534,20 @@ bool UnwindAssemblyInstEmulation::WriteRegister(
   case EmulateInstruction::eContextAbsoluteBranchRegister:
   case EmulateInstruction::eContextRelativeBranchImmediate: {
     if (context.GetInfoType() == EmulateInstruction::eInfoTypeISAAndImmediate &&
-        context.info.ISAAndImmediate.unsigned_data32 > 0) {
+        context.info.ISAAndImmediate.unsigned_data32 != 0) {
       m_branch_offset = context.info.ISAAndImmediate.unsigned_data32;
     } else if (context.GetInfoType() ==
                    EmulateInstruction::eInfoTypeISAAndImmediateSigned &&
-               context.info.ISAAndImmediateSigned.signed_data32 > 0) {
+               context.info.ISAAndImmediateSigned.signed_data32 != 0) {
       m_branch_offset =
           context.info.ISAAndImmediateSigned.signed_data32;
     } else if (context.GetInfoType() ==
                    EmulateInstruction::eInfoTypeImmediate &&
-               context.info.unsigned_immediate > 0) {
+               context.info.unsigned_immediate != 0) {
       m_branch_offset = context.info.unsigned_immediate;
     } else if (context.GetInfoType() ==
                    EmulateInstruction::eInfoTypeImmediateSigned &&
-               context.info.signed_immediate > 0) {
+               context.info.signed_immediate != 0) {
       m_branch_offset = context.info.signed_immediate;
     }
   } break;
diff --git a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.h b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.h
index 1c80199235b4b..97f1093ba50bd 100644
--- a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.h
+++ b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.h
@@ -152,7 +152,7 @@ class UnwindAssemblyInstEmulation : public lldb_private::UnwindAssembly {
   bool m_curr_row_modified;
   // The instruction is branching forward with the given offset. 0 value means
   // no branching.
-  uint32_t m_branch_offset = 0;
+  int32_t m_branch_offset = 0;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_UNWINDASSEMBLY_INSTEMULATION_UNWINDASSEMBLYINSTEMULATION_H
diff --git a/lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp b/lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp
index 6c74860971674..b006103359801 100644
--- a/lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp
+++ b/lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp
@@ -1054,12 +1054,19 @@ TEST_F(TestArm64InstEmulation, TestMidFunctionEpilogueAndBackwardsJump) {
   EXPECT_TRUE(row->GetCFAValue().GetRegisterNumber() == gpr_sp_arm64);
   EXPECT_EQ(row->GetCFAValue().GetOffset(), 0);
 
-  // FIXME: Row for offset +32 incorrectly inherits the state of the `ret`
-  // instruction, but +32 _never_ executes after the `ret`.
+  // Row for offset +32 should not inherits the state of the `ret` instruction
+  // in +28. Instead, it should inherit the state of the branch in +64.
+  // Check for register x22, which is available in row +64.
   // <+28>: ret
   // <+32>: mov    x23, #0x1
   row = unwind_plan.GetRowForFunctionOffset(32);
-  // FIXME: EXPECT_NE(28, row->GetOffset());
+  EXPECT_EQ(32, row->GetOffset());
+  {
+    UnwindPlan::Row::AbstractRegisterLocation loc;
+    EXPECT_TRUE(row->GetRegisterInfo(gpr_x22_arm64, loc));
+    EXPECT_TRUE(loc.IsAtCFAPlusOffset());
+    EXPECT_EQ(loc.GetOffset(), -32);
+  }
 
   // Check that the state of this branch
   // <+16>: b.ne   ; <+52> DO_SOMETHING_AND_GOTO_AFTER_EPILOGUE
@@ -1070,4 +1077,12 @@ TEST_F(TestArm64InstEmulation, TestMidFunctionEpilogueAndBackwardsJump) {
   EXPECT_TRUE(row->GetCFAValue().IsRegisterPlusOffset());
   EXPECT_EQ(row->GetCFAValue().GetRegisterNumber(), gpr_fp_arm64);
   EXPECT_EQ(row->GetCFAValue().GetOffset(), 16);
+
+  row = unwind_plan.GetRowForFunctionOffset(64);
+  {
+    UnwindPlan::Row::AbstractRegisterLocation loc;
+    EXPECT_TRUE(row->GetRegisterInfo(gpr_x22_arm64, loc));
+    EXPECT_TRUE(loc.IsAtCFAPlusOffset());
+    EXPECT_EQ(loc.GetOffset(), -32);
+  }
 }

[DavidSpickett]

Do we know what the range of this is? In theory a very large immediate assigned into an int32_t is going to come out mangled.

[DavidSpickett]

Anything with the top bit set would be incorrect.

[felipepiovezan]

I think we need to make m_branch_offset and int64_t.

[felipepiovezan]

Actually, those are offsets encoded in instructions, so I think it is very unlikely they would ever be 32bits long, but it doesn't hurt to change the underlying type of m_branch_offset

[DavidSpickett]

Istr Arm having some "genius" way of encoding immediates where in some cases the immediate decodes to more than the field it was stored in. But yeah, easier to change the type than go check all the instructions.

[DavidSpickett]

inherits -> inherit

[DavidSpickett]

Do you need to update the comment above:

      // AFTER_EPILOGUE:  LLDB computes the next 5 unwind states incorrectly.