support on-demand indexing in ManualDWARFIndex

[Eric]

Summary

Currently we always do a full index in ManualDWARFIndex when any variables are looked up. This can be expensive as it requires loading every dwo. With this change, we support partial indexing where we only load what is necessary for a specific dwo when asked for variables in a specific DWARFUnit

[Eric]

I can combine this with https://reviews.llvm.org/D100299 if it would make reviewing easier.

I wasn't sure if the DWO Id was the right ID to use to find the unit. The issue is that I get the non-skeleton unit and need to search for the matching skeleton unit. I had to update some tests where the .dwos did not have a dwo id to match the skeleton unit. If there are symbol files like that in the wild this could break variable lookup for them.

[Eric]

Rebased to not depend on other cl

[labath]

I'm sorry about the delay.

Before I review this in detail, I'd like to understand your use case better. Do I understand correctly that you want to open a binary, and the _only_ thing you want to do with it is look up a bunch of variables?

AFAICS, that's the only use case where this will make a difference. Any other action will still trigger a full index, and I don't see how that could be avoided like you did here (when setting a breakpoint, for example, you don't know which compile unit it's going to be in, so you need to search through all units anyway...)

Am I missing something?

In D100771#2698860, @Eric wrote:

I can combine this with https://reviews.llvm.org/D100299 if it would make reviewing easier.

Splitting it up is great.

I wasn't sure if the DWO Id was the right ID to use to find the unit. The issue is that I get the non-skeleton unit and need to search for the matching skeleton unit. I had to update some tests where the .dwos did not have a dwo id to match the skeleton unit. If there are symbol files like that in the wild this could break variable lookup for them.

I'm pretty sure that the test issues are just a result of over-reduction.

That said, I think that a better solution to this would be remove the ->GetNonSkeletonUnit() bit from the call in SymbolFileDWARF::ParseVariablesForContext (and add to this function, where needed).

[Eric]

Changed contract of DWARFIndex::GetGlobalVariables to take a non-const reference to the skeleton unit and changed each implementation to get the skeleton unit. This allowed further changes to cache by offset using existing m_units_to_avoid rather than cache by dwo id.

[Eric]

Before I review this in detail, I'd like to understand your use case better. Do I understand correctly that you want to open a binary, and the _only_ thing you want to do with it is look up a bunch of variables?

and set breakpoints by source code line, and step, and translate the location of exceptions to source code lines...

AFAICS, that's the only use case where this will make a difference. Any other action will still trigger a full index, and I don't see how that could be avoided like you did here (when setting a breakpoint, for example, you don't know which compile unit it's going to be in, so you need to search through all units anyway...)

The other scenarios I care about are taken care of in https://reviews.llvm.org/D100299 (though only for DWARF5, which is a limitation I can live with). Please review that change as well!

I wasn't sure if the DWO Id was the right ID to use to find the unit. The issue is that I get the non-skeleton unit and need to search for the matching skeleton unit. I had to update some tests where the .dwos did not have a dwo id to match the skeleton unit. If there are symbol files like that in the wild this could break variable lookup for them.

I'm pretty sure that the test issues are just a result of over-reduction.

That said, I think that a better solution to this would be remove the ->GetNonSkeletonUnit() bit from the call in SymbolFileDWARF::ParseVariablesForContext (and add to this function, where needed).

Done. Turns out this requires changing every DWARFIndex implementation, but it's still a straightforward change that simplifies the ManualDWARFIndex change a little. Hope you like this better!

[labath]

Thank you for explaining the use case. The new implementation does look better, though I have some questions/suggestions for improvement. See inline comments for details.

I think we should also have a test case for this feature. IIUC, this feature only kicks when the setting target.preload-symbols is false, and we only have like one or two tests that exercise that. It would be good to have a test case which explicitly exercises the global variable fetching scenario without symbol preloading.

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

82–85

btw, the apple index is not compatible with split dwarf, so this change is a no-op.

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

93 ↗

