Manual DWARF index: don't skip over -gmodules debug info

[aprantl]

Summary

This fixes a regression introduced by https://reviews.llvm.org/D131437. The intention of the patch was to avoid indexing DWO skeleton units, but it also skipped over full DWARF compile units linked via a -gmodules DW_AT_dwo_name attribute. This patch restores the functionality and adds a test for it.

[clayborg]

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

168

Won't this return nullptr in the fission case where the .dwo file is missing or deleted?

181–190

Can we do a bit more to ensure we aren't using fission here? Are there no other differences between -gmodules and fission we can key off of? Existence of .debug_addr section for fission only?

[aprantl]

I added an unambiguous check for DWARF5+. The remaining incorrectly handled case is a DWARF4 + GNU fission extension where the .dwo file has been deleted. Is there any harm in indexing the skeleton in that case? It shouldn't be able to interfere with the declarations in the (missing) .dwo file in that case, right?
Let me know if this is not enough and I'll try finding another heuristic that we could apply here.

[aprantl]

In D142683#4086026, @aprantl wrote:

It shouldn't be able to interfere with the declarations in the (missing) .dwo file in that case, right?

Maybe I should have bothered reading your comment in the code. That's exactly the case you were trying to avoid m-(

[aprantl]

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

181–190

The .debug_addr section only exists in DWARF 5 so the IsSkeletonUnit() check covers this.

[aprantl]

Added another heuristic to distinguish gmodules and fission.

[clayborg]

Much better, thanks for making sure we don't end up indexing real skeleton units as they cause problems if they do get indexed.

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

die_iterator_range dies() {
     ExtractDIEsIfNeeded();
     return die_iterator_range(m_die_array.begin(), m_die_array.end());
   }
 
   DIERef::Section GetDebugSection() const { return m_section; }
 
   uint8_t GetUnitType() const { return m_header.GetUnitType(); }
   bool IsTypeUnit() const { return m_header.IsTypeUnit(); }
+  /// Note that this check only works for DWARF5+.
+  bool IsSkeletonUnit() const { return GetUnitType() == llvm::dwarf::DW_UT_skeleton; }
 
   std::optional<uint64_t> GetStringOffsetSectionItem(uint32_t index) const;
 
   /// Return a list of address ranges resulting from a (possibly encoded)
   /// range list starting at a given offset in the appropriate ranges section.
   llvm::Expected<DWARFRangeList> FindRnglistFromOffset(dw_offset_t offset);
 
   /// Return a list of address ranges retrieved from an encoded range

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

void ManualDWARFIndex::IndexUnit(DWARFUnit &unit, SymbolFileDWARFDwo *dwp,
   // copy of all DW_TAG_subprogram and any contained DW_TAG_inline_subroutine
   // DIEs so that symbolication will still work in the absence of the .dwo/.dwp
   // file, but the functions have no return types and all arguments and locals
   // have been removed. So we don't want to index any of these hacked up
   // function types. Types can still exist in the skeleton compile unit DWARF
   // though as some functions have template parameter types and other things
   // that cause extra copies of types to be included, but we should find these
   // types in the .dwo file only as methods could have return types removed and
-  // we don't have to index incomplete types from the skeletone compile unit.
+  // we don't have to index incomplete types from the skeleton compile unit.
   if (unit.GetDWOId()) {
+    // Index the .dwo or dwp instead of the skeleton unit.
     if (SymbolFileDWARFDwo *dwo_symbol_file = unit.GetDwoSymbolFile()) {
       // Type units in a dwp file are indexed separately, so we just need to
-      // process the split unit here. However, if the split unit is in a dwo file,
-      // then we need to process type units here.
+      // process the split unit here. However, if the split unit is in a dwo
+      // file, then we need to process type units here.
       if (dwo_symbol_file == dwp) {
         IndexUnitImpl(unit.GetNonSkeletonUnit(), cu_language, set);
       } else {
         DWARFDebugInfo &dwo_info = dwo_symbol_file->DebugInfo();
         for (size_t i = 0; i < dwo_info.GetNumUnits(); ++i)
           IndexUnitImpl(*dwo_info.GetUnitAtIndex(i), cu_language, set);
       }
+      return;
     }
-  } else {
-    // We either have a normal compile unit which we want to index.
-    IndexUnitImpl(unit, cu_language, set);
+    // This was a DWARF5 skeleton CU and the .dwo file couldn't be located.
+    if (unit.IsSkeletonUnit())
+      return;
+
+    // Otherwise, the unit has a dwo_id, but this isn't a .dwo
+    // skeleton unit, so the assumption is that this is a file
+    // produced by -gmodules and that we want to index it.
   }
+  // We have a normal compile unit which we want to index.
+  IndexUnitImpl(unit, cu_language, set);
 }
 
 void ManualDWARFIndex::IndexUnitImpl(DWARFUnit &unit,
                                      const LanguageType cu_language,
                                      IndexSet &set) {
   for (const DWARFDebugInfoEntry &die : unit.dies()) {
     const dw_tag_t tag = die.Tag();
 

lldb/test/Shell/SymbolFile/DWARF/Inputs/pch.h

+typedef int anchor_t;
+
+struct TypeFromPCH {
+  int field;
+};

lldb/test/Shell/SymbolFile/DWARF/clang-gmodules-type-lookup.c

+// UNSUPPORTED: system-windows
+
+// Test that LLDB can follow DWO links produced by -gmodules debug
+// info to find a type in a precompiled header.
+// 
+// RUN: %clangxx_host -g -gmodules -fmodules -std=c99 -x c-header %S/Inputs/pch.h -g -c -o %t.pch
+// RUN: %clangxx_host -g -gmodules -fmodules -std=c99 -x c -include-pch %t.pch %s -c -o %t.o
+// RUN: %clangxx_host %t.o -o %t.exe
+// RUN: lldb-test symbols -dump-clang-ast -find type --language=C99 \
+// RUN:   -compiler-context 'AnyModule:*,Struct:TypeFromPCH' %t.exe | FileCheck %s
+
+anchor_t anchor;
+
+int main(int argc, char **argv) { return 0; }
+
+// CHECK: Found 1 type
+// CHECK: "TypeFromPCH"
+// CHECK: FieldDecl {{.*}} field