Skip to content

Commit f984113

Browse files
[lldb] Add test showing UnwindAssemblyInstEmulation can't handle backwards branches
If we have a conditional branch, followed by an epilogue, followed by more code, LLDB will incorrectly compute unwind information through instruction emulation. Consider this: ``` // ... <+16>: b.ne ; <+52> DO_SOMETHING_AND_GOTO_AFTER_EPILOGUE // epilogue start <+20>: ldp x29, x30, [sp, #0x20] <+24>: add sp, sp, #0x30 <+28>: ret // epilogue end AFTER_EPILOGUE: <+32>: do something // ... <+48>: ret DO_SOMETHING_AND_GOTO_AFTER_EPILOGUE: <+52>: stp x22, x23, [sp, #0x10] <+56>: mov x22, #0x1 <+64>: b ; <+32> AFTER_EPILOGUE ``` LLDB will think that the unwind state of +32 is the same as +28. This is false, as +32 _never_ executes after +28. The root cause of the problem is the order in which instructions are visited; they are visited in the order they appear in the text, with unwind state always being forwarded to positive branch offsets, but never to negative offsets. In the example above, `AFTER_EPILOGUE` should inherit the state of the branch in +64, but it doesn't because `AFTER_EPILOGUE` is visited right after the `ret` in +28. Fixing this should be simple: maintain a stack of instructions to visit. While the stack is not empty, take the next instruction on stack and visit it. * After visiting a non-branching instruction, push the next instruction and forward unwind state to it. * After visiting a branch with one or more known targets, push the known branch targets and forward state to them. * In all other cases (ret, or branch to register), don't push nor forward anything. Never push an instruction already on the stack. Like the algorithm today, this new algorithm also assume that, if two instructions branch to the same target, the unwind state in both better be the same. (Note: yes, branch to register is also handled incorrectly today, and will still be incorrect).
1 parent e69d2bf commit f984113

File tree

1 file changed

+107
-0
lines changed

1 file changed

+107
-0
lines changed

lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -856,3 +856,110 @@ TEST_F(TestArm64InstEmulation, TestCFAResetToSP) {
856856
EXPECT_TRUE(row->GetCFAValue().GetRegisterNumber() == gpr_sp_arm64);
857857
EXPECT_TRUE(row->GetCFAValue().IsRegisterPlusOffset() == true);
858858
}
859+
860+
TEST_F(TestArm64InstEmulation, TestMidFunctionEpilogueAndBackwardsJump) {
861+
ArchSpec arch("arm64-apple-ios15");
862+
std::unique_ptr<UnwindAssemblyInstEmulation> engine(
863+
static_cast<UnwindAssemblyInstEmulation *>(
864+
UnwindAssemblyInstEmulation::CreateInstance(arch)));
865+
ASSERT_NE(nullptr, engine);
866+
867+
const UnwindPlan::Row *row;
868+
AddressRange sample_range;
869+
UnwindPlan unwind_plan(eRegisterKindLLDB);
870+
UnwindPlan::Row::AbstractRegisterLocation regloc;
871+
872+
// clang-format off
873+
uint8_t data[] = {
874+
0xff, 0xc3, 0x00, 0xd1, // <+0>: sub sp, sp, #0x30
875+
0xfd, 0x7b, 0x02, 0xa9, // <+4>: stp x29, x30, [sp, #0x20]
876+
0xfd, 0x83, 0x00, 0x91, // <+8>: add x29, sp, #0x20
877+
0x1f, 0x04, 0x00, 0xf1, // <+12>: cmp x0, #0x1
878+
0x21, 0x01, 0x00, 0x54, // <+16>: b.ne ; <+52> DO_SOMETHING_AND_GOTO_AFTER_EPILOGUE
879+
0xfd, 0x7b, 0x42, 0xa9, // <+20>: ldp x29, x30, [sp, #0x20]
880+
0xff, 0xc3, 0x00, 0x91, // <+24>: add sp, sp, #0x30
881+
0xc0, 0x03, 0x5f, 0xd6, // <+28>: ret
882+
// AFTER_EPILOGUE: LLDB computes the next 5 unwind states incorrectly.
883+
0x37, 0x00, 0x80, 0xd2, // <+32>: mov x23, #0x1
884+
0xf6, 0x5f, 0x41, 0xa9, // <+36>: ldp x22, x23, [sp, #0x10]
885+
0xfd, 0x7b, 0x42, 0xa9, // <+40>: ldp x29, x30, [sp, #0x20]
886+
0xff, 0xc3, 0x00, 0x91, // <+44>: add sp, sp, #0x30
887+
0xc0, 0x03, 0x5f, 0xd6, // <+48>: ret
888+
// DO_SOMETHING_AND_GOTO_AFTER_EPILOGUE
889+
0xf6, 0x5f, 0x01, 0xa9, // <+52>: stp x22, x23, [sp, #0x10]
890+
0x36, 0x00, 0x80, 0xd2, // <+56>: mov x22, #0x1
891+
0x37, 0x00, 0x80, 0xd2, // <+60>: mov x23, #0x1
892+
0xf8, 0xff, 0xff, 0x17, // <+64>: b ; <+32> AFTER_EPILOGUE
893+
};
894+
895+
// UnwindPlan we expect:
896+
// row[0]: 0: CFA=sp +0 =>
897+
// row[1]: 4: CFA=sp+48 =>
898+
// row[2]: 8: CFA=sp+16 => fp=[CFA-16] lr=[CFA-8]
899+
// row[3]: 12: CFA=fp+16 => fp=[CFA-16] lr=[CFA-8]
900+
// row[4]: 24: CFA=sp+48 => fp=<same> lr=<same>
901+
//
902+
// This must come from +56
903+
// row[5]: 32: CFA=fp+16 => fp=[CFA-16] lr=[CFA-8] x22=[CFA-24], x23=[CFA-32]
904+
// row[6]: 40: CFA=fp+16 => fp=[CFA-16] lr=[CFA-8] x22=same, x23 = same
905+
// row[6]: 44: CFA=sp+48 => fp=same lr=same x22=same, x23 = same
906+
// row[6]: 48: CFA=sp0 => fp=same lr=same x22=same, x23 = same
907+
//
908+
// row[x]: 52: CFA=fp+16 => fp=[CFA-16] lr=[CFA-8]
909+
// row[x]: 56: CFA=fp+16 => fp=[CFA-16] lr=[CFA-8] x22=[CFA-24], x23=[CFA-32]
910+
// clang-format on
911+
912+
sample_range = AddressRange(0x1000, sizeof(data));
913+
914+
EXPECT_TRUE(engine->GetNonCallSiteUnwindPlanFromAssembly(
915+
sample_range, data, sizeof(data), unwind_plan));
916+
917+
// At the end of prologue (+12), CFA = fp + 16.
918+
// <+0>: sub sp, sp, #0x30
919+
// <+4>: stp x29, x30, [sp, #0x20]
920+
// <+8>: add x29, sp, #0x20
921+
row = unwind_plan.GetRowForFunctionOffset(12);
922+
EXPECT_EQ(12, row->GetOffset());
923+
EXPECT_TRUE(row->GetCFAValue().IsRegisterPlusOffset());
924+
EXPECT_EQ(row->GetCFAValue().GetRegisterNumber(), gpr_fp_arm64);
925+
EXPECT_EQ(row->GetCFAValue().GetOffset(), 16);
926+
927+
// +16 and +20 are the same as +12.
928+
// <+12>: cmp x0, #0x1
929+
// <+16>: b.ne ; <+52> DO_SOMETHING_AND_GOTO_AFTER_EPILOGUE
930+
EXPECT_EQ(12, unwind_plan.GetRowForFunctionOffset(16)->GetOffset());
931+
EXPECT_EQ(12, unwind_plan.GetRowForFunctionOffset(20)->GetOffset());
932+
933+
// After restoring $fp to caller's value, CFA = $sp + 48
934+
// <+20>: ldp x29, x30, [sp, #0x20]
935+
row = unwind_plan.GetRowForFunctionOffset(24);
936+
EXPECT_EQ(24, row->GetOffset());
937+
EXPECT_TRUE(row->GetCFAValue().IsRegisterPlusOffset());
938+
EXPECT_TRUE(row->GetCFAValue().GetRegisterNumber() == gpr_sp_arm64);
939+
EXPECT_EQ(row->GetCFAValue().GetOffset(), 48);
940+
941+
// $sp has been restored
942+
// <+24>: add sp, sp, #0x30
943+
row = unwind_plan.GetRowForFunctionOffset(28);
944+
EXPECT_EQ(28, row->GetOffset());
945+
EXPECT_TRUE(row->GetCFAValue().IsRegisterPlusOffset());
946+
EXPECT_TRUE(row->GetCFAValue().GetRegisterNumber() == gpr_sp_arm64);
947+
EXPECT_EQ(row->GetCFAValue().GetOffset(), 0);
948+
949+
// FIXME: Row for offset +32 incorrectly inherits the state of the `ret`
950+
// instruction, but +32 _never_ executes after the `ret`.
951+
// <+28>: ret
952+
// <+32>: mov x23, #0x1
953+
row = unwind_plan.GetRowForFunctionOffset(32);
954+
// FIXME: EXPECT_NE(32, row->GetOffset());
955+
956+
// Check that the state of this branch
957+
// <+16>: b.ne ; <+52> DO_SOMETHING_AND_GOTO_AFTER_EPILOGUE
958+
// was forwarded to the branch target:
959+
// <+52>: stp x22, x23, [sp, #0x10]
960+
row = unwind_plan.GetRowForFunctionOffset(52);
961+
EXPECT_EQ(52, row->GetOffset());
962+
EXPECT_TRUE(row->GetCFAValue().IsRegisterPlusOffset());
963+
EXPECT_EQ(row->GetCFAValue().GetRegisterNumber(), gpr_fp_arm64);
964+
EXPECT_EQ(row->GetCFAValue().GetOffset(), 16);
965+
}

0 commit comments

Comments
 (0)