After merge, address review comments and remove dead code.

Author: clayborg

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp

@@ -222,20 +222,6 @@ DWARFUnit *DWARFDebugInfo::GetUnitAtOffset(DIERef::Section section,
   return result;
 }
 
-// DWARFUnit *DWARFDebugInfo::GetUnit(const DIERef &die_ref) {
-//   // Make sure we get the correct SymbolFileDWARF from the DIERef before
-//   // asking for information from a debug info object. We might start with the
-//   // DWARFDebugInfo for the main executable in a split DWARF and the DIERef
-//   // might be pointing to a specific .dwo file or to the .dwp file. So this
-//   // makes sure we get the right SymbolFileDWARF instance before finding the
-//   // DWARFUnit that contains the offset. If we just use this object to do the
-//   // search, we might be using the wrong .debug_info section from the wrong
-//   // file with an offset meant for a different section.
-//   SymbolFileDWARF *dwarf = m_dwarf.GetDIERefSymbolFile(die_ref);
-//   return dwarf->DebugInfo().GetUnitContainingDIEOffset(die_ref.section(),
-//                                                        die_ref.die_offset());
-// }
-
 DWARFUnit *
 DWARFDebugInfo::GetUnitContainingDIEOffset(DIERef::Section section,
                                            dw_offset_t die_offset) {
@@ -246,7 +232,7 @@ DWARFDebugInfo::GetUnitContainingDIEOffset(DIERef::Section section,
   return result;
 }
 
-const std::shared_ptr<SymbolFileDWARFDwo> DWARFDebugInfo::GetDwpSymbolFile() {
+const std::shared_ptr<SymbolFileDWARFDwo> &DWARFDebugInfo::GetDwpSymbolFile() {
   return m_dwarf.GetDwpSymbolFile();
 }
 

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h

@@ -52,7 +52,7 @@ class DWARFDebugInfo {
 
   const DWARFDebugAranges &GetCompileUnitAranges();
 
-  const std::shared_ptr<SymbolFileDWARFDwo> GetDwpSymbolFile();
+  const std::shared_ptr<SymbolFileDWARFDwo> &GetDwpSymbolFile();
 
 protected:
   typedef std::vector<DWARFUnitSP> UnitColl;

lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp

@@ -37,7 +37,7 @@ DebugNamesDWARFIndex::Create(Module &module, DWARFDataExtractor debug_names,
 
 
 llvm::DenseSet<uint64_t>
-DebugNamesDWARFIndex::GetTypeUnitSigs(const DebugNames &debug_names) {
+DebugNamesDWARFIndex::GetTypeUnitSignatures(const DebugNames &debug_names) {
   llvm::DenseSet<uint64_t> result;
   for (const DebugNames::NameIndex &ni : debug_names) {
     const uint32_t num_tus = ni.getForeignTUCount();

lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h

@@ -71,7 +71,7 @@ class DebugNamesDWARFIndex : public DWARFIndex {
         m_debug_names_data(debug_names_data), m_debug_str_data(debug_str_data),
         m_debug_names_up(std::move(debug_names_up)),
         m_fallback(module, dwarf, GetUnits(*m_debug_names_up),
-                   GetTypeUnitSigs(*m_debug_names_up)) {}
+                   GetTypeUnitSignatures(*m_debug_names_up)) {}
 
   DWARFDebugInfo &m_debug_info;
 
@@ -87,15 +87,14 @@ class DebugNamesDWARFIndex : public DWARFIndex {
   DWARFUnit *GetNonSkeletonUnit(const DebugNames::Entry &entry) const;
   DWARFDIE GetDIE(const DebugNames::Entry &entry) const;
 
-  // std::optional<DIERef> ToDIERef(const DebugNames::Entry &entry) const;
-
   /// Checks if an entry is a foreign TU and fetch the type unit.
   ///
   /// This function checks if the DebugNames::Entry refers to a foreign TU and
-  /// returns true or false to indicate this. The \a foreign_tu pointer will be
-  /// filled in if this entry matches the type unit's originating .dwo file by
-  /// verifying that the DW_TAG_type_unit DIE has a DW_AT_dwo_name that matches
-  /// the DWO name from the originating skeleton compile unit.
+  /// returns an optional with a value of the \a entry is a foreign type unit
+  /// entry. A valid pointer will be returned if this entry is from a .dwo file
+  /// or if it is from a .dwp file and it matches the type unit's originating
+  /// .dwo file by verifying that the DW_TAG_type_unit DIE has a DW_AT_dwo_name
+  /// that matches the DWO name from the originating skeleton compile unit.
   ///
   /// \param[in] entry
   ///   The accelerator table entry to check.
@@ -112,9 +111,6 @@ class DebugNamesDWARFIndex : public DWARFIndex {
   std::optional<DWARFTypeUnit *>
   IsForeignTypeUnit(const DebugNames::Entry &entry) const;
 
-  DWARFTypeUnit *GetForeignTypeUnit(const DebugNames::Entry &entry) const;
-
-  std::optional<DIERef> ToDIERef(const DebugNames::Entry &entry) const;
   bool ProcessEntry(const DebugNames::Entry &entry,
                     llvm::function_ref<bool(DWARFDIE die)> callback);
 
@@ -127,7 +123,7 @@ class DebugNamesDWARFIndex : public DWARFIndex {
                                   llvm::StringRef name);
 
   static llvm::DenseSet<dw_offset_t> GetUnits(const DebugNames &debug_names);
-  static llvm::DenseSet<uint64_t> GetTypeUnitSigs(const DebugNames &debug_names);
+  static llvm::DenseSet<uint64_t> GetTypeUnitSignatures(const DebugNames &debug_names);
 };
 
 } // namespace dwarf

lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

@@ -61,7 +61,7 @@ void ManualDWARFIndex::Index() {
   if (dwp_info && dwp_info->ContainsTypeUnits()) {
     for (size_t U = 0; U < dwp_info->GetNumUnits(); ++U) {
       if (auto *tu = llvm::dyn_cast<DWARFTypeUnit>(dwp_info->GetUnitAtIndex(U))) {
-        if (m_type_sigs_to_avoid.count(tu->GetTypeHash()) == 0)
+        if (!m_type_sigs_to_avoid.contains(tu->GetTypeHash()))
           units_to_index.push_back(tu);
       }
     }

lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h

@@ -25,7 +25,7 @@ class ManualDWARFIndex : public DWARFIndex {
                    llvm::DenseSet<uint64_t> type_sigs_to_avoid = {})
       : DWARFIndex(module), m_dwarf(&dwarf),
         m_units_to_avoid(std::move(units_to_avoid)),
-        m_type_sigs_to_avoid(type_sigs_to_avoid) {}
+        m_type_sigs_to_avoid(std::move(type_sigs_to_avoid)) {}
 
   void Preload() override { Index(); }
 

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

@@ -693,6 +693,7 @@ llvm::DWARFDebugAbbrev *SymbolFileDWARF::DebugAbbrev() {
   if (debug_abbrev_data.GetByteSize() == 0)
     return nullptr;
 
+  ElapsedTime elapsed(m_parse_time);
   auto abbr =
       std::make_unique<llvm::DWARFDebugAbbrev>(debug_abbrev_data.GetAsLLVM());
   llvm::Error error = abbr->parse();
@@ -2735,6 +2736,7 @@ void SymbolFileDWARF::FindTypes(const TypeQuery &query, TypeResults &results) {
       die_context = die.GetDeclContext();
     else
       die_context = die.GetTypeLookupContext();
+    assert(!die_context.empty());
     if (!query.ContextMatches(die_context))
       return true; // Keep iterating over index types, context mismatch.
 

lldb/test/Shell/SymbolFile/DWARF/x86/dwp-foreign-type-units.cpp

@@ -2,19 +2,19 @@
 
 // This test will make a type that will be compiled differently into two
 // different .dwo files in a type unit with the same type hash, but with
-// differing contents. I have discovered that the hash for the type unit is
-// simply based off of the typename and doesn't seem to differ when the contents
-// differ, so that will help us test foreign type units in the .debug_names
-// section of the main executable. When a DWP file is made, only one type unit
-// will be kept and the type unit that is kept has the .dwo file name that it
-// came from. When LLDB loads the foreign type units, it needs to verify that
-// any entries from foreign type units come from the right .dwo file. We test
-// this since the contents of type units are not always the same even though
-// they have the same type hash. We don't want invalid accelerator table entries
-// to come from one .dwo file and be used on a type unit from another since this
-// could cause invalid lookups to happen. LLDB knows how to track down which
-// .dwo file a type unit comes from by looking at the DW_AT_dwo_name attribute
-// in the DW_TAG_type_unit.
+// differing contents. Clang's type unit signature is based only on the mangled
+// name of the type, regardless of the contents of the type, so that will help
+// us test foreign type units in the .debug_names section of the main
+// executable. When a DWP file is made, only one type unit will be kept and the
+// type unit that is kept has the .dwo file name that it came from. When LLDB
+// loads the foreign type units, it needs to verify that any entries from
+// foreign type units come from the right .dwo file. We test this since the
+// contents of type units are not always the same even though they have the same
+// type hash. We don't want invalid accelerator table entries to come from one
+// .dwo file and be used on a type unit from another since this could cause
+// invalid lookups to happen. LLDB knows how to track down which .dwo file a
+// type unit comes from by looking at the DW_AT_dwo_name attribute in the
+// DW_TAG_type_unit.
 
 // Now test with DWARF5
 // RUN: %clang -target x86_64-pc-linux -gdwarf-5 -gsplit-dwarf \