[modules] Remove some redundant work when building a lookup table for a DeclContext.

When we need to build the lookup table for a DeclContext, we used to pull in
all lexical declarations for the context; instead, just build a lookup table
for the local lexical declarations. We previously didn't guarantee that the
imported declarations would be in the returned map, but in some cases we'd
happen to put them all in there regardless. Now we're even lazier about this.

This unnecessary work was papering over some other bugs:

 - LookupVisibleDecls would use the DC for name lookups in the TU in C, and
   this was not guaranteed to find all imported names (generally, the DC for
   the TU in C is not a reliable place to perform lookups). We now use an
   identifier-based lookup mechanism for this.

 - We didn't actually load in the list of eagerly-deserialized declarations
   when importing a module (so external definitions in a module wouldn't be
   emitted by users of those modules unless they happened to be deserialized
   by the user of the module).

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@232793 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/AST/DeclBase.h b/include/clang/AST/DeclBase.h
index 121bd005353867537a741cbaf5821106a4f8dc9d..30ce2601a16c2d66c7cc8cb0d74c4b4529da7ca3 100644 (file)
--- a/include/clang/AST/DeclBase.h
+++ b/include/clang/AST/DeclBase.h
@@ -1722,8 +1722,6 @@ private:
   friend class DependentDiagnostic;
   StoredDeclsMap *CreateStoredDeclsMap(ASTContext &C) const;
 
-  template<decl_iterator (DeclContext::*Begin)() const,
-           decl_iterator (DeclContext::*End)() const>
   void buildLookupImpl(DeclContext *DCtx, bool Internal);
   void makeDeclVisibleInContextWithFlags(NamedDecl *D, bool Internal,
                                          bool Rediscoverable);

diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp
index 89e1d52c64374c4e72fc2ec5baeb12c89cdaf351..57621a7b1f647d5158d2b20bcb52ba88613b2ec8 100644 (file)
--- a/lib/AST/ASTContext.cpp
+++ b/lib/AST/ASTContext.cpp
@@ -7946,7 +7946,7 @@ static GVALinkage basicGVALinkageForVariable(const ASTContext &Context,
     while (LexicalContext && !isa<FunctionDecl>(LexicalContext))
       LexicalContext = LexicalContext->getLexicalParent();
 
-    // Let the static local variable inherit it's linkage from the nearest
+    // Let the static local variable inherit its linkage from the nearest
     // enclosing function.
     if (LexicalContext)
       StaticLocalLinkage =

diff --git a/lib/AST/DeclBase.cpp b/lib/AST/DeclBase.cpp
index a8e151cc937d83da1e520f7e58c18fc299f6e7c5..0053c3da8bca908c94bfe8ecdbb5afff1cebb0e9 100644 (file)
--- a/lib/AST/DeclBase.cpp
+++ b/lib/AST/DeclBase.cpp
@@ -1263,15 +1263,13 @@ static bool shouldBeHidden(NamedDecl *D) {
 StoredDeclsMap *DeclContext::buildLookup() {
   assert(this == getPrimaryContext() && "buildLookup called on non-primary DC");
 
-  // FIXME: Should we keep going if hasExternalVisibleStorage?
   if (!LookupPtr.getInt())
     return LookupPtr.getPointer();
 
   SmallVector<DeclContext *, 2> Contexts;
   collectAllContexts(Contexts);
   for (unsigned I = 0, N = Contexts.size(); I != N; ++I)
-    buildLookupImpl<&DeclContext::decls_begin,
-                    &DeclContext::decls_end>(Contexts[I], false);
+    buildLookupImpl(Contexts[I], hasExternalVisibleStorage());
 
   // We no longer have any lazy decls.
   LookupPtr.setInt(false);
@@ -1282,13 +1280,8 @@ StoredDeclsMap *DeclContext::buildLookup() {
 /// declarations contained within DCtx, which will either be this
 /// DeclContext, a DeclContext linked to it, or a transparent context
 /// nested within it.
-template<DeclContext::decl_iterator (DeclContext::*Begin)() const,
-         DeclContext::decl_iterator (DeclContext::*End)() const>
 void DeclContext::buildLookupImpl(DeclContext *DCtx, bool Internal) {
-  for (decl_iterator I = (DCtx->*Begin)(), E = (DCtx->*End)();
-       I != E; ++I) {
-    Decl *D = *I;
-
+  for (Decl *D : DCtx->noload_decls()) {
     // Insert this declaration into the lookup structure, but only if
     // it's semantically within its decl context. Any other decls which
     // should be found in this context are added eagerly.
@@ -1309,7 +1302,7 @@ void DeclContext::buildLookupImpl(DeclContext *DCtx, bool Internal) {
     // context (recursively).
     if (DeclContext *InnerCtx = dyn_cast<DeclContext>(D))
       if (InnerCtx->isTransparentContext() || InnerCtx->isInlineNamespace())
-        buildLookupImpl<Begin, End>(InnerCtx, Internal);
+        buildLookupImpl(InnerCtx, Internal);
   }
 }
 
@@ -1388,26 +1381,8 @@ DeclContext::noload_lookup(DeclarationName Name) {
   if (PrimaryContext != this)
     return PrimaryContext->noload_lookup(Name);
 
-  StoredDeclsMap *Map = LookupPtr.getPointer();
-  if (LookupPtr.getInt()) {
-    // Carefully build the lookup map, without deserializing anything.
-    SmallVector<DeclContext *, 2> Contexts;
-    collectAllContexts(Contexts);
-    for (unsigned I = 0, N = Contexts.size(); I != N; ++I)
-      buildLookupImpl<&DeclContext::noload_decls_begin,
-                      &DeclContext::noload_decls_end>(Contexts[I], true);
-
-    // We no longer have any lazy decls.
-    LookupPtr.setInt(false);
-
-    // There may now be names for which we have local decls but are
-    // missing the external decls. FIXME: Just set the hasExternalDecls
-    // flag on those names that have external decls.
-    NeedToReconcileExternalVisibleStorage = true;
-
-    Map = LookupPtr.getPointer();
-  }
-
+  // Note that buildLookups does not trigger any deserialization.
+  StoredDeclsMap *Map = buildLookup();
   if (!Map)
     return lookup_result();
 

diff --git a/lib/Sema/IdentifierResolver.cpp b/lib/Sema/IdentifierResolver.cpp
index a6efe96573a0cdd3dbb877a531920f8cd5e87281..53263bac546f2ddc05d030104ac534d2674d5051 100644 (file)
--- a/lib/Sema/IdentifierResolver.cpp
+++ b/lib/Sema/IdentifierResolver.cpp
@@ -98,7 +98,7 @@ bool IdentifierResolver::isDeclInScope(Decl *D, DeclContext *Ctx, Scope *S,
                                        bool AllowInlineNamespace) const {
   Ctx = Ctx->getRedeclContext();
 
-  if (Ctx->isFunctionOrMethod() || S->isFunctionPrototypeScope()) {
+  if (Ctx->isFunctionOrMethod() || (S && S->isFunctionPrototypeScope())) {
     // Ignore the scopes associated within transparent declaration contexts.
     while (S->getEntity() && S->getEntity()->isTransparentContext())
       S = S->getParent();

diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp
index e15e532be19ee4b440f4c18cf23511a7dd5a65df..0b406184058202a4eddb325123310b555b95a213 100644 (file)
--- a/lib/Sema/SemaLookup.cpp
+++ b/lib/Sema/SemaLookup.cpp
@@ -3021,17 +3021,45 @@ static void LookupVisibleDecls(DeclContext *Ctx, LookupResult &Result,
   if (Visited.visitedContext(Ctx->getPrimaryContext()))
     return;
 
+  // Outside C++, lookup results for the TU live on identifiers.
+  if (isa<TranslationUnitDecl>(Ctx) &&
+      !Result.getSema().getLangOpts().CPlusPlus) {
+    auto &S = Result.getSema();
+    auto &Idents = S.Context.Idents;
+
+    // Ensure all external identifiers are in the identifier table.
+    if (IdentifierInfoLookup *External = Idents.getExternalIdentifierLookup()) {
+      std::unique_ptr<IdentifierIterator> Iter(External->getIdentifiers());
+      for (StringRef Name = Iter->Next(); !Name.empty(); Name = Iter->Next())
+        Idents.get(Name);
+    }
+
+    // Walk all lookup results in the TU for each identifier.
+    for (const auto &Ident : Idents) {
+      for (auto I = S.IdResolver.begin(Ident.getValue()),
+                E = S.IdResolver.end();
+           I != E; ++I) {
+        if (S.IdResolver.isDeclInScope(*I, Ctx)) {
+          if (NamedDecl *ND = Result.getAcceptableDecl(*I)) {
+            Consumer.FoundDecl(ND, Visited.checkHidden(ND), Ctx, InBaseClass);
+            Visited.add(ND);
+          }
+        }
+      }
+    }
+
+    return;
+  }
+
   if (CXXRecordDecl *Class = dyn_cast<CXXRecordDecl>(Ctx))
     Result.getSema().ForceDeclarationOfImplicitMembers(Class);
 
   // Enumerate all of the results in this context.
   for (const auto &R : Ctx->lookups()) {
-    for (auto *I : R) {
-      if (NamedDecl *ND = dyn_cast<NamedDecl>(I)) {
-        if ((ND = Result.getAcceptableDecl(ND))) {
-          Consumer.FoundDecl(ND, Visited.checkHidden(ND), Ctx, InBaseClass);
-          Visited.add(ND);
-        }
+    for (auto *D : R) {
+      if (auto *ND = Result.getAcceptableDecl(D)) {
+        Consumer.FoundDecl(ND, Visited.checkHidden(ND), Ctx, InBaseClass);
+        Visited.add(ND);
       }
     }
   }

diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp
index f4b4c4b9879f5bf83b14d4d47c9667ab9967537c..93544ef20fc23345ca2336185d0b3deb56684c77 100644 (file)
--- a/lib/Serialization/ASTReader.cpp
+++ b/lib/Serialization/ASTReader.cpp
@@ -2835,6 +2835,8 @@ ASTReader::ReadASTBlock(ModuleFile &F, unsigned ClientLoadCapabilities) {
     }
 
     case EAGERLY_DESERIALIZED_DECLS:
+      // FIXME: Skip reading this record if our ASTConsumer doesn't care
+      // about "interesting" decls (for instance, if we're building a module).
       for (unsigned I = 0, N = Record.size(); I != N; ++I)
         EagerlyDeserializedDecls.push_back(getGlobalDeclID(F, Record[I]));
       break;
@@ -6832,6 +6834,12 @@ void ASTReader::PassInterestingDeclsToConsumer() {
   SaveAndRestore<bool> GuardPassingDeclsToConsumer(PassingDeclsToConsumer,
                                                    true);
 
+  // Ensure that we've loaded all potentially-interesting declarations
+  // that need to be eagerly loaded.
+  for (auto ID : EagerlyDeserializedDecls)
+    GetDecl(ID);
+  EagerlyDeserializedDecls.clear();
+
   while (!InterestingDecls.empty()) {
     Decl *D = InterestingDecls.front();
     InterestingDecls.pop_front();
@@ -6850,17 +6858,8 @@ void ASTReader::PassInterestingDeclToConsumer(Decl *D) {
 void ASTReader::StartTranslationUnit(ASTConsumer *Consumer) {
   this->Consumer = Consumer;
 
-  if (!Consumer)
-    return;
-
-  for (unsigned I = 0, N = EagerlyDeserializedDecls.size(); I != N; ++I) {
-    // Force deserialization of this decl, which will cause it to be queued for
-    // passing to the consumer.
-    GetDecl(EagerlyDeserializedDecls[I]);
-  }
-  EagerlyDeserializedDecls.clear();
-
-  PassInterestingDeclsToConsumer();
+  if (Consumer)
+    PassInterestingDeclsToConsumer();
 
   if (DeserializationListener)
     DeserializationListener->ReaderInitialized(this);

diff --git a/test/Modules/cxx-templates.cpp b/test/Modules/cxx-templates.cpp
index 46c2f33ef15f7079f8a78c8ced7af130fa85b2d3..41b0f2cd92d57ce3a10f1f8dda31979ff067dc5a 100644 (file)
--- a/test/Modules/cxx-templates.cpp
+++ b/test/Modules/cxx-templates.cpp
@@ -1,7 +1,7 @@
 // RUN: rm -rf %t
 // RUN: not %clang_cc1 -x objective-c++ -fmodules -fno-modules-error-recovery -fmodules-cache-path=%t -I %S/Inputs %s -std=c++11 -ast-dump-lookups | FileCheck %s --check-prefix=CHECK-GLOBAL
 // RUN: not %clang_cc1 -x objective-c++ -fmodules -fno-modules-error-recovery -fmodules-cache-path=%t -I %S/Inputs %s -std=c++11 -ast-dump-lookups -ast-dump-filter N | FileCheck %s --check-prefix=CHECK-NAMESPACE-N
-// RUN: not %clang_cc1 -x objective-c++ -fmodules -fno-modules-error-recovery -fmodules-cache-path=%t -I %S/Inputs %s -std=c++11 -ast-dump | FileCheck %s --check-prefix=CHECK-DUMP
+// RUN: not %clang_cc1 -x objective-c++ -fmodules -fno-modules-error-recovery -fmodules-cache-path=%t -I %S/Inputs %s -std=c++11 -ast-dump -ast-dump-filter SomeTemplate | FileCheck %s --check-prefix=CHECK-DUMP
 // RUN: %clang_cc1 -x objective-c++ -fmodules -fno-modules-error-recovery -fmodules-cache-path=%t -I %S/Inputs %s -verify -std=c++11
 // RUN: %clang_cc1 -x objective-c++ -fmodules -fno-modules-error-recovery -fmodules-cache-path=%t -I %S/Inputs %s -verify -std=c++11 -DEARLY_IMPORT
 
@@ -125,9 +125,10 @@ void g() {
 static_assert(Outer<int>::Inner<int>::f() == 1, "");
 static_assert(Outer<int>::Inner<int>::g() == 2, "");
 
-#ifndef EARLY_IMPORT
-// FIXME: The textual inclusion above shouldn't cause this to fail!
-static_assert(MergeTemplateDefinitions<int>::f() == 1, "");
+// FIXME: We're too lazy in merging class definitions to find the definition
+// of this function.
+static_assert(MergeTemplateDefinitions<int>::f() == 1, ""); // expected-error {{constant expression}} expected-note {{undefined}}
+// expected-note@cxx-templates-c.h:10 {{here}}
 static_assert(MergeTemplateDefinitions<int>::g() == 2, "");
 
 RedeclaredAsFriend<int> raf1;
@@ -140,7 +141,6 @@ MergeSpecializations<int[]>::partially_specialized_in_c spec_in_c_1;
 MergeSpecializations<char>::explicitly_specialized_in_a spec_in_a_2;
 MergeSpecializations<double>::explicitly_specialized_in_b spec_in_b_2;
 MergeSpecializations<bool>::explicitly_specialized_in_c spec_in_c_2;
-#endif
 
 MergeAnonUnionMember<> maum_main;
 typedef DontWalkPreviousDeclAfterMerging<int> dwpdam_typedef_2;

diff --git a/test/Modules/odr.cpp b/test/Modules/odr.cpp
index 120ca20e0a873eccd972f6dad7d042b8da247921..4ac257cd03aadb2aff19306d54f7e2f7c074544d 100644 (file)
--- a/test/Modules/odr.cpp
+++ b/test/Modules/odr.cpp
@@ -15,9 +15,9 @@ bool b = F<int>{0} == F<int>{1};
 int x = f() + g();
 
 // expected-note@a.h:5 {{definition has no member 'e2'}}
-// expected-note@a.h:3 {{declaration of 'f' does not match}}
-// expected-note@a.h:1 {{definition has no member 'm'}}
+// expected-note@b.h:3 {{declaration of 'f' does not match}}
+// expected-note@b.h:1 {{definition has no member 'n'}}
 
 // expected-error@b.h:5 {{'E::e2' from module 'b' is not present in definition of 'E' in module 'a'}}
-// expected-error@b.h:3 {{'Y::f' from module 'b' is not present in definition of 'Y' in module 'a'}}
-// expected-error@b.h:2 {{'Y::m' from module 'b' is not present in definition of 'Y' in module 'a'}}
+// expected-error@a.h:3 {{'Y::f' from module 'a' is not present in definition of 'Y' in module 'b'}}
+// expected-error@a.h:2 {{'Y::n' from module 'a' is not present in definition of 'Y' in module 'b'}}

diff --git a/test/Modules/redecl-add-after-load.cpp b/test/Modules/redecl-add-after-load.cpp
index 68deaf8b4ef9fdea4e3d8188e53484d6468bed9c..53e54c84cc3a2dbcb097591492d287d466029e11 100644 (file)
--- a/test/Modules/redecl-add-after-load.cpp
+++ b/test/Modules/redecl-add-after-load.cpp
@@ -29,7 +29,7 @@ struct D {
   static constexpr int function(); // expected-note {{here}}
 };
 typedef D::A DB;
-constexpr int D_test(bool b) { return b ? D::variable : D::function(); } // expected-note {{subexpression}} expected-note {{undefined}}
+constexpr int D_test(bool b) { return b ? D::variable : D::function(); } // expected-note {{undefined}}
 #endif
 
 @import redecl_add_after_load;
@@ -54,6 +54,6 @@ constexpr int merged_struct_variable_test = D_test(true);
 constexpr int merged_struct_function_test = D_test(false);
 #ifndef IMPORT_DECLS
 // expected-error@-4 {{incomplete}}
-// expected-error@-4 {{constant}} expected-note@-4 {{in call to}}
+// @-4: definition of D::variable must be emitted, so it gets imported eagerly
 // expected-error@-4 {{constant}} expected-note@-4 {{in call to}}
 #endif