[lldb] Implement DW_CFA_val_offset and DW_CFA_val_offset_sf #150732
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The test for this is artificial as I'm not aware of any upstream targets that use DW_CFA_val_offset RegisterContextUnwind::ReadFrameAddress now reports how it's attempting to obtain the CFA unless all success/failure cases emit logs that clearly identify the method it was attempting. Previously several of the existing failure paths emit no message or a message that's indistinguishable from those on other paths.
Member
|
@llvm/pr-subscribers-lldb Author: Daniel Sanders (dsandersllvm) ChangesThe test for this is artificial as I'm not aware of any upstream targets that use DW_CFA_val_offset RegisterContextUnwind::ReadFrameAddress now reports how it's attempting to obtain the CFA unless all success/failure cases emit logs that clearly identify the method it was attempting. Previously several of the existing failure paths emit no message or a message that's indistinguishable from those on other paths. Full diff: https://github.com/llvm/llvm-project/pull/150732.diff 6 Files Affected:
diff --git a/lldb/include/lldb/Symbol/UnwindPlan.h b/lldb/include/lldb/Symbol/UnwindPlan.h
index fe8081f83c590..9587f1312aa2e 100644
--- a/lldb/include/lldb/Symbol/UnwindPlan.h
+++ b/lldb/include/lldb/Symbol/UnwindPlan.h
@@ -67,6 +67,7 @@ class UnwindPlan {
atAFAPlusOffset, // reg = deref(AFA + offset)
isAFAPlusOffset, // reg = AFA + offset
inOtherRegister, // reg = other reg
+ isOtherRegisterPlusOffset, // reg = other reg + offset
atDWARFExpression, // reg = deref(eval(dwarf_expr))
isDWARFExpression, // reg = eval(dwarf_expr)
isConstant // reg = constant
@@ -102,6 +103,10 @@ class UnwindPlan {
bool IsInOtherRegister() const { return m_type == inOtherRegister; }
+ bool IsOtherRegisterPlusOffset() const {
+ return m_type == isOtherRegisterPlusOffset;
+ }
+
bool IsAtDWARFExpression() const { return m_type == atDWARFExpression; }
bool IsDWARFExpression() const { return m_type == isDWARFExpression; }
@@ -140,9 +145,17 @@ class UnwindPlan {
m_location.reg_num = reg_num;
}
+ void SetIsRegisterPlusOffset(uint32_t reg_num, int32_t offset = 0) {
+ m_type = isOtherRegisterPlusOffset;
+ m_location.reg_plus_offset.reg_num = reg_num;
+ m_location.reg_plus_offset.offset = offset;
+ }
+
uint32_t GetRegisterNumber() const {
if (m_type == inOtherRegister)
return m_location.reg_num;
+ if (m_type == isOtherRegisterPlusOffset)
+ return m_location.reg_plus_offset.reg_num;
return LLDB_INVALID_REGNUM;
}
@@ -156,6 +169,8 @@ class UnwindPlan {
case atAFAPlusOffset:
case isAFAPlusOffset:
return m_location.offset;
+ case inOtherRegister:
+ return m_location.reg_plus_offset.offset;
default:
return 0;
}
@@ -204,6 +219,11 @@ class UnwindPlan {
} expr;
// For m_type == isConstant
uint64_t constant_value;
+ // For m_type == inOtherRegisterPlusOffset
+ struct {
+ uint32_t reg_num;
+ int32_t offset;
+ } reg_plus_offset;
} m_location;
};
diff --git a/lldb/include/lldb/Target/UnwindLLDB.h b/lldb/include/lldb/Target/UnwindLLDB.h
index f2f65e67a7640..88180b37fd93a 100644
--- a/lldb/include/lldb/Target/UnwindLLDB.h
+++ b/lldb/include/lldb/Target/UnwindLLDB.h
@@ -49,6 +49,9 @@ class UnwindLLDB : public lldb_private::Unwind {
// target mem (target_memory_location)
eRegisterInRegister, // register is available in a (possible other)
// register (register_number)
+ eRegisterIsRegisterPlusOffset, // register is available in a (possible
+ // other) register (register_number) with
+ // an offset applied
eRegisterSavedAtHostMemoryLocation, // register is saved at a word in
// lldb's address space
eRegisterValueInferred, // register val was computed (and is in
@@ -64,6 +67,11 @@ class UnwindLLDB : public lldb_private::Unwind {
void *host_memory_location;
uint64_t inferred_value; // eRegisterValueInferred - e.g. stack pointer ==
// cfa + offset
+ struct {
+ uint32_t
+ register_number; // in eRegisterKindLLDB register numbering system
+ uint64_t offset;
+ } reg_plus_offset;
} location;
};
diff --git a/lldb/source/Symbol/DWARFCallFrameInfo.cpp b/lldb/source/Symbol/DWARFCallFrameInfo.cpp
index a2d748adad64a..2f8f9e9182fb2 100644
--- a/lldb/source/Symbol/DWARFCallFrameInfo.cpp
+++ b/lldb/source/Symbol/DWARFCallFrameInfo.cpp
@@ -766,8 +766,32 @@ DWARFCallFrameInfo::ParseFDE(dw_offset_t dwarf_offset,
break;
}
- case DW_CFA_val_offset: // 0x14
- case DW_CFA_val_offset_sf: // 0x15
+ case DW_CFA_val_offset: { // 0x14
+ // takes two unsigned LEB128 operands representing a register number
+ // and a factored offset. The required action is to change the rule
+ // for the register indicated by the register number to be a
+ // val_offset(N) rule where the value of N is factored_offset*
+ // data_alignment_factor
+ uint32_t reg_num = (uint32_t)m_cfi_data.GetULEB128(&offset);
+ int32_t op_offset =
+ (int32_t)m_cfi_data.GetULEB128(&offset) * data_align;
+ reg_location.SetIsCFAPlusOffset(op_offset);
+ row.SetRegisterInfo(reg_num, reg_location);
+ break;
+ }
+ case DW_CFA_val_offset_sf: { // 0x15
+ // takes two operands: an unsigned LEB128 value representing a
+ // register number and a signed LEB128 factored offset. This
+ // instruction is identical to DW_CFA_val_offset except that the
+ // second operand is signed and factored. The resulting offset is
+ // factored_offset* data_alignment_factor.
+ uint32_t reg_num = (uint32_t)m_cfi_data.GetULEB128(&offset);
+ int32_t op_offset =
+ (int32_t)m_cfi_data.GetSLEB128(&offset) * data_align;
+ reg_location.SetIsCFAPlusOffset(op_offset);
+ row.SetRegisterInfo(reg_num, reg_location);
+ break;
+ }
default:
break;
}
diff --git a/lldb/source/Symbol/UnwindPlan.cpp b/lldb/source/Symbol/UnwindPlan.cpp
index 9245e52732061..93587568a7e64 100644
--- a/lldb/source/Symbol/UnwindPlan.cpp
+++ b/lldb/source/Symbol/UnwindPlan.cpp
@@ -42,6 +42,12 @@ bool UnwindPlan::Row::AbstractRegisterLocation::operator==(
case inOtherRegister:
return m_location.reg_num == rhs.m_location.reg_num;
+ case isOtherRegisterPlusOffset:
+ return m_location.reg_plus_offset.reg_num ==
+ rhs.m_location.reg_plus_offset.reg_num &&
+ m_location.reg_plus_offset.offset ==
+ rhs.m_location.reg_plus_offset.offset;
+
case atDWARFExpression:
case isDWARFExpression:
if (m_location.expr.length == rhs.m_location.expr.length)
@@ -145,6 +151,17 @@ void UnwindPlan::Row::AbstractRegisterLocation::Dump(
s.Printf("=reg(%u)", m_location.reg_num);
} break;
+ case isOtherRegisterPlusOffset: {
+ const RegisterInfo *other_reg_info = nullptr;
+ if (unwind_plan)
+ other_reg_info = unwind_plan->GetRegisterInfo(thread, m_location.reg_num);
+ if (other_reg_info)
+ s.Printf("=%s", other_reg_info->name);
+ else
+ s.Printf("=reg(%u)+%u", m_location.reg_plus_offset.reg_num,
+ m_location.reg_plus_offset.offset);
+ } break;
+
case atDWARFExpression:
case isDWARFExpression: {
s.PutChar('=');
diff --git a/lldb/source/Target/RegisterContextUnwind.cpp b/lldb/source/Target/RegisterContextUnwind.cpp
index 880300d0637fb..80345e6d3588c 100644
--- a/lldb/source/Target/RegisterContextUnwind.cpp
+++ b/lldb/source/Target/RegisterContextUnwind.cpp
@@ -1118,6 +1118,27 @@ bool RegisterContextUnwind::ReadRegisterValueFromRegisterLocation(
success = GetNextFrame()->ReadRegister(other_reg_info, value);
}
} break;
+ case UnwindLLDB::ConcreteRegisterLocation::eRegisterIsRegisterPlusOffset: {
+ auto regnum = regloc.location.reg_plus_offset.register_number;
+ const RegisterInfo *other_reg_info =
+ GetRegisterInfoAtIndex(regloc.location.reg_plus_offset.register_number);
+
+ if (!other_reg_info)
+ return false;
+
+ if (IsFrameZero()) {
+ success =
+ m_thread.GetRegisterContext()->ReadRegister(other_reg_info, value);
+ } else {
+ success = GetNextFrame()->ReadRegister(other_reg_info, value);
+ }
+ if (success) {
+ UnwindLogMsg("read (%d)'s location", regnum);
+ value = value.GetAsUInt64(~0ull, &success) +
+ regloc.location.reg_plus_offset.offset;
+ UnwindLogMsg("success %s", success ? "yes" : "no");
+ }
+ } break;
case UnwindLLDB::ConcreteRegisterLocation::eRegisterValueInferred:
success =
value.SetUInt(regloc.location.inferred_value, reg_info->byte_size);
@@ -1164,6 +1185,7 @@ bool RegisterContextUnwind::WriteRegisterValueToRegisterLocation(
success = GetNextFrame()->WriteRegister(other_reg_info, value);
}
} break;
+ case UnwindLLDB::ConcreteRegisterLocation::eRegisterIsRegisterPlusOffset:
case UnwindLLDB::ConcreteRegisterLocation::eRegisterValueInferred:
case UnwindLLDB::ConcreteRegisterLocation::eRegisterNotSaved:
break;
@@ -1633,6 +1655,30 @@ RegisterContextUnwind::SavedLocationForRegister(
return UnwindLLDB::RegisterSearchResult::eRegisterFound;
}
+ if (abs_regloc->IsOtherRegisterPlusOffset()) {
+ uint32_t unwindplan_regnum = abs_regloc->GetRegisterNumber();
+ int unwindplan_offset = abs_regloc->GetOffset();
+ RegisterNumber row_regnum(m_thread, abs_regkind, unwindplan_regnum);
+ if (row_regnum.GetAsKind(eRegisterKindLLDB) == LLDB_INVALID_REGNUM) {
+ UnwindLogMsg("could not supply caller's %s (%d) location - was saved in "
+ "another reg+offset but couldn't convert that regnum",
+ regnum.GetName(), regnum.GetAsKind(eRegisterKindLLDB));
+ return UnwindLLDB::RegisterSearchResult::eRegisterNotFound;
+ }
+ regloc.type =
+ UnwindLLDB::ConcreteRegisterLocation::eRegisterIsRegisterPlusOffset;
+ regloc.location.reg_plus_offset.register_number =
+ row_regnum.GetAsKind(eRegisterKindLLDB);
+ regloc.location.reg_plus_offset.offset = unwindplan_offset;
+ m_registers[regnum.GetAsKind(eRegisterKindLLDB)] = regloc;
+ UnwindLogMsg("supplying caller's register %s (%d), "
+ "from register %s (%d) plus offset %u",
+ regnum.GetName(), regnum.GetAsKind(eRegisterKindLLDB),
+ row_regnum.GetName(), row_regnum.GetAsKind(eRegisterKindLLDB),
+ unwindplan_offset);
+ return UnwindLLDB::RegisterSearchResult::eRegisterFound;
+ }
+
if (abs_regloc->IsDWARFExpression() || abs_regloc->IsAtDWARFExpression()) {
DataExtractor dwarfdata(abs_regloc->GetDWARFExpressionBytes(),
abs_regloc->GetDWARFExpressionLength(),
@@ -1959,6 +2005,7 @@ bool RegisterContextUnwind::ReadFrameAddress(
switch (fa.GetValueType()) {
case UnwindPlan::Row::FAValue::isRegisterDereferenced: {
+ UnwindLogMsg("CFA value via dereferencing reg");
RegisterNumber cfa_reg(m_thread, row_register_kind,
fa.GetRegisterNumber());
if (ReadGPRValue(cfa_reg, cfa_reg_contents)) {
@@ -1991,6 +2038,7 @@ bool RegisterContextUnwind::ReadFrameAddress(
break;
}
case UnwindPlan::Row::FAValue::isRegisterPlusOffset: {
+ UnwindLogMsg("CFA value via register plus offset");
RegisterNumber cfa_reg(m_thread, row_register_kind,
fa.GetRegisterNumber());
if (ReadGPRValue(cfa_reg, cfa_reg_contents)) {
@@ -2012,10 +2060,13 @@ bool RegisterContextUnwind::ReadFrameAddress(
address, cfa_reg.GetName(), cfa_reg.GetAsKind(eRegisterKindLLDB),
cfa_reg_contents, fa.GetOffset());
return true;
- }
+ } else
+ UnwindLogMsg("unable to read CFA register %s (%d)", cfa_reg.GetName(),
+ cfa_reg.GetAsKind(eRegisterKindLLDB));
break;
}
case UnwindPlan::Row::FAValue::isDWARFExpression: {
+ UnwindLogMsg("CFA value via DWARF expression");
ExecutionContext exe_ctx(m_thread.shared_from_this());
Process *process = exe_ctx.GetProcessPtr();
DataExtractor dwarfdata(fa.GetDWARFExpressionBytes(),
@@ -2042,6 +2093,7 @@ bool RegisterContextUnwind::ReadFrameAddress(
break;
}
case UnwindPlan::Row::FAValue::isRaSearch: {
+ UnwindLogMsg("CFA value via heuristic search");
Process &process = *m_thread.GetProcess();
lldb::addr_t return_address_hint = GetReturnAddressHint(fa.GetOffset());
if (return_address_hint == LLDB_INVALID_ADDRESS)
diff --git a/lldb/unittests/Symbol/TestDWARFCallFrameInfo.cpp b/lldb/unittests/Symbol/TestDWARFCallFrameInfo.cpp
index c1dcab02227da..a75a4acd304b2 100644
--- a/lldb/unittests/Symbol/TestDWARFCallFrameInfo.cpp
+++ b/lldb/unittests/Symbol/TestDWARFCallFrameInfo.cpp
@@ -39,6 +39,7 @@ class DWARFCallFrameInfoTest : public testing::Test {
protected:
void TestBasic(DWARFCallFrameInfo::Type type, llvm::StringRef symbol);
+ void TestValOffset(DWARFCallFrameInfo::Type type, llvm::StringRef symbol);
};
namespace lldb_private {
@@ -256,3 +257,126 @@ TEST_F(DWARFCallFrameInfoTest, Basic_dwarf4) {
TEST_F(DWARFCallFrameInfoTest, Basic_eh) {
TestBasic(DWARFCallFrameInfo::EH, "eh_frame");
}
+
+static UnwindPlan::Row GetValOffsetExpectedRow0() {
+ UnwindPlan::Row row;
+ row.SetOffset(0);
+ row.GetCFAValue().SetIsRegisterPlusOffset(dwarf_rsp_x86_64, 16);
+ row.SetRegisterLocationToAtCFAPlusOffset(dwarf_rip_x86_64, -8, false);
+ row.SetRegisterLocationToIsCFAPlusOffset(dwarf_rbp_x86_64, -16, false);
+ return row;
+}
+
+void DWARFCallFrameInfoTest::TestValOffset(DWARFCallFrameInfo::Type type,
+ llvm::StringRef symbol) {
+ // This test is artificial as X86 does not use DW_CFA_val_offset but this
+ // test verifies that we can successfully interpret them if they do occur.
+ // Note the distinction between RBP and RIP in this part of the DWARF dump:
+ // 0x0: CFA=RSP+16: RBP=CFA-16, RIP=[CFA-8]
+ // Whereas RIP is stored in the memory CFA-8 points at, RBP is reconstructed
+ // from the CFA without any memory access.
+ auto ExpectedFile = TestFile::fromYaml(R"(
+--- !ELF
+FileHeader:
+ Class: ELFCLASS64
+ Data: ELFDATA2LSB
+ Type: ET_REL
+ Machine: EM_X86_64
+ SectionHeaderStringTable: .strtab
+Sections:
+ - Name: .text
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ AddressAlign: 0x4
+ Content: 0F1F00
+ - Name: .debug_frame
+ Type: SHT_PROGBITS
+ AddressAlign: 0x8
+#00000000 00000014 ffffffff CIE
+# Format: DWARF32
+# Version: 4
+# Augmentation: ""
+# Address size: 8
+# Segment desc size: 0
+# Code alignment factor: 1
+# Data alignment factor: -8
+# Return address column: 16
+#
+# DW_CFA_def_cfa: RSP +8
+# DW_CFA_offset: RIP -8
+# DW_CFA_nop:
+# DW_CFA_nop:
+# DW_CFA_nop:
+# DW_CFA_nop:
+#
+# CFA=RSP+8: RIP=[CFA-8]
+#
+#00000018 0000001c 00000000 FDE cie=00000000 pc=00000000...00000003
+# Format: DWARF32
+# DW_CFA_def_cfa_offset: +16
+# DW_CFA_val_offset: RBP -16
+# DW_CFA_nop:
+# DW_CFA_nop:
+# DW_CFA_nop:
+#
+# 0x0: CFA=RSP+16: RBP=CFA-16, RIP=[CFA-8]
+ Content: 14000000FFFFFFFF040008000178100C07089001000000001C00000000000000000000000000000003000000000000000E10140602000000
+ - Name: .rela.debug_frame
+ Type: SHT_RELA
+ Flags: [ SHF_INFO_LINK ]
+ Link: .symtab
+ AddressAlign: 0x8
+ Info: .debug_frame
+ Relocations:
+ - Offset: 0x1C
+ Symbol: .debug_frame
+ Type: R_X86_64_32
+ - Offset: 0x20
+ Symbol: .text
+ Type: R_X86_64_64
+ - Type: SectionHeaderTable
+ Sections:
+ - Name: .strtab
+ - Name: .text
+ - Name: .debug_frame
+ - Name: .rela.debug_frame
+ - Name: .symtab
+Symbols:
+ - Name: .text
+ Type: STT_SECTION
+ Section: .text
+ - Name: debug_frame3
+ Section: .text
+ - Name: .debug_frame
+ Type: STT_SECTION
+ Section: .debug_frame
+...
+)");
+ ASSERT_THAT_EXPECTED(ExpectedFile, llvm::Succeeded());
+
+ auto module_sp = std::make_shared<Module>(ExpectedFile->moduleSpec());
+ SectionList *list = module_sp->GetSectionList();
+ ASSERT_NE(nullptr, list);
+
+ auto section_sp = list->FindSectionByType(type == DWARFCallFrameInfo::EH
+ ? eSectionTypeEHFrame
+ : eSectionTypeDWARFDebugFrame,
+ false);
+ ASSERT_NE(nullptr, section_sp);
+
+ DWARFCallFrameInfo cfi(*module_sp->GetObjectFile(), section_sp, type);
+
+ const Symbol *sym = module_sp->FindFirstSymbolWithNameAndType(
+ ConstString(symbol), eSymbolTypeAny);
+ ASSERT_NE(nullptr, sym);
+
+ std::unique_ptr<UnwindPlan> plan_up = cfi.GetUnwindPlan(sym->GetAddress());
+ ASSERT_TRUE(plan_up);
+ ASSERT_EQ(1, plan_up->GetRowCount());
+ EXPECT_THAT(plan_up->GetRowAtIndex(0), testing::Pointee(GetValOffsetExpectedRow0()));
+}
+
+TEST_F(DWARFCallFrameInfoTest, ValOffset_dwarf3) {
+ TestValOffset(DWARFCallFrameInfo::DWARF, "debug_frame3");
+}
+
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Collaborator
|
Would it make sense to also remove the |
Collaborator
Author
|
While looking into that, I found a couple mistakes that made me question how it managed to work and I found that that bit is not actually used. ConcreteRegisterLocation uses eRegisterIsRegisterPlusOffset for all reg+offset's including reg=CFA, but AbstractRegisterLocation has a specific isCFAPlusOffset for the reg=CFA case so I didn't need to add isOtherRegisterPlusOffset I just pushed an update that removes that unnecessary code and fixes the clang-format issue raised by the bot |
Collaborator
|
This looks better, though it makes me even more convinced that this could handled by extending the existing case rather than introducing a new one. The code you added to |
Collaborator
Author
|
I don't think combining them in ConcreteRegisterLocation completely makes sense like it did for AbstractRegisterLocation because there's a difference in semantics. eRegisterInRegister refers to a storage location whereas eRegisterIsRegisterPlusOffset is a computed value without any storage. It's true they're very similar in ReadRegisterValueFromRegisterLocation() and we could potentially combine those two implementations in that function, but there's nowhere to write eRegisterIsRegisterPlusOffset to in WriteRegisterValueToRegisterLocation(). I could conditionally drop the write if offset != 0 but I think it's potentially confusing to have the semantics depend on the values rather than the enumerator |
Collaborator
|
Okay I see what you mean now. I agree with your assessment. The code looks code. The test you added only checks that the eh_frame section was parsed correctly. Could you also add a test to verify that we compute the right register value? I'm imagining something like test/Shell/Unwind/eh-frame-dwarf-unwind.test, maybe using |
Collaborator
Author
|
I've added a test case to confirm it is read correctly computing the value. As I pushed it, it occurred to me that I should make the offset non-zero so I'll fix that quick. I don't think the non-standard return pointer bit from eh-frame-dwarf-unwind.test should be necessary but if I remove it, lldb starts trying to read the code instead of using the CFI and it stops unwinding correctly |
labath
approved these changes
Aug 4, 2025
Collaborator
labath
left a comment
labath
left a comment
There was a problem hiding this comment.
I don't think the non-standard return pointer bit from eh-frame-dwarf-unwind.test should be necessary but if I remove it, lldb starts trying to read the code instead of using the CFI and it stops unwinding correctly
Yeah, that's because of lldb prefers the assembly unwind plan for frame zero unless it thinks the function is using a non-standard calling convention. (see FuncUnwinders::GetUnwindPlanAtNonCallSite). Using a non-standard rule for the PC disables that. It doesn't need to be that complicated, but I think it's okay -- just add a comment that this is there to force the usage of eh_frame for frame zero.
| stepi | ||
| stepi | ||
| stepi | ||
| print/x $r12 |
Collaborator
There was a problem hiding this comment.
If possible, replace these with register read r12 to avoid involving the expression evaluator.
Collaborator
Author
|
That makes sense, I've added the comment and switched to register read to avoid the expression evaluator |
Collaborator
Author
|
/cherry-pick c455c4e |
Member
|
/pull-request #166611 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The test for this is artificial as I'm not aware of any upstream targets that use DW_CFA_val_offset
RegisterContextUnwind::ReadFrameAddress now reports how it's attempting to obtain the CFA unless all success/failure cases emit logs that clearly identify the method it was attempting. Previously several of the existing failure paths emit no message or a message that's indistinguishable from those on other paths.