Skip to content

[lldb] Handle backwards branches in UnwindAssemblyInstEmulation #169633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: users/felipepiovezan/spr/main.lldb-handle-backwards-branches-in-unwindassemblyinstemulation
Choose a base branch
from
+28 −9
Open

[lldb] Handle backwards branches in UnwindAssemblyInstEmulation #169633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b9ca2d8
[𝘀𝗽𝗿] initial version
felipepiovezan Nov 26, 2025
d50c702
rebase
felipepiovezan Nov 26, 2025
6c51a00
Address review comment
felipepiovezan Nov 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -530,19 +534,19 @@ 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;
Copy link
Collaborator

@DavidSpickett DavidSpickett Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@DavidSpickett DavidSpickett Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything with the top bit set would be incorrect.

Copy link
Contributor Author

@felipepiovezan felipepiovezan Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to make m_branch_offset and int64_t.

Copy link
Contributor Author

@felipepiovezan felipepiovezan Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@DavidSpickett DavidSpickett Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} 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;
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
int64_t m_branch_offset = 0;
};

#endif // LLDB_SOURCE_PLUGINS_UNWINDASSEMBLY_INSTEMULATION_UNWINDASSEMBLYINSTEMULATION_H
23 changes: 19 additions & 4 deletions lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -987,7 +987,7 @@ TEST_F(TestArm64InstEmulation, TestMidFunctionEpilogueAndBackwardsJump) {
0xfd, 0x7b, 0x42, 0xa9, // <+20>: ldp x29, x30, [sp, #0x20]
0xff, 0xc3, 0x00, 0x91, // <+24>: add sp, sp, #0x30
0xc0, 0x03, 0x5f, 0xd6, // <+28>: ret
// AFTER_EPILOGUE: LLDB computes the next 5 unwind states incorrectly.
// AFTER_EPILOGUE
0x37, 0x00, 0x80, 0xd2, // <+32>: mov x23, #0x1
0xf6, 0x5f, 0x41, 0xa9, // <+36>: ldp x22, x23, [sp, #0x10]
0xfd, 0x7b, 0x42, 0xa9, // <+40>: ldp x29, x30, [sp, #0x20]
Expand Down Expand Up @@ -1054,12 +1054,19 @@ TEST_F(TestArm64InstEmulation, TestMidFunctionEpilogueAndBackwardsJump) {
EXPECT_TRUE(row->GetCFAValue().GetRegisterNumber() == gpr_sp_arm64);
Copy link
Collaborator

@DavidSpickett DavidSpickett Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to update the comment above:

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

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 inherit 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
Expand All @@ -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);
}
}
Loading