[DWARF5] Fix offset check when using .debug_names

[kimanh]

Summary

When going through the CU entries in the name index,
make sure to compare the name entry's CU
offset against the skeleton CU's offset.

Previously there would be a mismatch, since the
wrong offset was compared, and thus no suitable
entry was found.

[JDevlieghere]

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

82

I assume the const of the DWARFUnit is being dropped because GetNonSkeletonUnit? Rather than doing that, can't that function be const instead?

[kimanh]

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

82

Yes correct. I agree with your point that dropping const here is not very nice, but I'm not sure whether we can make that function const easily.
As is GetNonSkeletonUnit is updating itself in ExtractUnitDIEIfNeeded(), which kicks off many non-const operations for updating itself. As far as I understand, if we would want to make it const, we'd need to change the architectural logic in how the skeleton and non-skeleton units are connected. I'm not very familiar with the code base, so any advice here is welcome!

[kimanh]

Ping on this thread, any comment/guidance on this would help us a lot!

[shafik]

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

3073

So is the problem that calling GetNonSkeletonUnit() does not work here b/c in DebugNamesDWARFIndex case we don't want this but in the other cases we do?

[kimanh]

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

3073

Yes, that's correct!

[jankratochvil]

LGTM

lldb/test/Shell/SymbolFile/DWARF/x86/find-variable-file.cpp

34

I see ld.lld is being run directly even in other testcases so OK.

[kimanh]

Thanks for the review! Landing now.

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

