From bd2320f5d1153f2623f3104ea78e2cc5dbae6afb Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 17 May 2017 00:24:14 +0000 Subject: [PATCH] [modules] When creating a declaration, cache its owning module immediately rather than waiting until it's queried. Currently this is only applied to local submodule visibility mode, as we don't yet allocate storage for the owning module in non-local-visibility modules compilations. This reinstates r302965, reverted in r303037, with a fix for the reported crash, which occurred when reparenting a local declaration to be a child of a hidden imported declaration (specifically during template instantiation). git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@303224 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Decl.h | 10 ---- include/clang/AST/DeclBase.h | 14 +++++ include/clang/Basic/LangOptions.h | 5 ++ include/clang/Sema/Sema.h | 4 +- lib/AST/ASTDumper.cpp | 6 +- lib/AST/Decl.cpp | 4 +- lib/AST/DeclBase.cpp | 10 +++- lib/Sema/Sema.cpp | 9 ++- lib/Sema/SemaDecl.cpp | 15 +++++ lib/Sema/SemaLookup.cpp | 57 ------------------- test/Modules/Inputs/submodule-visibility/b.h | 6 +- .../Inputs/submodule-visibility/other.h | 9 +++ test/Modules/submodule-visibility.cpp | 9 +++ 13 files changed, 73 insertions(+), 85 deletions(-) diff --git a/include/clang/AST/Decl.h b/include/clang/AST/Decl.h index facef8e55f..4f8042ac92 100644 --- a/include/clang/AST/Decl.h +++ b/include/clang/AST/Decl.h @@ -301,16 +301,6 @@ public: using Decl::isModulePrivate; using Decl::setModulePrivate; - /// \brief Determine whether this declaration is hidden from name lookup. - bool isHidden() const { return Hidden; } - - /// \brief Set whether this declaration is hidden from name lookup. - void setHidden(bool Hide) { - assert((!Hide || isFromASTFile() || hasLocalOwningModuleStorage()) && - "declaration with no owning module can't be hidden"); - Hidden = Hide; - } - /// \brief Determine whether this declaration is a C++ class member. bool isCXXClassMember() const { const DeclContext *DC = getDeclContext(); diff --git a/include/clang/AST/DeclBase.h b/include/clang/AST/DeclBase.h index 15ac11a5a7..08879b36cc 100644 --- a/include/clang/AST/DeclBase.h +++ b/include/clang/AST/DeclBase.h @@ -706,6 +706,20 @@ public: reinterpret_cast(this)[-1] = M; } + Module *getOwningModule() const { + return isFromASTFile() ? getImportedOwningModule() : getLocalOwningModule(); + } + + /// \brief Determine whether this declaration is hidden from name lookup. + bool isHidden() const { return Hidden; } + + /// \brief Set whether this declaration is hidden from name lookup. + void setHidden(bool Hide) { + assert((!Hide || isFromASTFile() || hasLocalOwningModuleStorage()) && + "declaration with no owning module can't be hidden"); + Hidden = Hide; + } + unsigned getIdentifierNamespace() const { return IdentifierNamespace; } diff --git a/include/clang/Basic/LangOptions.h b/include/clang/Basic/LangOptions.h index 20a0e58456..ceaedf5857 100644 --- a/include/clang/Basic/LangOptions.h +++ b/include/clang/Basic/LangOptions.h @@ -166,6 +166,11 @@ public: return getCompilingModule() != CMK_None; } + /// Do we need to track the owning module for a local declaration? + bool trackLocalOwningModule() const { + return ModulesLocalVisibility; + } + bool isSignedOverflowDefined() const { return getSignedOverflowBehavior() == SOB_Defined; } diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index e910be14f9..ba2da92c5b 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -1467,11 +1467,9 @@ private: VisibleModuleSet VisibleModules; - Module *CachedFakeTopLevelModule; - public: /// \brief Get the module owning an entity. - Module *getOwningModule(Decl *Entity); + Module *getOwningModule(Decl *Entity) { return Entity->getOwningModule(); } /// \brief Make a merged definition of an existing hidden definition \p ND /// visible at the specified location. diff --git a/lib/AST/ASTDumper.cpp b/lib/AST/ASTDumper.cpp index ef491ab06f..d89be0d9e6 100644 --- a/lib/AST/ASTDumper.cpp +++ b/lib/AST/ASTDumper.cpp @@ -1038,10 +1038,10 @@ void ASTDumper::dumpDecl(const Decl *D) { dumpSourceRange(D->getSourceRange()); OS << ' '; dumpLocation(D->getLocation()); - if (Module *M = D->getImportedOwningModule()) + if (D->isFromASTFile()) + OS << " imported"; + if (Module *M = D->getOwningModule()) OS << " in " << M->getFullModuleName(); - else if (Module *M = D->getLocalOwningModule()) - OS << " in (local) " << M->getFullModuleName(); if (auto *ND = dyn_cast(D)) for (Module *M : D->getASTContext().getModulesWithMergedDefinition( const_cast(ND))) diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index 0f2558e24b..a1342f477b 100644 --- a/lib/AST/Decl.cpp +++ b/lib/AST/Decl.cpp @@ -47,9 +47,7 @@ bool Decl::isOutOfLine() const { TranslationUnitDecl::TranslationUnitDecl(ASTContext &ctx) : Decl(TranslationUnit, nullptr, SourceLocation()), - DeclContext(TranslationUnit), Ctx(ctx), AnonymousNamespace(nullptr) { - Hidden = Ctx.getLangOpts().ModulesLocalVisibility; -} + DeclContext(TranslationUnit), Ctx(ctx), AnonymousNamespace(nullptr) {} //===----------------------------------------------------------------------===// // NamedDecl Implementation diff --git a/lib/AST/DeclBase.cpp b/lib/AST/DeclBase.cpp index 5c2c9cbd01..f6f8169261 100644 --- a/lib/AST/DeclBase.cpp +++ b/lib/AST/DeclBase.cpp @@ -75,7 +75,7 @@ void *Decl::operator new(std::size_t Size, const ASTContext &Ctx, assert(!Parent || &Parent->getParentASTContext() == &Ctx); // With local visibility enabled, we track the owning module even for local // declarations. - if (Ctx.getLangOpts().ModulesLocalVisibility) { + if (Ctx.getLangOpts().trackLocalOwningModule()) { // Ensure required alignment of the resulting object by adding extra // padding at the start if required. size_t ExtraAlign = @@ -83,7 +83,9 @@ void *Decl::operator new(std::size_t Size, const ASTContext &Ctx, char *Buffer = reinterpret_cast( ::operator new(ExtraAlign + sizeof(Module *) + Size + Extra, Ctx)); Buffer += ExtraAlign; - return new (Buffer) Module*(nullptr) + 1; + auto *ParentModule = + Parent ? cast(Parent)->getOwningModule() : nullptr; + return new (Buffer) Module*(ParentModule) + 1; } return ::operator new(Size + Extra, Ctx); } @@ -94,7 +96,7 @@ Module *Decl::getOwningModuleSlow() const { } bool Decl::hasLocalOwningModuleStorage() const { - return getASTContext().getLangOpts().ModulesLocalVisibility; + return getASTContext().getLangOpts().trackLocalOwningModule(); } const char *Decl::getDeclKindName() const { @@ -273,6 +275,8 @@ void Decl::setLexicalDeclContext(DeclContext *DC) { getMultipleDC()->LexicalDC = DC; } Hidden = cast(DC)->Hidden; + if (Hidden && !isFromASTFile() && hasLocalOwningModuleStorage()) + setLocalOwningModule(cast(DC)->getOwningModule()); } void Decl::setDeclContextsImpl(DeclContext *SemaDC, DeclContext *LexicalDC, diff --git a/lib/Sema/Sema.cpp b/lib/Sema/Sema.cpp index ca1d27e950..e7b0914641 100644 --- a/lib/Sema/Sema.cpp +++ b/lib/Sema/Sema.cpp @@ -93,11 +93,10 @@ Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer, ValueWithBytesObjCTypeMethod(nullptr), NSArrayDecl(nullptr), ArrayWithObjectsMethod(nullptr), NSDictionaryDecl(nullptr), DictionaryWithObjectsMethod(nullptr), GlobalNewDeleteDeclared(false), - TUKind(TUKind), NumSFINAEErrors(0), CachedFakeTopLevelModule(nullptr), - AccessCheckingSFINAE(false), InNonInstantiationSFINAEContext(false), - NonInstantiationEntries(0), ArgumentPackSubstitutionIndex(-1), - CurrentInstantiationScope(nullptr), DisableTypoCorrection(false), - TyposCorrected(0), AnalysisWarnings(*this), + TUKind(TUKind), NumSFINAEErrors(0), AccessCheckingSFINAE(false), + InNonInstantiationSFINAEContext(false), NonInstantiationEntries(0), + ArgumentPackSubstitutionIndex(-1), CurrentInstantiationScope(nullptr), + DisableTypoCorrection(false), TyposCorrected(0), AnalysisWarnings(*this), ThreadSafetyDeclCache(nullptr), VarDataSharingAttributesStack(nullptr), CurScope(nullptr), Ident_super(nullptr), Ident___float128(nullptr) { TUScope = nullptr; diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp index 2e069a9def..dca51b0e8c 100644 --- a/lib/Sema/SemaDecl.cpp +++ b/lib/Sema/SemaDecl.cpp @@ -16047,6 +16047,14 @@ void Sema::ActOnModuleBegin(SourceLocation DirectiveLoc, Module *Mod) { ModuleScopes.back().OuterVisibleModules = std::move(VisibleModules); VisibleModules.setVisible(Mod, DirectiveLoc); + + // The enclosing context is now part of this module. + // FIXME: Consider creating a child DeclContext to hold the entities + // lexically within the module. + if (getLangOpts().trackLocalOwningModule()) { + cast(CurContext)->setHidden(true); + cast(CurContext)->setLocalOwningModule(Mod); + } } void Sema::ActOnModuleEnd(SourceLocation EomLoc, Module *Mod) { @@ -16075,6 +16083,13 @@ void Sema::ActOnModuleEnd(SourceLocation EomLoc, Module *Mod) { DirectiveLoc = EomLoc; } BuildModuleInclude(DirectiveLoc, Mod); + + // Any further declarations are in whatever module we returned to. + if (getLangOpts().trackLocalOwningModule()) { + cast(CurContext)->setLocalOwningModule(getCurrentModule()); + if (!getCurrentModule()) + cast(CurContext)->setHidden(false); + } } void Sema::createImplicitModuleImportForErrorRecovery(SourceLocation Loc, diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp index c5b579a4b2..04350831c6 100644 --- a/lib/Sema/SemaLookup.cpp +++ b/lib/Sema/SemaLookup.cpp @@ -1326,62 +1326,6 @@ bool Sema::CppLookupName(LookupResult &R, Scope *S) { return !R.empty(); } -Module *Sema::getOwningModule(Decl *Entity) { - // If it's imported, grab its owning module. - Module *M = Entity->getImportedOwningModule(); - if (M || !isa(Entity) || !cast(Entity)->isHidden()) - return M; - assert(!Entity->isFromASTFile() && - "hidden entity from AST file has no owning module"); - - if (!getLangOpts().ModulesLocalVisibility) { - // If we're not tracking visibility locally, the only way a declaration - // can be hidden and local is if it's hidden because it's parent is (for - // instance, maybe this is a lazily-declared special member of an imported - // class). - auto *Parent = cast(Entity->getDeclContext()); - assert(Parent->isHidden() && "unexpectedly hidden decl"); - return getOwningModule(Parent); - } - - // It's local and hidden; grab or compute its owning module. - M = Entity->getLocalOwningModule(); - if (M) - return M; - - if (auto *Containing = - PP.getModuleContainingLocation(Entity->getLocation())) { - M = Containing; - } else if (Entity->isInvalidDecl() || Entity->getLocation().isInvalid()) { - // Don't bother tracking visibility for invalid declarations with broken - // locations. - cast(Entity)->setHidden(false); - } else { - // We need to assign a module to an entity that exists outside of any - // module, so that we can hide it from modules that we textually enter. - // Invent a fake module for all such entities. - if (!CachedFakeTopLevelModule) { - CachedFakeTopLevelModule = - PP.getHeaderSearchInfo().getModuleMap().findOrCreateModule( - "", nullptr, false, false).first; - - auto &SrcMgr = PP.getSourceManager(); - SourceLocation StartLoc = - SrcMgr.getLocForStartOfFile(SrcMgr.getMainFileID()); - auto &TopLevel = ModuleScopes.empty() - ? VisibleModules - : ModuleScopes[0].OuterVisibleModules; - TopLevel.setVisible(CachedFakeTopLevelModule, StartLoc); - } - - M = CachedFakeTopLevelModule; - } - - if (M) - Entity->setLocalOwningModule(M); - return M; -} - void Sema::makeMergedDefinitionVisible(NamedDecl *ND) { if (auto *M = getCurrentModule()) Context.mergeDefinitionIntoModule(ND, M); @@ -1520,7 +1464,6 @@ bool LookupResult::isVisibleSlow(Sema &SemaRef, NamedDecl *D) { if (SemaRef.getLangOpts().ModulesLocalVisibility) { DeclModule = SemaRef.getOwningModule(D); if (!DeclModule) { - // getOwningModule() may have decided the declaration should not be hidden. assert(!D->isHidden() && "hidden decl not from a module"); return true; } diff --git a/test/Modules/Inputs/submodule-visibility/b.h b/test/Modules/Inputs/submodule-visibility/b.h index 67ef6529db..39df6a02cb 100644 --- a/test/Modules/Inputs/submodule-visibility/b.h +++ b/test/Modules/Inputs/submodule-visibility/b.h @@ -4,7 +4,11 @@ int m = n; #include "c.h" #if defined(A) && !defined(ALLOW_NAME_LEAKAGE) -#error A is defined +#warning A is defined #endif #define B + +template void b_template() { + N::C::f(0); +} diff --git a/test/Modules/Inputs/submodule-visibility/other.h b/test/Modules/Inputs/submodule-visibility/other.h index f40c757ca6..4b68c48915 100644 --- a/test/Modules/Inputs/submodule-visibility/other.h +++ b/test/Modules/Inputs/submodule-visibility/other.h @@ -1 +1,10 @@ #include "c.h" + +#ifndef OTHER_H +#define OTHER_H +namespace N { + struct C { + template static void f(U) {} + }; +} +#endif diff --git a/test/Modules/submodule-visibility.cpp b/test/Modules/submodule-visibility.cpp index 345ae155bb..4c066e6ab9 100644 --- a/test/Modules/submodule-visibility.cpp +++ b/test/Modules/submodule-visibility.cpp @@ -3,6 +3,11 @@ // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-local-submodule-visibility -fmodules-cache-path=%t -I%S/Inputs/submodule-visibility -verify %s -DIMPORT // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-local-submodule-visibility -fmodules-cache-path=%t -fmodule-name=x -I%S/Inputs/submodule-visibility -verify %s // RUN: %clang_cc1 -fimplicit-module-maps -fmodules-local-submodule-visibility -fmodules-cache-path=%t -I%S/Inputs/submodule-visibility -verify %s +// +// Explicit module builds. +// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility -emit-module -x c++-module-map %S/Inputs/submodule-visibility/module.modulemap -fmodule-name=other -o %t/other.pcm +// RUN: %clang_cc1 -fmodules -fmodule-map-file=%S/Inputs/submodule-visibility/module.modulemap -fmodules-local-submodule-visibility -fmodule-file=%t/other.pcm -verify -fmodule-name=x -I%S/Inputs/submodule-visibility %s +// RUN: %clang_cc1 -fmodules -fmodule-map-file=%S/Inputs/submodule-visibility/module.modulemap -fmodule-file=%t/other.pcm -verify -fmodule-name=x -I%S/Inputs/submodule-visibility %s -DALLOW_TEXTUAL_NAME_LEAKAGE #include "a.h" #include "b.h" @@ -11,6 +16,8 @@ // expected-no-diagnostics #elif IMPORT // expected-error@-6 {{could not build module 'x'}} +#elif ALLOW_TEXTUAL_NAME_LEAKAGE +// expected-warning@b.h:7 {{A is defined}} #else // The use of -fmodule-name=x causes us to textually include the above headers. // The submodule visibility rules are still applied in this case. @@ -35,3 +42,5 @@ typedef struct { int p; void (*f)(int p); } name_for_linkage; + +void g() { b_template(); } -- 2.40.0