[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
https://github.com/al45tair created
https://github.com/llvm/llvm-project/pull/90099
Section unification cannot just use names, because it's valid for ELF binaries
to have multiple sections with the same name. We should check other section
properties too.
Fixes #88001.
rdar://124467787
>From ce54a7fb339a00029da266c9f518e344aac5d19e Mon Sep 17 00:00:00 2001
From: Alastair Houghton
Date: Thu, 25 Apr 2024 11:35:55 +0100
Subject: [PATCH] [LLDB][ELF] Fix section unification to not just use names.
Section unification cannot just use names, because it's valid for ELF
binaries to have multiple sections with the same name. We should check
other section properties too.
rdar://124467787
---
.../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 64 +++
1 file changed, 53 insertions(+), 11 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
index 0d95a1c12bde35..60fc68c96bcc1c 100644
--- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
+sect_sp = *sect_iter;
+break;
+ } else {
+sect_sp = FindMatchingSection((*sect_iter)->GetChildren(),
+ section);
+if (sect_sp)
+ break;
+ }
+}
+
+return sect_sp;
+ }
+}
+
void ObjectFileELF::CreateSections(SectionList &unified_section_list) {
if (m_sections_up)
return;
@@ -2067,10 +2110,8 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
SectionList *module_section_list =
module_sp ? module_sp->GetSectionList() : nullptr;
- // Local cache to avoid doing a FindSectionByName for each symbol. The "const
- // char*" key must came from a ConstString object so they can be compared by
- // pointer
- std::unordered_map section_name_to_section;
+ // Cache the section mapping
+ std::unordered_map section_map;
unsigned i;
for (i = 0; i < num_symbols; ++i) {
@@ -2275,14 +2316,15 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
if (symbol_section_sp && module_section_list &&
module_section_list != section_list) {
- ConstString sect_name = symbol_section_sp->GetName();
- auto section_it = section_name_to_section.find(sect_name.GetCString());
- if (section_it == section_name_to_section.end())
+ auto section_it = section_map.find(symbol_section_sp);
+ if (section_it == section_map.end()) {
section_it =
-section_name_to_section
-.emplace(sect_name.GetCString(),
- module_section_list->FindSectionByName(sect_name))
-.first;
+ section_map
+ .emplace(symbol_section_sp,
+ FindMatchingSection(*module_section_list,
+ symbol_section_sp))
+ .first;
+ }
if (section_it->second)
symbol_section_sp = section_it->second;
}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
llvmbot wrote:
@llvm/pr-subscribers-lldb
Author: Alastair Houghton (al45tair)
Changes
Section unification cannot just use names, because it's valid for ELF binaries
to have multiple sections with the same name. We should check other section
properties too.
Fixes #88001.
rdar://124467787
---
Full diff: https://github.com/llvm/llvm-project/pull/90099.diff
1 Files Affected:
- (modified) lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp (+53-11)
``diff
diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
index 0d95a1c12bde35..60fc68c96bcc1c 100644
--- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
+sect_sp = *sect_iter;
+break;
+ } else {
+sect_sp = FindMatchingSection((*sect_iter)->GetChildren(),
+ section);
+if (sect_sp)
+ break;
+ }
+}
+
+return sect_sp;
+ }
+}
+
void ObjectFileELF::CreateSections(SectionList &unified_section_list) {
if (m_sections_up)
return;
@@ -2067,10 +2110,8 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
SectionList *module_section_list =
module_sp ? module_sp->GetSectionList() : nullptr;
- // Local cache to avoid doing a FindSectionByName for each symbol. The "const
- // char*" key must came from a ConstString object so they can be compared by
- // pointer
- std::unordered_map section_name_to_section;
+ // Cache the section mapping
+ std::unordered_map section_map;
unsigned i;
for (i = 0; i < num_symbols; ++i) {
@@ -2275,14 +2316,15 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
if (symbol_section_sp && module_section_list &&
module_section_list != section_list) {
- ConstString sect_name = symbol_section_sp->GetName();
- auto section_it = section_name_to_section.find(sect_name.GetCString());
- if (section_it == section_name_to_section.end())
+ auto section_it = section_map.find(symbol_section_sp);
+ if (section_it == section_map.end()) {
section_it =
-section_name_to_section
-.emplace(sect_name.GetCString(),
- module_section_list->FindSectionByName(sect_name))
-.first;
+ section_map
+ .emplace(symbol_section_sp,
+ FindMatchingSection(*module_section_list,
+ symbol_section_sp))
+ .first;
+ }
if (section_it->second)
symbol_section_sp = section_it->second;
}
``
https://github.com/llvm/llvm-project/pull/90099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff f5953f46aa0a664461584b78c14cb141a3be2b9d
ce54a7fb339a00029da266c9f518e344aac5d19e --
lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
``
View the diff from clang-format here.
``diff
diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
index 60fc68c96b..80be674f76 100644
--- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1855,47 +1855,44 @@ public:
}
namespace {
- // We have to do this because ELF doesn't have section IDs, and also
- // doesn't require section names to be unique. (We use the section index
- // for section IDs, but that isn't guaranteed to be the same in separate
- // debug images.)
- SectionSP FindMatchingSection(const SectionList §ion_list,
-SectionSP section) {
-SectionSP sect_sp;
-
-addr_t vm_addr = section->GetFileAddress();
-ConstString name = section->GetName();
-offset_t file_size = section->GetFileSize();
-offset_t byte_size = section->GetByteSize();
-SectionType type = section->GetType();
-bool thread_specific = section->IsThreadSpecific();
-uint32_t permissions = section->GetPermissions();
-uint32_t alignment = section->GetLog2Align();
-
-for (auto sect_iter = section_list.begin();
- sect_iter != section_list.end();
- ++sect_iter) {
- if ((*sect_iter)->GetName() == name
- && (*sect_iter)->GetType() == type
- && (*sect_iter)->IsThreadSpecific() == thread_specific
- && (*sect_iter)->GetPermissions() == permissions
- && (*sect_iter)->GetFileSize() == file_size
- && (*sect_iter)->GetByteSize() == byte_size
- && (*sect_iter)->GetFileAddress() == vm_addr
- && (*sect_iter)->GetLog2Align() == alignment) {
-sect_sp = *sect_iter;
+// We have to do this because ELF doesn't have section IDs, and also
+// doesn't require section names to be unique. (We use the section index
+// for section IDs, but that isn't guaranteed to be the same in separate
+// debug images.)
+SectionSP FindMatchingSection(const SectionList §ion_list,
+ SectionSP section) {
+ SectionSP sect_sp;
+
+ addr_t vm_addr = section->GetFileAddress();
+ ConstString name = section->GetName();
+ offset_t file_size = section->GetFileSize();
+ offset_t byte_size = section->GetByteSize();
+ SectionType type = section->GetType();
+ bool thread_specific = section->IsThreadSpecific();
+ uint32_t permissions = section->GetPermissions();
+ uint32_t alignment = section->GetLog2Align();
+
+ for (auto sect_iter = section_list.begin(); sect_iter != section_list.end();
+ ++sect_iter) {
+if ((*sect_iter)->GetName() == name && (*sect_iter)->GetType() == type &&
+(*sect_iter)->IsThreadSpecific() == thread_specific &&
+(*sect_iter)->GetPermissions() == permissions &&
+(*sect_iter)->GetFileSize() == file_size &&
+(*sect_iter)->GetByteSize() == byte_size &&
+(*sect_iter)->GetFileAddress() == vm_addr &&
+(*sect_iter)->GetLog2Align() == alignment) {
+ sect_sp = *sect_iter;
+ break;
+} else {
+ sect_sp = FindMatchingSection((*sect_iter)->GetChildren(), section);
+ if (sect_sp)
break;
- } else {
-sect_sp = FindMatchingSection((*sect_iter)->GetChildren(),
- section);
-if (sect_sp)
- break;
- }
}
-
-return sect_sp;
}
+
+ return sect_sp;
}
+} // namespace
void ObjectFileELF::CreateSections(SectionList &unified_section_list) {
if (m_sections_up)
@@ -2318,12 +2315,11 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
module_section_list != section_list) {
auto section_it = section_map.find(symbol_section_sp);
if (section_it == section_map.end()) {
-section_it =
- section_map
- .emplace(symbol_section_sp,
- FindMatchingSection(*module_section_list,
- symbol_section_sp))
- .first;
+section_it = section_map
+ .emplace(symbol_section_sp,
+ FindMatchingSection(*module_section_list,
+ symbol_section_sp))
+ .first;
}
if (section_it->second)
symbol_section_sp = section_it->second;
``
https://github.com/llvm/llvm-project/pull/90099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinf
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
https://github.com/JDevlieghere edited https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
https://github.com/JDevlieghere commented: Can we add a test for this with obj2yaml? https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
JDevlieghere wrote:
This seems like it could be an `operator ==` in the Section class.
https://github.com/llvm/llvm-project/pull/90099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
JDevlieghere wrote:
Can this be a range-based for loop?
https://github.com/llvm/llvm-project/pull/90099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
JDevlieghere wrote:
Nit: LLVM and LLDB prefer static functions over anonymous namespaces.
https://github.com/llvm/llvm-project/pull/90099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
@@ -2067,10 +2110,8 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab, user_id_t start_id, SectionList *module_section_list = module_sp ? module_sp->GetSectionList() : nullptr; - // Local cache to avoid doing a FindSectionByName for each symbol. The "const - // char*" key must came from a ConstString object so they can be compared by - // pointer - std::unordered_map section_name_to_section; + // Cache the section mapping JDevlieghere wrote: Nit: missing period. A map from SectionSP to SectionSP looks somewhat suspicious, might be worth explaining what the key and the value mean. https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
al45tair wrote:
This file already uses anonymous namespaces, but I'll change the function to
`static` instead.
https://github.com/llvm/llvm-project/pull/90099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
al45tair wrote:
I didn't want to do that, because I didn't want to give the impression that
this was a general purpose way to compare two `Section`s. While it checks
_many_ of the section attributes, it doesn't check all of them (intentionally
in the case of which object they come from), and I think that would be
surprising behaviour from an `==` operator.
https://github.com/llvm/llvm-project/pull/90099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
al45tair wrote:
Yes, it can (I just checked and apparently we can use C++17 in LLVM code, so
that's nice :-)).
https://github.com/llvm/llvm-project/pull/90099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
@@ -2067,10 +2110,8 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab, user_id_t start_id, SectionList *module_section_list = module_sp ? module_sp->GetSectionList() : nullptr; - // Local cache to avoid doing a FindSectionByName for each symbol. The "const - // char*" key must came from a ConstString object so they can be compared by - // pointer - std::unordered_map section_name_to_section; + // Cache the section mapping al45tair wrote: I'll improve the comment. https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
al45tair wrote: > Can we add a test for this with obj2yaml? Not sure what you're thinking here. We can't use `yaml2obj` to generate an object with this problem because that tool assumes symbols are in a uniquely named section (which is mostly reasonable), and `obj2yaml` doesn't exercise the code in LLDB. I'll take a look and see what we can do as a test. https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
https://github.com/al45tair updated
https://github.com/llvm/llvm-project/pull/90099
>From ce54a7fb339a00029da266c9f518e344aac5d19e Mon Sep 17 00:00:00 2001
From: Alastair Houghton
Date: Thu, 25 Apr 2024 11:35:55 +0100
Subject: [PATCH 1/2] [LLDB][ELF] Fix section unification to not just use
names.
Section unification cannot just use names, because it's valid for ELF
binaries to have multiple sections with the same name. We should check
other section properties too.
rdar://124467787
---
.../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 64 +++
1 file changed, 53 insertions(+), 11 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
index 0d95a1c12bde35..60fc68c96bcc1c 100644
--- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
+sect_sp = *sect_iter;
+break;
+ } else {
+sect_sp = FindMatchingSection((*sect_iter)->GetChildren(),
+ section);
+if (sect_sp)
+ break;
+ }
+}
+
+return sect_sp;
+ }
+}
+
void ObjectFileELF::CreateSections(SectionList &unified_section_list) {
if (m_sections_up)
return;
@@ -2067,10 +2110,8 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
SectionList *module_section_list =
module_sp ? module_sp->GetSectionList() : nullptr;
- // Local cache to avoid doing a FindSectionByName for each symbol. The "const
- // char*" key must came from a ConstString object so they can be compared by
- // pointer
- std::unordered_map section_name_to_section;
+ // Cache the section mapping
+ std::unordered_map section_map;
unsigned i;
for (i = 0; i < num_symbols; ++i) {
@@ -2275,14 +2316,15 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
if (symbol_section_sp && module_section_list &&
module_section_list != section_list) {
- ConstString sect_name = symbol_section_sp->GetName();
- auto section_it = section_name_to_section.find(sect_name.GetCString());
- if (section_it == section_name_to_section.end())
+ auto section_it = section_map.find(symbol_section_sp);
+ if (section_it == section_map.end()) {
section_it =
-section_name_to_section
-.emplace(sect_name.GetCString(),
- module_section_list->FindSectionByName(sect_name))
-.first;
+ section_map
+ .emplace(symbol_section_sp,
+ FindMatchingSection(*module_section_list,
+ symbol_section_sp))
+ .first;
+ }
if (section_it->second)
symbol_section_sp = section_it->second;
}
>From ccad6269dea865ee61f876e0f28d9cc825d507b0 Mon Sep 17 00:00:00 2001
From: Alastair Houghton
Date: Fri, 26 Apr 2024 14:53:20 +0100
Subject: [PATCH 2/2] [LLDB][ELF] Address review feedback, add test.
Fixed a couple of nits from review, and fixed up formatting.
Also added a test.
rdar://124467787
---
.../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 87 +-
.../ELF/Inputs/two-text-sections.elf | Bin 0 -> 4976 bytes
.../ObjectFile/ELF/two-text-sections.test | 8 ++
3 files changed, 50 insertions(+), 45 deletions(-)
create mode 100644 lldb/test/Shell/ObjectFile/ELF/Inputs/two-text-sections.elf
create mode 100644 lldb/test/Shell/ObjectFile/ELF/two-text-sections.test
diff --git a/ll
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
https://github.com/al45tair updated
https://github.com/llvm/llvm-project/pull/90099
>From ce54a7fb339a00029da266c9f518e344aac5d19e Mon Sep 17 00:00:00 2001
From: Alastair Houghton
Date: Thu, 25 Apr 2024 11:35:55 +0100
Subject: [PATCH 1/2] [LLDB][ELF] Fix section unification to not just use
names.
Section unification cannot just use names, because it's valid for ELF
binaries to have multiple sections with the same name. We should check
other section properties too.
rdar://124467787
---
.../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 64 +++
1 file changed, 53 insertions(+), 11 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
index 0d95a1c12bde35..60fc68c96bcc1c 100644
--- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
+sect_sp = *sect_iter;
+break;
+ } else {
+sect_sp = FindMatchingSection((*sect_iter)->GetChildren(),
+ section);
+if (sect_sp)
+ break;
+ }
+}
+
+return sect_sp;
+ }
+}
+
void ObjectFileELF::CreateSections(SectionList &unified_section_list) {
if (m_sections_up)
return;
@@ -2067,10 +2110,8 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
SectionList *module_section_list =
module_sp ? module_sp->GetSectionList() : nullptr;
- // Local cache to avoid doing a FindSectionByName for each symbol. The "const
- // char*" key must came from a ConstString object so they can be compared by
- // pointer
- std::unordered_map section_name_to_section;
+ // Cache the section mapping
+ std::unordered_map section_map;
unsigned i;
for (i = 0; i < num_symbols; ++i) {
@@ -2275,14 +2316,15 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
if (symbol_section_sp && module_section_list &&
module_section_list != section_list) {
- ConstString sect_name = symbol_section_sp->GetName();
- auto section_it = section_name_to_section.find(sect_name.GetCString());
- if (section_it == section_name_to_section.end())
+ auto section_it = section_map.find(symbol_section_sp);
+ if (section_it == section_map.end()) {
section_it =
-section_name_to_section
-.emplace(sect_name.GetCString(),
- module_section_list->FindSectionByName(sect_name))
-.first;
+ section_map
+ .emplace(symbol_section_sp,
+ FindMatchingSection(*module_section_list,
+ symbol_section_sp))
+ .first;
+ }
if (section_it->second)
symbol_section_sp = section_it->second;
}
>From 9653351729b4ef2d98faba936b8fa6fb51a9a47c Mon Sep 17 00:00:00 2001
From: Alastair Houghton
Date: Fri, 26 Apr 2024 14:53:20 +0100
Subject: [PATCH 2/2] [LLDB][ELF] Address review feedback, add test.
Fixed a couple of nits from review, and fixed up formatting.
Also added a test.
rdar://124467787
---
.../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 86 +-
.../ELF/Inputs/two-text-sections.elf | Bin 0 -> 4976 bytes
.../ObjectFile/ELF/two-text-sections.test | 8 ++
3 files changed, 49 insertions(+), 45 deletions(-)
create mode 100644 lldb/test/Shell/ObjectFile/ELF/Inputs/two-text-sections.elf
create mode 100644 lldb/test/Shell/ObjectFile/ELF/two-text-sections.test
diff --git a/ll
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
labath wrote: > > Can we add a test for this with obj2yaml? > > Not sure what you're thinking here. We can't use `yaml2obj` to generate an > object with this problem because that tool assumes symbols are in a uniquely > named section [this](https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/yaml2obj/ELF/duplicate-section-names.yaml) test seems to indicate that's possible (the trick appears to me in `(n)` name tag suffixes). Can you give that a shot? https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
labath wrote:
Am I correct in understanding that this code is used when unifying the sections
of a separate debug file with a stripped file? If so, then I believe this
comparison is too strict. An object file produced by `objcopy
--only-keep-debug` will only have placeholder sections instead of the sections
that contained actual code. That means (at least) their file size and file
offset will be different from that in the original file.
I think it would be sufficient to use the file address (called virtual address
in ELF), possibly confirmed by section name, as the unification key.
https://github.com/llvm/llvm-project/pull/90099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
al45tair wrote:
I did wonder about exactly how strict we want to be (and exactly which things
to compare). This isn't testing file offset, but you're right that it does
check file size and maybe it shouldn't.
https://github.com/llvm/llvm-project/pull/90099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
al45tair wrote:
@labath Let's get rid of the sizes; the other things should all be the same,
correct?
https://github.com/llvm/llvm-project/pull/90099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
al45tair wrote: > [this](https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/yaml2obj/ELF/duplicate-section-names.yaml) > test seems to indicate that's possible (the trick appears to me in `(n)` > name tag suffixes). Can you give that a shot? I've actually just included a binary file that exhibits the problem. https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
https://github.com/al45tair updated
https://github.com/llvm/llvm-project/pull/90099
>From ce54a7fb339a00029da266c9f518e344aac5d19e Mon Sep 17 00:00:00 2001
From: Alastair Houghton
Date: Thu, 25 Apr 2024 11:35:55 +0100
Subject: [PATCH 1/3] [LLDB][ELF] Fix section unification to not just use
names.
Section unification cannot just use names, because it's valid for ELF
binaries to have multiple sections with the same name. We should check
other section properties too.
rdar://124467787
---
.../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 64 +++
1 file changed, 53 insertions(+), 11 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
index 0d95a1c12bde35..60fc68c96bcc1c 100644
--- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
+sect_sp = *sect_iter;
+break;
+ } else {
+sect_sp = FindMatchingSection((*sect_iter)->GetChildren(),
+ section);
+if (sect_sp)
+ break;
+ }
+}
+
+return sect_sp;
+ }
+}
+
void ObjectFileELF::CreateSections(SectionList &unified_section_list) {
if (m_sections_up)
return;
@@ -2067,10 +2110,8 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
SectionList *module_section_list =
module_sp ? module_sp->GetSectionList() : nullptr;
- // Local cache to avoid doing a FindSectionByName for each symbol. The "const
- // char*" key must came from a ConstString object so they can be compared by
- // pointer
- std::unordered_map section_name_to_section;
+ // Cache the section mapping
+ std::unordered_map section_map;
unsigned i;
for (i = 0; i < num_symbols; ++i) {
@@ -2275,14 +2316,15 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
if (symbol_section_sp && module_section_list &&
module_section_list != section_list) {
- ConstString sect_name = symbol_section_sp->GetName();
- auto section_it = section_name_to_section.find(sect_name.GetCString());
- if (section_it == section_name_to_section.end())
+ auto section_it = section_map.find(symbol_section_sp);
+ if (section_it == section_map.end()) {
section_it =
-section_name_to_section
-.emplace(sect_name.GetCString(),
- module_section_list->FindSectionByName(sect_name))
-.first;
+ section_map
+ .emplace(symbol_section_sp,
+ FindMatchingSection(*module_section_list,
+ symbol_section_sp))
+ .first;
+ }
if (section_it->second)
symbol_section_sp = section_it->second;
}
>From 9653351729b4ef2d98faba936b8fa6fb51a9a47c Mon Sep 17 00:00:00 2001
From: Alastair Houghton
Date: Fri, 26 Apr 2024 14:53:20 +0100
Subject: [PATCH 2/3] [LLDB][ELF] Address review feedback, add test.
Fixed a couple of nits from review, and fixed up formatting.
Also added a test.
rdar://124467787
---
.../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 86 +-
.../ELF/Inputs/two-text-sections.elf | Bin 0 -> 4976 bytes
.../ObjectFile/ELF/two-text-sections.test | 8 ++
3 files changed, 49 insertions(+), 45 deletions(-)
create mode 100644 lldb/test/Shell/ObjectFile/ELF/Inputs/two-text-sections.elf
create mode 100644 lldb/test/Shell/ObjectFile/ELF/two-text-sections.test
diff --git a/ll
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
https://github.com/al45tair edited https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
labath wrote: > > [this](https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/yaml2obj/ELF/duplicate-section-names.yaml) > > test seems to indicate that's possible (the trick appears to me in `(n)` > > name tag suffixes). Can you give that a shot? > > I've actually just included a binary file that exhibits the problem. I saw that, but a textual test is definitely preferable, particularly after the linux xz fiasco. This wouldn't be the first test binary in our repo, but in this case, I think it actually adds a lot of value, since yaml2obj operates on the same level as what you are testing (so you can see the input of the test eyeball-verify that the expected output makes sense). https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
al45tair wrote: > I saw that, but a textual test is definitely preferable, particularly after > the linux xz fiasco. This wouldn't be the first test binary in our repo, but > in this case, I think it actually adds a lot of value, since yaml2obj > operates on the same level as what you are testing (so you can see the input > of the test eyeball-verify that the expected output makes sense). :-) Fair point (though this is very much not like the xz incident — I'm a colleague of Jonas's at Apple, albeit in a different team, with a fairly longstanding history of contributing to OSS projects, and doing something like that would be… very career limiting). I'll have a go at changing things to use `yaml2obj`. https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
https://github.com/al45tair updated
https://github.com/llvm/llvm-project/pull/90099
>From ce54a7fb339a00029da266c9f518e344aac5d19e Mon Sep 17 00:00:00 2001
From: Alastair Houghton
Date: Thu, 25 Apr 2024 11:35:55 +0100
Subject: [PATCH 1/4] [LLDB][ELF] Fix section unification to not just use
names.
Section unification cannot just use names, because it's valid for ELF
binaries to have multiple sections with the same name. We should check
other section properties too.
rdar://124467787
---
.../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 64 +++
1 file changed, 53 insertions(+), 11 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
index 0d95a1c12bde35..60fc68c96bcc1c 100644
--- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
+sect_sp = *sect_iter;
+break;
+ } else {
+sect_sp = FindMatchingSection((*sect_iter)->GetChildren(),
+ section);
+if (sect_sp)
+ break;
+ }
+}
+
+return sect_sp;
+ }
+}
+
void ObjectFileELF::CreateSections(SectionList &unified_section_list) {
if (m_sections_up)
return;
@@ -2067,10 +2110,8 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
SectionList *module_section_list =
module_sp ? module_sp->GetSectionList() : nullptr;
- // Local cache to avoid doing a FindSectionByName for each symbol. The "const
- // char*" key must came from a ConstString object so they can be compared by
- // pointer
- std::unordered_map section_name_to_section;
+ // Cache the section mapping
+ std::unordered_map section_map;
unsigned i;
for (i = 0; i < num_symbols; ++i) {
@@ -2275,14 +2316,15 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
if (symbol_section_sp && module_section_list &&
module_section_list != section_list) {
- ConstString sect_name = symbol_section_sp->GetName();
- auto section_it = section_name_to_section.find(sect_name.GetCString());
- if (section_it == section_name_to_section.end())
+ auto section_it = section_map.find(symbol_section_sp);
+ if (section_it == section_map.end()) {
section_it =
-section_name_to_section
-.emplace(sect_name.GetCString(),
- module_section_list->FindSectionByName(sect_name))
-.first;
+ section_map
+ .emplace(symbol_section_sp,
+ FindMatchingSection(*module_section_list,
+ symbol_section_sp))
+ .first;
+ }
if (section_it->second)
symbol_section_sp = section_it->second;
}
>From 9653351729b4ef2d98faba936b8fa6fb51a9a47c Mon Sep 17 00:00:00 2001
From: Alastair Houghton
Date: Fri, 26 Apr 2024 14:53:20 +0100
Subject: [PATCH 2/4] [LLDB][ELF] Address review feedback, add test.
Fixed a couple of nits from review, and fixed up formatting.
Also added a test.
rdar://124467787
---
.../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 86 +-
.../ELF/Inputs/two-text-sections.elf | Bin 0 -> 4976 bytes
.../ObjectFile/ELF/two-text-sections.test | 8 ++
3 files changed, 49 insertions(+), 45 deletions(-)
create mode 100644 lldb/test/Shell/ObjectFile/ELF/Inputs/two-text-sections.elf
create mode 100644 lldb/test/Shell/ObjectFile/ELF/two-text-sections.test
diff --git a/ll
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
labath wrote:
The section type can also change. You can see that with .text, because there we
have a name-based fallback, but with anything else, it changes from "code" to
"regular":
```$ bin/lldb-test object-file /tmp/a.debug | grep -e foo -C 3
Showing 1 subsections
Index: 0
ID: 0x6
Name: .foo
Type: regular
Permissions: r-x
Thread specific: no
$ bin/lldb-test object-file /tmp/a.out | grep -e foo -C 3
Showing 1 subsections
Index: 0
ID: 0x6
Name: .foo
Type: code
Permissions: r-x
Thread specific: no
```
The other fields are _probably_ fine.
https://github.com/llvm/llvm-project/pull/90099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
labath wrote: > > I saw that, but a textual test is definitely preferable, particularly after > > the linux xz fiasco. This wouldn't be the first test binary in our repo, > > but in this case, I think it actually adds a lot of value, since yaml2obj > > operates on the same level as what you are testing (so you can see the > > input of the test eyeball-verify that the expected output makes sense). > > :-) Fair point (though this is very much not like the xz incident — I'm a > colleague of Jonas's at Apple, albeit in a different team, with a fairly > longstanding history of contributing to OSS projects, and doing something > like that would be… very career limiting). > > I'll have a go at changing things to use `yaml2obj`. The new test looks great. https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
https://github.com/al45tair updated
https://github.com/llvm/llvm-project/pull/90099
>From ce54a7fb339a00029da266c9f518e344aac5d19e Mon Sep 17 00:00:00 2001
From: Alastair Houghton
Date: Thu, 25 Apr 2024 11:35:55 +0100
Subject: [PATCH 1/5] [LLDB][ELF] Fix section unification to not just use
names.
Section unification cannot just use names, because it's valid for ELF
binaries to have multiple sections with the same name. We should check
other section properties too.
rdar://124467787
---
.../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 64 +++
1 file changed, 53 insertions(+), 11 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
index 0d95a1c12bde35..60fc68c96bcc1c 100644
--- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
+sect_sp = *sect_iter;
+break;
+ } else {
+sect_sp = FindMatchingSection((*sect_iter)->GetChildren(),
+ section);
+if (sect_sp)
+ break;
+ }
+}
+
+return sect_sp;
+ }
+}
+
void ObjectFileELF::CreateSections(SectionList &unified_section_list) {
if (m_sections_up)
return;
@@ -2067,10 +2110,8 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
SectionList *module_section_list =
module_sp ? module_sp->GetSectionList() : nullptr;
- // Local cache to avoid doing a FindSectionByName for each symbol. The "const
- // char*" key must came from a ConstString object so they can be compared by
- // pointer
- std::unordered_map section_name_to_section;
+ // Cache the section mapping
+ std::unordered_map section_map;
unsigned i;
for (i = 0; i < num_symbols; ++i) {
@@ -2275,14 +2316,15 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
if (symbol_section_sp && module_section_list &&
module_section_list != section_list) {
- ConstString sect_name = symbol_section_sp->GetName();
- auto section_it = section_name_to_section.find(sect_name.GetCString());
- if (section_it == section_name_to_section.end())
+ auto section_it = section_map.find(symbol_section_sp);
+ if (section_it == section_map.end()) {
section_it =
-section_name_to_section
-.emplace(sect_name.GetCString(),
- module_section_list->FindSectionByName(sect_name))
-.first;
+ section_map
+ .emplace(symbol_section_sp,
+ FindMatchingSection(*module_section_list,
+ symbol_section_sp))
+ .first;
+ }
if (section_it->second)
symbol_section_sp = section_it->second;
}
>From 9653351729b4ef2d98faba936b8fa6fb51a9a47c Mon Sep 17 00:00:00 2001
From: Alastair Houghton
Date: Fri, 26 Apr 2024 14:53:20 +0100
Subject: [PATCH 2/5] [LLDB][ELF] Address review feedback, add test.
Fixed a couple of nits from review, and fixed up formatting.
Also added a test.
rdar://124467787
---
.../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 86 +-
.../ELF/Inputs/two-text-sections.elf | Bin 0 -> 4976 bytes
.../ObjectFile/ELF/two-text-sections.test | 8 ++
3 files changed, 49 insertions(+), 45 deletions(-)
create mode 100644 lldb/test/Shell/ObjectFile/ELF/Inputs/two-text-sections.elf
create mode 100644 lldb/test/Shell/ObjectFile/ELF/two-text-sections.test
diff --git a/ll
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
al45tair wrote:
OK. I've removed the section type test as well.
https://github.com/llvm/llvm-project/pull/90099
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
al45tair wrote: @labath @JDevlieghere Are we happy with this PR now? If so, I'll cherry pick it to the various Apple forks; I need it in order to merge https://github.com/apple/swift/pull/72061, which we need for both my fully statically linked Swift SDK work and for Amazon Linux 2023 support in Swift. https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
https://github.com/al45tair updated
https://github.com/llvm/llvm-project/pull/90099
>From ce54a7fb339a00029da266c9f518e344aac5d19e Mon Sep 17 00:00:00 2001
From: Alastair Houghton
Date: Thu, 25 Apr 2024 11:35:55 +0100
Subject: [PATCH 1/5] [LLDB][ELF] Fix section unification to not just use
names.
Section unification cannot just use names, because it's valid for ELF
binaries to have multiple sections with the same name. We should check
other section properties too.
rdar://124467787
---
.../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 64 +++
1 file changed, 53 insertions(+), 11 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
index 0d95a1c12bde35..60fc68c96bcc1c 100644
--- a/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ b/lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}
+namespace {
+ // We have to do this because ELF doesn't have section IDs, and also
+ // doesn't require section names to be unique. (We use the section index
+ // for section IDs, but that isn't guaranteed to be the same in separate
+ // debug images.)
+ SectionSP FindMatchingSection(const SectionList §ion_list,
+SectionSP section) {
+SectionSP sect_sp;
+
+addr_t vm_addr = section->GetFileAddress();
+ConstString name = section->GetName();
+offset_t file_size = section->GetFileSize();
+offset_t byte_size = section->GetByteSize();
+SectionType type = section->GetType();
+bool thread_specific = section->IsThreadSpecific();
+uint32_t permissions = section->GetPermissions();
+uint32_t alignment = section->GetLog2Align();
+
+for (auto sect_iter = section_list.begin();
+ sect_iter != section_list.end();
+ ++sect_iter) {
+ if ((*sect_iter)->GetName() == name
+ && (*sect_iter)->GetType() == type
+ && (*sect_iter)->IsThreadSpecific() == thread_specific
+ && (*sect_iter)->GetPermissions() == permissions
+ && (*sect_iter)->GetFileSize() == file_size
+ && (*sect_iter)->GetByteSize() == byte_size
+ && (*sect_iter)->GetFileAddress() == vm_addr
+ && (*sect_iter)->GetLog2Align() == alignment) {
+sect_sp = *sect_iter;
+break;
+ } else {
+sect_sp = FindMatchingSection((*sect_iter)->GetChildren(),
+ section);
+if (sect_sp)
+ break;
+ }
+}
+
+return sect_sp;
+ }
+}
+
void ObjectFileELF::CreateSections(SectionList &unified_section_list) {
if (m_sections_up)
return;
@@ -2067,10 +2110,8 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
SectionList *module_section_list =
module_sp ? module_sp->GetSectionList() : nullptr;
- // Local cache to avoid doing a FindSectionByName for each symbol. The "const
- // char*" key must came from a ConstString object so they can be compared by
- // pointer
- std::unordered_map section_name_to_section;
+ // Cache the section mapping
+ std::unordered_map section_map;
unsigned i;
for (i = 0; i < num_symbols; ++i) {
@@ -2275,14 +2316,15 @@ unsigned ObjectFileELF::ParseSymbols(Symtab *symtab,
user_id_t start_id,
if (symbol_section_sp && module_section_list &&
module_section_list != section_list) {
- ConstString sect_name = symbol_section_sp->GetName();
- auto section_it = section_name_to_section.find(sect_name.GetCString());
- if (section_it == section_name_to_section.end())
+ auto section_it = section_map.find(symbol_section_sp);
+ if (section_it == section_map.end()) {
section_it =
-section_name_to_section
-.emplace(sect_name.GetCString(),
- module_section_list->FindSectionByName(sect_name))
-.first;
+ section_map
+ .emplace(symbol_section_sp,
+ FindMatchingSection(*module_section_list,
+ symbol_section_sp))
+ .first;
+ }
if (section_it->second)
symbol_section_sp = section_it->second;
}
>From 9653351729b4ef2d98faba936b8fa6fb51a9a47c Mon Sep 17 00:00:00 2001
From: Alastair Houghton
Date: Fri, 26 Apr 2024 14:53:20 +0100
Subject: [PATCH 2/5] [LLDB][ELF] Address review feedback, add test.
Fixed a couple of nits from review, and fixed up formatting.
Also added a test.
rdar://124467787
---
.../Plugins/ObjectFile/ELF/ObjectFileELF.cpp | 86 +-
.../ELF/Inputs/two-text-sections.elf | Bin 0 -> 4976 bytes
.../ObjectFile/ELF/two-text-sections.test | 8 ++
3 files changed, 49 insertions(+), 45 deletions(-)
create mode 100644 lldb/test/Shell/ObjectFile/ELF/Inputs/two-text-sections.elf
create mode 100644 lldb/test/Shell/ObjectFile/ELF/two-text-sections.test
diff --git a/ll
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
https://github.com/labath approved this pull request. https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
https://github.com/JDevlieghere approved this pull request. Looks good to me as well. https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB][ELF] Fix section unification to not just use names. (PR #90099)
https://github.com/JDevlieghere closed https://github.com/llvm/llvm-project/pull/90099 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
