[llvm] Modify llvm-gsymutil lookups to handle overlapping ranges correctly. (PR #72350)
Greg Clayton via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 14 22:08:36 PST 2023
https://github.com/clayborg created https://github.com/llvm/llvm-project/pull/72350
llvm-gsymutil allows address ranges to overlap. There was a bug where if we had debug info for a function with a range like [0x100-0x200) and a symbol at the same start address yet with a larger range like [0x100-0x300), we would randomly get either only information from the first or second entry. This could cause lookups to fail due to the way the binary search worked.
This patch makes sure that when lookups happen we find the first address table entry that can match an address, and also ensures that we always select the first FunctionInfo that could match. FunctionInfo entries are sorted such that the most debug info rich entries come first. And if we have two ranges that have the same start address, the smaller range comes first and the larger one comes next. This patch also adds the ability to iterate over all function infos with the same start address to always find a range that contains the address.
Added a unit test to test this functionality that failed prior to this fix and now succeeds.
>From e13847e863c05ff0d805b0f7da3397637c7e1a86 Mon Sep 17 00:00:00 2001
From: Greg Clayton <clayborg at gmail.com>
Date: Tue, 14 Nov 2023 22:02:47 -0800
Subject: [PATCH] Modify llvm-gsymutil lookups to handle overlapping ranges
correctly.
llvm-gsymutil allows address ranges to overlap. There was a bug where if we had debug info for a function with a range like [0x100-0x200) and a symbol at the same start address yet with a larger range like [0x100-0x300), we would randomly get either only information from the first or second entry. This could cause lookups to fail due to the way the binary search worked.
This patch makes sure that when lookups happen we find the first address table entry that can match an address, and also ensures that we always select the first FunctionInfo that could match. FunctionInfo entries are sorted such that the most debug info rich entries come first. And if we have two ranges that have the same start address, the smaller range comes first and the larger one comes next. This patch also adds the ability to iterate over all function infos with the same start address to always find a range that contains the address.
Added a unit test to test this functionality that failed prior to this fix and now succeeds.
---
llvm/include/llvm/DebugInfo/GSYM/GsymReader.h | 34 ++++
llvm/lib/DebugInfo/GSYM/GsymReader.cpp | 89 +++++----
llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp | 177 ++++++++++++++++++
3 files changed, 264 insertions(+), 36 deletions(-)
diff --git a/llvm/include/llvm/DebugInfo/GSYM/GsymReader.h b/llvm/include/llvm/DebugInfo/GSYM/GsymReader.h
index cd4fdfa0e9e37d9..f7258abb707309a 100644
--- a/llvm/include/llvm/DebugInfo/GSYM/GsymReader.h
+++ b/llvm/include/llvm/DebugInfo/GSYM/GsymReader.h
@@ -266,6 +266,18 @@ class GsymReader {
return std::nullopt;
if (Iter == End || AddrOffset < *Iter)
--Iter;
+
+ // GSYM files store the richest information first in the file, so always
+ // backup as much as possible as long as the address offset is the same
+ // as the previous entry.
+ while (Iter != Begin) {
+ auto Prev = Iter - 1;
+ if (*Prev == *Iter)
+ Iter = Prev;
+ else
+ break;
+ }
+
return std::distance(Begin, Iter);
}
@@ -303,6 +315,28 @@ class GsymReader {
/// \returns An optional GSYM data offset for the offset of the FunctionInfo
/// that needs to be decoded.
std::optional<uint64_t> getAddressInfoOffset(size_t Index) const;
+
+ /// Given an address, find the correct function info data and function
+ /// address.
+ ///
+ /// Binary search the address table and find the matching address info
+ /// and make sure that the function info contains the address. GSYM allows
+ /// functions to overlap, and the most debug info in contained in the first
+ /// entries due to the sorting when GSYM files are created. We can have
+ /// multiple function info that start at the same address only if their
+ /// address range doesn't match. So find the first entry that matches \a Addr
+ /// and iiterate forward until we find one that contains the address.
+ ///
+ /// \param[in] Addr A virtual address that matches the original object file
+ /// to lookup.
+ ///
+ /// \param[out] FuncStartAddr A virtual address that is the base address of
+ /// the function that is used for decoding the FunctionInfo.
+ ///
+ /// \returns An valid data extractor on success, or an error if we fail to
+ /// find the address in a function info or corrrectly decode the data
+ llvm::Expected<llvm::DataExtractor>
+ getFunctionInfoData(uint64_t Addr, uint64_t &FuncStartAddr) const;
};
} // namespace gsym
diff --git a/llvm/lib/DebugInfo/GSYM/GsymReader.cpp b/llvm/lib/DebugInfo/GSYM/GsymReader.cpp
index 1fe90ef579a3d91..bc9ba55332347e3 100644
--- a/llvm/lib/DebugInfo/GSYM/GsymReader.cpp
+++ b/llvm/lib/DebugInfo/GSYM/GsymReader.cpp
@@ -253,49 +253,66 @@ GsymReader::getAddressIndex(const uint64_t Addr) const {
}
-llvm::Expected<FunctionInfo> GsymReader::getFunctionInfo(uint64_t Addr) const {
- Expected<uint64_t> AddressIndex = getAddressIndex(Addr);
- if (!AddressIndex)
- return AddressIndex.takeError();
- // Address info offsets size should have been checked in parse().
- assert(*AddressIndex < AddrInfoOffsets.size());
- auto AddrInfoOffset = AddrInfoOffsets[*AddressIndex];
- assert(
- (Endian == llvm::endianness::big || Endian == llvm::endianness::little) &&
- "Endian must be either big or little");
- DataExtractor Data(MemBuffer->getBuffer().substr(AddrInfoOffset),
- Endian == llvm::endianness::little, 4);
- if (std::optional<uint64_t> OptAddr = getAddress(*AddressIndex)) {
- auto ExpectedFI = FunctionInfo::decode(Data, *OptAddr);
- if (ExpectedFI) {
- if (ExpectedFI->Range.contains(Addr) || ExpectedFI->Range.size() == 0)
- return ExpectedFI;
+llvm::Expected<DataExtractor>
+GsymReader::getFunctionInfoData(uint64_t Addr, uint64_t &FuncAddr) const {
+ Expected<uint64_t> ExpectedAddrIdx = getAddressIndex(Addr);
+ if (!ExpectedAddrIdx)
+ return ExpectedAddrIdx.takeError();
+ const uint64_t FirstAddrIdx = *ExpectedAddrIdx;
+ std::optional<uint64_t> OptFirstAddr = getAddress(FirstAddrIdx);
+ if (!OptFirstAddr)
+ return createStringError(std::errc::invalid_argument,
+ "failed to extract address[%" PRIu64 "]",
+ FirstAddrIdx);
+ // The AddrIdx is the first index of the function info entries that match
+ // \a Addr. We need to iterate over all function info objects that start with
+ // the same address until we find a match.
+ const auto FirstAddr = *OptFirstAddr;
+ const size_t NumAddresses = getNumAddresses();
+ assert((Endian == endianness::big || Endian == endianness::little) &&
+ "Endian must be either big or little");
+ for (uint64_t AddrIdx = FirstAddrIdx; AddrIdx < NumAddresses; ++AddrIdx) {
+ // Extract the function address and make sure it matches FirstAddr
+ std::optional<uint64_t> OptFuncAddr = getAddress(AddrIdx);
+ if (!OptFuncAddr)
return createStringError(std::errc::invalid_argument,
- "address 0x%" PRIx64 " is not in GSYM", Addr);
+ "failed to extract address[%" PRIu64 "]",
+ AddrIdx);
+ if (*OptFuncAddr != FirstAddr)
+ break; // Done with consecutive function info entries with same address.
+
+ // Address info offsets size should have been checked in parse().
+ auto AddrInfoOffset = AddrInfoOffsets[AddrIdx];
+ DataExtractor Data(MemBuffer->getBuffer().substr(AddrInfoOffset),
+ Endian == llvm::endianness::little, 4);
+ uint64_t Offset = 0;
+ // Some symbols on Darwin don't have valid sizes. If we run into a symbol
+ // with zero size, then we have found a match.
+ uint32_t FuncSize = Data.getU32(&Offset);
+ if (FuncSize == 0 ||
+ AddressRange(*OptFuncAddr, *OptFuncAddr + FuncSize).contains(Addr)) {
+ FuncAddr = *OptFuncAddr;
+ return Data;
}
}
return createStringError(std::errc::invalid_argument,
- "failed to extract address[%" PRIu64 "]",
- *AddressIndex);
+ "address 0x%" PRIx64 " is not in GSYM", Addr);
+}
+
+llvm::Expected<FunctionInfo> GsymReader::getFunctionInfo(uint64_t Addr) const {
+ uint64_t FuncAddr = 0;
+ if (auto ExpectedData = getFunctionInfoData(Addr, FuncAddr))
+ return FunctionInfo::decode(*ExpectedData, FuncAddr);
+ else
+ return ExpectedData.takeError();
}
llvm::Expected<LookupResult> GsymReader::lookup(uint64_t Addr) const {
- Expected<uint64_t> AddressIndex = getAddressIndex(Addr);
- if (!AddressIndex)
- return AddressIndex.takeError();
- // Address info offsets size should have been checked in parse().
- assert(*AddressIndex < AddrInfoOffsets.size());
- auto AddrInfoOffset = AddrInfoOffsets[*AddressIndex];
- assert(
- (Endian == llvm::endianness::big || Endian == llvm::endianness::little) &&
- "Endian must be either big or little");
- DataExtractor Data(MemBuffer->getBuffer().substr(AddrInfoOffset),
- Endian == llvm::endianness::little, 4);
- if (std::optional<uint64_t> OptAddr = getAddress(*AddressIndex))
- return FunctionInfo::lookup(Data, *this, *OptAddr, Addr);
- return createStringError(std::errc::invalid_argument,
- "failed to extract address[%" PRIu64 "]",
- *AddressIndex);
+ uint64_t FuncAddr = 0;
+ if (auto ExpectedData = getFunctionInfoData(Addr, FuncAddr))
+ return FunctionInfo::lookup(*ExpectedData, *this, FuncAddr, Addr);
+ else
+ return ExpectedData.takeError();
}
void GsymReader::dump(raw_ostream &OS) {
diff --git a/llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp b/llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp
index 53de96cc6953c2d..fec75d1ca619ca9 100644
--- a/llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp
+++ b/llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp
@@ -4681,3 +4681,180 @@ TEST(GSYMTest, TestHandlingOfInvalidFileIndexes) {
for (const auto &Error : ExpectedLogErrors)
EXPECT_TRUE(errors.find(Error) != std::string::npos);
}
+
+TEST(GSYMTest, TestLookupsOfOverlappingAndUnequalRanges) {
+ // Test that llvm-gsymutil lookup the correct funtion info when address
+ // ranges overlap. When functions overlap we always want to pick the first
+ // function info when symbolicating if there are multiple entries with the
+ // same address. Previous to this fix we would just binary search the address
+ // table and pick the first function info that matched the address. After
+ // this fix we now always select the first matching entry whose address range
+ // contains the lookup address to ensure we have the most debug info. We have
+ // seen case where the debug info would contain a small range and a symbol
+ // would have the same start address but the range was larger and sometimes,
+ // depending on how the binary search of the address table happened, we would
+ // pick these latter entries. We want the first entries because they always
+ // have the most debug info.
+ //
+ // To repro this case, we just make some simple DWARF that has two
+ // overlapping ranges and ensure that any lookups between 0x1000 and 0x104f
+ // match "foo", and any ranges between 0x1050 and 0x1fff match "bar".
+ //
+ // 0x0000000b: DW_TAG_compile_unit
+ // DW_AT_name ("/tmp/main.cpp")
+ // DW_AT_language (DW_LANG_C)
+ // DW_AT_stmt_list (0x00000000)
+ //
+ // 0x00000015: DW_TAG_subprogram
+ // DW_AT_name ("foo")
+ // DW_AT_low_pc (0x0000000000001000)
+ // DW_AT_high_pc (0x0000000000001050)
+ //
+ // 0x0000002a: DW_TAG_subprogram
+ // DW_AT_name ("bar")
+ // DW_AT_low_pc (0x0000000000001000)
+ // DW_AT_high_pc (0x0000000000001100)
+ //
+ // 0x0000003f: NULL
+
+ StringRef yamldata = R"(
+ debug_str:
+ - ''
+ - '/tmp/main.cpp'
+ - foo
+ - bar
+ debug_abbrev:
+ - ID: 0
+ Table:
+ - Code: 0x1
+ Tag: DW_TAG_compile_unit
+ Children: DW_CHILDREN_yes
+ Attributes:
+ - Attribute: DW_AT_name
+ Form: DW_FORM_strp
+ - Attribute: DW_AT_language
+ Form: DW_FORM_udata
+ - Attribute: DW_AT_stmt_list
+ Form: DW_FORM_sec_offset
+ - Code: 0x2
+ Tag: DW_TAG_subprogram
+ Children: DW_CHILDREN_no
+ Attributes:
+ - Attribute: DW_AT_name
+ Form: DW_FORM_strp
+ - Attribute: DW_AT_low_pc
+ Form: DW_FORM_addr
+ - Attribute: DW_AT_high_pc
+ Form: DW_FORM_addr
+ debug_info:
+ - Length: 0x3C
+ Version: 4
+ AbbrevTableID: 0
+ AbbrOffset: 0x0
+ AddrSize: 8
+ Entries:
+ - AbbrCode: 0x1
+ Values:
+ - Value: 0x1
+ - Value: 0x2
+ - Value: 0x0
+ - AbbrCode: 0x2
+ Values:
+ - Value: 0xF
+ - Value: 0x1000
+ - Value: 0x1050
+ - AbbrCode: 0x2
+ Values:
+ - Value: 0x13
+ - Value: 0x1000
+ - Value: 0x1100
+ - AbbrCode: 0x0
+ debug_line:
+ - Length: 71
+ Version: 2
+ PrologueLength: 36
+ MinInstLength: 1
+ DefaultIsStmt: 1
+ LineBase: 251
+ LineRange: 14
+ OpcodeBase: 13
+ StandardOpcodeLengths: [ 0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 ]
+ IncludeDirs:
+ - '/tmp'
+ Files:
+ - Name: main.cpp
+ DirIdx: 1
+ ModTime: 0
+ Length: 0
+ Opcodes:
+ - Opcode: DW_LNS_extended_op
+ ExtLen: 9
+ SubOpcode: DW_LNE_set_address
+ Data: 4096
+ - Opcode: DW_LNS_advance_line
+ SData: 9
+ Data: 0
+ - Opcode: DW_LNS_copy
+ Data: 0
+ - Opcode: DW_LNS_advance_pc
+ Data: 16
+ - Opcode: DW_LNS_advance_line
+ SData: 1
+ Data: 0
+ - Opcode: DW_LNS_copy
+ Data: 0
+ - Opcode: DW_LNS_advance_line
+ SData: 1
+ Data: 0
+ - Opcode: DW_LNS_copy
+ Data: 0
+ - Opcode: DW_LNS_advance_pc
+ Data: 64
+ - Opcode: DW_LNS_advance_line
+ SData: 1
+ Data: 0
+ - Opcode: DW_LNS_extended_op
+ ExtLen: 1
+ SubOpcode: DW_LNE_end_sequence
+ Data: 0
+ )";
+ auto ErrOrSections = DWARFYAML::emitDebugSections(yamldata);
+ ASSERT_THAT_EXPECTED(ErrOrSections, Succeeded());
+ std::unique_ptr<DWARFContext> DwarfContext =
+ DWARFContext::create(*ErrOrSections, 8);
+ ASSERT_TRUE(DwarfContext.get() != nullptr);
+ std::string errors;
+ raw_string_ostream OS(errors);
+ GsymCreator GC;
+ DwarfTransformer DT(*DwarfContext, GC);
+ const uint32_t ThreadCount = 1;
+ ASSERT_THAT_ERROR(DT.convert(ThreadCount, &OS), Succeeded());
+ ASSERT_THAT_ERROR(GC.finalize(OS), Succeeded());
+ OS.flush();
+ SmallString<512> Str;
+ raw_svector_ostream OutStrm(Str);
+ const auto ByteOrder = llvm::endianness::native;
+ FileWriter FW(OutStrm, ByteOrder);
+ ASSERT_THAT_ERROR(GC.encode(FW), Succeeded());
+ Expected<GsymReader> GR = GsymReader::copyBuffer(OutStrm.str());
+ ASSERT_THAT_EXPECTED(GR, Succeeded());
+ // There should be two functions in our GSYM.
+ EXPECT_EQ(GR->getNumAddresses(), 2u);
+ // Verify "foo" is correctly looked up for each of its addresses.
+ for (uint64_t Addr = 0x1000; Addr < 0x1050; ++Addr) {
+ auto ExpFI = GR->getFunctionInfo(Addr);
+ ASSERT_THAT_EXPECTED(ExpFI, Succeeded());
+ ASSERT_EQ(ExpFI->Range, AddressRange(0x1000, 0x1050));
+ StringRef FuncName = GR->getString(ExpFI->Name);
+ EXPECT_EQ(FuncName, "foo");
+ }
+
+ // Verify "bar" is correctly looked up for each of its addresses.
+ for (uint64_t Addr = 0x1050; Addr < 0x1100; ++Addr) {
+ auto ExpFI = GR->getFunctionInfo(Addr);
+ ASSERT_THAT_EXPECTED(ExpFI, Succeeded());
+ ASSERT_EQ(ExpFI->Range, AddressRange(0x1000, 0x1100));
+ StringRef FuncName = GR->getString(ExpFI->Name);
+ EXPECT_EQ(FuncName, "bar");
+ }
+}
More information about the llvm-commits
mailing list