[lldb] Make expression lookup find only the first variable in the closest module

[teemperor]

Summary

Usually when searching for variables we stop after we find the first one (see for example
ClangExpressionDeclMap::LookupLocalVariable or
ClangExpressionDeclMap::FindGlobalVariable), as there are legitimate reasons for multiple
variables with the same name being defined in the ASTs that LLDB searches. For example,
a header with at static global variable that is included by multiple shared libraries will
produce such duplicate variables.

However, when resolving lookups in namespaces, LLDB currently iterates over all lldb_private::Modules
that are known to contain the given namespace and then searches in all of these modules for
the given lookup. This causes that when a static global variable from a header is included in
different shared libraries, LLDB will add all these variables to the lookup results. The results
will then cause Clang to emit a diagnostic about an ambiguous lookup and the expression
will fail to compile.

This patch solves this by only adding the first variable that is found to the lookup and ignoring
the others. It also changes the search order to first look in the current module of the
execution context and then iterate over all other modules, so that LLDB will always find the
'closest' definition of the given variable.

Also adds a test for the case where all these variables are in one module, but solving that issue
is yet another patch.

Just to state the obvious: Picking the first variable won't work for variable templates, but we anyway
don't really support those yet (beside pretending they are just normal variables). Once we properly
model those this logic just needs minor tweaking to make them work as expected.

Fixes rdar://63222982

[shafik]

lldb/test/API/lang/cpp/namespace_search/TestNamespaceSearch.py

93

It have read this a few times, I don't quite understand why this case and the ones below don't work.

[teemperor]

lldb/test/API/lang/cpp/namespace_search/TestNamespaceSearch.py

93

