[LLDB] Fix 64 bit support for CIE and FDE handling in DWARFCallFrameInfo #158350
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
- Introduced a new helper function `IsCIEMarker` to determine if a given `cie_id` indicates a CIE (Common Information Entry) or FDE (Frame Description Entry). - New helper function can now correctly identify both 32-bit and 64-bit CIE based on the DWARF specifications. - Updated the `ParseCIE` and `GetFDEIndex` methods to utilize the new helper function for improved clarity and correctness in identifying CIE and FDE entries. - Replaced direct comparisons with `UINT32_MAX` and `UINT64_MAX` with `std::numeric_limits` for better readability.
|
@llvm/pr-subscribers-lldb Author: Andrew Gontarek (agontarek) Changes
I do not know the proper way to test this change and am open to recommendations on how to add a test. Full diff: https://github.com/llvm/llvm-project/pull/158350.diff 1 Files Affected:
diff --git a/lldb/source/Symbol/DWARFCallFrameInfo.cpp b/lldb/source/Symbol/DWARFCallFrameInfo.cpp
index 2f8f9e9182fb2..6ba5c6660140b 100644
--- a/lldb/source/Symbol/DWARFCallFrameInfo.cpp
+++ b/lldb/source/Symbol/DWARFCallFrameInfo.cpp
@@ -20,7 +20,9 @@
#include "lldb/Utility/LLDBLog.h"
#include "lldb/Utility/Log.h"
#include "lldb/Utility/Timer.h"
+#include <cstdint>
#include <cstring>
+#include <limits>
#include <list>
#include <optional>
@@ -147,6 +149,27 @@ GetGNUEHPointer(const DataExtractor &DE, lldb::offset_t *offset_ptr,
return baseAddress + addressValue;
}
+// Check if the given cie_id value indicates a CIE (Common Information Entry)
+// as opposed to an FDE (Frame Description Entry).
+//
+// For eh_frame sections: CIE is marked with cie_id == 0
+// For debug_frame sections:
+// - DWARF32: CIE is marked with cie_id ==
+// std::numeric_limits<uint32_t>::max()
+// - DWARF64: CIE is marked with cie_id ==
+// std::numeric_limits<uint64_t>::max()
+static bool IsCIEMarker(uint64_t cie_id, bool is_64bit,
+ DWARFCallFrameInfo::Type type) {
+ if (type == DWARFCallFrameInfo::EH)
+ return cie_id == 0;
+
+ // DWARF type
+ if (is_64bit)
+ return cie_id == std::numeric_limits<uint64_t>::max();
+
+ return cie_id == std::numeric_limits<uint32_t>::max();
+}
+
DWARFCallFrameInfo::DWARFCallFrameInfo(ObjectFile &objfile,
SectionSP §ion_sp, Type type)
: m_objfile(objfile), m_section_sp(section_sp), m_type(type) {}
@@ -283,7 +306,7 @@ DWARFCallFrameInfo::ParseCIE(const dw_offset_t cie_offset) {
GetCFIData();
uint32_t length = m_cfi_data.GetU32(&offset);
dw_offset_t cie_id, end_offset;
- bool is_64bit = (length == UINT32_MAX);
+ bool is_64bit = (length == std::numeric_limits<uint32_t>::max());
if (is_64bit) {
length = m_cfi_data.GetU64(&offset);
cie_id = m_cfi_data.GetU64(&offset);
@@ -292,8 +315,9 @@ DWARFCallFrameInfo::ParseCIE(const dw_offset_t cie_offset) {
cie_id = m_cfi_data.GetU32(&offset);
end_offset = cie_offset + length + 4;
}
- if (length > 0 && ((m_type == DWARF && cie_id == UINT32_MAX) ||
- (m_type == EH && cie_id == 0ul))) {
+
+ // Check if this is a CIE or FDE based on the CIE ID marker
+ if (length > 0 && IsCIEMarker(cie_id, is_64bit, m_type)) {
size_t i;
// cie.offset = cie_offset;
// cie.length = length;
@@ -470,7 +494,7 @@ void DWARFCallFrameInfo::GetFDEIndex() {
const dw_offset_t current_entry = offset;
dw_offset_t cie_id, next_entry, cie_offset;
uint32_t len = m_cfi_data.GetU32(&offset);
- bool is_64bit = (len == UINT32_MAX);
+ bool is_64bit = (len == std::numeric_limits<uint32_t>::max());
if (is_64bit) {
len = m_cfi_data.GetU64(&offset);
cie_id = m_cfi_data.GetU64(&offset);
@@ -493,11 +517,8 @@ void DWARFCallFrameInfo::GetFDEIndex() {
return;
}
- // An FDE entry contains CIE_pointer in debug_frame in same place as cie_id
- // in eh_frame. CIE_pointer is an offset into the .debug_frame section. So,
- // variable cie_offset should be equal to cie_id for debug_frame.
- // FDE entries with cie_id == 0 shouldn't be ignored for it.
- if ((cie_id == 0 && m_type == EH) || cie_id == UINT32_MAX || len == 0) {
+ // Check if this is a CIE or FDE based on the CIE ID marker
+ if (IsCIEMarker(cie_id, is_64bit, m_type) || len == 0) {
auto cie_sp = ParseCIE(current_entry);
if (!cie_sp) {
// Cannot parse, the reason is already logged
@@ -568,7 +589,7 @@ DWARFCallFrameInfo::ParseFDE(dw_offset_t dwarf_offset,
uint32_t length = m_cfi_data.GetU32(&offset);
dw_offset_t cie_offset;
- bool is_64bit = (length == UINT32_MAX);
+ bool is_64bit = (length == std::numeric_limits<uint32_t>::max());
if (is_64bit) {
length = m_cfi_data.GetU64(&offset);
cie_offset = m_cfi_data.GetU64(&offset);
@@ -577,7 +598,8 @@ DWARFCallFrameInfo::ParseFDE(dw_offset_t dwarf_offset,
}
// FDE entries with zeroth cie_offset may occur for debug_frame.
- assert(!(m_type == EH && 0 == cie_offset) && cie_offset != UINT32_MAX);
+ assert(!(m_type == EH && 0 == cie_offset) &&
+ cie_offset != std::numeric_limits<uint32_t>::max());
// Translate the CIE_id from the eh_frame format, which is relative to the
// FDE offset, into a __eh_frame section offset
|
walter-erquinigo
requested review from
clayborg,
dmpots and
walter-erquinigo
agontarek
commented
Sep 12, 2025
clayborg
reviewed
Sep 15, 2025
clayborg
reviewed
Sep 15, 2025
We should be able to add a test to DWARFCallFrameInfoTest using an ELF YAML file that has a 32-bit/64-bit CIE. Here is an example where I added a minimal CIE entry for testing. |
dmpots
requested changes
Sep 16, 2025
This commit refactors the CIE marker checks in DWARFCallFrameInfo.cpp to utilize LLVM's predefined constants for DWARF CIE IDs.
This commit introduces new unit tests to verify the handling of invalid Frame Description Entries (FDEs) with invalid CIE ID values for both DWARF32 and DWARF64 formats. Additionally, it includes tests for valid CIE markers in both eh_frame and debug_frame formats, ensuring that the unwind plans are correctly generated or returned as null when encountering invalid entries.
dmpots
approved these changes
Sep 23, 2025
There was a problem hiding this comment.
LGTM. Thanks for the changes and adding the test!
I do not know the proper way to test this change and am open to recommendations on how to add a test.
We can update the commit message now that we have tests!
Question for my understanding. What would be the behavior of the tests without your fix? Would it crash in that case?
Fails on 64 bit CIE marker as expected: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
IsCIEMarkerto determine if a givencie_idindicates a CIE (Common Information Entry) or FDE (Frame Description Entry).ParseCIEandGetFDEIndexmethods to utilize the new helper function for improved clarity and correctness in identifying CIE and FDE entries.UINT32_MAXandUINT64_MAXwithstd::numeric_limitsfor better readability.