r303224 - [modules] When creating a declaration, cache its owning module immediately

Raphael Isemann via cfe-commits cfe-commits at lists.llvm.org
Thu May 18 07:15:14 PDT 2017


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 at 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), 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;
>
> 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 at -6 {{could not build module 'x'}}
> +#elif ALLOW_TEXTUAL_NAME_LEAKAGE
> +// expected-warning at 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


More information about the cfe-commits mailing list