Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[LLDB][ELF] Fix section unification to not just use names. #90099

Merged
merged 5 commits into from
May 1, 2024
Merged

[LLDB][ELF] Fix section unification to not just use names. #90099

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce54a7f
[LLDB][ELF] Fix section unification to not just use names.
al45tair Apr 25, 2024
9653351
[LLDB][ELF] Address review feedback, add test.
al45tair Apr 26, 2024
9cbd304
[LLDB][ELF] Don't match on file size.
al45tair Apr 26, 2024
39c6fce
[LLDB][ELF] Use `yaml2obj` for the test.
al45tair Apr 29, 2024
5e28741
[LLDB][ELF] Also ignore section type when matching.
al45tair Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 53 additions & 11 deletions lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1854,6 +1854,49 @@ class VMAddressProvider {
};
}

namespace {
al45tair marked this conversation as resolved.
Show resolved Hide resolved
// 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 &section_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 marked this conversation as resolved.
Show resolved Hide resolved
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) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it could be an operator == in the Section class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Sections. 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@al45tair al45tair Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@labath Let's get rid of the file sizes; the other things should all be the same, correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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":

  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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I've removed the section type test as well.

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;
Expand Down Expand Up @@ -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<const char *, lldb::SectionSP> section_name_to_section;
// Cache the section mapping
al45tair marked this conversation as resolved.
Show resolved Hide resolved
std::unordered_map<lldb::SectionSP, lldb::SectionSP> section_map;

unsigned i;
for (i = 0; i < num_symbols; ++i) {
Expand Down Expand Up @@ -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;
}
Expand Down
Loading