From c9e137359d69ba06ab347da130bbc5046452cb69 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Fri, 1 May 2015 21:22:17 +0000 Subject: [PATCH] [modules] Add -fmodules-local-submodule-visibility flag. This flag specifies that the normal visibility rules should be used even for local submodules (submodules of the currently-being-built module). Thus names will only be visible if a header / module that declares them has actually been included / imported, and not merely because a submodule that happened to be built earlier declared those names. This also removes the need to modularize bottom-up: textually-included headers will be included into every submodule that includes them, since their include guards will not leak between modules. So far, this only governs visibility of macros, not of declarations, so is not ready for real use yet. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@236350 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/LangOptions.def | 1 + include/clang/Basic/Module.h | 12 ++++- include/clang/Driver/CC1Options.td | 4 ++ include/clang/Lex/Preprocessor.h | 23 +++++----- lib/Frontend/CompilerInvocation.cpp | 2 + lib/Lex/PPLexerChange.cpp | 69 ++++++++++++++++++++++------- lib/Serialization/ASTWriter.cpp | 45 +++++++------------ test/Modules/macro-ambiguity.cpp | 21 +++++++++ test/Modules/macro-reexport.cpp | 9 ++++ test/Modules/macros.c | 24 ++++++---- test/Modules/macros2.c | 17 ++++--- 11 files changed, 153 insertions(+), 74 deletions(-) diff --git a/include/clang/Basic/LangOptions.def b/include/clang/Basic/LangOptions.def index bd90a2c2d7..baabfe3a49 100644 --- a/include/clang/Basic/LangOptions.def +++ b/include/clang/Basic/LangOptions.def @@ -130,6 +130,7 @@ COMPATIBLE_LANGOPT(ModulesStrictDeclUse, 1, 0, "require declaration of module us LANGOPT(ModulesErrorRecovery, 1, 1, "automatically import modules as needed when performing error recovery") BENIGN_LANGOPT(ModulesImplicitMaps, 1, 1, "use files called module.modulemap implicitly as module maps") BENIGN_LANGOPT(ImplicitModules, 1, 1, "build modules that are not specified via -fmodule-file") +COMPATIBLE_LANGOPT(ModulesLocalVisibility, 1, 0, "local submodule visibility") COMPATIBLE_LANGOPT(Optimize , 1, 0, "__OPTIMIZE__ predefined macro") COMPATIBLE_LANGOPT(OptimizeSize , 1, 0, "__OPTIMIZE_SIZE__ predefined macro") LANGOPT(Static , 1, 0, "__STATIC__ predefined macro (as opposed to __DYNAMIC__)") diff --git a/include/clang/Basic/Module.h b/include/clang/Basic/Module.h index 3c08b24d51..2ce880ee7b 100644 --- a/include/clang/Basic/Module.h +++ b/include/clang/Basic/Module.h @@ -480,14 +480,24 @@ private: class VisibleModuleSet { public: VisibleModuleSet() : Generation(0) {} + VisibleModuleSet(VisibleModuleSet &&O) + : ImportLocs(std::move(O.ImportLocs)), Generation(O.Generation ? 1 : 0) { + O.ImportLocs.clear(); + ++O.Generation; + } + /// Move from another visible modules set. Guaranteed to leave the source + /// empty and bump the generation on both. VisibleModuleSet &operator=(VisibleModuleSet &&O) { ImportLocs = std::move(O.ImportLocs); + O.ImportLocs.clear(); + ++O.Generation; ++Generation; return *this; } - /// \brief Get the current visibility generation. + /// \brief Get the current visibility generation. Incremented each time the + /// set of visible modules changes in any way. unsigned getGeneration() const { return Generation; } /// \brief Determine whether a module is visible. diff --git a/include/clang/Driver/CC1Options.td b/include/clang/Driver/CC1Options.td index 12b4c65574..196035e2ef 100644 --- a/include/clang/Driver/CC1Options.td +++ b/include/clang/Driver/CC1Options.td @@ -347,6 +347,10 @@ def fmodule_map_file_home_is_cwd : Flag<["-"], "fmodule-map-file-home-is-cwd">, def fmodule_feature : Separate<["-"], "fmodule-feature">, MetaVarName<"">, HelpText<"Enable in module map requires declarations">; +def fmodules_local_submodule_visibility : + Flag<["-"], "fmodules-local-submodule-visibility">, + HelpText<"Enforce name visibility rules across submodules of the same " + "top-level module.">; let Group = Action_Group in { diff --git a/include/clang/Lex/Preprocessor.h b/include/clang/Lex/Preprocessor.h index 9950974aeb..108fc37ac2 100644 --- a/include/clang/Lex/Preprocessor.h +++ b/include/clang/Lex/Preprocessor.h @@ -466,12 +466,15 @@ class Preprocessor : public RefCountedBase { return Info->OverriddenMacros; return None; } - void setOverriddenMacros(ArrayRef Overrides) { + void setOverriddenMacros(Preprocessor &PP, + ArrayRef Overrides) { auto *Info = State.dyn_cast(); if (!Info) { - assert(Overrides.empty() && - "have overrides but never had module macro"); - return; + if (Overrides.empty()) + return; + Info = new (PP.getPreprocessorAllocator()) + ModuleMacroInfo(State.get()); + State = Info; } Info->OverriddenMacros.clear(); Info->OverriddenMacros.insert(Info->OverriddenMacros.end(), @@ -498,16 +501,11 @@ class Preprocessor : public RefCountedBase { Module *M; /// The location at which the module was included. SourceLocation ImportLoc; - - struct SavedMacroInfo { - SavedMacroInfo() : Latest(nullptr) {} - MacroDirective *Latest; - llvm::TinyPtrVector Overridden; - }; /// The macros that were visible before we entered the module. - llvm::DenseMap Macros; + MacroMap Macros; + /// The set of modules that was visible in the surrounding submodule. + VisibleModuleSet VisibleModules; - // FIXME: VisibleModules? // FIXME: CounterValue? // FIXME: PragmaPushMacroInfo? }; @@ -662,6 +660,7 @@ public: HeaderSearch &getHeaderSearchInfo() const { return HeaderInfo; } IdentifierTable &getIdentifierTable() { return Identifiers; } + const IdentifierTable &getIdentifierTable() const { return Identifiers; } SelectorTable &getSelectorTable() { return Selectors; } Builtin::Context &getBuiltinInfo() { return BuiltinInfo; } llvm::BumpPtrAllocator &getPreprocessorAllocator() { return BP; } diff --git a/lib/Frontend/CompilerInvocation.cpp b/lib/Frontend/CompilerInvocation.cpp index a91e31e36c..79f80d0df7 100644 --- a/lib/Frontend/CompilerInvocation.cpp +++ b/lib/Frontend/CompilerInvocation.cpp @@ -1508,6 +1508,8 @@ static void ParseLangArgs(LangOptions &Opts, ArgList &Args, InputKind IK, Opts.ModulesStrictDeclUse = Args.hasArg(OPT_fmodules_strict_decluse); Opts.ModulesDeclUse = Args.hasArg(OPT_fmodules_decluse) || Opts.ModulesStrictDeclUse; + Opts.ModulesLocalVisibility = + Args.hasArg(OPT_fmodules_local_submodule_visibility); Opts.ModulesSearchAll = Opts.Modules && !Args.hasArg(OPT_fno_modules_search_all) && Args.hasArg(OPT_fmodules_search_all); diff --git a/lib/Lex/PPLexerChange.cpp b/lib/Lex/PPLexerChange.cpp index 6766fdfcd0..c506aa7f6f 100644 --- a/lib/Lex/PPLexerChange.cpp +++ b/lib/Lex/PPLexerChange.cpp @@ -612,16 +612,25 @@ void Preprocessor::HandleMicrosoftCommentPaste(Token &Tok) { void Preprocessor::EnterSubmodule(Module *M, SourceLocation ImportLoc) { // Save the current state for future imports. BuildingSubmoduleStack.push_back(BuildingSubmoduleInfo(M, ImportLoc)); - auto &Info = BuildingSubmoduleStack.back(); - // Copy across our macros and start the submodule with the current state. - // FIXME: We should start each submodule with just the predefined macros. - for (auto &M : Macros) { - BuildingSubmoduleInfo::SavedMacroInfo SMI; - SMI.Latest = M.second.getLatest(); - auto O = M.second.getOverriddenMacros(); - SMI.Overridden.insert(SMI.Overridden.end(), O.begin(), O.end()); - Info.Macros.insert(std::make_pair(M.first, SMI)); + Info.Macros.swap(Macros); + // Save our visible modules set. This is guaranteed to clear the set. + if (getLangOpts().ModulesLocalVisibility) + Info.VisibleModules = std::move(VisibleModules); + + // Determine the set of starting macros for this submodule. + // FIXME: If we re-enter a submodule, should we restore its MacroDirectives? + auto &StartingMacros = (getLangOpts().ModulesLocalVisibility && + BuildingSubmoduleStack.size() > 1) + ? BuildingSubmoduleStack[0].Macros + : Info.Macros; + + // Restore to the starting state. + // FIXME: Do this lazily, when each macro name is first referenced. + for (auto &Macro : StartingMacros) { + MacroState MS(Macro.second.getLatest()); + MS.setOverriddenMacros(*this, MS.getOverriddenMacros()); + Macros.insert(std::make_pair(Macro.first, std::move(MS))); } } @@ -631,19 +640,35 @@ void Preprocessor::LeaveSubmodule() { // Create ModuleMacros for any macros defined in this submodule. for (auto &Macro : Macros) { auto *II = const_cast(Macro.first); - auto SavedInfo = Info.Macros.lookup(II); + auto &OuterInfo = Info.Macros[II]; + + // Find the starting point for the MacroDirective chain in this submodule. + auto *OldMD = OuterInfo.getLatest(); + if (getLangOpts().ModulesLocalVisibility && + BuildingSubmoduleStack.size() > 1) { + auto &PredefMacros = BuildingSubmoduleStack[0].Macros; + auto PredefMacroIt = PredefMacros.find(Macro.first); + if (PredefMacroIt == PredefMacros.end()) + OldMD = nullptr; + else + OldMD = PredefMacroIt->second.getLatest(); + } // This module may have exported a new macro. If so, create a ModuleMacro // representing that fact. bool ExplicitlyPublic = false; - for (auto *MD = Macro.second.getLatest(); MD != SavedInfo.Latest; + for (auto *MD = Macro.second.getLatest(); MD != OldMD; MD = MD->getPrevious()) { assert(MD && "broken macro directive chain"); // Skip macros defined in other submodules we #included along the way. - Module *Mod = getModuleContainingLocation(MD->getLocation()); - if (Mod != Info.M) - continue; + // There's no point doing this if we're tracking local submodule + // visibiltiy, since there can be no such directives in our list. + if (!getLangOpts().ModulesLocalVisibility) { + Module *Mod = getModuleContainingLocation(MD->getLocation()); + if (Mod != Info.M) + continue; + } if (auto *VisMD = dyn_cast(MD)) { // The latest visibility directive for a name in a submodule affects @@ -667,11 +692,21 @@ void Preprocessor::LeaveSubmodule() { } } - // Restore the macro's overrides list. - Macro.second.setOverriddenMacros(SavedInfo.Overridden); + // Maintain a single macro directive chain if we're not tracking + // per-submodule macro visibility. + if (!getLangOpts().ModulesLocalVisibility) + OuterInfo.setLatest(Macro.second.getLatest()); } - makeModuleVisible(Info.M, Info.ImportLoc); + // Put back the old macros. + std::swap(Info.Macros, Macros); + + if (getLangOpts().ModulesLocalVisibility) + VisibleModules = std::move(Info.VisibleModules); + + // A nested #include makes the included submodule visible. + if (BuildingSubmoduleStack.size() > 1) + makeModuleVisible(Info.M, Info.ImportLoc); BuildingSubmoduleStack.pop_back(); } diff --git a/lib/Serialization/ASTWriter.cpp b/lib/Serialization/ASTWriter.cpp index 8a3b9d5163..7ea50e5003 100644 --- a/lib/Serialization/ASTWriter.cpp +++ b/lib/Serialization/ASTWriter.cpp @@ -2019,37 +2019,23 @@ void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule) { // Loop over all the macro directives that are live at the end of the file, // emitting each to the PP section. - // Construct the list of macro directives that need to be serialized. - typedef std::pair MacroChain; - SmallVector MacroDirectives; - for (Preprocessor::macro_iterator - I = PP.macro_begin(/*IncludeExternalMacros=*/false), - E = PP.macro_end(/*IncludeExternalMacros=*/false); - I != E; ++I) { - MacroDirectives.push_back(std::make_pair(I->first, I->second.getLatest())); - } - + // Construct the list of identifiers with macro directives that need to be + // serialized. + SmallVector MacroIdentifiers; + for (auto &Id : PP.getIdentifierTable()) + if (Id.second->hadMacroDefinition() && + (!Id.second->isFromAST() || + Id.second->hasChangedSinceDeserialization())) + MacroIdentifiers.push_back(Id.second); // Sort the set of macro definitions that need to be serialized by the // name of the macro, to provide a stable ordering. - int (*Cmp)(const MacroChain*, const MacroChain*) = - [](const MacroChain *A, const MacroChain *B) -> int { - return A->first->getName().compare(B->first->getName()); - }; - llvm::array_pod_sort(MacroDirectives.begin(), MacroDirectives.end(), Cmp); + std::sort(MacroIdentifiers.begin(), MacroIdentifiers.end(), + llvm::less_ptr()); // Emit the macro directives as a list and associate the offset with the // identifier they belong to. - for (auto &Chain : MacroDirectives) { - const IdentifierInfo *Name = Chain.first; - MacroDirective *MD = Chain.second; - - // If the macro or identifier need no updates, don't write the macro history - // for this one. - // FIXME: Chain the macro history instead of re-writing it. - if (MD && MD->isFromPCH() && - Name->isFromAST() && !Name->hasChangedSinceDeserialization()) - continue; - + for (const IdentifierInfo *Name : MacroIdentifiers) { + MacroDirective *MD = PP.getLocalMacroDirectiveHistory(Name); auto StartOffset = Stream.GetCurrentBitNo(); // Emit the macro directives in reverse source order. @@ -2069,6 +2055,7 @@ void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule) { } // Write out any exported module macros. + bool EmittedModuleMacros = false; if (IsModule) { auto Leafs = PP.getLeafModuleMacros(Name); SmallVector Worklist(Leafs.begin(), Leafs.end()); @@ -2079,7 +2066,7 @@ void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule) { // Emit a record indicating this submodule exports this macro. ModuleMacroRecord.push_back( getSubmoduleID(Macro->getOwningModule())); - ModuleMacroRecord.push_back(getMacroID(Macro->getMacroInfo())); + ModuleMacroRecord.push_back(getMacroRef(Macro->getMacroInfo(), Name)); for (auto *M : Macro->overrides()) ModuleMacroRecord.push_back(getSubmoduleID(M->getOwningModule())); @@ -2090,10 +2077,12 @@ void ASTWriter::WritePreprocessor(const Preprocessor &PP, bool IsModule) { for (auto *M : Macro->overrides()) if (++Visits[M] == M->getNumOverridingMacros()) Worklist.push_back(M); + + EmittedModuleMacros = true; } } - if (Record.empty()) + if (Record.empty() && !EmittedModuleMacros) continue; IdentMacroDirectivesOffsetMap[Name] = StartOffset; diff --git a/test/Modules/macro-ambiguity.cpp b/test/Modules/macro-ambiguity.cpp index ea9e4f549f..af43b35839 100644 --- a/test/Modules/macro-ambiguity.cpp +++ b/test/Modules/macro-ambiguity.cpp @@ -57,6 +57,27 @@ // RUN: -fmodule-file=%t/c.pcm \ // RUN: -fmodule-file=%t/d.pcm \ // RUN: -Wambiguous-macro -verify macro-ambiguity.cpp +// +// RUN: %clang_cc1 -fmodules -x c++ -fmodules-cache-path=%t \ +// RUN: -v -fmodules-local-submodule-visibility \ +// RUN: -iquote Inputs/macro-ambiguity/a/quote \ +// RUN: -isystem Inputs/macro-ambiguity/a/system \ +// RUN: -iquote Inputs/macro-ambiguity/b/quote \ +// RUN: -isystem Inputs/macro-ambiguity/b/system \ +// RUN: -iquote Inputs/macro-ambiguity/c/quote \ +// RUN: -isystem Inputs/macro-ambiguity/c/system \ +// RUN: -iquote Inputs/macro-ambiguity/d/quote \ +// RUN: -isystem Inputs/macro-ambiguity/d/system \ +// RUN: -iquote Inputs/macro-ambiguity/e/quote \ +// RUN: -isystem Inputs/macro-ambiguity/e/system \ +// RUN: -fno-implicit-modules -fno-modules-implicit-maps \ +// RUN: -fmodule-map-file-home-is-cwd \ +// RUN: -fmodule-map-file=Inputs/macro-ambiguity/module.modulemap \ +// RUN: -fmodule-file=%t/a.pcm \ +// RUN: -fmodule-file=%t/b.pcm \ +// RUN: -fmodule-file=%t/c.pcm \ +// RUN: -fmodule-file=%t/d.pcm \ +// RUN: -Wambiguous-macro -verify macro-ambiguity.cpp // Include the textual headers first to maximize the ways in which things can // become ambiguous. diff --git a/test/Modules/macro-reexport.cpp b/test/Modules/macro-reexport.cpp index 1df49b948d..2be6f1532c 100644 --- a/test/Modules/macro-reexport.cpp +++ b/test/Modules/macro-reexport.cpp @@ -7,6 +7,15 @@ // RUN: %clang_cc1 -fsyntax-only -DD2 -I%S/Inputs/macro-reexport -fmodules %s -fmodules-cache-path=%t -verify // RUN: %clang_cc1 -fsyntax-only -DF1 -I%S/Inputs/macro-reexport %s -fmodules-cache-path=%t -verify // RUN: %clang_cc1 -fsyntax-only -DF1 -I%S/Inputs/macro-reexport -fmodules %s -fmodules-cache-path=%t -verify +// +// RUN: %clang_cc1 -fmodules-local-submodule-visibility -fsyntax-only -DC1 -I%S/Inputs/macro-reexport %s -fmodules-cache-path=%t -verify +// RUN: %clang_cc1 -fmodules-local-submodule-visibility -fsyntax-only -DC1 -I%S/Inputs/macro-reexport -fmodules %s -fmodules-cache-path=%t -verify +// RUN: %clang_cc1 -fmodules-local-submodule-visibility -fsyntax-only -DD1 -I%S/Inputs/macro-reexport %s -fmodules-cache-path=%t -verify +// RUN: %clang_cc1 -fmodules-local-submodule-visibility -fsyntax-only -DD1 -I%S/Inputs/macro-reexport -fmodules %s -fmodules-cache-path=%t -verify +// RUN: %clang_cc1 -fmodules-local-submodule-visibility -fsyntax-only -DD2 -I%S/Inputs/macro-reexport %s -fmodules-cache-path=%t -verify +// RUN: %clang_cc1 -fmodules-local-submodule-visibility -fsyntax-only -DD2 -I%S/Inputs/macro-reexport -fmodules %s -fmodules-cache-path=%t -verify +// RUN: %clang_cc1 -fmodules-local-submodule-visibility -fsyntax-only -DF1 -I%S/Inputs/macro-reexport %s -fmodules-cache-path=%t -verify +// RUN: %clang_cc1 -fmodules-local-submodule-visibility -fsyntax-only -DF1 -I%S/Inputs/macro-reexport -fmodules %s -fmodules-cache-path=%t -verify #if defined(F1) #include "f1.h" diff --git a/test/Modules/macros.c b/test/Modules/macros.c index e367fb685b..076166966b 100644 --- a/test/Modules/macros.c +++ b/test/Modules/macros.c @@ -1,9 +1,6 @@ // RUN: rm -rf %t -// RUN: %clang_cc1 -fmodules -x objective-c -emit-module -fmodules-cache-path=%t -fmodule-name=macros_top %S/Inputs/module.map -// RUN: %clang_cc1 -fmodules -x objective-c -emit-module -fmodules-cache-path=%t -fmodule-name=macros_left %S/Inputs/module.map -// RUN: %clang_cc1 -fmodules -x objective-c -emit-module -fmodules-cache-path=%t -fmodule-name=macros_right %S/Inputs/module.map -// RUN: %clang_cc1 -fmodules -x objective-c -emit-module -fmodules-cache-path=%t -fmodule-name=macros %S/Inputs/module.map // RUN: %clang_cc1 -fmodules -x objective-c -verify -fmodules-cache-path=%t -I %S/Inputs %s +// RUN: %clang_cc1 -fmodules -DLOCAL_VISIBILITY -fmodules-local-submodule-visibility -x objective-c -verify -fmodules-cache-path=%t -I %S/Inputs %s // RUN: not %clang_cc1 -E -fmodules -x objective-c -fmodules-cache-path=%t -I %S/Inputs %s | FileCheck -check-prefix CHECK-PREPROCESSED %s // FIXME: When we have a syntax for modules in C, use that. // These notes come from headers in modules, and are bogus. @@ -143,11 +140,20 @@ TOP_DEF_RIGHT_UNDEF *TDRUf() { return TDRUp; } int TOP_DEF_RIGHT_UNDEF; // ok, no longer defined -// FIXME: When macros_right.undef is built, macros_top is visible because -// the state from building macros_right leaks through, so macros_right.undef -// undefines macros_top's macro. -#ifdef TOP_RIGHT_UNDEF -# error TOP_RIGHT_UNDEF should not be defined +#ifdef LOCAL_VISIBILITY +// TOP_RIGHT_UNDEF should not be undefined, because macros_right.undef does +// not undefine macros_right's macro. +# ifndef TOP_RIGHT_UNDEF +# error TOP_RIGHT_UNDEF should still be defined +# endif +#else +// When macros_right.undef is built and local submodule visibility is not +// enabled, macros_top is visible because the state from building +// macros_right leaks through, so macros_right.undef undefines macros_top's +// macro. +# ifdef TOP_RIGHT_UNDEF +# error TOP_RIGHT_UNDEF should not be defined +# endif #endif @import macros_other; diff --git a/test/Modules/macros2.c b/test/Modules/macros2.c index c4c8059011..addb9b495c 100644 --- a/test/Modules/macros2.c +++ b/test/Modules/macros2.c @@ -1,9 +1,6 @@ // RUN: rm -rf %t -// RUN: %clang_cc1 -fmodules -x objective-c -emit-module -fmodules-cache-path=%t -fmodule-name=macros_top %S/Inputs/module.map -// RUN: %clang_cc1 -fmodules -x objective-c -emit-module -fmodules-cache-path=%t -fmodule-name=macros_left %S/Inputs/module.map -// RUN: %clang_cc1 -fmodules -x objective-c -emit-module -fmodules-cache-path=%t -fmodule-name=macros_right %S/Inputs/module.map -// RUN: %clang_cc1 -fmodules -x objective-c -emit-module -fmodules-cache-path=%t -fmodule-name=macros %S/Inputs/module.map // RUN: %clang_cc1 -fmodules -x objective-c -verify -fmodules-cache-path=%t -I %S/Inputs %s +// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility -x objective-c -verify -fmodules-cache-path=%t -I %S/Inputs %s -DLOCAL_VISIBILITY // This test checks some of the same things as macros.c, but imports modules in // a different order. @@ -49,9 +46,15 @@ void test() { @import macros_right.undef; -// FIXME: See macros.c. -#ifdef TOP_RIGHT_UNDEF -# error TOP_RIGHT_UNDEF should not be defined +// See macros.c. +#ifdef LOCAL_VISIBILITY +# ifndef TOP_RIGHT_UNDEF +# error TOP_RIGHT_UNDEF should still be defined +# endif +#else +# ifdef TOP_RIGHT_UNDEF +# error TOP_RIGHT_UNDEF should not be defined +# endif #endif #ifndef TOP_OTHER_UNDEF1 -- 2.40.0