[clang][ASTImporter] DeclContext::localUncachedLookup: Continue lookup into decl chain when regular lookup fails

[Michael137]

Summary

The uncached lookup is mainly used in the ASTImporter/LLDB code-path
where we're not allowed to load from external storage. When importing
a FieldDecl with a DeclContext that had no external visible storage
(but came from a Clang module or PCH) the regular DeclContext::lookup
fails because:

1. DeclContext::buildLookup doesn't set LookupPtr for decls that came from a module
2. LLDB doesn't use the SharedImporterState

In such a case we would never continue with the "slow" path of iterating
through the decl chain on the DeclContext. In some cases this means that
ASTNodeImporter::VisitFieldDecl ends up importing a decl into the
DeclContext a second time.

The patch removes the short-circuit in the case where we don't find
any decls via the regular lookup.

Tests

• Un-skip the failing LLDB API tests

[Michael137]

clang/lib/AST/DeclBase.cpp

1777–1778

Could merge the two if-blocks now I suppose

[Michael137]

• Merge if-blocks
• Reword commit

[Michael137]

• Undo incorrect previous change

[Michael137]

clang/lib/AST/DeclBase.cpp

1777–1778

Nevermind, not true

[martong]

LGTM, Thanks!

clang/lib/AST/DeclBase.cpp

1777–1778

Yeah, we set HasLazyExternalLexicalLookups to true in https://reviews.llvm.org/D61333 exactly for the reason to continue the lookup into decl chain.

clang/unittests/AST/ASTImporterTest.cpp

4927–4929

Very good, now the behavior is the same that we have in case of the ASTImporterLookupTable.

[Michael137]

• Rebase

clang/lib/AST/DeclBase.cpp

void DeclContext::localUncachedLookup(DeclarationName Name,
                                       SmallVectorImpl<NamedDecl *> &Results) {
   Results.clear();
 
   // If there's no external storage, just perform a normal lookup and copy
   // the results.
   if (!hasExternalVisibleStorage() && !hasExternalLexicalStorage() && Name) {
     lookup_result LookupResults = lookup(Name);
     Results.insert(Results.end(), LookupResults.begin(), LookupResults.end());
+    if (!Results.empty())
       return;
-  }
 
     // If we have a lookup table, check there first. Maybe we'll get lucky.
     // FIXME: Should we be checking these flags on the primary context?
-  if (Name && !hasLazyLocalLexicalLookups() &&
-      !hasLazyExternalLexicalLookups()) {
     if (StoredDeclsMap *Map = LookupPtr) {
       StoredDeclsMap::iterator Pos = Map->find(Name);
       if (Pos != Map->end()) {
         Results.insert(Results.end(),
                        Pos->second.getLookupResult().begin(),
                        Pos->second.getLookupResult().end());
         return;
       }

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterLookupTableTest,
   ASSERT_EQ(cast<Decl>(FooDC), ToTU);
   DeclarationName FooName = Foo->getDeclName();
 
   // Cannot find in the LookupTable of its DC (TUDecl).
   SmallVector<NamedDecl *, 2> FoundDecls;
   FooDC->getRedeclContext()->localUncachedLookup(FooName, FoundDecls);
   EXPECT_EQ(FoundDecls.size(), 0u);
 
-  // Cannot find in the LookupTable of its LexicalDC (A).
+  // Finds via linear search of its LexicalDC (A).
   FooLexicalDC->getRedeclContext()->localUncachedLookup(FooName, FoundDecls);
-  EXPECT_EQ(FoundDecls.size(), 0u);
+  EXPECT_EQ(FoundDecls.size(), 1u);
 
   // Can't find in the list of Decls of the DC.
   EXPECT_EQ(findInDeclListOfDC(FooDC, FooName), nullptr);
 
   // Can find in the list of Decls of the LexicalDC.
   EXPECT_EQ(findInDeclListOfDC(FooLexicalDC, FooName), Foo);
 
   // ASTImporter specific lookup finds it.

lldb/test/API/lang/cpp/gmodules/base-template-with-same-arg/TestBaseTemplateWithSameArg.py

 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 
 class TestBaseTemplateWithSameArg(TestBase):
 
     @add_test_categories(["gmodules"])
-    @skipIf(bugnumber='rdar://96581048')
     def test_same_base_template_arg(self):
         self.build()
 
         self.main_source_file = lldb.SBFileSpec("main.cpp")
 
         (target, process, main_thread, _) = lldbutil.run_to_source_breakpoint(self,
                                                 "Break here", self.main_source_file)