public:
 
   void
   GetGlobalVariables(ConstString basename,
                      llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void
   GetGlobalVariables(const RegularExpression &regex,
                      llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void
-  GetGlobalVariables(const DWARFUnit &cu,
+  GetGlobalVariables(DWARFUnit &cu,
                      llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void GetObjCMethods(ConstString class_name,
                       llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void GetCompleteObjCClass(
       ConstString class_name, bool must_be_implementation,
       llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void GetTypes(ConstString name,
                 llvm::function_ref<bool(DWARFDIE die)> callback) override;

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

void AppleDWARFIndex::GetGlobalVariables(
   DWARFMappedHash::DIEInfoArray hash_data;
   m_apple_names_up->AppendAllDIEsThatMatchingRegex(regex, hash_data);
   // This is not really the DIE name.
   DWARFMappedHash::ExtractDIEArray(hash_data,
                                    DIERefCallback(callback, regex.GetText()));
 }
 
 void AppleDWARFIndex::GetGlobalVariables(
-    const DWARFUnit &cu, llvm::function_ref<bool(DWARFDIE die)> callback) {
+    DWARFUnit &cu, llvm::function_ref<bool(DWARFDIE die)> callback) {
   if (!m_apple_names_up)
     return;
 
+  const DWARFUnit &non_skeleton_cu = cu.GetNonSkeletonUnit();
   DWARFMappedHash::DIEInfoArray hash_data;
-  m_apple_names_up->AppendAllDIEsInRange(cu.GetOffset(), cu.GetNextUnitOffset(),
+  m_apple_names_up->AppendAllDIEsInRange(non_skeleton_cu.GetOffset(),
+                                         non_skeleton_cu.GetNextUnitOffset(),
                                          hash_data);
   DWARFMappedHash::ExtractDIEArray(hash_data, DIERefCallback(callback));
 }
 
 void AppleDWARFIndex::GetObjCMethods(
     ConstString class_name, llvm::function_ref<bool(DWARFDIE die)> callback) {
   if (!m_apple_objc_up)
     return;

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

public:
   virtual void
   GetGlobalVariables(ConstString basename,
                      llvm::function_ref<bool(DWARFDIE die)> callback) = 0;
 
   virtual void
   GetGlobalVariables(const RegularExpression &regex,
                      llvm::function_ref<bool(DWARFDIE die)> callback) = 0;
   virtual void
-  GetGlobalVariables(const DWARFUnit &cu,
+  GetGlobalVariables(DWARFUnit &cu,
                      llvm::function_ref<bool(DWARFDIE die)> callback) = 0;
   virtual void
   GetObjCMethods(ConstString class_name,
                  llvm::function_ref<bool(DWARFDIE die)> callback) = 0;
   virtual void
   GetCompleteObjCClass(ConstString class_name, bool must_be_implementation,
                        llvm::function_ref<bool(DWARFDIE die)> callback) = 0;
   virtual void GetTypes(ConstString name,

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

public:
 
   void
   GetGlobalVariables(ConstString basename,
                      llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void
   GetGlobalVariables(const RegularExpression &regex,
                      llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void
-  GetGlobalVariables(const DWARFUnit &cu,
+  GetGlobalVariables(DWARFUnit &cu,
                      llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void
   GetObjCMethods(ConstString class_name,
                  llvm::function_ref<bool(DWARFDIE die)> callback) override {}
   void GetCompleteObjCClass(
       ConstString class_name, bool must_be_implementation,
       llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void GetTypes(ConstString name,

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

for (DebugNames::NameTableEntry nte: ni) {
       MaybeLogLookupError(entry_or.takeError(), ni, nte.getString());
     }
   }
 
   m_fallback.GetGlobalVariables(regex, callback);
 }
 
 void DebugNamesDWARFIndex::GetGlobalVariables(
-    const DWARFUnit &cu, llvm::function_ref<bool(DWARFDIE die)> callback) {
+    DWARFUnit &cu, llvm::function_ref<bool(DWARFDIE die)> callback) {
   uint64_t cu_offset = cu.GetOffset();
   bool found_entry_for_cu = false;
   for (const DebugNames::NameIndex &ni: *m_debug_names_up) {
     for (DebugNames::NameTableEntry nte: ni) {
       uint64_t entry_offset = nte.getEntryOffset();
       llvm::Expected<DebugNames::Entry> entry_or = ni.getEntry(&entry_offset);
       for (; entry_or; entry_or = ni.getEntry(&entry_offset)) {
         if (entry_or->tag() != DW_TAG_variable)

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

public:
 
   void
   GetGlobalVariables(ConstString basename,
                      llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void
   GetGlobalVariables(const RegularExpression &regex,
                      llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void
-  GetGlobalVariables(const DWARFUnit &unit,
+  GetGlobalVariables(DWARFUnit &unit,
                      llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void GetObjCMethods(ConstString class_name,
                       llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void GetCompleteObjCClass(
       ConstString class_name, bool must_be_implementation,
       llvm::function_ref<bool(DWARFDIE die)> callback) override;
   void GetTypes(ConstString name,
                 llvm::function_ref<bool(DWARFDIE die)> callback) override;

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

 void ManualDWARFIndex::GetGlobalVariables(
     const RegularExpression &regex,
     llvm::function_ref<bool(DWARFDIE die)> callback) {
   Index();
   m_set.globals.Find(regex, DIERefCallback(callback, regex.GetText()));
 }
 
 void ManualDWARFIndex::GetGlobalVariables(
-    const DWARFUnit &unit, llvm::function_ref<bool(DWARFDIE die)> callback) {
+    DWARFUnit &unit, llvm::function_ref<bool(DWARFDIE die)> callback) {
   Index();
-  m_set.globals.FindAllEntriesForUnit(unit, DIERefCallback(callback));
+  m_set.globals.FindAllEntriesForUnit(unit.GetNonSkeletonUnit(),
+                                      DIERefCallback(callback));
 }
 
 void ManualDWARFIndex::GetObjCMethods(
     ConstString class_name, llvm::function_ref<bool(DWARFDIE die)> callback) {
   Index();
   m_set.objc_class_selectors.Find(
       class_name, DIERefCallback(callback, class_name.GetStringRef()));
 }

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

if (sc.function) {
 
       uint32_t vars_added = 0;
       VariableListSP variables(sc.comp_unit->GetVariableList(false));
 
       if (variables.get() == nullptr) {
         variables = std::make_shared<VariableList>();
         sc.comp_unit->SetVariableList(variables);
 
-        m_index->GetGlobalVariables(
-            dwarf_cu->GetNonSkeletonUnit(), [&](DWARFDIE die) {
-              VariableSP var_sp(
-                  ParseVariableDIE(sc, die, LLDB_INVALID_ADDRESS));
-              if (var_sp) {
-                variables->AddVariableIfUnique(var_sp);
-                ++vars_added;
-              }
-              return true;
-            });
+        m_index->GetGlobalVariables(*dwarf_cu, [&](DWARFDIE die) {
+          VariableSP var_sp(ParseVariableDIE(sc, die, LLDB_INVALID_ADDRESS));
       }
       return vars_added;
     }
   }
   return 0;
 }
 
 VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,

lldb/test/Shell/SymbolFile/DWARF/x86/find-variable-file.cpp

 // RUN: %clang -c -o %t-2.o --target=x86_64-pc-linux -gdwarf-5 -gpubnames %S/Inputs/find-variable-file-2.cpp
 // RUN: ld.lld %t-1.o %t-2.o -o %t
 // RUN: llvm-readobj --sections %t | FileCheck %s --check-prefix NAMES
 // RUN: lldb-test symbols --file=find-variable-file.cpp --find=variable %t | \
 // RUN:   FileCheck --check-prefix=ONE %s
 // RUN: lldb-test symbols --file=find-variable-file-2.cpp --find=variable %t | \
 // RUN:   FileCheck --check-prefix=TWO %s
 
+// Run the same test with split dwarf and pubnames to check whether we can find
+// the compile unit using the name index if it is split.
+// RUN: %clang -c -o %t-1.o --target=x86_64-pc-linux -gdwarf-5 -gsplit-dwarf -gpubnames %s
+// RUN: %clang -c -o %t-2.o --target=x86_64-pc-linux -gdwarf-5 -gsplit-dwarf -gpubnames %S/Inputs/find-variable-file-2.cpp
+// RUN: ld.lld %t-1.o %t-2.o -o %t
+// RUN: llvm-readobj --sections %t | FileCheck %s --check-prefix NAMES
+// RUN: lldb-test symbols --file=find-variable-file.cpp --find=variable %t | \
+// RUN:   FileCheck --check-prefix=ONE %s
+// RUN: lldb-test symbols --file=find-variable-file-2.cpp --find=variable %t | \
+// RUN:   FileCheck --check-prefix=TWO %s
+
 // NAMES: Name: .debug_names
 
 // ONE: Found 1 variables:
 namespace one {
 int foo;
 // ONE-DAG: name = "foo", type = {{.*}} (int), {{.*}} decl = find-variable-file.cpp:[[@LINE-1]]
 } // namespace one
 
 extern "C" void _start() {}
 
 // TWO: Found 1 variables:
 // TWO-DAG: name = "foo", {{.*}} decl = find-variable-file-2.cpp:2