[lldb] Fix a crash when using .dwp files and make type lookup reliable with the index cache

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

@@ -24,6 +24,7 @@
 #include "DWARFDebugInfoEntry.h"
 #include "DWARFFormValue.h"
 #include "DWARFTypeUnit.h"
+#include "LogChannelDWARF.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -81,27 +82,90 @@ void DWARFDebugInfo::ParseUnitsFor(DIERef::Section section) {
                                 : m_context.getOrLoadDebugInfoData();
   lldb::offset_t offset = 0;
   while (data.ValidOffset(offset)) {
-    llvm::Expected<DWARFUnitSP> unit_sp = DWARFUnit::extract(
-        m_dwarf, m_units.size(), data, section, &offset);
+    const lldb::offset_t unit_header_offset = offset;
+    llvm::Expected<DWARFUnitSP> expected_unit_sp =
+        DWARFUnit::extract(m_dwarf, m_units.size(), data, section, &offset);
 
-    if (!unit_sp) {
-      // FIXME: Propagate this error up.
-      llvm::consumeError(unit_sp.takeError());
+    if (!expected_unit_sp) {
+      Log *log = GetLog(DWARFLog::DebugInfo);
+      LLDB_LOG(log, "Unable to extract DWARFUnitHeader at {0:x}: {1}",
+               unit_header_offset, llvm::toString(expected_unit_sp.takeError()));
       return;
     }
 
+    DWARFUnitSP unit_sp = *expected_unit_sp;
+
     // If it didn't return an error, then it should be returning a valid Unit.
-    assert(*unit_sp);
-    m_units.push_back(*unit_sp);
-    offset = (*unit_sp)->GetNextUnitOffset();
+    assert((bool)unit_sp);
+
+    // Keep a map of DWO ID back to the skeleton units. Sometimes accelerator
+    // table lookups can cause the DWO files to be accessed before the skeleton
+    // compile unit is parsed, so we keep a map to allow us to match up the DWO
+    // file to the back to the skeleton compile units.
+    if (unit_sp->GetUnitType() == lldb_private::dwarf::DW_UT_skeleton) {
+      if (std::optional<uint64_t> unit_dwo_id = unit_sp->GetHeaderDWOId())
+        m_dwarf5_dwo_id_to_skeleton_unit[*unit_dwo_id] = unit_sp.get();
+    }
 
-    if (auto *type_unit = llvm::dyn_cast<DWARFTypeUnit>(unit_sp->get())) {
+    m_units.push_back(unit_sp);
+    offset = unit_sp->GetNextUnitOffset();
+
+    if (auto *type_unit = llvm::dyn_cast<DWARFTypeUnit>(unit_sp.get())) {
       m_type_hash_to_unit_index.emplace_back(type_unit->GetTypeHash(),
-                                             unit_sp.get()->GetID());
+                                             unit_sp->GetID());
     }
   }
 }
 
