[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

1781

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

1781

Nevermind, not true

[martong]

LGTM, Thanks!

clang/lib/AST/DeclBase.cpp

1781

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/template-with-same-arg/TestTemplateWithSameArg.py

 class TestTemplateWithSameArg(TestBase):
 
     def setUp(self):
         TestBase.setUp(self)
         self.build()
         self.main_source_file = lldb.SBFileSpec("main.cpp")
 
     @add_test_categories(["gmodules"])
-    @skipIf(bugnumber='rdar://96581048')
     def test_same_template_arg(self):
         lldbutil.run_to_source_breakpoint(self, "Break here", self.main_source_file)
 
         self.expect_expr("FromMod1", result_type="ClassInMod1", result_children=[
                 ValueCheck(name="VecInMod1", children=[
                             ValueCheck(name="Member", value="137")
                     ])
             ])
 
         self.expect_expr("FromMod2", result_type="ClassInMod2", result_children=[
                 ValueCheck(name="VecInMod2", children=[
                             ValueCheck(name="Member", value="42")
                     ])
             ])
 
     @add_test_categories(["gmodules"])
-    @skipIf(bugnumber='rdar://96581048')
     def test_duplicate_decls(self):
         lldbutil.run_to_source_breakpoint(self, "Break here", self.main_source_file)
 
         self.expect_expr("(intptr_t)&FromMod1 + (intptr_t)&FromMod2")
 
         # Make sure we only have a single 'Member' decl on the AST
         self.filecheck("target module dump ast", __file__)
 # CHECK:      ClassTemplateSpecializationDecl {{.*}} imported in Module2 struct ClassInMod3 definition