From a534a31c5e73163eaa61560cd4e0c4c19bcbcfb1 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 21 Jul 2015 23:54:07 +0000 Subject: [PATCH] [modules] In C++, stop serializing and deserializing a list of declarations in the identifier table. This is redundant, since the TU-scope lookups are also serialized as part of the TU DeclContext, and wasteful in a number of ways. We still emit the decls for PCH / preamble builds, since for those we want identical results, not merely semantically equivalent ones. llvm-svn: 242855 --- clang/include/clang/AST/DeclObjC.h | 18 ++------ clang/include/clang/Sema/Lookup.h | 23 +++++++--- clang/lib/Sema/SemaDecl.cpp | 45 +++---------------- clang/lib/Sema/SemaLookup.cpp | 26 ++++++++--- clang/lib/Serialization/ASTReader.cpp | 19 +++++--- clang/lib/Serialization/ASTWriter.cpp | 43 ++++++++++-------- .../test/CXX/class.access/class.friend/p1.cpp | 5 ++- .../CXX/class.access/class.friend/p11.cpp | 11 ++--- clang/test/Modules/linkage-merge.cpp | 4 -- 9 files changed, 94 insertions(+), 100 deletions(-) diff --git a/clang/include/clang/AST/DeclObjC.h b/clang/include/clang/AST/DeclObjC.h index c42764b6f3bf..96563ee68836 100644 --- a/clang/include/clang/AST/DeclObjC.h +++ b/clang/include/clang/AST/DeclObjC.h @@ -1202,13 +1202,8 @@ public: // might bring in a definition. // Note: a null value indicates that we don't have a definition and that // modules are enabled. - if (!Data.getOpaqueValue()) { - if (IdentifierInfo *II = getIdentifier()) { - if (II->isOutOfDate()) { - updateOutOfDate(*II); - } - } - } + if (!Data.getOpaqueValue()) + getMostRecentDecl(); return Data.getPointer(); } @@ -1851,13 +1846,8 @@ public: // might bring in a definition. // Note: a null value indicates that we don't have a definition and that // modules are enabled. - if (!Data.getOpaqueValue()) { - if (IdentifierInfo *II = getIdentifier()) { - if (II->isOutOfDate()) { - updateOutOfDate(*II); - } - } - } + if (!Data.getOpaqueValue()) + getMostRecentDecl(); return Data.getPointer(); } diff --git a/clang/include/clang/Sema/Lookup.h b/clang/include/clang/Sema/Lookup.h index 5bfee8b0d037..bc9cf4279ace 100644 --- a/clang/include/clang/Sema/Lookup.h +++ b/clang/include/clang/Sema/Lookup.h @@ -140,6 +140,7 @@ public: HideTags(true), Diagnose(Redecl == Sema::NotForRedeclaration), AllowHidden(Redecl == Sema::ForRedeclaration), + AllowHiddenInternal(AllowHidden), Shadowed(false) { configure(); @@ -162,6 +163,7 @@ public: HideTags(true), Diagnose(Redecl == Sema::NotForRedeclaration), AllowHidden(Redecl == Sema::ForRedeclaration), + AllowHiddenInternal(AllowHidden), Shadowed(false) { configure(); @@ -182,6 +184,7 @@ public: HideTags(Other.HideTags), Diagnose(false), AllowHidden(Other.AllowHidden), + AllowHiddenInternal(Other.AllowHiddenInternal), Shadowed(false) {} @@ -223,13 +226,20 @@ public: /// \brief Specify whether hidden declarations are visible, e.g., /// for recovery reasons. void setAllowHidden(bool AH) { - AllowHidden = AH; + AllowHiddenInternal = AllowHidden = AH; + } + + /// \brief Specify whether hidden internal declarations are visible. + void setAllowHiddenInternal(bool AHI) { + AllowHiddenInternal = AHI; } /// \brief Determine whether this lookup is permitted to see hidden /// declarations, such as those in modules that have not yet been imported. - bool isHiddenDeclarationVisible() const { - return AllowHidden || LookupKind == Sema::LookupTagName; + bool isHiddenDeclarationVisible(NamedDecl *ND) const { + return (AllowHidden && + (AllowHiddenInternal || ND->isExternallyVisible())) || + LookupKind == Sema::LookupTagName; } /// Sets whether tag declarations should be hidden by non-tag @@ -302,7 +312,7 @@ public: if (!D->isInIdentifierNamespace(IDNS)) return nullptr; - if (isHiddenDeclarationVisible() || isVisible(getSema(), D)) + if (isHiddenDeclarationVisible(D) || isVisible(getSema(), D)) return D; return getAcceptableDeclSlow(D); @@ -511,7 +521,7 @@ public: /// \brief Change this lookup's redeclaration kind. void setRedeclarationKind(Sema::RedeclarationKind RK) { Redecl = RK; - AllowHidden = (RK == Sema::ForRedeclaration); + AllowHiddenInternal = AllowHidden = (RK == Sema::ForRedeclaration); configure(); } @@ -678,6 +688,9 @@ private: /// \brief True if we should allow hidden declarations to be 'visible'. bool AllowHidden; + /// \brief True if we should allow hidden internal declarations to be visible. + bool AllowHiddenInternal; + /// \brief True if the found declarations were shadowed by some other /// declaration that we skipped. This only happens when \c LookupKind /// is \c LookupRedeclarationWithLinkage. diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 042cffc8b4ad..22803653edff 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -1796,37 +1796,6 @@ NamedDecl *Sema::LazilyCreateBuiltin(IdentifierInfo *II, unsigned ID, return New; } -/// \brief Filter out any previous declarations that the given declaration -/// should not consider because they are not permitted to conflict, e.g., -/// because they come from hidden sub-modules and do not refer to the same -/// entity. -static void filterNonConflictingPreviousDecls(Sema &S, - NamedDecl *decl, - LookupResult &previous){ - // This is only interesting when modules are enabled. - if ((!S.getLangOpts().Modules && !S.getLangOpts().ModulesLocalVisibility) || - !S.getLangOpts().ModulesHideInternalLinkage) - return; - - // Empty sets are uninteresting. - if (previous.empty()) - return; - - LookupResult::Filter filter = previous.makeFilter(); - while (filter.hasNext()) { - NamedDecl *old = filter.next(); - - // Non-hidden declarations are never ignored. - if (S.isVisible(old)) - continue; - - if (!old->isExternallyVisible()) - filter.erase(); - } - - filter.done(); -} - /// Typedef declarations don't have linkage, but they still denote the same /// entity if their types are the same. /// FIXME: This is notionally doing the same thing as ASTReaderDecl's @@ -4801,6 +4770,13 @@ NamedDecl *Sema::HandleDeclarator(Scope *S, Declarator &D, LookupResult Previous(*this, NameInfo, LookupOrdinaryName, ForRedeclaration); + // If we're hiding internal-linkage symbols in modules from redeclaration + // lookup, let name lookup know. + if ((getLangOpts().Modules || getLangOpts().ModulesLocalVisibility) && + getLangOpts().ModulesHideInternalLinkage && + D.getDeclSpec().getStorageClassSpec() != DeclSpec::SCS_typedef) + Previous.setAllowHiddenInternal(false); + // See if this is a redefinition of a variable in the same scope. if (!D.getCXXScopeSpec().isSet()) { bool IsLinkageLookup = false; @@ -6500,9 +6476,6 @@ bool Sema::CheckVariableDeclaration(VarDecl *NewVD, LookupResult &Previous) { checkForConflictWithNonVisibleExternC(*this, NewVD, Previous)) Previous.setShadowed(); - // Filter out any non-conflicting previous declarations. - filterNonConflictingPreviousDecls(*this, NewVD, Previous); - if (!Previous.empty()) { MergeVarDecl(NewVD, Previous); return true; @@ -8058,9 +8031,6 @@ bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD, bool MergeTypeWithPrevious = !getLangOpts().CPlusPlus && !Previous.isShadowed(); - // Filter out any non-conflicting previous declarations. - filterNonConflictingPreviousDecls(*this, NewFD, Previous); - bool Redeclaration = false; NamedDecl *OldDecl = nullptr; @@ -8114,7 +8084,6 @@ bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD, // Check for a previous extern "C" declaration with this name. if (!Redeclaration && checkForConflictWithNonVisibleExternC(*this, NewFD, Previous)) { - filterNonConflictingPreviousDecls(*this, NewFD, Previous); if (!Previous.empty()) { // This is an extern "C" declaration with the same name as a previous // declaration, and thus redeclares that entity... diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp index 3fd1f21ba3fd..99b945597bc7 100644 --- a/clang/lib/Sema/SemaLookup.cpp +++ b/clang/lib/Sema/SemaLookup.cpp @@ -378,7 +378,7 @@ void LookupResult::resolveKind() { // Don't do any extra resolution if we've already resolved as ambiguous. if (ResultKind == Ambiguous) return; - llvm::SmallPtrSet Unique; + llvm::SmallDenseMap Unique; llvm::SmallPtrSet UniqueTypes; bool Ambiguous = false; @@ -414,14 +414,26 @@ void LookupResult::resolveKind() { } } - if (!Unique.insert(D).second) { + auto UniqueResult = Unique.insert(std::make_pair(D, I)); + if (!UniqueResult.second) { // If it's not unique, pull something off the back (and // continue at this index). - // FIXME: This is wrong. We need to take the more recent declaration in - // order to get the right type, default arguments, etc. We also need to - // prefer visible declarations to hidden ones (for redeclaration lookup - // in modules builds). - Decls[I] = Decls[--N]; + auto ExistingI = UniqueResult.first->second; + auto *Existing = Decls[ExistingI]->getUnderlyingDecl(); + for (Decl *Prev = Decls[I]->getUnderlyingDecl()->getPreviousDecl(); /**/; + Prev = Prev->getPreviousDecl()) { + if (Prev == Existing) { + // Existing result is older. Replace it with the new one. + Decls[ExistingI] = Decls[I]; + Decls[I] = Decls[--N]; + break; + } + if (!Prev) { + // New decl is older. Keep the existing one. + Decls[I] = Decls[--N]; + break; + } + } continue; } diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index 1f47f57f41af..cddbd62eb630 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -735,12 +735,14 @@ ASTIdentifierLookupTraitBase::ReadKey(const unsigned char* d, unsigned n) { } /// \brief Whether the given identifier is "interesting". -static bool isInterestingIdentifier(IdentifierInfo &II, bool IsModule) { +static bool isInterestingIdentifier(ASTReader &Reader, IdentifierInfo &II, + bool IsModule) { return II.hadMacroDefinition() || II.isPoisoned() || (IsModule ? II.hasRevertedBuiltin() : II.getObjCOrBuiltinID()) || II.hasRevertedTokenIDToIdentifier() || - II.getFETokenInfo(); + (!(IsModule && Reader.getContext().getLangOpts().CPlusPlus) && + II.getFETokenInfo()); } static bool readBit(unsigned &Bits) { @@ -767,7 +769,7 @@ IdentifierInfo *ASTIdentifierLookupTrait::ReadData(const internal_key_type& k, } if (!II->isFromAST()) { II->setIsFromAST(); - if (isInterestingIdentifier(*II, F.isModule())) + if (isInterestingIdentifier(Reader, *II, F.isModule())) II->setChangedSinceDeserialization(); } Reader.markIdentifierUpToDate(II); @@ -5883,10 +5885,13 @@ void ASTReader::CompleteRedeclChain(const Decl *D) { if (isa(DC) || isa(DC) || isa(DC) || isa(DC)) { if (DeclarationName Name = cast(D)->getDeclName()) { - auto *II = Name.getAsIdentifierInfo(); - if (isa(DC) && II) { + if (!getContext().getLangOpts().CPlusPlus && + isa(DC)) { // Outside of C++, we don't have a lookup table for the TU, so update - // the identifier instead. In C++, either way should work fine. + // the identifier instead. (For C++ modules, we don't store decls + // in the serialized identifier table, so we do the lookup in the TU.) + auto *II = Name.getAsIdentifierInfo(); + assert(II && "non-identifier name in C?"); if (II->isOutOfDate()) updateOutOfDateIdentifier(*II); } else @@ -8443,7 +8448,7 @@ void ASTReader::pushExternalDeclIntoScope(NamedDecl *D, DeclarationName Name) { // Remove any fake results before adding any real ones. auto It = PendingFakeLookupResults.find(II); if (It != PendingFakeLookupResults.end()) { - for (auto *ND : PendingFakeLookupResults[II]) + for (auto *ND : It->second) SemaObj->IdResolver.RemoveDecl(ND); // FIXME: this works around module+PCH performance issue. // Rather than erase the result from the map, which is O(n), just clear diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index 1c08cbf53a7f..ba42cd3173dc 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -3103,6 +3103,7 @@ class ASTIdentifierTableTrait { Preprocessor &PP; IdentifierResolver &IdResolver; bool IsModule; + bool NeedDecls; /// \brief Determines whether this is an "interesting" identifier that needs a /// full IdentifierInfo structure written into the hash table. Notably, this @@ -3113,7 +3114,7 @@ class ASTIdentifierTableTrait { II->isPoisoned() || (IsModule ? II->hasRevertedBuiltin() : II->getObjCOrBuiltinID()) || II->hasRevertedTokenIDToIdentifier() || - II->getFETokenInfo()) + (NeedDecls && II->getFETokenInfo())) return true; return false; @@ -3131,7 +3132,8 @@ public: ASTIdentifierTableTrait(ASTWriter &Writer, Preprocessor &PP, IdentifierResolver &IdResolver, bool IsModule) - : Writer(Writer), PP(PP), IdResolver(IdResolver), IsModule(IsModule) {} + : Writer(Writer), PP(PP), IdResolver(IdResolver), IsModule(IsModule), + NeedDecls(!IsModule || !Writer.getLangOpts().CPlusPlus) {} static hash_value_type ComputeHash(const IdentifierInfo* II) { return llvm::HashString(II->getName()); @@ -3152,10 +3154,12 @@ public: if (MacroOffset) DataLen += 4; // MacroDirectives offset. - for (IdentifierResolver::iterator D = IdResolver.begin(II), - DEnd = IdResolver.end(); - D != DEnd; ++D) - DataLen += 4; + if (NeedDecls) { + for (IdentifierResolver::iterator D = IdResolver.begin(II), + DEnd = IdResolver.end(); + D != DEnd; ++D) + DataLen += 4; + } } using namespace llvm::support; endian::Writer LE(Out); @@ -3205,18 +3209,21 @@ public: if (HadMacroDefinition) LE.write(MacroOffset); - // Emit the declaration IDs in reverse order, because the - // IdentifierResolver provides the declarations as they would be - // visible (e.g., the function "stat" would come before the struct - // "stat"), but the ASTReader adds declarations to the end of the list - // (so we need to see the struct "stat" before the function "stat"). - // Only emit declarations that aren't from a chained PCH, though. - SmallVector Decls(IdResolver.begin(II), IdResolver.end()); - for (SmallVectorImpl::reverse_iterator D = Decls.rbegin(), - DEnd = Decls.rend(); - D != DEnd; ++D) - LE.write( - Writer.getDeclID(getDeclForLocalLookup(PP.getLangOpts(), *D))); + if (NeedDecls) { + // Emit the declaration IDs in reverse order, because the + // IdentifierResolver provides the declarations as they would be + // visible (e.g., the function "stat" would come before the struct + // "stat"), but the ASTReader adds declarations to the end of the list + // (so we need to see the struct "stat" before the function "stat"). + // Only emit declarations that aren't from a chained PCH, though. + SmallVector Decls(IdResolver.begin(II), + IdResolver.end()); + for (SmallVectorImpl::reverse_iterator D = Decls.rbegin(), + DEnd = Decls.rend(); + D != DEnd; ++D) + LE.write( + Writer.getDeclID(getDeclForLocalLookup(PP.getLangOpts(), *D))); + } } }; } // end anonymous namespace diff --git a/clang/test/CXX/class.access/class.friend/p1.cpp b/clang/test/CXX/class.access/class.friend/p1.cpp index 4a681629eeae..54069b6b87be 100644 --- a/clang/test/CXX/class.access/class.friend/p1.cpp +++ b/clang/test/CXX/class.access/class.friend/p1.cpp @@ -8,11 +8,12 @@ // friends members of the befriending class. struct S { static void f(); }; // expected-note 2 {{'S' declared here}} -S* g() { return 0; } // expected-note 2 {{'g' declared here}} +S* g() { return 0; } struct X { friend struct S; - friend S* g(); + friend S* g(); // expected-note 2 {{'g' declared here}} + // FIXME: The above two notes would be better attached to line 11. }; void test1() { diff --git a/clang/test/CXX/class.access/class.friend/p11.cpp b/clang/test/CXX/class.access/class.friend/p11.cpp index a5107fd20728..0d25c59c9b62 100644 --- a/clang/test/CXX/class.access/class.friend/p11.cpp +++ b/clang/test/CXX/class.access/class.friend/p11.cpp @@ -24,13 +24,12 @@ namespace test2 { void foo() { // expected-note {{'::test2::foo' declared here}} struct S1 { friend void foo(); // expected-error {{no matching function 'foo' found in local scope; did you mean '::test2::foo'?}} - // expected-note@-1{{'::test2::foo' declared here}} - // TODO: the above note should go on line 24 }; void foo(); // expected-note {{local declaration nearly matches}} struct S2 { - friend void foo(); + friend void foo(); // expected-note{{'::test2::foo' declared here}} + // TODO: the above note should go on line 24 }; { @@ -48,8 +47,8 @@ namespace test2 { struct S4 { friend void bar(); // expected-error {{no matching function 'bar' found in local scope; did you mean '::test2::bar'?}} - // expected-note@-1 2 {{'::test2::bar' declared here}} - // TODO: the above two notes should go on line 22 + // expected-note@-1 {{'::test2::bar' declared here}} + // TODO: the above note should go on line 22 }; { void bar(); } @@ -82,6 +81,8 @@ namespace test2 { struct S9 { struct Inner { friend void baz(); // expected-error {{no matching function 'baz' found in local scope; did you mean 'bar'?}} + // expected-note@-1 {{'::test2::bar' declared here}} + // TODO: the above note should go on line 22 }; }; diff --git a/clang/test/Modules/linkage-merge.cpp b/clang/test/Modules/linkage-merge.cpp index 005206afadb3..dc2ad759127e 100644 --- a/clang/test/Modules/linkage-merge.cpp +++ b/clang/test/Modules/linkage-merge.cpp @@ -7,9 +7,5 @@ static int f(int); int f(int); static void g(int); -// FIXME: Whether we notice the problem here depends on the order in which we -// happen to find lookup results for 'g'; LookupResult::resolveKind needs to -// be taught to prefer a visible result over a non-visible one. -// // expected-error@9 {{functions that differ only in their return type cannot be overloaded}} // expected-note@Inputs/linkage-merge-foo.h:2 {{previous declaration is here}}