+DWARFUnit *DWARFDebugInfo::GetSkeletonUnit(DWARFUnit *dwo_unit) {
+  // If this isn't a DWO unit, don't try and find the skeleton unit.
+  if (!dwo_unit->IsDWOUnit())
+    return nullptr;
+
+  auto dwo_id = dwo_unit->GetDWOId();
+  if (!dwo_id.has_value())
+    return nullptr;
+
+  // Parse the unit headers so that m_dwarf5_dwo_id_to_skeleton_unit is filled
+  // in with all of the DWARF5 skeleton compile units DWO IDs since it is easy
+  // to access the DWO IDs in the DWARFUnitHeader for each DWARFUnit.
+  ParseUnitHeadersIfNeeded();
+
+  // Find the value in our cache and return it we we find it. This cache may
+  // only contain DWARF5 units.
+  auto iter = m_dwarf5_dwo_id_to_skeleton_unit.find(*dwo_id);
+  if (iter != m_dwarf5_dwo_id_to_skeleton_unit.end())
+    return iter->second;
+
+  // DWARF5 unit headers have the DWO ID and should have already been in the map
+  // so if it wasn't found in the above find() call, then we didn't find it and
+  // don't need to do the more expensive DWARF4 search.
+  if (dwo_unit->GetVersion() >= 5)
+    return nullptr;
+
+  // Parse all DWO IDs from all DWARF4 and earlier compile units that have DWO
+  // IDs. It is more expensive to get the DWO IDs from DWARF4 compile units as
+  // we need to parse the unit DIE and extract the DW_AT_dwo_id or
+  // DW_AT_GNU_dwo_id attribute values, so do this only if we didn't find our
+  // match above search and only for DWARF4 and earlier compile units.
+  llvm::call_once(m_dwarf4_dwo_id_to_skeleton_unit_once_flag, [this]() {
+    for (uint32_t i = 0, num = GetNumUnits(); i < num; ++i) {
+      if (DWARFUnit *unit = GetUnitAtIndex(i)) {
+        if (unit->GetVersion() < 5) {
+          if (std::optional<uint64_t> unit_dwo_id = unit->GetDWOId())
+            m_dwarf4_dwo_id_to_skeleton_unit[*unit_dwo_id] = unit;
+        }
+      }
+    }
+  });
+
+  // Search the DWARF4 DWO results that we parsed lazily.
+  iter = m_dwarf4_dwo_id_to_skeleton_unit.find(*dwo_id);
+  if (iter != m_dwarf4_dwo_id_to_skeleton_unit.end())
+    return iter->second;
+  return nullptr;
+}
+
 void DWARFDebugInfo::ParseUnitHeadersIfNeeded() {
   llvm::call_once(m_units_once_flag, [&] {
     ParseUnitsFor(DIERef::Section::DebugInfo);

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

@@ -39,6 +39,7 @@ class DWARFDebugInfo {
   DWARFUnit *GetUnitContainingDIEOffset(DIERef::Section section,
                                         dw_offset_t die_offset);
   DWARFUnit *GetUnit(const DIERef &die_ref);
+  DWARFUnit *GetSkeletonUnit(DWARFUnit *dwo_unit);
   DWARFTypeUnit *GetTypeUnitForHash(uint64_t hash);
   bool ContainsTypeUnits();
   DWARFDIE GetDIE(const DIERef &die_ref);
@@ -70,6 +71,9 @@ class DWARFDebugInfo {
       m_cu_aranges_up; // A quick address to compile unit table
 
   std::vector<std::pair<uint64_t, uint32_t>> m_type_hash_to_unit_index;
+  llvm::DenseMap<uint64_t, DWARFUnit *> m_dwarf5_dwo_id_to_skeleton_unit;
+  llvm::DenseMap<uint64_t, DWARFUnit *> m_dwarf4_dwo_id_to_skeleton_unit;
+  llvm::once_flag m_dwarf4_dwo_id_to_skeleton_unit_once_flag;
 
 private:
   // All parsing needs to be done partially any managed by this class as

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

@@ -97,7 +97,12 @@ void DWARFUnit::ExtractUnitDIEIfNeeded() {
         *m_dwo_id, m_first_die.GetOffset()));
     return; // Can't fetch the compile unit from the dwo file.
   }
-  dwo_cu->SetUserData(this);
+  // If the skeleton compile unit gets its unit DIE parsed first, then this
+  // will fill in the DWO file's back pointer to this skeleton compile unit.
+  // If the DWO files get parsed on their own first the skeleton back link
+  // can be done manually in DWARFUnit::GetSkeletonCompileUnit() which will
+  // do a reverse lookup and cache the result.
+  dwo_cu->SetSkeletonUnit(this);
 
   DWARFBaseDIE dwo_cu_die = dwo_cu->GetUnitDIEOnly();
   if (!dwo_cu_die.IsValid()) {
@@ -702,9 +707,25 @@ uint8_t DWARFUnit::GetAddressByteSize(const DWARFUnit *cu) {
 
 uint8_t DWARFUnit::GetDefaultAddressSize() { return 4; }
 
-void *DWARFUnit::GetUserData() const { return m_user_data; }
+DWARFCompileUnit *DWARFUnit::GetSkeletonUnit() {
+  if (m_skeleton_unit == nullptr && IsDWOUnit()) {
+    SymbolFileDWARFDwo *dwo =
+        llvm::dyn_cast_or_null<SymbolFileDWARFDwo>(&GetSymbolFileDWARF());
+    // Do a reverse lookup if the skeleton compile unit wasn't set.
+    if (dwo)
+      m_skeleton_unit = dwo->GetBaseSymbolFile().GetSkeletonUnit(this);
+  }
+  return llvm::dyn_cast_or_null<DWARFCompileUnit>(m_skeleton_unit);
+}
 
-void DWARFUnit::SetUserData(void *d) { m_user_data = d; }
+void DWARFUnit::SetSkeletonUnit(DWARFUnit *skeleton_unit) {
+  // If someone is re-setting the skeleton compile unit backlink, make sure
+  // it is setting it to a valid value when it wasn't valid, or if the
+  // value in m_skeleton_unit was valid, it should be the same value.
+  assert(skeleton_unit);
+  assert(m_skeleton_unit == nullptr || m_skeleton_unit == skeleton_unit);
+  m_skeleton_unit = skeleton_unit;
+}
 
 bool DWARFUnit::Supports_DW_AT_APPLE_objc_complete_type() {
   return GetProducer() != eProducerLLVMGCC;

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

@@ -98,8 +98,14 @@ class DWARFUnit : public UserID {
   virtual ~DWARFUnit();
 
   bool IsDWOUnit() { return m_is_dwo; }
+  /// Get the DWO ID from the DWARFUnitHeader for DWARF5, or from the unit DIE's
+  /// DW_AT_dwo_id or DW_AT_GNU_dwo_id for DWARF4 and earlier.
   std::optional<uint64_t> GetDWOId();
-
+  /// Get the DWO ID from the DWARFUnitHeader only. DWARF5 skeleton units have
+  /// the DWO ID in the compile unit header and we sometimes only want to access
+  /// this cheap value without causing the more expensive attribute fetches that
+  /// GetDWOId() uses.
+  std::optional<uint64_t> GetHeaderDWOId() { return m_header.GetDWOId(); }
   void ExtractUnitDIEIfNeeded();
   void ExtractUnitDIENoDwoIfNeeded();
   void ExtractDIEsIfNeeded();
@@ -198,9 +204,21 @@ class DWARFUnit : public UserID {
 
   static uint8_t GetDefaultAddressSize();
 
-  void *GetUserData() const;
+  lldb_private::CompileUnit *GetLLDBCompUnit() const { return m_lldb_cu; }
+
+  void SetLLDBCompUnit(lldb_private::CompileUnit *cu) { m_lldb_cu = cu; }
+
+  /// Get the skeleton compile unit for a DWO file.
+  ///
+  /// We need to keep track of the skeleton compile unit for a DWO file so
+  /// we can access it. Sometimes this value is cached when the skeleton
+  /// compile unit is first parsed, but if a .dwp file parses all of the
+  /// DWARFUnits in the file, the skeleton compile unit might not have been
+  /// parsed yet, to there might not be a backlink. This accessor handles
+  /// both cases correctly and avoids crashes.
+  DWARFCompileUnit *GetSkeletonUnit();
 
-  void SetUserData(void *d);
+  void SetSkeletonUnit(DWARFUnit *skeleton_unit);
 
   bool Supports_DW_AT_APPLE_objc_complete_type();
 
@@ -336,7 +354,9 @@ class DWARFUnit : public UserID {
   std::shared_ptr<DWARFUnit> m_dwo;
   DWARFUnitHeader m_header;
   const llvm::DWARFAbbreviationDeclarationSet *m_abbrevs = nullptr;
-  void *m_user_data = nullptr;
+  lldb_private::CompileUnit *m_lldb_cu = nullptr;
+  // If this is a DWO file, we have a backlink to our skeleton compile unit.
+  DWARFUnit *m_skeleton_unit = nullptr;
   // The compile unit debug information entry item
   DWARFDebugInfoEntry::collection m_die_array;
   mutable llvm::sys::RWMutex m_die_array_mutex;

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

@@ -722,8 +722,8 @@ DWARFCompileUnit *SymbolFileDWARF::GetDWARFCompileUnit(CompileUnit *comp_unit) {
 
   // The compile unit ID is the index of the DWARF unit.
   DWARFUnit *dwarf_cu = DebugInfo().GetUnitAtIndex(comp_unit->GetID());
-  if (dwarf_cu && dwarf_cu->GetUserData() == nullptr)
-    dwarf_cu->SetUserData(comp_unit);
+  if (dwarf_cu && dwarf_cu->GetLLDBCompUnit() == nullptr)
+    dwarf_cu->SetLLDBCompUnit(comp_unit);
 
   // It must be DWARFCompileUnit when it created a CompileUnit.
   return llvm::cast_or_null<DWARFCompileUnit>(dwarf_cu);
@@ -771,15 +771,15 @@ static const char *GetDWOName(DWARFCompileUnit &dwarf_cu,
 
 lldb::CompUnitSP SymbolFileDWARF::ParseCompileUnit(DWARFCompileUnit &dwarf_cu) {
   CompUnitSP cu_sp;
-  CompileUnit *comp_unit = (CompileUnit *)dwarf_cu.GetUserData();
+  CompileUnit *comp_unit = dwarf_cu.GetLLDBCompUnit();
   if (comp_unit) {
     // We already parsed this compile unit, had out a shared pointer to it
     cu_sp = comp_unit->shared_from_this();
   } else {
     if (GetDebugMapSymfile()) {
       // Let the debug map create the compile unit
       cu_sp = m_debug_map_symfile->GetCompileUnit(this, dwarf_cu);
-      dwarf_cu.SetUserData(cu_sp.get());
+      dwarf_cu.SetLLDBCompUnit(cu_sp.get());
     } else {
       ModuleSP module_sp(m_objfile_sp->GetModule());
       if (module_sp) {
@@ -792,7 +792,7 @@ lldb::CompUnitSP SymbolFileDWARF::ParseCompileUnit(DWARFCompileUnit &dwarf_cu) {
               *GetDWARFUnitIndex(dwarf_cu.GetID()), cu_language,
               eLazyBoolCalculate, std::move(support_files));
 
-          dwarf_cu.SetUserData(cu_sp.get());
+          dwarf_cu.SetLLDBCompUnit(cu_sp.get());
 
           SetCompileUnitAtIndex(dwarf_cu.GetID(), cu_sp);
         };
@@ -1675,20 +1675,20 @@ Type *SymbolFileDWARF::ResolveType(const DWARFDIE &die,
 
 CompileUnit *
 SymbolFileDWARF::GetCompUnitForDWARFCompUnit(DWARFCompileUnit &dwarf_cu) {
+
   if (dwarf_cu.IsDWOUnit()) {
-    DWARFCompileUnit *non_dwo_cu =
-        static_cast<DWARFCompileUnit *>(dwarf_cu.GetUserData());
+    DWARFCompileUnit *non_dwo_cu = dwarf_cu.GetSkeletonUnit();
     assert(non_dwo_cu);
     return non_dwo_cu->GetSymbolFileDWARF().GetCompUnitForDWARFCompUnit(
         *non_dwo_cu);
   }
   // Check if the symbol vendor already knows about this compile unit?
-  if (dwarf_cu.GetUserData() == nullptr) {
-    // The symbol vendor doesn't know about this compile unit, we need to parse
-    // and add it to the symbol vendor object.
-    return ParseCompileUnit(dwarf_cu).get();
-  }
-  return static_cast<CompileUnit *>(dwarf_cu.GetUserData());
+  CompileUnit *lldb_cu = dwarf_cu.GetLLDBCompUnit();
+  if (lldb_cu)
+    return lldb_cu;
+  // The symbol vendor doesn't know about this compile unit, we need to parse
+  // and add it to the symbol vendor object.
+  return ParseCompileUnit(dwarf_cu).get();
 }
 
 void SymbolFileDWARF::GetObjCMethods(
@@ -1750,7 +1750,7 @@ SymbolFileDWARF::GetDIE(const DIERef &die_ref) {
     }
 
     if (*file_index == DIERef::k_file_index_mask)
-      symbol_file = m_dwp_symfile.get(); // DWP case
+      symbol_file = GetDwpSymbolFile().get(); // DWP case
     else
       symbol_file = this->DebugInfo()
                         .GetUnitAtIndex(*die_ref.file_index())
@@ -1785,6 +1785,10 @@ std::optional<uint64_t> SymbolFileDWARF::GetDWOId() {
   return {};
 }
 
+DWARFUnit *SymbolFileDWARF::GetSkeletonUnit(DWARFUnit *dwo_unit) {
+  return DebugInfo().GetSkeletonUnit(dwo_unit);
+}
+
 std::shared_ptr<SymbolFileDWARFDwo>
 SymbolFileDWARF::GetDwoSymbolFileForCompileUnit(
     DWARFUnit &unit, const DWARFDebugInfoEntry &cu_die) {

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

@@ -252,6 +252,17 @@ class SymbolFileDWARF : public SymbolFileCommon {
   /// If this is a DWARF object with a single CU, return its DW_AT_dwo_id.
   std::optional<uint64_t> GetDWOId();
 
+  /// Given a DWO DWARFUnit, find the corresponding skeleton DWARFUnit
+  /// in the main symbol file. DWP files can have their DWARFUnits
+  /// parsed without the skeleton compile units having been parsed, so
+  /// sometimes we need to find the skeleton compile unit for a DWO
+  /// DWARFUnit so we can fill in this link. Currently unless the
+  /// skeleton compile unit has been parsed _and_ the Unit DIE has been
+  /// parsed, the DWO unit will not have a backward link setup correctly
+  /// which was causing crashes due to an assertion that was firing
+  /// in SymbolFileDWARF::GetCompUnitForDWARFCompUnit().
+  DWARFUnit *GetSkeletonUnit(DWARFUnit *dwo_unit);
+
   static bool DIEInDeclContext(const CompilerDeclContext &parent_decl_ctx,
                                const DWARFDIE &die,
                                bool only_root_namespaces = false);

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

@@ -160,3 +160,22 @@ void SymbolFileDWARFDwo::FindGlobalVariables(
   GetBaseSymbolFile().FindGlobalVariables(name, parent_decl_ctx, max_matches,
                                           variables);
 }
+
+bool SymbolFileDWARFDwo::GetDebugInfoIndexWasLoadedFromCache() const {
+  return GetBaseSymbolFile().GetDebugInfoIndexWasLoadedFromCache();
+}
+void SymbolFileDWARFDwo::SetDebugInfoIndexWasLoadedFromCache() {
+  GetBaseSymbolFile().SetDebugInfoIndexWasLoadedFromCache();
+}
+bool SymbolFileDWARFDwo::GetDebugInfoIndexWasSavedToCache() const {
+  return GetBaseSymbolFile().GetDebugInfoIndexWasSavedToCache();
+}
+void SymbolFileDWARFDwo::SetDebugInfoIndexWasSavedToCache() {
+  GetBaseSymbolFile().SetDebugInfoIndexWasSavedToCache();
+}
+bool SymbolFileDWARFDwo::GetDebugInfoHadFrameVariableErrors() const {
+  return GetBaseSymbolFile().GetDebugInfoHadFrameVariableErrors();
+}
+void SymbolFileDWARFDwo::SetDebugInfoHadFrameVariableErrors() {
+  return GetBaseSymbolFile().SetDebugInfoHadFrameVariableErrors();
+}

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

@@ -58,6 +58,15 @@ class SymbolFileDWARFDwo : public SymbolFileDWARF {
                            uint32_t max_matches,
                            VariableList &variables) override;
 
+  SymbolFileDWARF &GetBaseSymbolFile() const { return m_base_symbol_file; }
+
+  bool GetDebugInfoIndexWasLoadedFromCache() const override;
+  void SetDebugInfoIndexWasLoadedFromCache() override;
+  bool GetDebugInfoIndexWasSavedToCache() const override;
+  void SetDebugInfoIndexWasSavedToCache() override;
+  bool GetDebugInfoHadFrameVariableErrors() const override;
+  void SetDebugInfoHadFrameVariableErrors() override;
+
 protected:
   DIEToTypePtr &GetDIEToType() override;
 
@@ -77,8 +86,6 @@ class SymbolFileDWARFDwo : public SymbolFileDWARF {
                                        ConstString type_name,
                                        bool must_be_implementation) override;
 
-  SymbolFileDWARF &GetBaseSymbolFile() const { return m_base_symbol_file; }
-
   /// If this file contains exactly one compile unit, this function will return
   /// it. Otherwise it returns nullptr.
   DWARFCompileUnit *FindSingleCompileUnit();