[lldb][ClangExpression] Filter out non-root namespaces in FindNamespace

[Michael137]

Summary

Summary

In a program such as:

namespace A {
namespace B {
struct Bar {};
}
}

namespace B {
struct Foo {};
}

...LLDB would run into issues such as:

(lldb) expr ::B::Foo f
error: expression failed to parse:
error: <user expression 0>:1:6: no type named 'Foo' in namespace 'A::B'
::B::Foo f
~~~~~^

This is because the SymbolFileDWARF::FindNamespace implementation
will return *any* namespace it finds if the parent_decl_ctx provided
is empty. In FindExternalVisibleDecls we use this API to find the
namespace that symbol B refers to. If A::B happened to be the one
that SymbolFileDWARF::FindNamespace looked at first, we would try
to find struct Foo in A::B. Hence the error.

This patch proposes a new flag to SymbolFileDWARF::FindNamespace
allows us to only consider top-level namespaces in our search, which is what
FindExternalVisibleDecls is attempting anyway; it just never
accounted for multiple namespaces of the same name.

Testing

• Added API test-case

[Michael137]

• Add docs

[Michael137]

• Merge into single API

[Michael137]

• Adjust test-case. Add FIXME

[Michael137]

lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py

197 ↗

(On Diff #510489)

I need to double check how exactly this used to work and whether there's a fix for it on top of the current patch. But not supporting this seems less problematic than choosing the wrong namespace

[aprantl]

Just out of curiosity:

namespace A {
namespace B {
namespace C {
struct Bar {};
}
}
}

namespace B {
namespace C {
struct Foo {};
}
}

Does this work?

(lldb) expr C::Foo f

lldb/include/lldb/Symbol/SymbolFile.h

342

This looks like a great opportunity to add a Doxygen comment in the base class!

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

704

Maybe comment why?

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

2339

I find this condition with its double negative really hard to understand. Would it be easier to read as

if (!decl_ctx.IsValid()) {
    if (only_root_namespaces)
      return die.GetParent().Tag() == dwarf::DW_TAG_compile_unit;
   return true;
}

?

[Michael137]

In D147436#4241975, @aprantl wrote:

Just out of curiosity:

namespace A {
namespace B {
namespace C {
struct Bar {};
}
}
}

namespace B {
namespace C {
struct Foo {};
}
}

Does this work?

(lldb) expr C::Foo f

Yup that does currently work. With this patch it wouldn't anymore

[Michael137]

lldb/test/API/lang/cpp/namespace/TestNamespaceLookup.py

197 ↗

(On Diff #510489)

I checked how expression evaluation currently works at namespace scope and it does rely on FindExternalVisibleDecls including all namespaces (not only top-level) in its search. We do the additional filtering for which function symbol to pick based on SymbolContext after the fact (in LookupFunction). This seems rather fragile but there is a decent amount of code to make this happen so I wouldn't want to just blindly stop supporting it.

[aprantl]

So the new bool flag effectively implements the leading :: separator in the lookup. Seems reasonable!

[Michael137]

• Only search for root namespace if we're doing a qualified lookup

[Michael137]

• Move doxygen comment
• Add source comment

[Michael137]

• Make condition more readable
• Fix test

[Michael137]

• Fix test

lldb/include/lldb/Symbol/SymbolFile.h

virtual void GetTypes(lldb_private::SymbolContextScope *sc_scope,
                         lldb::TypeClass type_mask,
                         lldb_private::TypeList &type_list) = 0;
 
   virtual void PreloadSymbols();
 
   virtual llvm::Expected<lldb::TypeSystemSP>
   GetTypeSystemForLanguage(lldb::LanguageType language) = 0;
 
+  /// Finds a namespace of name \ref name and whose parent
+  /// context is \ref parent_decl_ctx.
+  ///
+  /// If \code{.cpp} !parent_decl_ctx.IsValid() \endcode
+  /// then this function will consider all namespaces that
+  /// match the name. If \ref only_root_namespaces is
+  /// true, only consider in the search those DIEs that
+  /// represent top-level namespaces.
   virtual CompilerDeclContext
-  FindNamespace(ConstString name, const CompilerDeclContext &parent_decl_ctx) {
+  FindNamespace(ConstString name, const CompilerDeclContext &parent_decl_ctx,
+                bool only_root_namespaces = false) {
     return CompilerDeclContext();
   }
 
   virtual ObjectFile *GetObjectFile() = 0;
   virtual const ObjectFile *GetObjectFile() const = 0;
   virtual ObjectFile *GetMainObjectFile() = 0;
 
   virtual std::vector<std::unique_ptr<CallEdge>>
   ParseCallEdgesInFunction(UserID func_id) {
     return {};

lldb/include/lldb/Symbol/SymbolFileOnDemand.h

public:
 
   void GetTypes(lldb_private::SymbolContextScope *sc_scope,
                 lldb::TypeClass type_mask,
                 lldb_private::TypeList &type_list) override;
 
   llvm::Expected<lldb::TypeSystemSP>
   GetTypeSystemForLanguage(lldb::LanguageType language) override;
 
-  lldb_private::CompilerDeclContext FindNamespace(
-      lldb_private::ConstString name,
-      const lldb_private::CompilerDeclContext &parent_decl_ctx) override;
+  lldb_private::CompilerDeclContext
+  FindNamespace(lldb_private::ConstString name,
+                const lldb_private::CompilerDeclContext &parent_decl_ctx,
+                bool only_root_namespaces) override;
 
   std::vector<std::unique_ptr<lldb_private::CallEdge>>
   ParseCallEdgesInFunction(UserID func_id) override;
 
   lldb::UnwindPlanSP
   GetUnwindPlan(const Address &address,
                 const RegisterInfoResolver &resolver) override;
 

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

for (lldb::ModuleSP image : m_target->GetImages().Modules()) {
 
     CompilerDeclContext found_namespace_decl;
 
     SymbolFile *symbol_file = image->GetSymbolFile();
 
     if (!symbol_file)
       continue;
 
-    found_namespace_decl = symbol_file->FindNamespace(name, namespace_decl);
+    // If namespace_decl is not valid, 'FindNamespace' would look for
+    // any namespace called 'name' (ignoring parent contexts) and return
+    // the first one it finds. Thus if we're doing a qualified lookup only
+    // consider root namespaces. E.g., in an expression ::A::B::Foo, the
+    // lookup of ::A will result in a qualified lookup. Note, namespace
+    // disambiguation for function calls are handled separately in
+    // SearchFunctionsInSymbolContexts.
+    const bool find_root_namespaces =
+        context.m_decl_context &&
+        context.m_decl_context->shouldUseQualifiedLookup();
+    found_namespace_decl = symbol_file->FindNamespace(
+        name, namespace_decl, /* only root namespaces */ find_root_namespaces);
 
     if (found_namespace_decl) {
       context.m_namespace_map->push_back(
           std::pair<lldb::ModuleSP, CompilerDeclContext>(image,
                                                          found_namespace_decl));
 
       LLDB_LOG(log, "  CAS::FEVD Found namespace {0} in module {1}", name,
                image->GetFileSpec().GetFilename());

lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.h

public:
 
   llvm::Expected<lldb::TypeSystemSP>
   GetTypeSystemForLanguage(lldb::LanguageType language) override {
     return llvm::make_error<llvm::StringError>(
         "SymbolFileBreakpad does not support GetTypeSystemForLanguage",
         llvm::inconvertibleErrorCode());
   }
 
-  CompilerDeclContext
-  FindNamespace(ConstString name,
-                const CompilerDeclContext &parent_decl_ctx) override {
+  CompilerDeclContext FindNamespace(ConstString name,
+                                    const CompilerDeclContext &parent_decl_ctx,
+                                    bool only_root_namespaces) override {
     return CompilerDeclContext();
   }
 
   void AddSymbols(Symtab &symtab) override;
 
   llvm::Expected<lldb::addr_t> GetParameterStackSize(Symbol &symbol) override;
 
   lldb::UnwindPlanSP

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

public:
 
   void GetTypes(lldb_private::SymbolContextScope *sc_scope,
                 lldb::TypeClass type_mask,
                 lldb_private::TypeList &type_list) override;
 
   llvm::Expected<lldb::TypeSystemSP>
   GetTypeSystemForLanguage(lldb::LanguageType language) override;
 
-  lldb_private::CompilerDeclContext FindNamespace(
-      lldb_private::ConstString name,
-      const lldb_private::CompilerDeclContext &parent_decl_ctx) override;
+  lldb_private::CompilerDeclContext
+  FindNamespace(lldb_private::ConstString name,
+                const lldb_private::CompilerDeclContext &parent_decl_ctx,
+                bool only_root_namespaces) override;
 
   void PreloadSymbols() override;
 
   std::recursive_mutex &GetModuleMutex() const override;
 
   // PluginInterface protocol
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 
public:
   GetDwoSymbolFileForCompileUnit(DWARFUnit &dwarf_cu,
                                  const DWARFDebugInfoEntry &cu_die);
 
   /// If this is a DWARF object with a single CU, return its DW_AT_dwo_id.
   std::optional<uint64_t> GetDWOId();
 
   static bool
   DIEInDeclContext(const lldb_private::CompilerDeclContext &parent_decl_ctx,
-                   const DWARFDIE &die);
+                   const DWARFDIE &die, bool only_root_namespaces = false);
 
   std::vector<std::unique_ptr<lldb_private::CallEdge>>
   ParseCallEdgesInFunction(lldb_private::UserID func_id) override;
 
   void Dump(lldb_private::Stream &s) override;
 
   void DumpClangAST(lldb_private::Stream &s) override;
 

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

if (GetFunction(die, sc)) {
     sc_list.Append(sc);
     return true;
   }
 
   return false;
 }
 
 bool SymbolFileDWARF::DIEInDeclContext(const CompilerDeclContext &decl_ctx,
-                                       const DWARFDIE &die) {
+                                       const DWARFDIE &die,
+                                       bool only_root_namespaces) {
   // If we have no parent decl context to match this DIE matches, and if the
   // parent decl context isn't valid, we aren't trying to look for any
   // particular decl context so any die matches.
-  if (!decl_ctx.IsValid())
+  if (!decl_ctx.IsValid()) {
+    // ...But if we are only checking root decl contexts, confirm that the
+    // 'die' is a top-level context.
+    if (only_root_namespaces)
+      return die.GetParent().Tag() == dwarf::DW_TAG_compile_unit;
+
     return true;
+  }
 
   if (die) {
     if (DWARFASTParser *dwarf_ast = GetDWARFParser(*die.GetCU())) {
       if (CompilerDeclContext actual_decl_ctx =
               dwarf_ast->GetDeclContextContainingUIDFromDWARF(die))
         return decl_ctx.IsContainedInLookup(actual_decl_ctx);
     }
   }
void SymbolFileDWARF::FindTypes(
   for (const auto &pair : m_external_type_modules)
     if (ModuleSP external_module_sp = pair.second)
       external_module_sp->FindTypes(pattern, languages, searched_symbol_files,
                                     types);
 }
 
 CompilerDeclContext
 SymbolFileDWARF::FindNamespace(ConstString name,
-                               const CompilerDeclContext &parent_decl_ctx) {
+                               const CompilerDeclContext &parent_decl_ctx,
+                               bool only_root_namespaces) {
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
   Log *log = GetLog(DWARFLog::Lookups);
 
   if (log) {
     GetObjectFile()->GetModule()->LogMessage(
         log, "SymbolFileDWARF::FindNamespace (sc, name=\"{0}\")",
         name.GetCString());
   }
 
   CompilerDeclContext namespace_decl_ctx;
 
   if (!DeclContextMatchesThisSymbolFile(parent_decl_ctx))
     return namespace_decl_ctx;
 
   m_index->GetNamespaces(name, [&](DWARFDIE die) {
-    if (!DIEInDeclContext(parent_decl_ctx, die))
+    if (!DIEInDeclContext(parent_decl_ctx, die, only_root_namespaces))
       return true; // The containing decl contexts don't match
 
     DWARFASTParser *dwarf_ast = GetDWARFParser(*die.GetCU());
     if (!dwarf_ast)
       return true;
 
     namespace_decl_ctx = dwarf_ast->GetDeclContextForUIDFromDWARF(die);
     return !namespace_decl_ctx.IsValid();

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

FindTypes(lldb_private::ConstString name,
             uint32_t max_matches,
             llvm::DenseSet<lldb_private::SymbolFile *> &searched_symbol_files,
             lldb_private::TypeMap &types) override;
   void
   FindTypes(llvm::ArrayRef<lldb_private::CompilerContext> context,
             lldb_private::LanguageSet languages,
             llvm::DenseSet<lldb_private::SymbolFile *> &searched_symbol_files,
             lldb_private::TypeMap &types) override;
-  lldb_private::CompilerDeclContext FindNamespace(
-      lldb_private::ConstString name,
-      const lldb_private::CompilerDeclContext &parent_decl_ctx) override;
+  lldb_private::CompilerDeclContext
+  FindNamespace(lldb_private::ConstString name,
+                const lldb_private::CompilerDeclContext &parent_decl_ctx,
+                bool only_root_namespaces) override;
   void GetTypes(lldb_private::SymbolContextScope *sc_scope,
                 lldb::TypeClass type_mask,
                 lldb_private::TypeList &type_list) override;
   std::vector<std::unique_ptr<lldb_private::CallEdge>>
   ParseCallEdgesInFunction(lldb_private::UserID func_id) override;
 
   void DumpClangAST(lldb_private::Stream &s) override;
 

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

void SymbolFileDWARFDebugMap::FindTypes(
   LLDB_SCOPED_TIMER();
   ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
     oso_dwarf->FindTypes(context, languages, searched_symbol_files, types);
     return false;
   });
 }
 
 CompilerDeclContext SymbolFileDWARFDebugMap::FindNamespace(
-    lldb_private::ConstString name,
-    const CompilerDeclContext &parent_decl_ctx) {
+    lldb_private::ConstString name, const CompilerDeclContext &parent_decl_ctx,
+    bool only_root_namespaces) {
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
   CompilerDeclContext matching_namespace;
 
   ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
-    matching_namespace = oso_dwarf->FindNamespace(name, parent_decl_ctx);
+    matching_namespace =
+        oso_dwarf->FindNamespace(name, parent_decl_ctx, only_root_namespaces);
 
     return (bool)matching_namespace;
   });
 
   return matching_namespace;
 }
 
 void SymbolFileDWARFDebugMap::DumpClangAST(Stream &s) {

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h

public:
 
   void FindTypes(llvm::ArrayRef<CompilerContext> pattern, LanguageSet languages,
                  llvm::DenseSet<SymbolFile *> &searched_symbol_files,
                  TypeMap &types) override;
 
   llvm::Expected<lldb::TypeSystemSP>
   GetTypeSystemForLanguage(lldb::LanguageType language) override;
 
-  CompilerDeclContext
-  FindNamespace(ConstString name,
-                const CompilerDeclContext &parent_decl_ctx) override;
+  CompilerDeclContext FindNamespace(ConstString name,
+                                    const CompilerDeclContext &parent_decl_ctx,
+                                    bool only_root_namespaces) override;
 
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 
   llvm::pdb::PDBFile &GetPDBFile() { return m_index->pdb(); }
   const llvm::pdb::PDBFile &GetPDBFile() const { return m_index->pdb(); }
 
   PdbIndex &GetIndex() { return *m_index; };
 

lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

bool SymbolFileNativePDB::CompleteType(CompilerType &compiler_type) {
 
   return ast_builder->CompleteType(qt);
 }
 
 void SymbolFileNativePDB::GetTypes(lldb_private::SymbolContextScope *sc_scope,
                                    TypeClass type_mask,
                                    lldb_private::TypeList &type_list) {}
 
-CompilerDeclContext
-SymbolFileNativePDB::FindNamespace(ConstString name,
-                                   const CompilerDeclContext &parent_decl_ctx) {
+CompilerDeclContext SymbolFileNativePDB::FindNamespace(
+    ConstString name, const CompilerDeclContext &parent_decl_ctx, bool) {
   return {};
 }
 
 llvm::Expected<lldb::TypeSystemSP>
 SymbolFileNativePDB::GetTypeSystemForLanguage(lldb::LanguageType language) {
   auto type_system_or_err =
       m_objfile_sp->GetModule()->GetTypeSystemForLanguage(language);
   if (type_system_or_err)

lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h

public:
 
   void GetTypes(lldb_private::SymbolContextScope *sc_scope,
                 lldb::TypeClass type_mask,
                 lldb_private::TypeList &type_list) override;
 
   llvm::Expected<lldb::TypeSystemSP>
   GetTypeSystemForLanguage(lldb::LanguageType language) override;
 
-  lldb_private::CompilerDeclContext FindNamespace(
-      lldb_private::ConstString name,
-      const lldb_private::CompilerDeclContext &parent_decl_ctx) override;
+  lldb_private::CompilerDeclContext
+  FindNamespace(lldb_private::ConstString name,
+                const lldb_private::CompilerDeclContext &parent_decl_ctx,
+                bool only_root_namespaces) override;
 
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 
   llvm::pdb::IPDBSession &GetPDBSession();
 
   const llvm::pdb::IPDBSession &GetPDBSession() const;
 
   void DumpClangAST(lldb_private::Stream &s) override;

lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp

PDBASTParser *SymbolFilePDB::GetPDBAstParser() {
   if (!clang_type_system)
     return nullptr;
 
   return clang_type_system->GetPDBParser();
 }
 
 lldb_private::CompilerDeclContext
 SymbolFilePDB::FindNamespace(lldb_private::ConstString name,
-                             const CompilerDeclContext &parent_decl_ctx) {
+                             const CompilerDeclContext &parent_decl_ctx, bool) {
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
   auto type_system_or_err =
       GetTypeSystemForLanguage(lldb::eLanguageTypeC_plus_plus);
   if (auto err = type_system_or_err.takeError()) {
     LLDB_LOG_ERROR(GetLog(LLDBLog::Symbols), std::move(err),
                    "Unable to find namespace {}", name.AsCString());
     return CompilerDeclContext();
   }

lldb/source/Symbol/SymbolFileOnDemand.cpp

return llvm::make_error<llvm::StringError>(
         "GetTypeSystemForLanguage is skipped by SymbolFileOnDemand",
         llvm::inconvertibleErrorCode());
   }
   return m_sym_file_impl->GetTypeSystemForLanguage(language);
 }
 
 CompilerDeclContext
 SymbolFileOnDemand::FindNamespace(ConstString name,
-                                  const CompilerDeclContext &parent_decl_ctx) {
+                                  const CompilerDeclContext &parent_decl_ctx,
+                                  bool only_root_namespaces) {
   if (!m_debug_info_enabled) {
     LLDB_LOG(GetLog(), "[{0}] {1}({2}) is skipped", GetSymbolFileName(),
              __FUNCTION__, name);
-    return SymbolFile::FindNamespace(name, parent_decl_ctx);
+    return SymbolFile::FindNamespace(name, parent_decl_ctx,
+                                     only_root_namespaces);
   }
-  return m_sym_file_impl->FindNamespace(name, parent_decl_ctx);
+  return m_sym_file_impl->FindNamespace(name, parent_decl_ctx,
+                                        only_root_namespaces);
 }
 
 std::vector<std::unique_ptr<lldb_private::CallEdge>>
 SymbolFileOnDemand::ParseCallEdgesInFunction(UserID func_id) {
   if (!m_debug_info_enabled) {
     Log *log = GetLog();
     LLDB_LOG(log, "[{0}] {1} is skipped", GetSymbolFileName(), __FUNCTION__);
     if (log) {

lldb/test/API/lang/cpp/namespace/TestNamespace.py

def test_with_run_command(self):
         # global namespace qualification with function in anonymous namespace
         self.expect_expr("myanonfunc(4)", result_value="8")
 
         self.expect("expression myanonfunc",
                     patterns=['\(anonymous namespace\)::myanonfunc\(int\)'])
 
         self.expect("expression variadic_sum", patterns=[
                     '\(anonymous namespace\)::variadic_sum\(int, ...\)'])
+
+        self.expect_expr("::B::Bar b; b.x()", result_type="int", result_value="42")
+        self.expect_expr("A::B::Bar b; b.y()", result_type="int", result_value="137")
+        self.expect_expr("::NS1::NS2::Foo{}.bar() == -2 && ::NS2::Foo{}.bar() == -3",
+                         result_type="bool", result_value="true")
+        # FIXME: C++ unqualified namespace lookups currently not supported when instantiating types.
+        self.expect_expr("NS2::Foo{}.bar() == -3", result_type="bool", result_value="false")
+        self.expect_expr("((::B::Bar*)&::B::bar)->x()", result_type="int", result_value="42")

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

int Foo::myfunc(int a)
     j = 4;
     printf("::i=%d\n", ::i);
     printf("A::B::j=%d\n", A::B::j);
     printf("variadic_sum=%d\n", variadic_sum(3, 1, 2, 3));
     myanonfunc(3);
     return myfunc2(3) + j + i + a + 2 + anon_uint + a_uint + b_uint + y_uint; // Set break point at this line.
 }
 
+namespace B {
+struct Bar {
+    int x() { return 42; }
+};
+Bar bar;
+} // namespace B
+
+namespace A::B {
+struct Bar {
+    int y() { return 137; }
+};
+} // namespace A::B
+
+namespace NS1::NS2 {
+struct Foo {
+    int bar() { return -2; }
+};
+} // namespace NS1::NS2
+
+namespace NS2 {
+struct Foo {
+    int bar() { return -3; }
+};
+} // namespace NS2
+
 int
 main (int argc, char const *argv[])
 {
     test_lookup_at_global_scope();
     test_lookup_at_file_scope();
     A::test_lookup_at_ns_scope();
     A::B::test_lookup_at_nested_ns_scope();
     A::B::test_lookup_at_nested_ns_scope_after_using();
     test_lookup_before_using_directive();
     test_lookup_after_using_directive();
-    return Foo::myfunc(12);
+    ::B::Bar bb;
+    A::B::Bar ab;
+    return Foo::myfunc(12) + bb.x() + ab.y() + NS1::NS2::Foo{}.bar() +
+           NS2::Foo{}.bar();
 }