[lldb] Store SupportFile in LineEntry (NFC)

[JDevlieghere]

Store a SupportFile, rather than a FileSpec, in LineEntry. This commit
works towards having the SourceManageroperate on SupportFiles so that it
can (1) validate the Checksum and (2) materialize the content of inline
source information.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

This is a series of 4 NFC commit to store SupportFiles rather than FileSpecs in LineEntry. This is work towards having the SourceManageroperate on SupportFiles so that it can (1) validate the Checksum and (2) materialize the content of inline source information.

I highly recommend looking at the individual commits rather than the unified diff.

---

Full diff: https://github.com/llvm/llvm-project/pull/77999.diff

9 Files Affected:

• (modified) lldb/include/lldb/Symbol/LineEntry.h (+3-7)
• (modified) lldb/include/lldb/Utility/FileSpecList.h (+7-32)
• (added) lldb/include/lldb/Utility/SupportFile.h (+53)
• (modified) lldb/include/lldb/lldb-forward.h (+2)
• (modified) lldb/source/Core/Disassembler.cpp (+3-2)
• (modified) lldb/source/Symbol/LineEntry.cpp (+4-17)
• (modified) lldb/source/Symbol/LineTable.cpp (+2-2)
• (modified) lldb/source/Symbol/SymbolContext.cpp (+2-2)
• (modified) lldb/source/Utility/FileSpecList.cpp (+8-1)

