[lldb][DWARF] Search for symbols in all external modules

[DavidSpickett]

The way this code was updated in dd95877 meant that if the first module did not have the symbol, the iteration stopped as returning true means stop. So only if every module had the symbol would we find it, in the last module.

Invert the condition to break when we find the first instance, which is what the previous code did.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: David Spickett (DavidSpickett)

Changes

The way this code was updated in dd95877 meant that if the first module did not have the symbol, the iteration stopped as returning true means stop. So only if every module had the symbol would we find it, in the last module.

Invert the condition to break when we find the first instance, which is what the previous code did.

---

Full diff: https://github.com/llvm/llvm-project/pull/75927.diff

1 Files Affected:

• (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+1-1)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 334876620249fc..a07fa760b1b401 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -175,7 +175,7 @@ TypeSP DWARFASTParserClang::ParseTypeFromClangModule(const SymbolContext &sc,
         *sc.comp_unit, results.GetSearchedSymbolFiles(), [&](Module &module) {
           module.FindTypes(query, results);
           pcm_type_sp = results.GetTypeMap().FirstType();
-          return !pcm_type_sp;
+          return pcm_type_sp != nullptr;
         });
   }
 

[DavidSpickett]

lldb/include/lldb/Symbol/CompileUnit.h

  /// \param[in] lambda
  ///     The lambda that should be applied to every function. The lambda can
  ///     return true if the iteration should be aborted earlier.
  ///
  /// \return
  ///     If the lambda early-exited, this function returns true to
  ///     propagate the early exit.
  virtual bool ForEachExternalModule(
      llvm::DenseSet<lldb_private::SymbolFile *> &visited_symbol_files,
      llvm::function_ref<bool(Module &)> lambda);

I would have guessed true means continue.

Also, changing this didn't break any tests. So it would be nice to cover this but I don't know how. Would the whole test suite have to be built with modules enabled?

[Michael137]

LGTM

[Michael137]

Would the whole test suite have to be built with modules enabled?

Hmmm we used to have a debug-info category for gmodules which ran each test with -gmodules. But in the vast majority of cases that wasn't actually adding any useful coverage so we removed it in favour of gmodules-oriented tests that explicitly add @add_test_categories(["gmodules"]). There's a handful of gmodules tests in lldb/test/API/lang/cpp/gmodules, but looks like they don't cover this codepath.

[DavidSpickett]

I would have thought lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py would have failed then as it has >1 module.

Do you know what "external module" means in this context, as opposed to just module? The code searches "the clang Module" first, then the external modules.

[Michael137]

I would have thought lldb/test/API/lang/cpp/gmodules/templates/TestGModules.py would have failed then as it has >1 module.

Do you know what "external module" means in this context, as opposed to just module? The code searches "the clang Module" first, then the external modules.

Looks like external module are DW_TAG_compile_units that have a DW_AT_GNU_dwo_name breadcrumb pointing to some module (described here: https://lldb.llvm.org/resources/extensions.html)

The executable for TestGModules does have those, but looks like the first FindTypes call finds the type and we don't iterate over the "external modules".

I wonder if this "search all breadcrumbs" codepath only gets triggered when we try to parse a forward declaration under a DW_TAG_module? @adrian-prantl

[DavidSpickett]

Didn't want to leave this broken over the holiday break so I've merged it as obvious.

I would still like to add a test so if @adrian-prantl or anyone else can tell me how to write one, I'll do that as a follow up. Need some clues as I've not used modules before.