[nfc] [lldb] Assertions for D106270 - [DWARF5] Fix offset check when using .debug_names

[jankratochvil]

Summary

As this issue is a bit difficult to debug it is better to make it safe against future changes.

[clayborg]

Can we not just grab the skeleton unit when/if needed instead of asserting in many places?

[jankratochvil]

In D107659#2931836, @clayborg wrote:

Can we not just grab the skeleton unit when/if needed instead of asserting in many places?

I agree it could be unified for API simplicity to be always the skeleton unit, thanks for the idea. Done in this patch. Still I believe it should be asserted as unfortunately with the current Unit types it cannot be compile-time checked.

[kimanh]

LGTM, I also support that assertions are added. This gives confidence in what's expected and also makes it easier to read/understand whether it's a skeleton/non-skeleton unit that we are dealing with.

[jankratochvil]

Thanks for checking it but ...

In D107659#2931836, @clayborg wrote:

Can we not just grab the skeleton unit when/if needed instead of asserting in many places?

... I am not sure @clayborg likes the assertions.

[clayborg]

In D107659#2934245, @jankratochvil wrote:

Thanks for checking it but ...

In D107659#2931836, @clayborg wrote:

Can we not just grab the skeleton unit when/if needed instead of asserting in many places?

... I am not sure @clayborg likes the assertions.

I don't mind lldbassert() calls as they are not in release builds, so it is nice to catch things during debug build testing. I prefer that asserts are not used when the program would knowingly crash immediately after the assert. I don't like asserts in code like:

assert(ptr)
ptr->foo();

This code will crash both when asserts are enabled and also when they aren't. And the llvm/clang/lldb code is used in a shared library that other tools link against. So the code really shouldn't crash when it is easy to work around and add code to avoid the crash. I still prefer the above code to look like:

assert(ptr)
if (ptr)
  ptr->foo();

As long as the program continues to function correctly without the assert, feel free to add any asserts to help catch issues during development.

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

DWARFMappedHash::ExtractDIEArray(hash_data,
                                    DIERefCallback(callback, regex.GetText()));
 }
 
 void AppleDWARFIndex::GetGlobalVariables(
     DWARFUnit &cu, llvm::function_ref<bool(DWARFDIE die)> callback) {
   if (!m_apple_names_up)
     return;
 
+  lldbassert(!cu.GetSymbolFileDWARF().GetDwoNum());
   const DWARFUnit &non_skeleton_cu = cu.GetNonSkeletonUnit();
   DWARFMappedHash::DIEInfoArray hash_data;
   m_apple_names_up->AppendAllDIEsInRange(non_skeleton_cu.GetOffset(),
                                          non_skeleton_cu.GetNextUnitOffset(),
                                          hash_data);
   DWARFMappedHash::ExtractDIEArray(hash_data, DIERefCallback(callback));
 }
 

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

public:
   /// the consumer.
   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;
+  /// \a cu must be the skeleton unit if possible, not GetNonSkeletonUnit().
   virtual void
   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,

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

for (const DebugNames::NameIndex &ni: *m_debug_names_up) {
     }
   }
 
   m_fallback.GetGlobalVariables(regex, callback);
 }
 
 void DebugNamesDWARFIndex::GetGlobalVariables(
     DWARFUnit &cu, llvm::function_ref<bool(DWARFDIE die)> callback) {
+  lldbassert(!cu.GetSymbolFileDWARF().GetDwoNum());
   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.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(
     DWARFUnit &unit, llvm::function_ref<bool(DWARFDIE die)> callback) {
+  lldbassert(!unit.GetSymbolFileDWARF().GetDwoNum());
   Index();
-  m_set.globals.FindAllEntriesForUnit(unit.GetNonSkeletonUnit(),
-                                      DIERefCallback(callback));
+  m_set.globals.FindAllEntriesForUnit(unit, 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/NameToDIE.h

public:
   void Finalize();
 
   bool Find(lldb_private::ConstString name,
             llvm::function_ref<bool(DIERef ref)> callback) const;
 
   bool Find(const lldb_private::RegularExpression &regex,
             llvm::function_ref<bool(DIERef ref)> callback) const;
 
+  /// \a unit must be the skeleton unit if possible, not GetNonSkeletonUnit().
   void
-  FindAllEntriesForUnit(const DWARFUnit &unit,
+  FindAllEntriesForUnit(DWARFUnit &unit,
                         llvm::function_ref<bool(DIERef ref)> callback) const;
 
   void
   ForEach(std::function<bool(lldb_private::ConstString name,
                              const DIERef &die_ref)> const
               &callback) const;
 
 protected:
   lldb_private::UniqueCStringMap<DIERef> m_map;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_NAMETODIE_H

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

for (const auto &entry : m_map)
     if (regex.Execute(entry.cstring.GetCString())) {
       if (!callback(entry.value))
         return false;
     }
   return true;
 }
 
 void NameToDIE::FindAllEntriesForUnit(
-    const DWARFUnit &unit,
-    llvm::function_ref<bool(DIERef ref)> callback) const {
+    DWARFUnit &s_unit, llvm::function_ref<bool(DIERef ref)> callback) const {
+  lldbassert(!s_unit.GetSymbolFileDWARF().GetDwoNum());
+  const DWARFUnit &ns_unit = s_unit.GetNonSkeletonUnit();
   const uint32_t size = m_map.GetSize();
   for (uint32_t i = 0; i < size; ++i) {
     const DIERef &die_ref = m_map.GetValueAtIndexUnchecked(i);
-    if (unit.GetSymbolFileDWARF().GetDwoNum() == die_ref.dwo_num() &&
-        unit.GetDebugSection() == die_ref.section() &&
-        unit.GetOffset() <= die_ref.die_offset() &&
-        die_ref.die_offset() < unit.GetNextUnitOffset()) {
+    if (ns_unit.GetSymbolFileDWARF().GetDwoNum() == die_ref.dwo_num() &&
+        ns_unit.GetDebugSection() == die_ref.section() &&
+        ns_unit.GetOffset() <= die_ref.die_offset() &&
+        die_ref.die_offset() < ns_unit.GetNextUnitOffset()) {
       if (!callback(die_ref))
         return;
     }
   }
 }
 
 void NameToDIE::Dump(Stream *s) {
   const uint32_t size = m_map.GetSize();

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

+// No variable should exist in this file.
+void f() {}

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

 // 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: %clang -c -o %t-3.o --target=x86_64-pc-linux -gdwarf-5 -gsplit-dwarf -gpubnames %S/Inputs/find-variable-file-3.cpp
+// RUN: ld.lld %t-1.o %t-2.o %t-3.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: lldb-test symbols --file=find-variable-file-3.cpp --find=variable \
+// RUN:   --name=notexists %t
 
 // 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