caio/clang-p2996
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[NFC] Refactor symbol table parsing.

Browse Source 
Symbol table parsing has evolved over the years and many plug-ins contained duplicate code in the ObjectFile::GetSymtab() that used to be pure virtual. With this change, the "Symbtab *ObjectFile::GetSymtab()" is no longer virtual and will end up calling a new "void ObjectFile::ParseSymtab(Symtab &symtab)" pure virtual function to actually do the parsing. This helps centralize the code for parsing the symbol table and allows the ObjectFile base class to do all of the common work, like taking the necessary locks and creating the symbol table object itself. Plug-ins now just need to parse when they are asked to parse as the ParseSymtab function will only get called once.

This is a retry of the original patch https://reviews.llvm.org/D113965 which was reverted. There was a deadlock in the Manual DWARF indexing code during symbol preloading where the module was asked on the main thread to preload its symbols, and this would in turn cause the DWARF manual indexing to use a thread pool to index all of the compile units, and if there were relocations on the debug information sections, these threads could ask the ObjectFile to load section contents, which could cause a call to ObjectFileELF::RelocateSection() which would ask for the symbol table from the module and it would deadlock. We can't lock the module in ObjectFile::GetSymtab(), so the solution I am using is to use a llvm::once_flag to create the symbol table object once and then lock the Symtab object. Since all APIs on the symbol table use this lock, this will prevent anyone from using the symbol table before it is parsed and finalized and will avoid the deadlock I mentioned. ObjectFileELF::GetSymtab() was never locking the module lock before and would put off creating the symbol table until somewhere inside ObjectFileELF::GetSymtab(). Now we create it one time inside of the ObjectFile::GetSymtab() and immediately lock it which should be safe enough. This avoids the deadlocks and still provides safety.

Differential Revision: https://reviews.llvm.org/D114288
This commit is contained in:
Greg Clayton
2021-11-17 21:18:24 -08:00
parent 80cdf0db67
commit 7e6df41f65
22 changed files with 355 additions and 375 deletions
Download Patch File Download Diff File Expand all files Collapse all files

83
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
View File