(On Diff #339534)

did this sneak in from the other change?

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

373

Any reason for not passing the unit directly? We could also use the convention that nullptr means "index everything" and ditch the first argument.

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

83

What's this used for? AFAICT, the type units will be indexed exactly the first time that a "full" index is performed...

[Eric]

I'll get started on the test, but I'll probably want to use the same test file to test both CLs. What I want to do is write a case that references two dwos, only one of which actually exists, so that I can verify that certain queries can be done without attempting to load the other one.

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

82–85

Noted. I think I'll leave it as is unless you object in case that changes in the future.

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

93 ↗

(On Diff #339534)

It's actually left over from the previous iteration of this change, where I needed to get a DWO id from a const reference.

Happy to delete.

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

83

I actually index the type units on a partial index. Some tests were broken when I didn't index them.

[labath]

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

82–85

I don't think that can change without a significant (incompatible) redesign of the apple index (at which point, one could just use debug_names), but I don't mind leaving this in.

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

83

Ah, I got the logic backwards. Sorry about that.

However, this broken tests remark has me worried, and I think we should understand what's going on here. I'm worried that those broken tests were a symptom of a deeper problem, which this logic fixes only in a limited fashion.

My reasoning is this: if in the dwp+type unit scenario, we need the type units to be indexed immediately, what will happen if we compile the same source code in a different mode. For example, if we're debugging from .dwo files, we will not be indexing /all/ type units immediately -- only those that code from the same dwo file. This may be sufficient, but it also may not be -- clang tries hard to avoid producing debug info if it knows its already available elsewhere, so it's possible the type not be available in the dwo file we are looking at. Something similar could happen in the no-dwo, no-type unit scenario. In this case the type could only be present in some different compile unit, and we may not find it until we index that particular type unit.

Overall, it seems to be that if this "on-demand" indexing idea is sound, it should not need to rely on pre-indexing of type units. I could be wrong, but it seems like its at least worth having a closer look.

[Eric]

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

83

After some debugging, it appears that the problem was due to the non-skeleton units in the dwp file not being indexed because they were type units. I think I prefer to leave this as is because adding additional logic to prevent indexing the units in the dwp would be more complicated than this, and because it seems to be a common behavior in lldb to iterate over all compile units and get their global variables, and with this code the equivalent to a full index would be done but I'm not sure I could guarantee that without eagerly evaluating type units.

[Eric]

My plan for testing this is not panning out, as lldb provides no error when it fails to load symbols and no way to find out what symbols have been loaded or have been attempted, so there's no way to test that it is behaving lazily. Is there any particular thing you want me to test? Many tests are already hitting this code.

[Eric]

This change is turning out to be less important as we seem to usually avoid building a full manual index if everything it built with -gpubnames. However the other change (https://reviews.llvm.org/D100299) is still necessary to prevent eagerly loading all non-skeleton information. Let's focus on that change and I will revisit this one if it turns out customers are still hitting the manual fallback.

[Eric]

Language in Dump now distinguishes between unknown and not loaded.

[jankratochvil]

@Eric you have update inappropriate review, this patch should have been in D100299.

[Eric]

Yes I did, thank you. This patch will probably be abandoned anyway if https://reviews.llvm.org/D106355 is accepted.

[jankratochvil]

@Eric you have (again :-) ) updated inappropriate review, this patch should have been in D100299.

[Eric]

Argh. Okay, I need to close this tab.

[Eric]

Restoring previous diff.

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

 //===-- AppleDWARFIndex.h --------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_APPLEDWARFINDEX_H
 #define LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_APPLEDWARFINDEX_H
 
 #include "Plugins/SymbolFile/DWARF/DWARFIndex.h"
 #include "Plugins/SymbolFile/DWARF/HashedNameToDIE.h"
 
 namespace lldb_private {
 class AppleDWARFIndex : public DWARFIndex {
 public:
   static std::unique_ptr<AppleDWARFIndex>
   Create(Module &module, DWARFDataExtractor apple_names,
          DWARFDataExtractor apple_namespaces, DWARFDataExtractor apple_types,
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

 //===-- AppleDWARFIndex.cpp -----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Plugins/SymbolFile/DWARF/AppleDWARFIndex.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDeclContext.h"
 #include "Plugins/SymbolFile/DWARF/DWARFUnit.h"
 #include "Plugins/SymbolFile/DWARF/LogChannelDWARF.h"
 
 #include "lldb/Core/Module.h"
 #include "lldb/Symbol/Function.h"
 
 using namespace lldb_private;
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 &full_unit = cu.GetNonSkeletonUnit();
   DWARFMappedHash::DIEInfoArray hash_data;
-  m_apple_names_up->AppendAllDIEsInRange(cu.GetOffset(), cu.GetNextUnitOffset(),
-                                         hash_data);
+  m_apple_names_up->AppendAllDIEsInRange(
+      full_unit.GetOffset(), full_unit.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;
   m_apple_objc_up->FindByName(

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

 //===-- DWARFIndex.h -------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_DWARFINDEX_H
 #define LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_DWARFINDEX_H
 
 #include "Plugins/SymbolFile/DWARF/DIERef.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDIE.h"
 #include "Plugins/SymbolFile/DWARF/DWARFFormValue.h"
 
 class DWARFDeclContext;
 class DWARFDIE;
 
 namespace lldb_private {
 class DWARFIndex {
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

 //===-- DebugNamesDWARFIndex.h ---------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_DEBUGNAMESDWARFINDEX_H
 #define LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_DEBUGNAMESDWARFINDEX_H
 
 #include "Plugins/SymbolFile/DWARF/DWARFIndex.h"
 #include "Plugins/SymbolFile/DWARF/LogChannelDWARF.h"
 #include "Plugins/SymbolFile/DWARF/ManualDWARFIndex.h"
 #include "Plugins/SymbolFile/DWARF/SymbolFileDWARF.h"
 #include "lldb/Utility/ConstString.h"
 #include "llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h"
 
 namespace lldb_private {
 class DebugNamesDWARFIndex : public DWARFIndex {
 public:
   static llvm::Expected<std::unique_ptr<DebugNamesDWARFIndex>>
   Create(Module &module, DWARFDataExtractor debug_names,
          DWARFDataExtractor debug_str, SymbolFileDWARF &dwarf);
 
   void Preload() override { m_fallback.Preload(); }
 
   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

 //===-- DebugNamesDWARFIndex.cpp ------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDebugInfo.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDeclContext.h"
 #include "Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Stream.h"
 
 using namespace lldb_private;
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) {
-  uint64_t cu_offset = cu.GetOffset();
+    DWARFUnit &cu, llvm::function_ref<bool(DWARFDIE die)> callback) {
+  uint64_t cu_offset = cu.GetNonSkeletonUnit().GetOffset();
   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)
           continue;
         if (entry_or->getCUOffset() != cu_offset)

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

 //===-- ManualDWARFIndex.h --------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_MANUALDWARFINDEX_H
 #define LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_MANUALDWARFINDEX_H
 
 #include "Plugins/SymbolFile/DWARF/DWARFIndex.h"
 #include "Plugins/SymbolFile/DWARF/NameToDIE.h"
 #include "llvm/ADT/DenseSet.h"
 
 class DWARFDebugInfo;
 class SymbolFileDWARFDwo;
 
 namespace lldb_private {
 class ManualDWARFIndex : public DWARFIndex {
 public:
   ManualDWARFIndex(Module &module, SymbolFileDWARF &dwarf,
                    llvm::DenseSet<dw_offset_t> units_to_avoid = {})
       : DWARFIndex(module), m_dwarf(&dwarf),
-        m_units_to_avoid(std::move(units_to_avoid)) {}
+        m_units_to_avoid(std::move(units_to_avoid)),
+        m_has_indexed_type_units(false) {}
 
   void Preload() override { Index(); }
 
   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;
struct IndexSet {
     NameToDIE function_methods;
     NameToDIE function_selectors;
     NameToDIE objc_class_selectors;
     NameToDIE globals;
     NameToDIE types;
     NameToDIE namespaces;
   };
   void Index();
+  void IndexImpl(DWARFUnit *unit_to_index);
   void IndexUnit(DWARFUnit &unit, SymbolFileDWARFDwo *dwp, IndexSet &set);
 
   static void IndexUnitImpl(DWARFUnit &unit,
                             const lldb::LanguageType cu_language,
                             IndexSet &set);
 
   /// The DWARF file which we are indexing. Set to nullptr after the index is
   /// built.
   SymbolFileDWARF *m_dwarf;
   /// Which dwarf units should we skip while building the index.
   llvm::DenseSet<dw_offset_t> m_units_to_avoid;
+  bool m_has_indexed_type_units;
 
   IndexSet m_set;
 };
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_MANUALDWARFINDEX_H

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

 //===-- ManualDWARFIndex.cpp ----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Plugins/SymbolFile/DWARF/ManualDWARFIndex.h"
 #include "Plugins/Language/ObjC/ObjCLanguage.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDebugInfo.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDeclContext.h"
 #include "Plugins/SymbolFile/DWARF/LogChannelDWARF.h"
 #include "Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Progress.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/Timer.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/ThreadPool.h"
 
 using namespace lldb_private;
 using namespace lldb;
 
-void ManualDWARFIndex::Index() {
+void ManualDWARFIndex::Index() { IndexImpl(nullptr); }
+
+void ManualDWARFIndex::IndexImpl(DWARFUnit *unit_to_index) {
   if (!m_dwarf)
     return;
 
   SymbolFileDWARF &main_dwarf = *m_dwarf;
+
+  if (!unit_to_index)
     m_dwarf = nullptr;
 
   LLDB_SCOPED_TIMERF("%p", static_cast<void *>(&main_dwarf));
 
   DWARFDebugInfo &main_info = main_dwarf.DebugInfo();
   SymbolFileDWARFDwo *dwp_dwarf = main_dwarf.GetDwpSymbolFile().get();
   DWARFDebugInfo *dwp_info = dwp_dwarf ? &dwp_dwarf->DebugInfo() : nullptr;
 
   std::vector<DWARFUnit *> units_to_index;
   units_to_index.reserve(main_info.GetNumUnits() +
                          (dwp_info ? dwp_info->GetNumUnits() : 0));
 
   // Process all units in the main file, as well as any type units in the dwp
   // file. Type units in dwo files are handled when we reach the dwo file in
   // IndexUnit.
   for (size_t U = 0; U < main_info.GetNumUnits(); ++U) {
     DWARFUnit *unit = main_info.GetUnitAtIndex(U);
-    if (unit && m_units_to_avoid.count(unit->GetOffset()) == 0)
+    if (unit && m_units_to_avoid.count(unit->GetOffset()) == 0) {
+      if (!unit_to_index || unit->GetOffset() == unit_to_index->GetOffset())
         units_to_index.push_back(unit);
     }
-  if (dwp_info && dwp_info->ContainsTypeUnits()) {
+  }
+
+  if (unit_to_index && !units_to_index.empty())
+    m_units_to_avoid.insert(unit_to_index->GetOffset()); // Don't index this again
+
+  if (!m_has_indexed_type_units && dwp_info && dwp_info->ContainsTypeUnits()) {
+    m_has_indexed_type_units = true;
     for (size_t U = 0; U < dwp_info->GetNumUnits(); ++U) {
       if (auto *tu = llvm::dyn_cast<DWARFTypeUnit>(dwp_info->GetUnitAtIndex(U)))
         units_to_index.push_back(tu);
     }
   }
 
   if (units_to_index.empty())
     return;

 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) {
-  Index();
-  m_set.globals.FindAllEntriesForUnit(unit, DIERefCallback(callback));
+    DWARFUnit &unit, llvm::function_ref<bool(DWARFDIE die)> callback) {
+  IndexImpl(&unit);
+  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,