diff --git a/lldb/include/lldb/Symbol/LineEntry.h b/lldb/include/lldb/Symbol/LineEntry.h
index 698d89974dc6346..a6508fe831ca846 100644
--- a/lldb/include/lldb/Symbol/LineEntry.h
+++ b/lldb/include/lldb/Symbol/LineEntry.h
@@ -11,6 +11,7 @@
 
 #include "lldb/Core/AddressRange.h"
 #include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/SupportFile.h"
 #include "lldb/lldb-private.h"
 
 namespace lldb_private {
@@ -23,12 +24,6 @@ struct LineEntry {
   /// Initialize all member variables to invalid values.
   LineEntry();
 
-  LineEntry(const lldb::SectionSP &section_sp, lldb::addr_t section_offset,
-            lldb::addr_t byte_size, const FileSpec &file, uint32_t _line,
-            uint16_t _column, bool _is_start_of_statement,
-            bool _is_start_of_basic_block, bool _is_prologue_end,
-            bool _is_epilogue_begin, bool _is_terminal_entry);
-
   /// Clear the object's state.
   ///
   /// Clears all member variables to invalid values.
@@ -139,7 +134,8 @@ struct LineEntry {
   AddressRange range; ///< The section offset address range for this line entry.
   FileSpec file; ///< The source file, possibly mapped by the target.source-map
                  ///setting
-  FileSpec original_file; ///< The original source file, from debug info.
+  lldb::SupportFileSP
+      original_file; ///< The original source file, from debug info.
   uint32_t line = LLDB_INVALID_LINE_NUMBER; ///< The source line number, or zero
                                             ///< if there is no line number
                                             /// information.
diff --git a/lldb/include/lldb/Utility/FileSpecList.h b/lldb/include/lldb/Utility/FileSpecList.h
index c1107ad1135dd25..49edc667ddd5b62 100644
--- a/lldb/include/lldb/Utility/FileSpecList.h
+++ b/lldb/include/lldb/Utility/FileSpecList.h
@@ -9,8 +9,9 @@
 #ifndef LLDB_CORE_FILESPECLIST_H
 #define LLDB_CORE_FILESPECLIST_H
 
-#include "lldb/Utility/Checksum.h"
 #include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/SupportFile.h"
+#include "lldb/lldb-forward.h"
 
 #include <cstddef>
 #include <vector>
@@ -18,33 +19,6 @@
 namespace lldb_private {
 class Stream;
 
-/// Wraps either a FileSpec that represents a local file or a source
-/// file whose contents is known (for example because it can be
-/// reconstructed from debug info), but that hasn't been written to a
-/// file yet.
-class SupportFile {
-protected:
-  FileSpec m_file_spec;
-  Checksum m_checksum;
-
-public:
-  SupportFile(const FileSpec &spec) : m_file_spec(spec), m_checksum() {}
-  SupportFile(const FileSpec &spec, const Checksum &checksum)
-      : m_file_spec(spec), m_checksum(checksum) {}
-  SupportFile(const SupportFile &other) = delete;
-  SupportFile(SupportFile &&other) = default;
-  virtual ~SupportFile() = default;
-  bool operator==(const SupportFile &other) {
-    return m_file_spec == other.m_file_spec && m_checksum == other.m_checksum;
-  }
-  /// Return the file name only. Useful for resolving breakpoints by file name.
-  const FileSpec &GetSpecOnly() const { return m_file_spec; };
-  /// Return the checksum or all zeros if there is none.
-  const Checksum &GetChecksum() const { return m_checksum; };
-  /// Materialize the file to disk and return the path to that temporary file.
-  virtual const FileSpec &Materialize() { return m_file_spec; }
-};
-
 /// A list of support files for a CompileUnit.
 class SupportFileList {
 public:
@@ -52,21 +26,22 @@ class SupportFileList {
   SupportFileList(const SupportFileList &) = delete;
   SupportFileList(SupportFileList &&other) = default;
 
-  typedef std::vector<std::unique_ptr<SupportFile>> collection;
+  typedef std::vector<std::shared_ptr<SupportFile>> collection;
   typedef collection::const_iterator const_iterator;
   const_iterator begin() const { return m_files.begin(); }
   const_iterator end() const { return m_files.end(); }
 
   void Append(const FileSpec &file) {
-    return Append(std::make_unique<SupportFile>(file));
+    return Append(std::make_shared<SupportFile>(file));
   }
-  void Append(std::unique_ptr<SupportFile> &&file) {
+  void Append(std::shared_ptr<SupportFile> &&file) {
     m_files.push_back(std::move(file));
   }
   // FIXME: Only used by SymbolFilePDB. Replace with a DenseSet at call site.
   bool AppendIfUnique(const FileSpec &file);
   size_t GetSize() const { return m_files.size(); }
   const FileSpec &GetFileSpecAtIndex(size_t idx) const;
+  lldb::SupportFileSP GetSupportFileAtIndex(size_t idx) const;
   size_t FindFileIndex(size_t idx, const FileSpec &file, bool full) const;
   /// Find a compatible file index.
   ///
@@ -96,7 +71,7 @@ class SupportFileList {
 
   template <class... Args> void EmplaceBack(Args &&...args) {
     m_files.push_back(
-        std::make_unique<SupportFile>(std::forward<Args>(args)...));
+        std::make_shared<SupportFile>(std::forward<Args>(args)...));
   }
 
 protected:
diff --git a/lldb/include/lldb/Utility/SupportFile.h b/lldb/include/lldb/Utility/SupportFile.h
new file mode 100644
index 000000000000000..0ae0e111cc3f851
--- /dev/null
+++ b/lldb/include/lldb/Utility/SupportFile.h
@@ -0,0 +1,53 @@
+//===-- SupportFile.h -------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_UTILITY_SUPPORTFILE_H
+#define LLDB_UTILITY_SUPPORTFILE_H
+
+#include "lldb/Utility/Checksum.h"
+#include "lldb/Utility/FileSpec.h"
+
+namespace lldb_private {
+
+/// Wraps either a FileSpec that represents a local file or a source
+/// file whose contents is known (for example because it can be
+/// reconstructed from debug info), but that hasn't been written to a
+/// file yet. This also stores an optional checksum of the on-disk content.
+class SupportFile {
+public:
+  SupportFile() : m_file_spec(), m_checksum() {}
+  SupportFile(const FileSpec &spec) : m_file_spec(spec), m_checksum() {}
+  SupportFile(const FileSpec &spec, const Checksum &checksum)
+      : m_file_spec(spec), m_checksum(checksum) {}
+
+  SupportFile(const SupportFile &other) = delete;
+  SupportFile(SupportFile &&other) = default;
+
+  virtual ~SupportFile() = default;
+
+  bool operator==(const SupportFile &other) {
+    return m_file_spec == other.m_file_spec && m_checksum == other.m_checksum;
+  }
+
+  /// Return the file name only. Useful for resolving breakpoints by file name.
+  const FileSpec &GetSpecOnly() const { return m_file_spec; };
+
+  /// Return the checksum or all zeros if there is none.
+  const Checksum &GetChecksum() const { return m_checksum; };
+
+  /// Materialize the file to disk and return the path to that temporary file.
+  virtual const FileSpec &Materialize() { return m_file_spec; }
+
+protected:
+  FileSpec m_file_spec;
+  Checksum m_checksum;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_UTILITY_SUPPORTFILE_H
diff --git a/lldb/include/lldb/lldb-forward.h b/lldb/include/lldb/lldb-forward.h
index 4e0c62fa26cae44..d89ad21512215f7 100644
--- a/lldb/include/lldb/lldb-forward.h
+++ b/lldb/include/lldb/lldb-forward.h
@@ -212,6 +212,7 @@ class StringList;
 class StringTableReader;
 class StructuredDataImpl;
 class StructuredDataPlugin;
+class SupportFile;
 class Symbol;
 class SymbolContext;
 class SymbolContextList;
@@ -462,6 +463,7 @@ typedef std::shared_ptr<lldb_private::TypeSummaryImpl> TypeSummaryImplSP;
 typedef std::shared_ptr<lldb_private::TypeSummaryOptions> TypeSummaryOptionsSP;
 typedef std::shared_ptr<lldb_private::ScriptedSyntheticChildren>
     ScriptedSyntheticChildrenSP;
+typedef std::shared_ptr<lldb_private::SupportFile> SupportFileSP;
 typedef std::shared_ptr<lldb_private::UnixSignals> UnixSignalsSP;
 typedef std::weak_ptr<lldb_private::UnixSignals> UnixSignalsWP;
 typedef std::shared_ptr<lldb_private::UnwindAssembly> UnwindAssemblySP;
diff --git a/lldb/source/Core/Disassembler.cpp b/lldb/source/Core/Disassembler.cpp
index 166b5fdf22f0b46..260a4f485bff025 100644
--- a/lldb/source/Core/Disassembler.cpp
+++ b/lldb/source/Core/Disassembler.cpp
@@ -202,7 +202,7 @@ Disassembler::GetFunctionDeclLineEntry(const SymbolContext &sc) {
   sc.function->GetStartLineSourceInfo(func_decl_file, func_decl_line);
 
   if (func_decl_file != prologue_end_line.file &&
-      func_decl_file != prologue_end_line.original_file)
+      func_decl_file != prologue_end_line.original_file->GetSpecOnly())
     return {};
 
   SourceLine decl_line;
@@ -407,7 +407,8 @@ void Disassembler::PrintInstructions(Debugger &debugger, const ArchSpec &arch,
                   sc.function->GetStartLineSourceInfo(func_decl_file,
                                                       func_decl_line);
                   if (func_decl_file == prologue_end_line.file ||
-                      func_decl_file == prologue_end_line.original_file) {
+                      func_decl_file ==
+                          prologue_end_line.original_file->GetSpecOnly()) {
                     // Add all the lines between the function declaration and
                     // the first non-prologue source line to the list of lines
                     // to print.
diff --git a/lldb/source/Symbol/LineEntry.cpp b/lldb/source/Symbol/LineEntry.cpp
index 1b2801cd0368358..95ea1e0674295ad 100644
--- a/lldb/source/Symbol/LineEntry.cpp
+++ b/lldb/source/Symbol/LineEntry.cpp
@@ -17,23 +17,10 @@ LineEntry::LineEntry()
     : range(), file(), is_start_of_statement(0), is_start_of_basic_block(0),
       is_prologue_end(0), is_epilogue_begin(0), is_terminal_entry(0) {}
 
-LineEntry::LineEntry(const lldb::SectionSP &section_sp,
-                     lldb::addr_t section_offset, lldb::addr_t byte_size,
-                     const FileSpec &_file, uint32_t _line, uint16_t _column,
-                     bool _is_start_of_statement, bool _is_start_of_basic_block,
-                     bool _is_prologue_end, bool _is_epilogue_begin,
-                     bool _is_terminal_entry)
-    : range(section_sp, section_offset, byte_size), file(_file),
-      original_file(_file), line(_line), column(_column),
-      is_start_of_statement(_is_start_of_statement),
-      is_start_of_basic_block(_is_start_of_basic_block),
-      is_prologue_end(_is_prologue_end), is_epilogue_begin(_is_epilogue_begin),
-      is_terminal_entry(_is_terminal_entry) {}
-
 void LineEntry::Clear() {
   range.Clear();
   file.Clear();
-  original_file.Clear();
+  original_file = std::make_shared<SupportFile>();
   line = LLDB_INVALID_LINE_NUMBER;
   column = 0;
   is_start_of_statement = 0;
@@ -195,7 +182,7 @@ AddressRange LineEntry::GetSameLineContiguousAddressRange(
   // different file / line number.
   AddressRange complete_line_range = range;
   auto symbol_context_scope = lldb::eSymbolContextLineEntry;
-  Declaration start_call_site(original_file, line);
+  Declaration start_call_site(original_file->GetSpecOnly(), line);
   if (include_inlined_functions)
     symbol_context_scope |= lldb::eSymbolContextBlock;
 
@@ -253,8 +240,8 @@ AddressRange LineEntry::GetSameLineContiguousAddressRange(
 void LineEntry::ApplyFileMappings(lldb::TargetSP target_sp) {
   if (target_sp) {
     // Apply any file remappings to our file.
-    if (auto new_file_spec =
-            target_sp->GetSourcePathMap().FindFile(original_file))
+    if (auto new_file_spec = target_sp->GetSourcePathMap().FindFile(
+            original_file->GetSpecOnly()))
       file = *new_file_spec;
   }
 }
diff --git a/lldb/source/Symbol/LineTable.cpp b/lldb/source/Symbol/LineTable.cpp
index abe4c98d5928789..f6918171415ad5b 100644
--- a/lldb/source/Symbol/LineTable.cpp
+++ b/lldb/source/Symbol/LineTable.cpp
@@ -291,7 +291,7 @@ bool LineTable::ConvertEntryAtIndexToLineEntry(uint32_t idx,
   line_entry.file =
       m_comp_unit->GetSupportFiles().GetFileSpecAtIndex(entry.file_idx);
   line_entry.original_file =
-      m_comp_unit->GetSupportFiles().GetFileSpecAtIndex(entry.file_idx);
+      m_comp_unit->GetSupportFiles().GetSupportFileAtIndex(entry.file_idx);
   line_entry.line = entry.line;
   line_entry.column = entry.column;
   line_entry.is_start_of_statement = entry.is_start_of_statement;
@@ -357,7 +357,7 @@ void LineTable::Dump(Stream *s, Target *target, Address::DumpStyle style,
                      Address::DumpStyle fallback_style, bool show_line_ranges) {
   const size_t count = m_entries.size();
   LineEntry line_entry;
-  FileSpec prev_file;
+  SupportFileSP prev_file;
   for (size_t idx = 0; idx < count; ++idx) {
     ConvertEntryAtIndexToLineEntry(idx, line_entry);
     line_entry.Dump(s, target, prev_file != line_entry.original_file, style,
diff --git a/lldb/source/Symbol/SymbolContext.cpp b/lldb/source/Symbol/SymbolContext.cpp
index 9fd40b5ca567f87..0a0cab7d407924b 100644
--- a/lldb/source/Symbol/SymbolContext.cpp
+++ b/lldb/source/Symbol/SymbolContext.cpp
@@ -489,8 +489,8 @@ bool SymbolContext::GetParentOfInlinedScope(const Address &curr_frame_pc,
         next_frame_sc.line_entry.range.GetBaseAddress() = next_frame_pc;
         next_frame_sc.line_entry.file =
             curr_inlined_block_inlined_info->GetCallSite().GetFile();
-        next_frame_sc.line_entry.original_file =
-            curr_inlined_block_inlined_info->GetCallSite().GetFile();
+        next_frame_sc.line_entry.original_file = std::make_shared<SupportFile>(
+            curr_inlined_block_inlined_info->GetCallSite().GetFile());
         next_frame_sc.line_entry.line =
             curr_inlined_block_inlined_info->GetCallSite().GetLine();
         next_frame_sc.line_entry.column =
diff --git a/lldb/source/Utility/FileSpecList.cpp b/lldb/source/Utility/FileSpecList.cpp
index 8d2cf81efe5b13f..7647e04a8204516 100644
--- a/lldb/source/Utility/FileSpecList.cpp
+++ b/lldb/source/Utility/FileSpecList.cpp
@@ -41,7 +41,7 @@ bool FileSpecList::AppendIfUnique(const FileSpec &file_spec) {
 bool SupportFileList::AppendIfUnique(const FileSpec &file_spec) {
   collection::iterator end = m_files.end();
   if (find_if(m_files.begin(), end,
-              [&](const std::unique_ptr<SupportFile> &support_file) {
+              [&](const std::shared_ptr<SupportFile> &support_file) {
                 return support_file->GetSpecOnly() == file_spec;
               }) == end) {
     Append(file_spec);
@@ -176,6 +176,13 @@ const FileSpec &SupportFileList::GetFileSpecAtIndex(size_t idx) const {
   return g_empty_file_spec;
 }
 
+std::shared_ptr<SupportFile>
+SupportFileList::GetSupportFileAtIndex(size_t idx) const {
+  if (idx < m_files.size())
+    return m_files[idx];
+  return {};
+}
+
 // Return the size in bytes that this object takes in memory. This returns the
 // size in bytes of this object's member variables and any FileSpec objects its
 // member variables contain, the result doesn't not include the string values

[adrian-prantl]

Is this default-constructing an empty object? Should it be a null shared_ptr?

[JDevlieghere]

Correct, this ensures I can always assume that the shared pointer is valid. The underlying FileSpec might be "empty" which matches the existing semantics.

[medismailben]

LGTM with nits

[medismailben]

nit: original_file_sp

[medismailben]

ditto