@@ -1311,22 +1311,6 @@ AddressClass ObjectFileMachO::GetAddressClass(lldb::addr_t file_addr) {
return AddressClass::eUnknown;
}
Symtab *ObjectFileMachO::GetSymtab() {
ModuleSP module_sp(GetModule());
if (module_sp) {
std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
if (m_symtab_up == nullptr) {
ElapsedTime elapsed(module_sp->GetSymtabParseTime());
m_symtab_up = std::make_unique<Symtab>(this);
std::lock_guard<std::recursive_mutex> symtab_guard(
m_symtab_up->GetMutex());
ParseSymtab();
m_symtab_up->Finalize();
}
}
return m_symtab_up.get();
}
bool ObjectFileMachO::IsStripped() {
if (m_dysymtab.cmd == 0) {
ModuleSP module_sp(GetModule());
@@ -2233,12 +2217,12 @@ ParseNList(DataExtractor &nlist_data, lldb::offset_t &nlist_data_offset,
enum { DebugSymbols = true, NonDebugSymbols = false };
size_t ObjectFileMachO::ParseSymtab() {
void ObjectFileMachO::ParseSymtab(Symtab &symtab) {
LLDB_SCOPED_TIMERF("ObjectFileMachO::ParseSymtab () module = %s",
m_file.GetFilename().AsCString(""));
ModuleSP module_sp(GetModule());
if (!module_sp)
return 0;
return;
Progress progress(llvm::formatv("Parsing symbol table for {0}",
m_file.GetFilename().AsCString("<Unknown>")));
@@ -2288,7 +2272,7 @@ size_t ObjectFileMachO::ParseSymtab() {
// Read in the rest of the symtab load command
if (m_data.GetU32(&offset, &symtab_load_command.symoff, 4) ==
nullptr) // fill in symoff, nsyms, stroff, strsize fields
return 0;
return;
break;
case LC_DYLD_INFO:
@@ -2347,12 +2331,11 @@ size_t ObjectFileMachO::ParseSymtab() {
}
if (!symtab_load_command.cmd)
return 0;
return;
Symtab *symtab = m_symtab_up.get();
SectionList *section_list = GetSectionList();
if (section_list == nullptr)
return 0;
return;
const uint32_t addr_byte_size = m_data.GetAddressByteSize();
const ByteOrder byte_order = m_data.GetByteOrder();
@@ -2489,7 +2472,7 @@ size_t ObjectFileMachO::ParseSymtab() {
// We shouldn't have exports data from both the LC_DYLD_INFO command
// AND the LC_DYLD_EXPORTS_TRIE command in the same binary:
lldbassert(!((dyld_info.export_size > 0)
lldbassert(!((dyld_info.export_size > 0)
&& (exports_trie_load_command.datasize > 0)));
if (dyld_info.export_size > 0) {
dyld_trie_data.SetData(m_data, dyld_info.export_off,
@@ -2881,10 +2864,10 @@ size_t ObjectFileMachO::ParseSymtab() {
// The normal nlist code cannot correctly size the Symbols
// array, we need to allocate it here.
sym = symtab->Resize(
sym = symtab.Resize(
symtab_load_command.nsyms + m_dysymtab.nindirectsyms +
unmapped_local_symbols_found - m_dysymtab.nlocalsym);
num_syms = symtab->GetNumSymbols();
num_syms = symtab.GetNumSymbols();
nlist_data_offset =
local_symbols_info.nlistOffset +
@@ -3016,7 +2999,7 @@ size_t ObjectFileMachO::ParseSymtab() {
// original
// STAB entry so we don't have
// to hunt for it later
symtab->SymbolAtIndex(N_FUN_indexes.back())
symtab.SymbolAtIndex(N_FUN_indexes.back())
->SetByteSize(nlist.n_value);
N_FUN_indexes.pop_back();
// We don't really need the end function STAB as
@@ -3096,7 +3079,7 @@ size_t ObjectFileMachO::ParseSymtab() {
// index of this N_SO so that we can always skip
// the entire N_SO if we need to navigate more
// quickly at the source level when parsing STABS
symbol_ptr = symtab->SymbolAtIndex(N_SO_index);
symbol_ptr = symtab.SymbolAtIndex(N_SO_index);
symbol_ptr->SetByteSize(sym_idx);
symbol_ptr->SetSizeIsSibling(true);
}
@@ -3203,7 +3186,7 @@ size_t ObjectFileMachO::ParseSymtab() {
// quickly at the source level when parsing STABS
if (!N_INCL_indexes.empty()) {
symbol_ptr =
symtab->SymbolAtIndex(N_INCL_indexes.back());
symtab.SymbolAtIndex(N_INCL_indexes.back());
symbol_ptr->SetByteSize(sym_idx + 1);
symbol_ptr->SetSizeIsSibling(true);
N_INCL_indexes.pop_back();
@@ -3268,7 +3251,7 @@ size_t ObjectFileMachO::ParseSymtab() {
nlist.n_value);
if (!N_BRAC_indexes.empty()) {
symbol_ptr =
symtab->SymbolAtIndex(N_BRAC_indexes.back());
symtab.SymbolAtIndex(N_BRAC_indexes.back());
symbol_ptr->SetByteSize(sym_idx + 1);
symbol_ptr->SetSizeIsSibling(true);
N_BRAC_indexes.pop_back();
@@ -3306,7 +3289,7 @@ size_t ObjectFileMachO::ParseSymtab() {
// parsing STABS
if (!N_COMM_indexes.empty()) {
symbol_ptr =
symtab->SymbolAtIndex(N_COMM_indexes.back());
symtab.SymbolAtIndex(N_COMM_indexes.back());
symbol_ptr->SetByteSize(sym_idx + 1);
symbol_ptr->SetSizeIsSibling(true);
N_COMM_indexes.pop_back();
@@ -3806,8 +3789,8 @@ size_t ObjectFileMachO::ParseSymtab() {
// symbols, create it now.
if (sym == nullptr) {
sym =
symtab->Resize(symtab_load_command.nsyms + m_dysymtab.nindirectsyms);
num_syms = symtab->GetNumSymbols();
symtab.Resize(symtab_load_command.nsyms + m_dysymtab.nindirectsyms);
num_syms = symtab.GetNumSymbols();
}
if (unmapped_local_symbols_found) {
@@ -3941,7 +3924,7 @@ size_t ObjectFileMachO::ParseSymtab() {
if (!N_FUN_indexes.empty()) {
// Copy the size of the function into the original STAB entry
// so we don't have to hunt for it later
symtab->SymbolAtIndex(N_FUN_indexes.back())
symtab.SymbolAtIndex(N_FUN_indexes.back())
->SetByteSize(nlist.n_value);
N_FUN_indexes.pop_back();
// We don't really need the end function STAB as it contains
@@ -4015,7 +3998,7 @@ size_t ObjectFileMachO::ParseSymtab() {
// N_SO so that we can always skip the entire N_SO if we need
// to navigate more quickly at the source level when parsing
// STABS
symbol_ptr = symtab->SymbolAtIndex(N_SO_index);
symbol_ptr = symtab.SymbolAtIndex(N_SO_index);
symbol_ptr->SetByteSize(sym_idx);
symbol_ptr->SetSizeIsSibling(true);
}
@@ -4109,7 +4092,7 @@ size_t ObjectFileMachO::ParseSymtab() {
// N_EINCL so that we can always skip the entire symbol if we need
// to navigate more quickly at the source level when parsing STABS
if (!N_INCL_indexes.empty()) {
symbol_ptr = symtab->SymbolAtIndex(N_INCL_indexes.back());
symbol_ptr = symtab.SymbolAtIndex(N_INCL_indexes.back());
symbol_ptr->SetByteSize(sym_idx + 1);
symbol_ptr->SetSizeIsSibling(true);
N_INCL_indexes.pop_back();
@@ -4168,7 +4151,7 @@ size_t ObjectFileMachO::ParseSymtab() {
// quickly at the source level when parsing STABS
symbol_section = section_info.GetSection(nlist.n_sect, nlist.n_value);
if (!N_BRAC_indexes.empty()) {
symbol_ptr = symtab->SymbolAtIndex(N_BRAC_indexes.back());
symbol_ptr = symtab.SymbolAtIndex(N_BRAC_indexes.back());
symbol_ptr->SetByteSize(sym_idx + 1);
symbol_ptr->SetSizeIsSibling(true);
N_BRAC_indexes.pop_back();
@@ -4202,7 +4185,7 @@ size_t ObjectFileMachO::ParseSymtab() {
// we need to navigate more quickly at the source level when
// parsing STABS
if (!N_COMM_indexes.empty()) {
symbol_ptr = symtab->SymbolAtIndex(N_COMM_indexes.back());
symbol_ptr = symtab.SymbolAtIndex(N_COMM_indexes.back());
symbol_ptr->SetByteSize(sym_idx + 1);
symbol_ptr->SetSizeIsSibling(true);
N_COMM_indexes.pop_back();
@@ -4658,7 +4641,7 @@ size_t ObjectFileMachO::ParseSymtab() {
if (num_syms < sym_idx + trie_symbol_table_augment_count) {
num_syms = sym_idx + trie_symbol_table_augment_count;
sym = symtab->Resize(num_syms);
sym = symtab.Resize(num_syms);
}
uint32_t synthetic_sym_id = symtab_load_command.nsyms;
@@ -4707,7 +4690,7 @@ size_t ObjectFileMachO::ParseSymtab() {
if (num_synthetic_function_symbols > 0) {
if (num_syms < sym_idx + num_synthetic_function_symbols) {
num_syms = sym_idx + num_synthetic_function_symbols;
sym = symtab->Resize(num_syms);
sym = symtab.Resize(num_syms);
}
for (i = 0; i < function_starts_count; ++i) {
const FunctionStarts::Entry *func_start_entry =
@@ -4762,7 +4745,7 @@ size_t ObjectFileMachO::ParseSymtab() {
// symbols.
if (sym_idx < num_syms) {
num_syms = sym_idx;
sym = symtab->Resize(num_syms);
sym = symtab.Resize(num_syms);
}
// Now synthesize indirect symbols
@@ -4807,11 +4790,11 @@ size_t ObjectFileMachO::ParseSymtab() {
if (index_pos != end_index_pos) {
// We have a remapping from the original nlist index to a
// current symbol index, so just look this up by index
stub_symbol = symtab->SymbolAtIndex(index_pos->second);
stub_symbol = symtab.SymbolAtIndex(index_pos->second);
} else {
// We need to lookup a symbol using the original nlist symbol
// index since this index is coming from the S_SYMBOL_STUBS
stub_symbol = symtab->FindSymbolByID(stub_sym_id);
stub_symbol = symtab.FindSymbolByID(stub_sym_id);
}
if (stub_symbol) {
@@ -4834,7 +4817,7 @@ size_t ObjectFileMachO::ParseSymtab() {
// Make a synthetic symbol to describe the trampoline stub
Mangled stub_symbol_mangled_name(stub_symbol->GetMangled());
if (sym_idx >= num_syms) {
sym = symtab->Resize(++num_syms);
sym = symtab.Resize(++num_syms);
stub_symbol = nullptr; // this pointer no longer valid
}
sym[sym_idx].SetID(synthetic_sym_id++);
@@ -4873,7 +4856,7 @@ size_t ObjectFileMachO::ParseSymtab() {
indirect_symbol_names.end()) {
// Make a synthetic symbol to describe re-exported symbol.
if (sym_idx >= num_syms)
sym = symtab->Resize(++num_syms);
sym = symtab.Resize(++num_syms);
sym[sym_idx].SetID(synthetic_sym_id++);
sym[sym_idx].GetMangled() = Mangled(e.entry.name);
sym[sym_idx].SetType(eSymbolTypeReExported);
@@ -4888,18 +4871,6 @@ size_t ObjectFileMachO::ParseSymtab() {
}
}
}
// StreamFile s(stdout, false);
// s.Printf ("Symbol table before CalculateSymbolSizes():\n");
// symtab->Dump(&s, NULL, eSortOrderNone);
// Set symbol byte sizes correctly since mach-o nlist entries don't have
// sizes
symtab->CalculateSymbolSizes();
// s.Printf ("Symbol table after CalculateSymbolSizes():\n");
// symtab->Dump(&s, NULL, eSortOrderNone);
return symtab->GetNumSymbols();
}
void ObjectFileMachO::Dump(Stream *s) {