r232793 - [modules] Remove some redundant work when building a lookup table for a DeclContext.
Vince Harron
vince at nethacker.com
Sat Mar 21 21:50:26 PDT 2015
Hi Richard,
This CL breaks some LLDB tests on the build server.
(A problem with the build server is making it difficult to see.)
Notice that there are a lot lines with "^FAIL:" failures in 1045 than in
1044
http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/1045/steps/test%20lldb/logs/stdio
I've also reproduced this increase in failures locally. (see attached log
files)
To reproduce locally, please run the attached script
./build.py 232792 232793
On Thu, Mar 19, 2015 at 7:17 PM, Richard Smith <richard-llvm at metafoo.co.uk>
wrote:
> Author: rsmith
> Date: Thu Mar 19 21:17:21 2015
> New Revision: 232793
>
> URL: http://llvm.org/viewvc/llvm-project?rev=232793&view=rev
> Log:
> [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).
>
> Modified:
> cfe/trunk/include/clang/AST/DeclBase.h
> cfe/trunk/lib/AST/ASTContext.cpp
> cfe/trunk/lib/AST/DeclBase.cpp
> cfe/trunk/lib/Sema/IdentifierResolver.cpp
> cfe/trunk/lib/Sema/SemaLookup.cpp
> cfe/trunk/lib/Serialization/ASTReader.cpp
> cfe/trunk/test/Modules/cxx-templates.cpp
> cfe/trunk/test/Modules/odr.cpp
> cfe/trunk/test/Modules/redecl-add-after-load.cpp
>
> Modified: cfe/trunk/include/clang/AST/DeclBase.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclBase.h?rev=232793&r1=232792&r2=232793&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/AST/DeclBase.h (original)
> +++ cfe/trunk/include/clang/AST/DeclBase.h Thu Mar 19 21:17:21 2015
> @@ -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);
>
> Modified: cfe/trunk/lib/AST/ASTContext.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTContext.cpp?rev=232793&r1=232792&r2=232793&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/AST/ASTContext.cpp (original)
> +++ cfe/trunk/lib/AST/ASTContext.cpp Thu Mar 19 21:17:21 2015
> @@ -7946,7 +7946,7 @@ static GVALinkage basicGVALinkageForVari
> 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 =
>
> Modified: cfe/trunk/lib/AST/DeclBase.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclBase.cpp?rev=232793&r1=232792&r2=232793&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/AST/DeclBase.cpp (original)
> +++ cfe/trunk/lib/AST/DeclBase.cpp Thu Mar 19 21:17:21 2015
> @@ -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(DeclCo
> // 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(DeclarationNa
> 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();
>
>
> Modified: cfe/trunk/lib/Sema/IdentifierResolver.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/IdentifierResolver.cpp?rev=232793&r1=232792&r2=232793&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/IdentifierResolver.cpp (original)
> +++ cfe/trunk/lib/Sema/IdentifierResolver.cpp Thu Mar 19 21:17:21 2015
> @@ -98,7 +98,7 @@ bool IdentifierResolver::isDeclInScope(D
> 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();
>
> Modified: cfe/trunk/lib/Sema/SemaLookup.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaLookup.cpp?rev=232793&r1=232792&r2=232793&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaLookup.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaLookup.cpp Thu Mar 19 21:17:21 2015
> @@ -3021,17 +3021,45 @@ static void LookupVisibleDecls(DeclConte
> 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);
> }
> }
> }
>
> Modified: cfe/trunk/lib/Serialization/ASTReader.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=232793&r1=232792&r2=232793&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Serialization/ASTReader.cpp (original)
> +++ cfe/trunk/lib/Serialization/ASTReader.cpp Thu Mar 19 21:17:21 2015
> @@ -2835,6 +2835,8 @@ ASTReader::ReadASTBlock(ModuleFile &F, u
> }
>
> 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::PassInterestingDeclsToCo
> 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::PassInterestingDeclToCon
> 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);
>
> Modified: cfe/trunk/test/Modules/cxx-templates.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/cxx-templates.cpp?rev=232793&r1=232792&r2=232793&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/Modules/cxx-templates.cpp (original)
> +++ cfe/trunk/test/Modules/cxx-templates.cpp Thu Mar 19 21:17:21 2015
> @@ -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 at cxx-templates-c.h:10 {{here}}
> static_assert(MergeTemplateDefinitions<int>::g() == 2, "");
>
> RedeclaredAsFriend<int> raf1;
> @@ -140,7 +141,6 @@ MergeSpecializations<int[]>::partially_s
> 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;
>
> Modified: cfe/trunk/test/Modules/odr.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/odr.cpp?rev=232793&r1=232792&r2=232793&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/Modules/odr.cpp (original)
> +++ cfe/trunk/test/Modules/odr.cpp Thu Mar 19 21:17:21 2015
> @@ -15,9 +15,9 @@ bool b = F<int>{0} == F<int>{1};
> int x = f() + g();
>
> // expected-note at a.h:5 {{definition has no member 'e2'}}
> -// expected-note at a.h:3 {{declaration of 'f' does not match}}
> -// expected-note at a.h:1 {{definition has no member 'm'}}
> +// expected-note at b.h:3 {{declaration of 'f' does not match}}
> +// expected-note at b.h:1 {{definition has no member 'n'}}
>
> // expected-error at b.h:5 {{'E::e2' from module 'b' is not present in
> definition of 'E' in module 'a'}}
> -// expected-error at b.h:3 {{'Y::f' from module 'b' is not present in
> definition of 'Y' in module 'a'}}
> -// expected-error at b.h:2 {{'Y::m' from module 'b' is not present in
> definition of 'Y' in module 'a'}}
> +// expected-error at a.h:3 {{'Y::f' from module 'a' is not present in
> definition of 'Y' in module 'b'}}
> +// expected-error at a.h:2 {{'Y::n' from module 'a' is not present in
> definition of 'Y' in module 'b'}}
>
> Modified: cfe/trunk/test/Modules/redecl-add-after-load.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/redecl-add-after-load.cpp?rev=232793&r1=232792&r2=232793&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/Modules/redecl-add-after-load.cpp (original)
> +++ cfe/trunk/test/Modules/redecl-add-after-load.cpp Thu Mar 19 21:17:21
> 2015
> @@ -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_tes
> constexpr int merged_struct_function_test = D_test(false);
> #ifndef IMPORT_DECLS
> // expected-error at -4 {{incomplete}}
> -// expected-error at -4 {{constant}} expected-note at -4 {{in call to}}
> +// @-4: definition of D::variable must be emitted, so it gets imported
> eagerly
> // expected-error at -4 {{constant}} expected-note at -4 {{in call to}}
> #endif
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150321/ff4a75b2/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 232792.log
Type: text/x-log
Size: 855127 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150321/ff4a75b2/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 232793.log
Type: text/x-log
Size: 881502 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150321/ff4a75b2/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: build.py
Type: text/x-python
Size: 729 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150321/ff4a75b2/attachment.py>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: svn.sh
Type: application/x-sh
Size: 1326 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150321/ff4a75b2/attachment.sh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: build.sh
Type: application/x-sh
Size: 48 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150321/ff4a75b2/attachment-0001.sh>
More information about the cfe-commits
mailing list