Skip to content

[lldb] Slide eh_frame unwind plan if it doesn't begin at function boundary #135333

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

Merged
merged 1 commit into from
Apr 22, 2025
Merged

[lldb] Slide eh_frame unwind plan if it doesn't begin at function boundary #135333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion lldb/source/Plugins/UnwindAssembly/x86/UnwindAssembly-x86.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ bool UnwindAssembly_x86::AugmentUnwindPlanFromCallSite(

int wordsize = 8;
ProcessSP process_sp(thread.GetProcess());
if (process_sp.get() == nullptr)
if (!process_sp || !first_row || !last_row)
return false;

wordsize = process_sp->GetTarget().GetArchitecture().GetAddressByteSize();
Expand Down
6 changes: 5 additions & 1 deletion lldb/source/Symbol/DWARFCallFrameInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,12 @@ bool DWARFCallFrameInfo::GetUnwindPlan(const AddressRange &range,
unwind_plan.SetUnwindPlanForSignalTrap(fde->for_signal_trap ? eLazyBoolYes
: eLazyBoolNo);
unwind_plan.SetReturnAddressRegister(fde->return_addr_reg_num);
for (UnwindPlan::Row &row : fde->rows)
int64_t slide =
fde->range.GetBaseAddress().GetFileAddress() - addr.GetFileAddress();
for (UnwindPlan::Row &row : fde->rows) {
row.SlideOffset(slide);
unwind_plan.AppendRow(std::move(row));
}

return true;
}
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Symbol/UnwindPlan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ bool UnwindPlan::PlanValidAtAddress(Address addr) const {
// If the 0th Row of unwind instructions is missing, or if it doesn't provide
// a register to use to find the Canonical Frame Address, this is not a valid
// UnwindPlan.
const Row *row0 = GetRowForFunctionOffset(0);
const Row *row0 = GetRowAtIndex(0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This actually changes code back to the state before I started messing with it (#127661). In that patch, I changed it to use GetRowForFunctionOffset because I thought it better captures the original intent of the code ("check the value of CFA at function entry").

I still sort of think that, but now I also think that check is too strict (just because we don't have a CFA at offset zero, it doesn't mean we won't have it at some other offset). So this goes back to checking the first row (whatever it's offset), but I'm also open to other options (checking that any row contains the CFA, or maybe removing the check entirely -- just checking that a row exists)

if (!row0 ||
row0->GetCFAValue().GetValueType() == Row::FAValue::unspecified) {
Log *log = GetLog(LLDBLog::Unwind);
Expand Down
12 changes: 10 additions & 2 deletions lldb/test/Shell/Unwind/Inputs/eh-frame-small-fde.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,20 @@ bar:

.type foo, @function
foo:
nop # Make the FDE entry start one byte later than the actual function.
# Make the FDE entry start 16 bytes later than the actual function. The
# size is chosen such that it is larger than the size of the FDE entry.
# This allows us to test that we're using the correct offset for
# unwinding (we'll stop 21 bytes into the function, but only 5 bytes
# into the FDE).
.nops 16
.cfi_startproc
.cfi_register %rip, %r13
call bar
addl $1, %eax
jmp *%r13 # Return
movq %r13, %r14
.cfi_register %rip, %r14
movq $0, %r13
jmp *%r14 # Return
.cfi_endproc
.size foo, .-foo

Expand Down
7 changes: 4 additions & 3 deletions lldb/test/Shell/Unwind/eh-frame-small-fde.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@ process launch

thread backtrace
# CHECK: frame #0: {{.*}}`bar
# CHECK: frame #1: {{.*}}`foo + 6
# CHECK: frame #1: {{.*}}`foo + 21
# CHECK: frame #2: {{.*}}`main + 20

target modules show-unwind -n foo
# CHECK: eh_frame UnwindPlan:
# CHECK: row[0]: 0: CFA=rsp +8 => rip=r13
# CHECK: eh_frame UnwindPlan:
# CHECK: row[0]: 16: CFA=rsp +8 => rip=r13
# CHECK-NEXT: row[1]: 27: CFA=rsp +8 => rip=r14
7 changes: 6 additions & 1 deletion lldb/unittests/Symbol/UnwindPlanTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,20 @@ TEST(UnwindPlan, PlanValidAtAddress) {
UnwindPlan::Row row2 = make_simple_row(10, 47);

UnwindPlan plan(eRegisterKindGeneric);
// When valid address range is not set, plans are valid as long as they have a
// row that sets the CFA.
EXPECT_FALSE(plan.PlanValidAtAddress(Address(0)));
EXPECT_FALSE(plan.PlanValidAtAddress(Address(10)));

plan.InsertRow(row2);
EXPECT_FALSE(plan.PlanValidAtAddress(Address(0)));
EXPECT_TRUE(plan.PlanValidAtAddress(Address(0)));
EXPECT_TRUE(plan.PlanValidAtAddress(Address(10)));

plan.InsertRow(row1);
EXPECT_TRUE(plan.PlanValidAtAddress(Address(0)));
EXPECT_TRUE(plan.PlanValidAtAddress(Address(10)));

// With an address range, they're only valid within that range.
plan.SetPlanValidAddressRanges({AddressRange(0, 5), AddressRange(15, 5)});
EXPECT_TRUE(plan.PlanValidAtAddress(Address(0)));
EXPECT_FALSE(plan.PlanValidAtAddress(Address(5)));
Expand Down