https://github.com/labath updated https://github.com/llvm/llvm-project/pull/127800
>From b82a233736efc391aaa408513cbea257871ab7b5 Mon Sep 17 00:00:00 2001 From: Pavel Labath <pa...@labath.sk> Date: Wed, 19 Feb 2025 15:03:40 +0100 Subject: [PATCH] [lldb] Renaissance LineTable sequences LineSeqeunce is basically a vector, except that users aren't supposed to know that. This implements the same concept in a slightly simpler fashion. --- lldb/include/lldb/Symbol/LineTable.h | 89 ++++++++----------- .../Breakpad/SymbolFileBreakpad.cpp | 12 ++- .../SymbolFile/DWARF/SymbolFileDWARF.cpp | 7 +- .../NativePDB/SymbolFileNativePDB.cpp | 11 ++- .../Plugins/SymbolFile/PDB/SymbolFilePDB.cpp | 26 +++--- lldb/source/Symbol/LineTable.cpp | 74 ++++++--------- lldb/unittests/Symbol/LineTableTest.cpp | 19 ++-- 7 files changed, 95 insertions(+), 143 deletions(-) diff --git a/lldb/include/lldb/Symbol/LineTable.h b/lldb/include/lldb/Symbol/LineTable.h index f66081b6ee110..2ce5d8cee6e95 100644 --- a/lldb/include/lldb/Symbol/LineTable.h +++ b/lldb/include/lldb/Symbol/LineTable.h @@ -20,25 +20,11 @@ namespace lldb_private { -/// \class LineSequence LineTable.h "lldb/Symbol/LineTable.h" An abstract base -/// class used during symbol table creation. -class LineSequence { -public: - LineSequence(); - - virtual ~LineSequence() = default; - - virtual void Clear() = 0; - -private: - LineSequence(const LineSequence &) = delete; - const LineSequence &operator=(const LineSequence &) = delete; -}; - /// \class LineTable LineTable.h "lldb/Symbol/LineTable.h" /// A line table class. class LineTable { public: + class Sequence; /// Construct with compile unit. /// /// \param[in] comp_unit @@ -49,8 +35,7 @@ class LineTable { /// /// \param[in] sequences /// Unsorted list of line sequences. - LineTable(CompileUnit *comp_unit, - std::vector<std::unique_ptr<LineSequence>> &&sequences); + LineTable(CompileUnit *comp_unit, std::vector<Sequence> &&sequences); /// Destructor. ~LineTable(); @@ -73,20 +58,17 @@ class LineTable { bool is_start_of_basic_block, bool is_prologue_end, bool is_epilogue_begin, bool is_terminal_entry); - // Used to instantiate the LineSequence helper class - static std::unique_ptr<LineSequence> CreateLineSequenceContainer(); - // Append an entry to a caller-provided collection that will later be // inserted in this line table. - static void AppendLineEntryToSequence(LineSequence *sequence, lldb::addr_t file_addr, - uint32_t line, uint16_t column, - uint16_t file_idx, bool is_start_of_statement, - bool is_start_of_basic_block, - bool is_prologue_end, bool is_epilogue_begin, - bool is_terminal_entry); + static void + AppendLineEntryToSequence(Sequence &sequence, lldb::addr_t file_addr, + uint32_t line, uint16_t column, uint16_t file_idx, + bool is_start_of_statement, + bool is_start_of_basic_block, bool is_prologue_end, + bool is_epilogue_begin, bool is_terminal_entry); // Insert a sequence of entries into this line table. - void InsertSequence(LineSequence *sequence); + void InsertSequence(Sequence sequence); /// Dump all line entries in this line table to the stream \a s. /// @@ -273,17 +255,6 @@ class LineTable { return 0; } - class LessThanBinaryPredicate { - public: - LessThanBinaryPredicate(LineTable *line_table); - bool operator()(const LineTable::Entry &, const LineTable::Entry &) const; - bool operator()(const std::unique_ptr<LineSequence> &, - const std::unique_ptr<LineSequence> &) const; - - protected: - LineTable *m_line_table; - }; - static bool EntryAddressLessThan(const Entry &lhs, const Entry &rhs) { return lhs.file_addr < rhs.file_addr; } @@ -315,6 +286,35 @@ class LineTable { uint16_t file_idx = 0; }; + class Sequence { + public: + Sequence() = default; + // Moving clears moved-from object so it can be used anew. Copying is + // generally an error. C++ doesn't guarantee that a moved-from vector is + // empty(), so we clear it explicitly. + Sequence(Sequence &&rhs) : m_entries(std::exchange(rhs.m_entries, {})) {} + Sequence &operator=(Sequence &&rhs) { + m_entries = std::exchange(rhs.m_entries, {}); + return *this; + } + Sequence(const Sequence &) = delete; + Sequence &operator=(const Sequence &) = delete; + + private: + std::vector<Entry> m_entries; + friend class LineTable; + }; + + class LessThanBinaryPredicate { + public: + LessThanBinaryPredicate(LineTable *line_table) : m_line_table(line_table) {} + bool operator()(const LineTable::Entry &, const LineTable::Entry &) const; + bool operator()(const Sequence &, const Sequence &) const; + + protected: + LineTable *m_line_table; + }; + protected: struct EntrySearchInfo { LineTable *line_table; @@ -333,19 +333,6 @@ class LineTable { entry_collection m_entries; ///< The collection of line entries in this line table. - // Helper class - class LineSequenceImpl : public LineSequence { - public: - LineSequenceImpl() = default; - - ~LineSequenceImpl() override = default; - - void Clear() override; - - entry_collection - m_entries; ///< The collection of line entries in this sequence. - }; - bool ConvertEntryAtIndexToLineEntry(uint32_t idx, LineEntry &line_entry); private: diff --git a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp index c7229568e1a0c..dee5a7ce2876d 100644 --- a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp +++ b/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp @@ -837,18 +837,16 @@ void SymbolFileBreakpad::ParseLineTableAndSupportFiles(CompileUnit &cu, "How did we create compile units without a base address?"); SupportFileMap map; - std::vector<std::unique_ptr<LineSequence>> sequences; - std::unique_ptr<LineSequence> line_seq_up = - LineTable::CreateLineSequenceContainer(); + std::vector<LineTable::Sequence> sequences; + LineTable::Sequence sequence; std::optional<addr_t> next_addr; auto finish_sequence = [&]() { LineTable::AppendLineEntryToSequence( - line_seq_up.get(), *next_addr, /*line=*/0, /*column=*/0, + sequence, *next_addr, /*line=*/0, /*column=*/0, /*file_idx=*/0, /*is_start_of_statement=*/false, /*is_start_of_basic_block=*/false, /*is_prologue_end=*/false, /*is_epilogue_begin=*/false, /*is_terminal_entry=*/true); - sequences.push_back(std::move(line_seq_up)); - line_seq_up = LineTable::CreateLineSequenceContainer(); + sequences.push_back(std::move(sequence)); }; LineIterator It(*m_objfile_sp, Record::Func, data.bookmark), @@ -870,7 +868,7 @@ void SymbolFileBreakpad::ParseLineTableAndSupportFiles(CompileUnit &cu, finish_sequence(); } LineTable::AppendLineEntryToSequence( - line_seq_up.get(), record->Address, record->LineNum, /*column=*/0, + sequence, record->Address, record->LineNum, /*column=*/0, map[record->FileNum], /*is_start_of_statement=*/true, /*is_start_of_basic_block=*/false, /*is_prologue_end=*/false, /*is_epilogue_begin=*/false, /*is_terminal_entry=*/false); diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp index a96757afabddf..58b544a9a137b 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp @@ -1232,7 +1232,7 @@ bool SymbolFileDWARF::ParseLineTable(CompileUnit &comp_unit) { // FIXME: Rather than parsing the whole line table and then copying it over // into LLDB, we should explore using a callback to populate the line table // while we parse to reduce memory usage. - std::vector<std::unique_ptr<LineSequence>> sequences; + std::vector<LineTable::Sequence> sequences; // The Sequences view contains only valid line sequences. Don't iterate over // the Rows directly. for (const llvm::DWARFDebugLine::Sequence &seq : line_table->Sequences) { @@ -1242,12 +1242,11 @@ bool SymbolFileDWARF::ParseLineTable(CompileUnit &comp_unit) { // m_first_code_address declaration for more details on this. if (seq.LowPC < m_first_code_address) continue; - std::unique_ptr<LineSequence> sequence = - LineTable::CreateLineSequenceContainer(); + LineTable::Sequence sequence; for (unsigned idx = seq.FirstRowIndex; idx < seq.LastRowIndex; ++idx) { const llvm::DWARFDebugLine::Row &row = line_table->Rows[idx]; LineTable::AppendLineEntryToSequence( - sequence.get(), row.Address.Address, row.Line, row.Column, row.File, + sequence, row.Address.Address, row.Line, row.Column, row.File, row.IsStmt, row.BasicBlock, row.PrologueEnd, row.EpilogueBegin, row.EndSequence); } diff --git a/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp b/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp index 6338f12402b73..4e472d0a0b0f2 100644 --- a/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp +++ b/lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp @@ -1310,18 +1310,17 @@ bool SymbolFileNativePDB::ParseLineTable(CompileUnit &comp_unit) { cii->m_global_line_table.Clear(); // Add line entries in line_set to line_table. - auto line_table = std::make_unique<LineTable>(&comp_unit); - std::unique_ptr<LineSequence> sequence( - line_table->CreateLineSequenceContainer()); + std::vector<LineTable::Sequence> sequence(1); for (const auto &line_entry : line_set) { - line_table->AppendLineEntryToSequence( - sequence.get(), line_entry.file_addr, line_entry.line, + LineTable::AppendLineEntryToSequence( + sequence.back(), line_entry.file_addr, line_entry.line, line_entry.column, line_entry.file_idx, line_entry.is_start_of_statement, line_entry.is_start_of_basic_block, line_entry.is_prologue_end, line_entry.is_epilogue_begin, line_entry.is_terminal_entry); } - line_table->InsertSequence(sequence.get()); + auto line_table = + std::make_unique<LineTable>(&comp_unit, std::move(sequence)); if (line_table->GetSize() == 0) return false; diff --git a/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp b/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp index 293be12ee6333..352163ceaae9e 100644 --- a/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp +++ b/lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp @@ -1761,11 +1761,10 @@ bool SymbolFilePDB::ParseCompileUnitLineTable(CompileUnit &comp_unit, if (!files) return false; - // For each source and header file, create a LineSequence for contributions - // to the compiland from that file, and add the sequence. + // For each source and header file, create a LineTable::Sequence for + // contributions to the compiland from that file, and add the sequence. while (auto file = files->getNext()) { - std::unique_ptr<LineSequence> sequence( - line_table->CreateLineSequenceContainer()); + LineTable::Sequence sequence; auto lines = m_session_up->findLineNumbers(*compiland_up, *file); if (!lines) continue; @@ -1794,12 +1793,11 @@ bool SymbolFilePDB::ParseCompileUnitLineTable(CompileUnit &comp_unit, // of the previous entry's address range if the current entry resulted in // a gap from the previous entry. if (is_gap && ShouldAddLine(match_line, prev_line, prev_length)) { - line_table->AppendLineEntryToSequence( - sequence.get(), prev_addr + prev_length, prev_line, 0, - prev_source_idx, false, false, false, false, true); + line_table->AppendLineEntryToSequence(sequence, prev_addr + prev_length, + prev_line, 0, prev_source_idx, + false, false, false, false, true); - line_table->InsertSequence(sequence.get()); - sequence = line_table->CreateLineSequenceContainer(); + line_table->InsertSequence(std::move(sequence)); } if (ShouldAddLine(match_line, lno, length)) { @@ -1818,7 +1816,7 @@ bool SymbolFilePDB::ParseCompileUnitLineTable(CompileUnit &comp_unit, is_epilogue = (addr == epilogue->getVirtualAddress()); } - line_table->AppendLineEntryToSequence(sequence.get(), addr, lno, col, + line_table->AppendLineEntryToSequence(sequence, addr, lno, col, source_idx, is_statement, false, is_prologue, is_epilogue, false); } @@ -1831,12 +1829,12 @@ bool SymbolFilePDB::ParseCompileUnitLineTable(CompileUnit &comp_unit, if (entry_count > 0 && ShouldAddLine(match_line, prev_line, prev_length)) { // The end is always a terminal entry, so insert it regardless. - line_table->AppendLineEntryToSequence( - sequence.get(), prev_addr + prev_length, prev_line, 0, - prev_source_idx, false, false, false, false, true); + line_table->AppendLineEntryToSequence(sequence, prev_addr + prev_length, + prev_line, 0, prev_source_idx, + false, false, false, false, true); } - line_table->InsertSequence(sequence.get()); + line_table->InsertSequence(std::move(sequence)); } if (line_table->GetSize()) { diff --git a/lldb/source/Symbol/LineTable.cpp b/lldb/source/Symbol/LineTable.cpp index aae4ab59ff156..25ef8ed79d138 100644 --- a/lldb/source/Symbol/LineTable.cpp +++ b/lldb/source/Symbol/LineTable.cpp @@ -21,15 +21,13 @@ using namespace lldb_private; LineTable::LineTable(CompileUnit *comp_unit) : m_comp_unit(comp_unit), m_entries() {} -LineTable::LineTable(CompileUnit *comp_unit, - std::vector<std::unique_ptr<LineSequence>> &&sequences) +LineTable::LineTable(CompileUnit *comp_unit, std::vector<Sequence> &&sequences) : m_comp_unit(comp_unit), m_entries() { - LineTable::Entry::LessThanBinaryPredicate less_than_bp(this); + LessThanBinaryPredicate less_than_bp(this); llvm::stable_sort(sequences, less_than_bp); - for (const auto &sequence : sequences) { - LineSequenceImpl *seq = static_cast<LineSequenceImpl *>(sequence.get()); - m_entries.insert(m_entries.end(), seq->m_entries.begin(), - seq->m_entries.end()); + for (const Sequence &seq : sequences) { + m_entries.insert(m_entries.end(), seq.m_entries.begin(), + seq.m_entries.end()); } } @@ -46,7 +44,7 @@ void LineTable::InsertLineEntry(lldb::addr_t file_addr, uint32_t line, is_start_of_basic_block, is_prologue_end, is_epilogue_begin, is_terminal_entry); - LineTable::Entry::LessThanBinaryPredicate less_than_bp(this); + LessThanBinaryPredicate less_than_bp(this); entry_collection::iterator pos = llvm::upper_bound(m_entries, entry, less_than_bp); @@ -58,25 +56,14 @@ void LineTable::InsertLineEntry(lldb::addr_t file_addr, uint32_t line, // Dump (&s, Address::DumpStyleFileAddress); } -LineSequence::LineSequence() = default; - -void LineTable::LineSequenceImpl::Clear() { m_entries.clear(); } - -std::unique_ptr<LineSequence> LineTable::CreateLineSequenceContainer() { - return std::make_unique<LineTable::LineSequenceImpl>(); -} - void LineTable::AppendLineEntryToSequence( - LineSequence *sequence, lldb::addr_t file_addr, uint32_t line, - uint16_t column, uint16_t file_idx, bool is_start_of_statement, - bool is_start_of_basic_block, bool is_prologue_end, bool is_epilogue_begin, - bool is_terminal_entry) { - assert(sequence != nullptr); - LineSequenceImpl *seq = reinterpret_cast<LineSequenceImpl *>(sequence); + Sequence &sequence, lldb::addr_t file_addr, uint32_t line, uint16_t column, + uint16_t file_idx, bool is_start_of_statement, bool is_start_of_basic_block, + bool is_prologue_end, bool is_epilogue_begin, bool is_terminal_entry) { Entry entry(file_addr, line, column, file_idx, is_start_of_statement, is_start_of_basic_block, is_prologue_end, is_epilogue_begin, is_terminal_entry); - entry_collection &entries = seq->m_entries; + entry_collection &entries = sequence.m_entries; // Replace the last entry if the address is the same, otherwise append it. If // we have multiple line entries at the same address, this indicates illegal // DWARF so this "fixes" the line table to be correct. If not fixed this can @@ -102,26 +89,24 @@ void LineTable::AppendLineEntryToSequence( entries.push_back(entry); } -void LineTable::InsertSequence(LineSequence *sequence) { - assert(sequence != nullptr); - LineSequenceImpl *seq = reinterpret_cast<LineSequenceImpl *>(sequence); - if (seq->m_entries.empty()) +void LineTable::InsertSequence(Sequence sequence) { + if (sequence.m_entries.empty()) return; - Entry &entry = seq->m_entries.front(); + const Entry &entry = sequence.m_entries.front(); // If the first entry address in this sequence is greater than or equal to // the address of the last item in our entry collection, just append. if (m_entries.empty() || !Entry::EntryAddressLessThan(entry, m_entries.back())) { - m_entries.insert(m_entries.end(), seq->m_entries.begin(), - seq->m_entries.end()); + m_entries.insert(m_entries.end(), sequence.m_entries.begin(), + sequence.m_entries.end()); return; } // Otherwise, find where this belongs in the collection entry_collection::iterator begin_pos = m_entries.begin(); entry_collection::iterator end_pos = m_entries.end(); - LineTable::Entry::LessThanBinaryPredicate less_than_bp(this); + LessThanBinaryPredicate less_than_bp(this); entry_collection::iterator pos = std::upper_bound(begin_pos, end_pos, entry, less_than_bp); @@ -139,15 +124,11 @@ void LineTable::InsertSequence(LineSequence *sequence) { assert(prev_pos->is_terminal_entry); } #endif - m_entries.insert(pos, seq->m_entries.begin(), seq->m_entries.end()); + m_entries.insert(pos, sequence.m_entries.begin(), sequence.m_entries.end()); } -LineTable::Entry::LessThanBinaryPredicate::LessThanBinaryPredicate( - LineTable *line_table) - : m_line_table(line_table) {} - -bool LineTable::Entry::LessThanBinaryPredicate:: -operator()(const LineTable::Entry &a, const LineTable::Entry &b) const { +bool LineTable::LessThanBinaryPredicate::operator()(const Entry &a, + const Entry &b) const { #define LT_COMPARE(a, b) \ if (a != b) \ return a < b @@ -166,12 +147,9 @@ operator()(const LineTable::Entry &a, const LineTable::Entry &b) const { #undef LT_COMPARE } -bool LineTable::Entry::LessThanBinaryPredicate:: -operator()(const std::unique_ptr<LineSequence> &sequence_a, - const std::unique_ptr<LineSequence> &sequence_b) const { - auto *seq_a = static_cast<const LineSequenceImpl *>(sequence_a.get()); - auto *seq_b = static_cast<const LineSequenceImpl *>(sequence_b.get()); - return (*this)(seq_a->m_entries.front(), seq_b->m_entries.front()); +bool LineTable::LessThanBinaryPredicate::operator()( + const Sequence &seq_a, const Sequence &seq_b) const { + return (*this)(seq_a.m_entries.front(), seq_b.m_entries.front()); } uint32_t LineTable::GetSize() const { return m_entries.size(); } @@ -447,7 +425,7 @@ size_t LineTable::GetContiguousFileAddressRanges(FileAddressRanges &file_ranges, LineTable *LineTable::LinkLineTable(const FileRangeMap &file_range_map) { std::unique_ptr<LineTable> line_table_up(new LineTable(m_comp_unit)); - LineSequenceImpl sequence; + Sequence sequence; const size_t count = m_entries.size(); LineEntry line_entry; const FileRangeMap::Entry *file_range_entry = nullptr; @@ -509,8 +487,7 @@ LineTable *LineTable::LinkLineTable(const FileRangeMap &file_range_map) { sequence.m_entries.back().is_terminal_entry = true; // Append the sequence since we just terminated the previous one - line_table_up->InsertSequence(&sequence); - sequence.Clear(); + line_table_up->InsertSequence(std::move(sequence)); } // Now link the current entry @@ -525,8 +502,7 @@ LineTable *LineTable::LinkLineTable(const FileRangeMap &file_range_map) { // insert this sequence into our new line table. if (!sequence.m_entries.empty() && sequence.m_entries.back().is_terminal_entry) { - line_table_up->InsertSequence(&sequence); - sequence.Clear(); + line_table_up->InsertSequence(std::move(sequence)); prev_entry_was_linked = false; } else { prev_entry_was_linked = file_range_entry != nullptr; diff --git a/lldb/unittests/Symbol/LineTableTest.cpp b/lldb/unittests/Symbol/LineTableTest.cpp index 2fa2913f67f9e..7d1bbe24e6a08 100644 --- a/lldb/unittests/Symbol/LineTableTest.cpp +++ b/lldb/unittests/Symbol/LineTableTest.cpp @@ -129,26 +129,21 @@ class LineTableTest : public testing::Test { class LineSequenceBuilder { public: - std::vector<std::unique_ptr<LineSequence>> Build() { - return std::move(m_sequences); - } + std::vector<LineTable::Sequence> Build() { return std::move(m_sequences); } enum Terminal : bool { Terminal = true }; void Entry(addr_t addr, bool terminal = false) { LineTable::AppendLineEntryToSequence( - m_seq_up.get(), addr, /*line=*/1, /*column=*/0, + m_sequence, addr, /*line=*/1, /*column=*/0, /*file_idx=*/0, /*is_start_of_statement=*/false, /*is_start_of_basic_block=*/false, /*is_prologue_end=*/false, /*is_epilogue_begin=*/false, terminal); - if (terminal) { - m_sequences.push_back(std::move(m_seq_up)); - m_seq_up = LineTable::CreateLineSequenceContainer(); - } + if (terminal) + m_sequences.push_back(std::move(m_sequence)); } private: - std::vector<std::unique_ptr<LineSequence>> m_sequences; - std::unique_ptr<LineSequence> m_seq_up = - LineTable::CreateLineSequenceContainer(); + std::vector<LineTable::Sequence> m_sequences; + LineTable::Sequence m_sequence; }; } // namespace @@ -156,7 +151,7 @@ class LineSequenceBuilder { char FakeSymbolFile::ID; static llvm::Expected<FakeModuleFixture> -CreateFakeModule(std::vector<std::unique_ptr<LineSequence>> line_sequences) { +CreateFakeModule(std::vector<LineTable::Sequence> line_sequences) { Expected<TestFile> file = TestFile::fromYaml(R"( --- !ELF FileHeader: _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits