From bf6143fc8779e84f996b74a929af6b8a00dca3c8 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 13 Aug 2014 01:23:33 +0000 Subject: [PATCH] [modules] When performing a lookup into a namespace, ensure that any later redefinitions of that namespace have already been loaded. When writing out the names in a namespace, if we see a name that is locally declared and had imported declarations merged on top of it, export the local declaration as the lookup result, because it will be the most recent declaration of that entity in the redeclaration chain of an importer of the module. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@215518 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/DeclBase.h | 6 ++- lib/AST/ASTDumper.cpp | 2 +- lib/AST/DeclBase.cpp | 5 +++ lib/Serialization/ASTReader.cpp | 41 ++++++++++--------- lib/Serialization/ASTReaderDecl.cpp | 4 +- lib/Serialization/ASTWriter.cpp | 16 +++++++- test/Modules/Inputs/cxx-lookup/a.h | 2 + test/Modules/Inputs/cxx-lookup/b.h | 3 ++ test/Modules/Inputs/cxx-lookup/c1.h | 3 ++ test/Modules/Inputs/cxx-lookup/c2.h | 2 + .../Inputs/cxx-lookup/module.modulemap | 8 ++++ test/Modules/Inputs/cxx-lookup/x.h | 2 + test/Modules/Inputs/cxx-lookup/y.h | 5 +++ test/Modules/cxx-lookup.cpp | 6 +++ test/Modules/cxx-templates.cpp | 4 +- 15 files changed, 82 insertions(+), 27 deletions(-) create mode 100644 test/Modules/Inputs/cxx-lookup/a.h create mode 100644 test/Modules/Inputs/cxx-lookup/b.h create mode 100644 test/Modules/Inputs/cxx-lookup/c1.h create mode 100644 test/Modules/Inputs/cxx-lookup/c2.h create mode 100644 test/Modules/Inputs/cxx-lookup/module.modulemap create mode 100644 test/Modules/Inputs/cxx-lookup/x.h create mode 100644 test/Modules/Inputs/cxx-lookup/y.h create mode 100644 test/Modules/cxx-lookup.cpp diff --git a/include/clang/AST/DeclBase.h b/include/clang/AST/DeclBase.h index 3fb68a47c0..551a883a8f 100644 --- a/include/clang/AST/DeclBase.h +++ b/include/clang/AST/DeclBase.h @@ -515,9 +515,13 @@ public: /// indicating the declaration is used. void markUsed(ASTContext &C); - /// \brief Whether this declaration was referenced. + /// \brief Whether any declaration of this entity was referenced. bool isReferenced() const; + /// \brief Whether this declaration was referenced. This should not be relied + /// upon for anything other than debugging. + bool isThisDeclarationReferenced() const { return Referenced; } + void setReferenced(bool R = true) { Referenced = R; } /// \brief Whether this declaration is a top-level declaration (function, diff --git a/lib/AST/ASTDumper.cpp b/lib/AST/ASTDumper.cpp index 5d6e18a450..d9dc8b57df 100644 --- a/lib/AST/ASTDumper.cpp +++ b/lib/AST/ASTDumper.cpp @@ -826,7 +826,7 @@ void ASTDumper::dumpDecl(const Decl *D) { OS << " implicit"; if (D->isUsed()) OS << " used"; - else if (D->isReferenced()) + else if (D->isThisDeclarationReferenced()) OS << " referenced"; if (D->isInvalidDecl()) OS << " invalid"; diff --git a/lib/AST/DeclBase.cpp b/lib/AST/DeclBase.cpp index 2b1506d191..49d05d0d40 100644 --- a/lib/AST/DeclBase.cpp +++ b/lib/AST/DeclBase.cpp @@ -1296,6 +1296,11 @@ DeclContext::lookup(DeclarationName Name) { if (PrimaryContext != this) return PrimaryContext->lookup(Name); + // If this is a namespace, ensure that any later redeclarations of it have + // been loaded, since they may add names to the result of this lookup. + if (auto *ND = dyn_cast(this)) + (void)ND->getMostRecentDecl(); + if (hasExternalVisibleStorage()) { if (NeedToReconcileExternalVisibleStorage) reconcileExternalVisibleStorage(); diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp index 1cce383f2d..d3d1e48ccb 100644 --- a/lib/Serialization/ASTReader.cpp +++ b/lib/Serialization/ASTReader.cpp @@ -1898,16 +1898,20 @@ void ASTReader::removeOverriddenMacros(IdentifierInfo *II, SubmoduleID OwnerID = Overrides[OI]; // If this macro is not yet visible, remove it from the hidden names list. + // It won't be there if we're in the middle of making the owner visible. Module *Owner = getSubmodule(OwnerID); - HiddenNames &Hidden = HiddenNamesMap[Owner]; - HiddenMacrosMap::iterator HI = Hidden.HiddenMacros.find(II); - if (HI != Hidden.HiddenMacros.end()) { - // Register the macro now so we don't lose it when we re-export. - PP.appendMacroDirective(II, HI->second->import(PP, ImportLoc)); - - auto SubOverrides = HI->second->getOverriddenSubmodules(); - Hidden.HiddenMacros.erase(HI); - removeOverriddenMacros(II, ImportLoc, Ambig, SubOverrides); + auto HiddenIt = HiddenNamesMap.find(Owner); + if (HiddenIt != HiddenNamesMap.end()) { + HiddenNames &Hidden = HiddenIt->second; + HiddenMacrosMap::iterator HI = Hidden.HiddenMacros.find(II); + if (HI != Hidden.HiddenMacros.end()) { + // Register the macro now so we don't lose it when we re-export. + PP.appendMacroDirective(II, HI->second->import(PP, ImportLoc)); + + auto SubOverrides = HI->second->getOverriddenSubmodules(); + Hidden.HiddenMacros.erase(HI); + removeOverriddenMacros(II, ImportLoc, Ambig, SubOverrides); + } } // If this macro is already in our list of conflicts, remove it from there. @@ -2675,7 +2679,6 @@ ASTReader::ReadASTBlock(ModuleFile &F, unsigned ClientLoadCapabilities) { auto *DC = cast(D); DC->getPrimaryContext()->setHasExternalVisibleStorage(true); auto *&LookupTable = F.DeclContextInfos[DC].NameLookupTableData; - // FIXME: There should never be an existing lookup table. delete LookupTable; LookupTable = Table; } else @@ -6051,12 +6054,6 @@ void ASTReader::CompleteRedeclChain(const Decl *D) { const DeclContext *DC = D->getDeclContext()->getRedeclContext(); - // Recursively ensure that the decl context itself is complete - // (in particular, this matters if the decl context is a namespace). - // - // FIXME: This should be performed by lookup instead of here. - cast(DC)->getMostRecentDecl(); - // If this is a named declaration, complete it by looking it up // within its context. // @@ -8246,9 +8243,16 @@ void ASTReader::finishPendingActions() { } void ASTReader::diagnoseOdrViolations() { + if (PendingOdrMergeFailures.empty() && PendingOdrMergeChecks.empty()) + return; + // Trigger the import of the full definition of each class that had any // odr-merging problems, so we can produce better diagnostics for them. - for (auto &Merge : PendingOdrMergeFailures) { + // These updates may in turn find and diagnose some ODR failures, so take + // ownership of the set first. + auto OdrMergeFailures = std::move(PendingOdrMergeFailures); + PendingOdrMergeFailures.clear(); + for (auto &Merge : OdrMergeFailures) { Merge.first->buildLookup(); Merge.first->decls_begin(); Merge.first->bases_begin(); @@ -8322,7 +8326,7 @@ void ASTReader::diagnoseOdrViolations() { } // Issue any pending ODR-failure diagnostics. - for (auto &Merge : PendingOdrMergeFailures) { + for (auto &Merge : OdrMergeFailures) { // If we've already pointed out a specific problem with this class, don't // bother issuing a general "something's different" diagnostic. if (!DiagnosedOdrMergeFailures.insert(Merge.first)) @@ -8361,7 +8365,6 @@ void ASTReader::diagnoseOdrViolations() { << Merge.first; } } - PendingOdrMergeFailures.clear(); } void ASTReader::FinishedDeserializing() { diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp index b153043082..46f6106c13 100644 --- a/lib/Serialization/ASTReaderDecl.cpp +++ b/lib/Serialization/ASTReaderDecl.cpp @@ -2619,11 +2619,11 @@ ASTReader::combineStoredMergedDecls(Decl *Canon, GlobalDeclID CanonID) { // Append the stored merged declarations to the merged declarations set. MergedDeclsMap::iterator Pos = MergedDecls.find(Canon); if (Pos == MergedDecls.end()) - Pos = MergedDecls.insert(std::make_pair(Canon, + Pos = MergedDecls.insert(std::make_pair(Canon, SmallVector())).first; Pos->second.append(StoredPos->second.begin(), StoredPos->second.end()); StoredMergedDecls.erase(StoredPos); - + // Sort and uniquify the set of merged declarations. llvm::array_pod_sort(Pos->second.begin(), Pos->second.end()); Pos->second.erase(std::unique(Pos->second.begin(), Pos->second.end()), diff --git a/lib/Serialization/ASTWriter.cpp b/lib/Serialization/ASTWriter.cpp index ccd57086c3..e8c3177cb8 100644 --- a/lib/Serialization/ASTWriter.cpp +++ b/lib/Serialization/ASTWriter.cpp @@ -3479,6 +3479,18 @@ void ASTWriter::WriteIdentifierTable(Preprocessor &PP, // DeclContext's Name Lookup Table Serialization //===----------------------------------------------------------------------===// +/// Determine the declaration that should be put into the name lookup table to +/// represent the given declaration in this module. This is usually D itself, +/// but if D was imported and merged into a local declaration, we want the most +/// recent local declaration instead. The chosen declaration will be the most +/// recent declaration in any module that imports this one. +static NamedDecl *getDeclForLocalLookup(NamedDecl *D) { + for (Decl *Redecl = D; Redecl; Redecl = Redecl->getPreviousDecl()) + if (!Redecl->isFromASTFile()) + return cast(Redecl); + return D; +} + namespace { // Trait used for the on-disk hash table used in the method pool. class ASTDeclContextNameLookupTrait { @@ -3596,7 +3608,7 @@ public: LE.write(Lookup.size()); for (DeclContext::lookup_iterator I = Lookup.begin(), E = Lookup.end(); I != E; ++I) - LE.write(Writer.GetDeclRef(*I)); + LE.write(Writer.GetDeclRef(getDeclForLocalLookup(*I))); assert(Out.tell() - Start == DataLen && "Data length is wrong"); } @@ -3642,7 +3654,7 @@ void ASTWriter::AddUpdatedDeclContext(const DeclContext *DC) { [&](DeclarationName Name, DeclContext::lookup_const_result Result) { for (auto *Decl : Result) - GetDeclRef(Decl); + GetDeclRef(getDeclForLocalLookup(Decl)); }); } } diff --git a/test/Modules/Inputs/cxx-lookup/a.h b/test/Modules/Inputs/cxx-lookup/a.h new file mode 100644 index 0000000000..25f614fe50 --- /dev/null +++ b/test/Modules/Inputs/cxx-lookup/a.h @@ -0,0 +1,2 @@ +// a +namespace llvm { class GlobalValue; } diff --git a/test/Modules/Inputs/cxx-lookup/b.h b/test/Modules/Inputs/cxx-lookup/b.h new file mode 100644 index 0000000000..c2ede9f28d --- /dev/null +++ b/test/Modules/Inputs/cxx-lookup/b.h @@ -0,0 +1,3 @@ +// b +namespace llvm { class GlobalValue; } +#include "y.h" diff --git a/test/Modules/Inputs/cxx-lookup/c1.h b/test/Modules/Inputs/cxx-lookup/c1.h new file mode 100644 index 0000000000..dba4a4c0a0 --- /dev/null +++ b/test/Modules/Inputs/cxx-lookup/c1.h @@ -0,0 +1,3 @@ +// c1 +#include "a.h" +#include "b.h" diff --git a/test/Modules/Inputs/cxx-lookup/c2.h b/test/Modules/Inputs/cxx-lookup/c2.h new file mode 100644 index 0000000000..463e270e26 --- /dev/null +++ b/test/Modules/Inputs/cxx-lookup/c2.h @@ -0,0 +1,2 @@ +// c2 +namespace llvm { class GlobalValue; } diff --git a/test/Modules/Inputs/cxx-lookup/module.modulemap b/test/Modules/Inputs/cxx-lookup/module.modulemap new file mode 100644 index 0000000000..6d397af250 --- /dev/null +++ b/test/Modules/Inputs/cxx-lookup/module.modulemap @@ -0,0 +1,8 @@ +module A { header "a.h" export * } +module B { header "b.h" export * } +module C { + module C2 { header "c2.h" export * } + module C1 { header "c1.h" export * } +} +module X { header "x.h" export * } +module Y { header "y.h" export * } diff --git a/test/Modules/Inputs/cxx-lookup/x.h b/test/Modules/Inputs/cxx-lookup/x.h new file mode 100644 index 0000000000..a8826e0058 --- /dev/null +++ b/test/Modules/Inputs/cxx-lookup/x.h @@ -0,0 +1,2 @@ +template class allocator; +struct X { virtual allocator f(); }; diff --git a/test/Modules/Inputs/cxx-lookup/y.h b/test/Modules/Inputs/cxx-lookup/y.h new file mode 100644 index 0000000000..8867e8a8be --- /dev/null +++ b/test/Modules/Inputs/cxx-lookup/y.h @@ -0,0 +1,5 @@ +#include "x.h" +namespace llvm { + struct ulittle32_t; + extern allocator *x; +} diff --git a/test/Modules/cxx-lookup.cpp b/test/Modules/cxx-lookup.cpp new file mode 100644 index 0000000000..47c879dbb6 --- /dev/null +++ b/test/Modules/cxx-lookup.cpp @@ -0,0 +1,6 @@ +// RUN: rm -rf %t +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t %s -I%S/Inputs/cxx-lookup -verify +// expected-no-diagnostics +namespace llvm {} +#include "c2.h" +llvm::GlobalValue *p; diff --git a/test/Modules/cxx-templates.cpp b/test/Modules/cxx-templates.cpp index 1b7c045fae..6f5d319a8e 100644 --- a/test/Modules/cxx-templates.cpp +++ b/test/Modules/cxx-templates.cpp @@ -29,8 +29,8 @@ void g() { N::f(1.0); N::f(); N::f(); // expected-error {{no matching function}} - // expected-note@Inputs/cxx-templates-a.h:6 {{couldn't infer template argument}} - // expected-note@Inputs/cxx-templates-a.h:7 {{requires 1 argument}} + // expected-note@Inputs/cxx-templates-b.h:6 {{couldn't infer template argument}} + // expected-note@Inputs/cxx-templates-b.h:7 {{requires single argument}} template_param_kinds_1<0>(); // ok, from cxx-templates-a.h template_param_kinds_1(); // ok, from cxx-templates-b.h -- 2.50.1