DWARF: Avoid storing DIERefs in long-lived containers

[labath]

Summary

A user_id_t carries the same information as a DIERef, but it takes up
less space.

Furthermore, DIERef::operator<'s implementation is very
questionable, as it does not take the cu_offset and section fields into
account. Using just the die offset was correct it the days when all
debug info lived in a single section, but since we started supporting
DWO debug info, this was no longer true. The comparison operator could
be fixed, but it seems like using the user_id_t for these purposes is a
better idea overall.

I think this did not cause any bugs, because the only place the
comparison operator was used is in m_function_scope_qualified_name_map,
and this one is local to a dwo file, but I am not 100% sure of that.

[JDevlieghere]

LGTM

[aprantl]

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

2970 ↗

(On Diff #204717)

If you find the time:

if (!die)
  return {};
auto *type_system = GetTypeSystemForLanguage(die.GetCU()->GetLanguageType());
if (!type_system)
  return {};
 DWARFASTParser *dwarf_ast = type_system->GetDWARFParser();
if (!dwarf_ast)
  return {};

[labath]

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

2970 ↗

(On Diff #204717)

Sure.

lldb/trunk/source/Plugins/SymbolFile/DWARF/DIERef.h

struct DIERef {
 
   DIERef() = default;
 
   DIERef(Section s, dw_offset_t c, dw_offset_t d)
       : section(s), cu_offset(c), die_offset(d) {}
 
   explicit DIERef(const DWARFFormValue &form_value);
 
-  bool operator<(const DIERef &ref) const {
-    return die_offset < ref.die_offset;
-  }
-
   explicit operator bool() const {
     return cu_offset != DW_INVALID_OFFSET || die_offset != DW_INVALID_OFFSET;
   }
 
   Section section = Section::DebugInfo;
   dw_offset_t cu_offset = DW_INVALID_OFFSET;
   dw_offset_t die_offset = DW_INVALID_OFFSET;
 };
 
 typedef std::vector<DIERef> DIEArray;
 
 #endif // SymbolFileDWARF_DIERef_h_

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

if (!attrs.is_forward_declaration) {
                "Type already in the forward declaration map!");
         // Can't assume m_ast.GetSymbolFile() is actually a
         // SymbolFileDWARF, it can be a SymbolFileDWARFDebugMap for Apple
         // binaries.
         dwarf->GetForwardDeclDieToClangType()[die.GetDIE()] =
             clang_type.GetOpaqueQualType();
         dwarf->GetForwardDeclClangTypeToDie()
             [ClangUtil::RemoveFastQualifiers(clang_type).GetOpaqueQualType()] =
-            die.GetDIERef();
+            die.GetID();
         m_ast.SetHasExternalStorage(clang_type.GetOpaqueQualType(), true);
       }
     }
 
     // If we made a clang type, set the trivial abi if applicable: We only
     // do this for pass by value - which implies the Trivial ABI. There
     // isn't a way to assert that something that would normally be pass by
     // value is pass by reference, so we ignore that attribute if set.

lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

 protected:
   typedef llvm::DenseMap<const DWARFDebugInfoEntry *, lldb_private::Type *>
       DIEToTypePtr;
   typedef llvm::DenseMap<const DWARFDebugInfoEntry *, lldb::VariableSP>
       DIEToVariableSP;
   typedef llvm::DenseMap<const DWARFDebugInfoEntry *,
                          lldb::opaque_compiler_type_t>
       DIEToClangType;
-  typedef llvm::DenseMap<lldb::opaque_compiler_type_t, DIERef> ClangTypeToDIE;
+  typedef llvm::DenseMap<lldb::opaque_compiler_type_t, lldb::user_id_t>
+      ClangTypeToDIE;
 
   struct DWARFDataSegment {
     llvm::once_flag m_flag;
     lldb_private::DWARFDataExtractor m_data;
   };
 
   DISALLOW_COPY_AND_ASSIGN(SymbolFileDWARF);
 
typedef std::unordered_map<lldb::offset_t, lldb_private::DebugMacrosSP>
       DebugMacrosMap;
   DebugMacrosMap m_debug_macros_map;
 
   ExternalTypeModuleMap m_external_type_modules;
   std::unique_ptr<lldb_private::DWARFIndex> m_index;
   bool m_fetched_external_modules : 1;
   lldb_private::LazyBool m_supports_DW_AT_APPLE_objc_complete_type;
 
-  typedef std::shared_ptr<std::set<DIERef>> DIERefSetSP;
-  typedef std::unordered_map<std::string, DIERefSetSP> NameToOffsetMap;
+  typedef std::set<lldb::user_id_t> DIERefSet;
+  typedef llvm::StringMap<DIERefSet> NameToOffsetMap;
   NameToOffsetMap m_function_scope_qualified_name_map;
   std::unique_ptr<DWARFDebugRangesBase> m_ranges;
   std::unique_ptr<DWARFDebugRangesBase> m_rnglists;
   UniqueDWARFASTTypeMap m_unique_ast_type_map;
   DIEToTypePtr m_die_to_type;
   DIEToVariableSP m_die_to_variable_sp;
   DIEToClangType m_forward_decl_die_to_clang_type;
   ClangTypeToDIE m_forward_decl_clang_type_to_die;
   llvm::DenseMap<dw_offset_t, lldb_private::FileSpecList>
       m_type_unit_support_files;
   std::vector<uint32_t> m_lldb_cu_to_dwarf_unit;
 };
 
 #endif // SymbolFileDWARF_SymbolFileDWARF_h_

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

for (uint32_t i = 0; i < num_comp_units; i++) {
     if (cu == nullptr)
       continue;
 
     SymbolFileDWARFDwo *dwo = cu->GetDwoSymbolFile();
     if (dwo)
       dwo->GetMangledNamesForFunction(scope_qualified_name, mangled_names);
   }
 
-  NameToOffsetMap::iterator iter =
-      m_function_scope_qualified_name_map.find(scope_qualified_name);
-  if (iter == m_function_scope_qualified_name_map.end())
-    return;
-
-  DIERefSetSP set_sp = (*iter).second;
-  std::set<DIERef>::iterator set_iter;
-  for (set_iter = set_sp->begin(); set_iter != set_sp->end(); set_iter++) {
-    DWARFDIE die = DebugInfo()->GetDIE(*set_iter);
+  for (lldb::user_id_t uid :
+       m_function_scope_qualified_name_map.lookup(scope_qualified_name)) {
+    DWARFDIE die = GetDIE(uid);
     mangled_names.push_back(ConstString(die.GetMangledName()));
   }
 }
 
 uint32_t SymbolFileDWARF::FindTypes(
     ConstString name, const CompilerDeclContext *parent_decl_ctx,
     bool append, uint32_t max_matches,
     llvm::DenseSet<lldb_private::SymbolFile *> &searched_symbol_files,
if (type_name) {
       }
     }
   }
   return type_sp;
 }
 
 TypeSP SymbolFileDWARF::ParseType(const SymbolContext &sc, const DWARFDIE &die,
                                   bool *type_is_new_ptr) {
-  TypeSP type_sp;
+  if (!die)
+    return {};
 
-  if (die) {
-    TypeSystem *type_system =
-        GetTypeSystemForLanguage(die.GetCU()->GetLanguageType());
+  if (!type_system)
+    return {};
 
-    if (type_system) {
-      DWARFASTParser *dwarf_ast = type_system->GetDWARFParser();
-      if (dwarf_ast) {
-        Log *log = LogChannelDWARF::GetLogIfAll(DWARF_LOG_DEBUG_INFO);
-        type_sp = dwarf_ast->ParseTypeFromDWARF(sc, die, log, type_is_new_ptr);
-        if (type_sp) {
-          TypeList *type_list = GetTypeList();
-          if (type_list)
-            type_list->Insert(type_sp);
+  if (!dwarf_ast)
+    return {};
+
+  TypeSP type_sp = dwarf_ast->ParseTypeFromDWARF(sc, die, log, type_is_new_ptr);
 
-          if (die.Tag() == DW_TAG_subprogram) {
-            DIERef die_ref = die.GetDIERef();
-            std::string scope_qualified_name(GetDeclContextForUID(die.GetID())
-                                                 .GetScopeQualifiedName()
-                                                 .AsCString(""));
-            if (scope_qualified_name.size()) {
-              NameToOffsetMap::iterator iter =
-                  m_function_scope_qualified_name_map.find(
-                      scope_qualified_name);
-              if (iter != m_function_scope_qualified_name_map.end())
-                (*iter).second->insert(die_ref);
-              else {
-                DIERefSetSP new_set(new std::set<DIERef>);
-                new_set->insert(die_ref);
-                m_function_scope_qualified_name_map.emplace(
-                    std::make_pair(scope_qualified_name, new_set));
-              }
-            }
-          }
-        }
+        m_function_scope_qualified_name_map[scope_qualified_name].insert(
+            die.GetID());
       }
     }
   }
 
   return type_sp;
 }
 
 size_t SymbolFileDWARF::ParseTypes(const SymbolContext &sc,