It's the test compiled into one executable (=module), so searching the closest module first isn't enough to find the right variable. And the current code for searching one module just picks the first VarDecl it finds in an AST (there is a better logic for non-namespace globals apparently, but it doesn't work for namespaces yet).

Better illustrated:

AST dump of test cases above:

Module 1: <- we'll first search this one with this patch
TranslationUnitDecl <<invalid sloc>> <invalid sloc>
|-NamespaceDecl <<invalid sloc>> <invalid sloc> Foo
| |-VarDecl <<invalid sloc>> <invalid sloc> VarNotInMain 'const char *' <- right variable
Module 2:
TranslationUnitDecl <<invalid sloc>> <invalid sloc>
|-NamespaceDecl <<invalid sloc>> <invalid sloc> Foo
| `-VarDecl <<invalid sloc>> <invalid sloc> VarNotInMain 'const char *'

AST dump of test cases below:

Module 1: <- we'll first search this one with this patch
TranslationUnitDecl <<invalid sloc>> <invalid sloc>
|-NamespaceDecl <<invalid sloc>> <invalid sloc> Foo
| |-VarDecl <<invalid sloc>> <invalid sloc> VarNotInMain 'const char *' <- tu0's variable
| `-VarDecl <<invalid sloc>> <invalid sloc> VarNotInMain 'const char *' <- tu1's variable (looks identical to the one above for LLDB).

[labath]

I can't say I'm super familiar with this stuff (e.g., I never understood why we need to handle the global scope specially everywhere), but the general idea seem pretty simple and straight-forward enough. Some minor comments inline.

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

708

I believe current_namespace == n should be enough. Or maybe use ClangASTImporter::NamespaceMapItem *current_namespace instead of the optional (compare with `current_namespace == &n) and avoid the copies and value comparisons...

lldb/test/API/lang/cpp/namespace_search/TestNamespaceSearch.py

22

what does the ld stand for ?

24

If you use self.registerSharedLibrariesWithTarget to get the environment, then the test will work correctly in remote setups too.

lldb/test/API/lang/cpp/namespace_search/tu0.cpp

12

I'm pretty sure that defining multiple functions like this (regardless of whether they are inline or not) is an ODR violation. If you did not want to test anything specific to odr violations, then I suggest making the function static instead. (And if you did, then I'd like to know what it is.)

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h

private:
 
   /// Deallocate struct variables
   void DisableStructVars() { m_struct_vars.reset(); }
 
   /// Get this parser's ID for use in extracting parser- and JIT-specific data
   /// from persistent variables.
   uint64_t GetParserID() { return (uint64_t) this; }
 
+  /// Returns the module belonging to the current execution context or a nullptr
+  /// if no module can be found for the current execution context.
+  lldb::ModuleSP GetCurrentExecutionContextModule();
+
   /// Should be called on all copied functions.
   void MaybeRegisterFunctionBody(clang::FunctionDecl *copied_function_decl);
 
   /// Searches the persistent decls of the target for entities with the
   /// given name.
   ///
   /// \param[in] context
   ///     The NameSearchContext that can construct Decls for this name.

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

TypeSystemClang *ClangExpressionDeclMap::GetTypeSystemClang() {
   CompilerDeclContext frame_decl_context = sym_ctx.block->GetDeclContext();
   if (!frame_decl_context)
     return nullptr;
 
   return llvm::dyn_cast_or_null<TypeSystemClang>(
       frame_decl_context.GetTypeSystem());
 }
 
+/// Searches the given namespace map for an NamespaceMapItem that fits to the
+/// given module.
+/// \return The NamespaceMapItem for the given module or None if there is no
+///         fititing NamespaceMapItem.
+static llvm::Optional<ClangASTImporter::NamespaceMapItem>
+FindExecutionContextNamespace(lldb::ModuleSP module,
+                              ClangASTImporter::NamespaceMapSP namespace_map) {
+  auto it = llvm::find_if(
+      *namespace_map, [&module](const ClangASTImporter::NamespaceMapItem &i) {
+        return i.first == module;
+      });
+  if (it != namespace_map->end())
+    return *it;
+  return llvm::None;
+}
+
 // Interface for ClangASTSource
 
 void ClangExpressionDeclMap::FindExternalVisibleDecls(
     NameSearchContext &context) {
   assert(m_ast_context);
 
   const ConstString name(context.m_decl_name.getAsString().c_str());
 
ClangASTImporter::NamespaceMapSP namespace_map =
         m_ast_importer_sp->GetNamespaceMap(namespace_context);
 
     if (!namespace_map)
       return;
 
     LLDB_LOGV(log, "  CEDM::FEVD Inspecting (NamespaceMap*){0:x} ({1} entries)",
               namespace_map.get(), namespace_map->size());
 
+    // First search the namespace in the current module we are evaluating the
+    // expression in.
+    llvm::Optional<ClangASTImporter::NamespaceMapItem> current_namespace;
+    if (lldb::ModuleSP module = GetCurrentExecutionContextModule())
+      current_namespace = FindExecutionContextNamespace(module, namespace_map);
+    if (current_namespace)
+      FindExternalVisibleDecls(context, current_namespace->first,
+                               current_namespace->second);
+
+    // Now search all other modules/namespaces except the current one.
+    // We can't early exit here as the other modules might contain function
+    // overloads we need for overload resolution.
     for (ClangASTImporter::NamespaceMapItem &n : *namespace_map) {
+      if (current_namespace && n == *current_namespace)
+        continue;
       LLDB_LOG(log, "  CEDM::FEVD Searching namespace {0} in module {1}",
                n.second.GetName(), n.first->GetFileSpec().GetFilename());
 
       FindExternalVisibleDecls(context, n.first, n.second);
     }
   } else if (isa<TranslationUnitDecl>(context.m_decl_context)) {
     CompilerDeclContext namespace_decl;
 
clang::NamedDecl *ClangExpressionDeclMap::GetPersistentDecl(ConstString name) {
 
   TypeSystemClang::GetScratch(*target);
 
   if (!m_parser_vars->m_persistent_vars)
     return nullptr;
   return m_parser_vars->m_persistent_vars->GetPersistentDecl(name);
 }
 
+ModuleSP ClangExpressionDeclMap::GetCurrentExecutionContextModule() {
+  if (!m_struct_vars)
+    return lldb::ModuleSP();
+  StackFrame *frame = m_parser_vars->m_exe_ctx.GetFramePtr();
+  if (!frame)
+    return lldb::ModuleSP();
+  Block *block = frame->GetFrameBlock();
+  if (!block)
+    return lldb::ModuleSP();
+  return block->CalculateSymbolContextModule();
+}
+
 void ClangExpressionDeclMap::SearchPersistenDecls(NameSearchContext &context,
                                                   const ConstString name) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
   NamedDecl *persistent_decl = GetPersistentDecl(name);
 
   if (!persistent_decl)
     return;

 
 void ClangExpressionDeclMap::AddOneVariable(NameSearchContext &context,
                                             VariableSP var,
                                             ValueObjectSP valobj) {
   assert(m_parser_vars.get());
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
 
+  if (context.MaybeGetFoundVariable()) {
+    LLDB_LOG(log,
+             "Not adding duplicate variable {0} to lookup because"
+             " we already found a variable.",
+             var->GetDecl().GetName());
+    return;
+  }
+
   TypeFromUser ut;
   TypeFromParser pt;
   Value var_location;
 
   if (!GetVariableValue(var, var_location, &ut, &pt))
     return;
 
   clang::QualType parser_opaque_type =

lldb/source/Plugins/ExpressionParser/Clang/NameSearchContext.h

struct NameSearchContext {
 
   /// Add a NamedDecl to the list of results.
   ///
   /// \param[in] decl
   ///     The NamedDecl, usually returned as the result
   ///     of querying a DeclContext.
   void AddNamedDecl(clang::NamedDecl *decl);
 
+  /// Returns the found variable for this lookup or a nullptr if no variable
+  /// has been found so far.
+  clang::VarDecl *MaybeGetFoundVariable() const;
+
 private:
   clang::ASTContext &GetASTContext() const {
     return m_clang_ts.getASTContext();
   }
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_SOURCE_PLUGINS_EXPRESSIONPARSER_CLANG_NAME_SEARCH_CONTEXT_H

lldb/source/Plugins/ExpressionParser/Clang/NameSearchContext.cpp

 void NameSearchContext::AddLookupResult(clang::DeclContextLookupResult result) {
   for (clang::NamedDecl *decl : result)
     m_decls.push_back(decl);
 }
 
 void NameSearchContext::AddNamedDecl(clang::NamedDecl *decl) {
   m_decls.push_back(decl);
 }
+
+VarDecl *NameSearchContext::MaybeGetFoundVariable() const {
+  for (clang::Decl *d : m_decls)
+    if (VarDecl *v = dyn_cast<VarDecl>(d))
+      return v;
+  return nullptr;
+}

lldb/test/API/lang/cpp/namespace_search/Makefile

+CXX_SOURCES := main.cpp
+USE_LIBDL := 1
+
+ifneq "$(BUILD_SINGLE_EXE)" ""
+# The test files all compiled into one executable.
+CXX_SOURCES += tu0.cpp tu1.cpp
+else
+# The test files but tu0 and tu1 are their own shared library.
+LD_EXTRAS := -L. -llib_tu0 -llib_tu1
+a.out: lib_tu0 lib_tu1
+
+lib_tu0:
+  $(MAKE) -f $(MAKEFILE_RULES) \
+  DYLIB_ONLY=YES DYLIB_CXX_SOURCES=tu0.cpp DYLIB_NAME=lib_tu0
+
+lib_tu1:
+  $(MAKE) -f $(MAKEFILE_RULES) \
+  DYLIB_ONLY=YES DYLIB_CXX_SOURCES=tu1.cpp DYLIB_NAME=lib_tu1
+endif
+
+include Makefile.rules

lldb/test/API/lang/cpp/namespace_search/TestNamespaceSearch.py

+"""
+Tests that LLDB searches namespaces across all modules in the correct order
+when resolving variables and functions.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def common_setup(self, build_single_exe):
+        if build_single_exe:
+          self.build(dictionary={"BUILD_SINGLE_EXE":1})
+        else:
+          self.build()
+
+        # Make sure our test shared libraries can be found.
+        self.ld_launch_info = lldb.SBLaunchInfo(None)
+        self.ld_launch_info.SetWorkingDirectory(self.getBuildDir())
+        env_expr = self.platformContext.shlib_environment_var + "=" + self.getBuildDir()
+        self.ld_launch_info.SetEnvironmentEntries([env_expr], True)
+
+    def common_tests(self, closest_tu):
+        """
+        Tests shared with all other tests. 'closest_tu' is the filename of
+        the closest tu*.cpp file (i.e., "tu0.cpp" or "tu1.cpp") that LLDB
+        should prefer when resolving variables.
+        """
+        closest_tu = '"' + closest_tu + '"'
+        # Make sure LLDB can find 'Foo::VarNotInMain' in the other TUs.
+        self.expect_expr("::Foo::VarNotInMain", result_summary=closest_tu)
+        self.expect_expr("Foo::VarNotInMain", result_summary=closest_tu)
+
+        # Find the variable that is only in the main.cpp TU.
+        self.expect_expr("::Foo::VarOnlyInMain", result_summary="\"only-main.cpp\"")
+        self.expect_expr("Foo::VarOnlyInMain", result_summary="\"only-main.cpp\"")
+
+        # Make sure LLDB still finds all functions in TUs to correctly resolve overloaded functions.
+        # In tu0 and tu1 there is an overload.
+        self.expect_expr("Foo::OverloadedInlineFunction(1)", result_summary=closest_tu)
+        # Only in main.cpp
+        self.expect_expr("Foo::OverloadedInlineFunction(1, 2)", result_summary="\"main.cpp\"")
+
+    def test_stopped_in_main_shared(self):
+        """ Tests lookup when stopped in main function with shared libraries"""
+        self.common_setup(build_single_exe=False)
+        lldbutil.run_to_source_breakpoint(self, "// break in main", lldb.SBFileSpec("main.cpp"), launch_info=self.ld_launch_info)
+        self.common_tests("tu0.cpp")
+
+        # Make sure LLDB prefers the variable in the current file.
+        self.expect_expr("::Foo::VarAlsoInMain", result_summary="\"also-main.cpp\"")
+        self.expect_expr("Foo::VarAlsoInMain", result_summary="\"also-main.cpp\"")
+
+    def test_stopped_in_shared_lib(self):
+        """ Tests lookup when stopped in tu1 with shared libraries"""
+        self.common_setup(build_single_exe=False)
+        # First stop in the main executable to make sure the shared library is loaded.
+        lldbutil.run_to_source_breakpoint(self, "// break before shared", lldb.SBFileSpec("main.cpp"), launch_info=self.ld_launch_info)
+        # Now set the breakpoint in the shared library and continue there.
+        breakpoint = self.target().BreakpointCreateBySourceRegex(
+            "// break in other tu", lldb.SBFileSpec("tu1.cpp"))
+        self.assertTrue(breakpoint.IsValid())
+        stopped_threads = lldbutil.continue_to_breakpoint(self.process(), breakpoint)
+        self.assertEqual(len(stopped_threads), 1)
+
+        self.common_tests("tu1.cpp")
+
+        # Make sure LLDB can find 'Foo::VarAlsoInMain' from the current shared library.
+        self.expect_expr("::Foo::VarAlsoInMain", result_summary="\"tu1.cpp\"")
+        self.expect_expr("Foo::VarAlsoInMain", result_summary="\"tu1.cpp\"")
+
+    def test_stopped_in_main(self):
+        """ Tests lookup when stopped in main function without shared libaries"""
+        self.common_setup(build_single_exe=True)
+        lldbutil.run_to_source_breakpoint(self, "// break in main", lldb.SBFileSpec("main.cpp"))
+        self.common_tests("tu0.cpp")
+
+        # Make sure LLDB prefers the variable in the current file.
+        self.expect_expr("::Foo::VarAlsoInMain", result_summary="\"also-main.cpp\"")
+        self.expect_expr("Foo::VarAlsoInMain", result_summary="\"also-main.cpp\"")
+
+    def test_stopped_in_other_tu(self):
+        """ Tests lookup when stopped in tu1 without shared libaries"""
+        self.common_setup(build_single_exe=True)
+        lldbutil.run_to_source_breakpoint(self, "// break in other tu", lldb.SBFileSpec("tu1.cpp"))
+
+        # FIXME: This finds always finds the tu0.cpp variables, but LLDB is stopped
+        # in tu1.cpp and should find the variables there.
+        self.common_tests("tu0.cpp") # FIXME: This should be tu1.cpp.
+
+        # Make sure LLDB can find 'Foo::VarAlsoInMain' from the current shared library.
+        # FIXME: This should find the tu1 variables instead.
+        self.expect_expr("::Foo::VarAlsoInMain", result_summary="\"also-main.cpp\"")
+        self.expect_expr("Foo::VarAlsoInMain", result_summary="\"also-main.cpp\"")

lldb/test/API/lang/cpp/namespace_search/main.cpp

+// Forward declare functions in tu0/tu1.
+int setup_tu0();
+int setup_tu1();
+
+namespace Foo {
+static const char *VarAlsoInMain = "also-main.cpp";
+static const char *VarOnlyInMain = "only-main.cpp";
+inline const char *OverloadedInlineFunction(int i, int j) { return "main.cpp"; }
+} // namespace Foo
+
+int main() {
+  const char *x = Foo::VarAlsoInMain;
+  x = Foo::VarOnlyInMain;
+  x = Foo::OverloadedInlineFunction(1, 2);
+  x = "foo"; // break before shared
+  setup_tu0();
+  setup_tu1();
+  return 0; // break in main
+}

lldb/test/API/lang/cpp/namespace_search/tu0.cpp

+// Same contents as tu1.cpp. Used for testing in which order LLDB searches
+// modules/TUs during lookup.
+// Note: This file should always come before tu1.cpp (both alphabetically
+// and in compiler/linker invocations) so that all checks that evaluate
+// expressions in tu1.cpp know that LLDB found declarations in tu1 not just
+// by picking the first declaration it found when iterating over some list
+// of modules/TUs.
+
+namespace Foo {
+static const char *VarAlsoInMain = "tu0.cpp";
+static const char *VarNotInMain = "tu0.cpp";
+inline const char *OverloadedInlineFunction(int i) { return "tu0.cpp"; }
+} // namespace Foo
+
+const char *setup_tu0() {
+  const char *x = Foo::OverloadedInlineFunction(1);
+  x = Foo::VarAlsoInMain;
+  x = Foo::VarNotInMain;
+  return x;
+}

lldb/test/API/lang/cpp/namespace_search/tu1.cpp

+namespace Foo {
+static const char *VarAlsoInMain = "tu1.cpp";
+static const char *VarNotInMain = "tu1.cpp";
+inline const char *OverloadedInlineFunction(int i) { return "tu1.cpp"; }
+} // namespace Foo
+
+const char *setup_tu1() {
+  const char *x = Foo::OverloadedInlineFunction(1);
+  x = Foo::VarAlsoInMain;
+  x = Foo::VarNotInMain;
+  return x; // break in other tu
+}

lldb/unittests/Expression/CMakeLists.txt

 add_lldb_unittest(ExpressionTests
   ClangParserTest.cpp
   ClangExpressionDeclMapTest.cpp
   DiagnosticManagerTest.cpp
   DWARFExpressionTest.cpp
   CppModuleConfigurationTest.cpp
+  NameSearchContextTest.cpp
 
   LINK_LIBS
     lldbCore
     lldbPluginExpressionParserClang
     lldbPluginTypeSystemClang
     lldbUtility
     lldbUtilityHelpers
     LLVMTestingSupport
   )

lldb/unittests/Expression/NameSearchContextTest.cpp

+//===-- NameSearchContextTest.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/ExpressionParser/Clang/NameSearchContext.h"
+#include "TestingSupport/SubsystemRAII.h"
+#include "TestingSupport/Symbol/ClangTestUtils.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace lldb_private;
+
+namespace {
+struct NameSearchContextTest : public testing::Test {
+  SubsystemRAII<FileSystem, HostInfo> subsystems;
+};
+} // namespace
+
+TEST_F(NameSearchContextTest, AlreadyFoundVarDecl) {
+  TypeSystemClang ts("test AST", HostInfo::GetTargetTriple());
+  llvm::SmallVector<NamedDecl *, 4> decls;
+  DeclarationName name = clang_utils::getDeclarationName(ts, "name");
+  NameSearchContext context(ts, decls, name, ts.GetTranslationUnitDecl());
+  // Default state is that no variable has been found.
+  EXPECT_FALSE(context.MaybeGetFoundVariable());
+
+  CompilerType record = clang_utils::createRecord(ts, "some_record");
+  context.AddNamedDecl(ClangUtil::GetAsTagDecl(record));
+  // Didn't find a variable, so AlreadyFoundVarDecl should still be false.
+  EXPECT_FALSE(context.MaybeGetFoundVariable());
+
+  QualType int_type(ts.getASTContext().IntTy);
+  VarDecl *var = ts.CreateVariableDeclaration(
+      ts.GetTranslationUnitDecl(), OptionalClangModuleID(), "name", int_type);
+  context.AddNamedDecl(var);
+  // Variable was found, so AlreadyFoundVarDecl should now be true.
+  EXPECT_TRUE(context.MaybeGetFoundVariable());
+
+  // Add another record and make sure AlreadyFoundVarDecl stays true.
+  CompilerType another_record = clang_utils::createRecord(ts, "some_record");
+  context.AddNamedDecl(ClangUtil::GetAsTagDecl(another_record));
+  EXPECT_TRUE(context.MaybeGetFoundVariable());
+}