Thanks for the awesome testcase, taking a look now. On 18 May 2017 at 07:15, Raphael Isemann <teempe...@gmail.com> wrote:
> Hi, > > this is breaking our STL module builds. Can we revert this? > > We also have a minimal reproducer for our crash here > http://teemperor.de/pub/stl_merging_issue.zip > > - Raphael > > 2017-05-17 2:24 GMT+02:00 Richard Smith via cfe-commits > <cfe-commits@lists.llvm.org>: > > Author: rsmith > > Date: Tue May 16 19:24:14 2017 > > New Revision: 303224 > > > > URL: http://llvm.org/viewvc/llvm-project?rev=303224&view=rev > > Log: > > [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). > > > > Modified: > > cfe/trunk/include/clang/AST/Decl.h > > cfe/trunk/include/clang/AST/DeclBase.h > > cfe/trunk/include/clang/Basic/LangOptions.h > > cfe/trunk/include/clang/Sema/Sema.h > > cfe/trunk/lib/AST/ASTDumper.cpp > > cfe/trunk/lib/AST/Decl.cpp > > cfe/trunk/lib/AST/DeclBase.cpp > > cfe/trunk/lib/Sema/Sema.cpp > > cfe/trunk/lib/Sema/SemaDecl.cpp > > cfe/trunk/lib/Sema/SemaLookup.cpp > > cfe/trunk/test/Modules/Inputs/submodule-visibility/b.h > > cfe/trunk/test/Modules/Inputs/submodule-visibility/other.h > > cfe/trunk/test/Modules/submodule-visibility.cpp > > > > Modified: cfe/trunk/include/clang/AST/Decl.h > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ > clang/AST/Decl.h?rev=303224&r1=303223&r2=303224&view=diff > > ============================================================ > ================== > > --- cfe/trunk/include/clang/AST/Decl.h (original) > > +++ cfe/trunk/include/clang/AST/Decl.h Tue May 16 19:24:14 2017 > > @@ -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(); > > > > Modified: cfe/trunk/include/clang/AST/DeclBase.h > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ > clang/AST/DeclBase.h?rev=303224&r1=303223&r2=303224&view=diff > > ============================================================ > ================== > > --- cfe/trunk/include/clang/AST/DeclBase.h (original) > > +++ cfe/trunk/include/clang/AST/DeclBase.h Tue May 16 19:24:14 2017 > > @@ -706,6 +706,20 @@ public: > > reinterpret_cast<Module **>(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; > > } > > > > Modified: cfe/trunk/include/clang/Basic/LangOptions.h > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ > clang/Basic/LangOptions.h?rev=303224&r1=303223&r2=303224&view=diff > > ============================================================ > ================== > > --- cfe/trunk/include/clang/Basic/LangOptions.h (original) > > +++ cfe/trunk/include/clang/Basic/LangOptions.h Tue May 16 19:24:14 2017 > > @@ -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; > > } > > > > Modified: cfe/trunk/include/clang/Sema/Sema.h > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ > clang/Sema/Sema.h?rev=303224&r1=303223&r2=303224&view=diff > > ============================================================ > ================== > > --- cfe/trunk/include/clang/Sema/Sema.h (original) > > +++ cfe/trunk/include/clang/Sema/Sema.h Tue May 16 19:24:14 2017 > > @@ -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. > > > > Modified: cfe/trunk/lib/AST/ASTDumper.cpp > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ > ASTDumper.cpp?rev=303224&r1=303223&r2=303224&view=diff > > ============================================================ > ================== > > --- cfe/trunk/lib/AST/ASTDumper.cpp (original) > > +++ cfe/trunk/lib/AST/ASTDumper.cpp Tue May 16 19:24:14 2017 > > @@ -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<NamedDecl>(D)) > > for (Module *M : D->getASTContext(). > getModulesWithMergedDefinition( > > const_cast<NamedDecl *>(ND))) > > > > Modified: cfe/trunk/lib/AST/Decl.cpp > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ > Decl.cpp?rev=303224&r1=303223&r2=303224&view=diff > > ============================================================ > ================== > > --- cfe/trunk/lib/AST/Decl.cpp (original) > > +++ cfe/trunk/lib/AST/Decl.cpp Tue May 16 19:24:14 2017 > > @@ -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 > > > > Modified: cfe/trunk/lib/AST/DeclBase.cpp > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ > DeclBase.cpp?rev=303224&r1=303223&r2=303224&view=diff > > ============================================================ > ================== > > --- cfe/trunk/lib/AST/DeclBase.cpp (original) > > +++ cfe/trunk/lib/AST/DeclBase.cpp Tue May 16 19:24:14 2017 > > @@ -75,7 +75,7 @@ void *Decl::operator new(std::size_t Siz > > 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 Siz > > char *Buffer = reinterpret_cast<char *>( > > ::operator new(ExtraAlign + sizeof(Module *) + Size + Extra, > Ctx)); > > Buffer += ExtraAlign; > > - return new (Buffer) Module*(nullptr) + 1; > > + auto *ParentModule = > > + Parent ? cast<Decl>(Parent)->getOwningModule() : nullptr; > > + return new (Buffer) Module*(ParentModule) + 1; > > } > > return ::operator new(Size + Extra, Ctx); > > } > > @@ -94,7 +96,7 @@ Module *Decl::getOwningModuleSlow() cons > > } > > > > bool Decl::hasLocalOwningModuleStorage() const { > > - return getASTContext().getLangOpts().ModulesLocalVisibility; > > + return getASTContext().getLangOpts().trackLocalOwningModule(); > > } > > > > const char *Decl::getDeclKindName() const { > > @@ -273,6 +275,8 @@ void Decl::setLexicalDeclContext(DeclCon > > getMultipleDC()->LexicalDC = DC; > > } > > Hidden = cast<Decl>(DC)->Hidden; > > + if (Hidden && !isFromASTFile() && hasLocalOwningModuleStorage()) > > + setLocalOwningModule(cast<Decl>(DC)->getOwningModule()); > > } > > > > void Decl::setDeclContextsImpl(DeclContext *SemaDC, DeclContext > *LexicalDC, > > > > Modified: cfe/trunk/lib/Sema/Sema.cpp > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/ > Sema.cpp?rev=303224&r1=303223&r2=303224&view=diff > > ============================================================ > ================== > > --- cfe/trunk/lib/Sema/Sema.cpp (original) > > +++ cfe/trunk/lib/Sema/Sema.cpp Tue May 16 19:24:14 2017 > > @@ -93,11 +93,10 @@ Sema::Sema(Preprocessor &pp, ASTContext > > ValueWithBytesObjCTypeMethod(nullptr), NSArrayDecl(nullptr), > > ArrayWithObjectsMethod(nullptr), NSDictionaryDecl(nullptr), > > DictionaryWithObjectsMethod(nullptr), > GlobalNewDeleteDeclared(false), > > - TUKind(TUKind), NumSFINAEErrors(0), CachedFakeTopLevelModule( > nullptr), > > - AccessCheckingSFINAE(false), InNonInstantiationSFINAEContex > t(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; > > > > Modified: cfe/trunk/lib/Sema/SemaDecl.cpp > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/ > SemaDecl.cpp?rev=303224&r1=303223&r2=303224&view=diff > > ============================================================ > ================== > > --- cfe/trunk/lib/Sema/SemaDecl.cpp (original) > > +++ cfe/trunk/lib/Sema/SemaDecl.cpp Tue May 16 19:24:14 2017 > > @@ -16047,6 +16047,14 @@ void Sema::ActOnModuleBegin(SourceLocati > > 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<Decl>(CurContext)->setHidden(true); > > + cast<Decl>(CurContext)->setLocalOwningModule(Mod); > > + } > > } > > > > void Sema::ActOnModuleEnd(SourceLocation EomLoc, Module *Mod) { > > @@ -16075,6 +16083,13 @@ void Sema::ActOnModuleEnd(SourceLocation > > DirectiveLoc = EomLoc; > > } > > BuildModuleInclude(DirectiveLoc, Mod); > > + > > + // Any further declarations are in whatever module we returned to. > > + if (getLangOpts().trackLocalOwningModule()) { > > + cast<Decl>(CurContext)->setLocalOwningModule(getCurrentModule()); > > + if (!getCurrentModule()) > > + cast<Decl>(CurContext)->setHidden(false); > > + } > > } > > > > void Sema::createImplicitModuleImportForErrorRecovery(SourceLocation > Loc, > > > > Modified: cfe/trunk/lib/Sema/SemaLookup.cpp > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/ > SemaLookup.cpp?rev=303224&r1=303223&r2=303224&view=diff > > ============================================================ > ================== > > --- cfe/trunk/lib/Sema/SemaLookup.cpp (original) > > +++ cfe/trunk/lib/Sema/SemaLookup.cpp Tue May 16 19:24:14 2017 > > @@ -1326,62 +1326,6 @@ bool Sema::CppLookupName(LookupResult &R > > return !R.empty(); > > } > > > > -Module *Sema::getOwningModule(Decl *Entity) { > > - // If it's imported, grab its owning module. > > - Module *M = Entity->getImportedOwningModule(); > > - if (M || !isa<NamedDecl>(Entity) || !cast<NamedDecl>(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<NamedDecl>(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<NamedDecl>(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( > > - "<top-level>", 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 &S > > 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; > > } > > > > Modified: cfe/trunk/test/Modules/Inputs/submodule-visibility/b.h > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ > Modules/Inputs/submodule-visibility/b.h?rev=303224&r1= > 303223&r2=303224&view=diff > > ============================================================ > ================== > > --- cfe/trunk/test/Modules/Inputs/submodule-visibility/b.h (original) > > +++ cfe/trunk/test/Modules/Inputs/submodule-visibility/b.h Tue May 16 > 19:24:14 2017 > > @@ -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<typename T> void b_template() { > > + N::C::f(0); > > +} > > > > Modified: cfe/trunk/test/Modules/Inputs/submodule-visibility/other.h > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ > Modules/Inputs/submodule-visibility/other.h?rev=303224& > r1=303223&r2=303224&view=diff > > ============================================================ > ================== > > --- cfe/trunk/test/Modules/Inputs/submodule-visibility/other.h > (original) > > +++ cfe/trunk/test/Modules/Inputs/submodule-visibility/other.h Tue May > 16 19:24:14 2017 > > @@ -1 +1,10 @@ > > #include "c.h" > > + > > +#ifndef OTHER_H > > +#define OTHER_H > > +namespace N { > > + struct C { > > + template<typename U> static void f(U) {} > > + }; > > +} > > +#endif > > > > Modified: cfe/trunk/test/Modules/submodule-visibility.cpp > > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ > Modules/submodule-visibility.cpp?rev=303224&r1=303223&r2=303224&view=diff > > ============================================================ > ================== > > --- cfe/trunk/test/Modules/submodule-visibility.cpp (original) > > +++ cfe/trunk/test/Modules/submodule-visibility.cpp Tue May 16 19:24:14 > 2017 > > @@ -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<int>(); } > > > > > > _______________________________________________ > > cfe-commits mailing list > > cfe-commits@lists.llvm.org > > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
