llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-modules Author: Michael Spencer (Bigcheese) <details> <summary>Changes</summary> Implicitly discovered module maps that reference files outside their module directory cause order dependent behavior when using implicitly discovered module maps. This adds an off by default diagnostic about these cases with the long term goal of removing import order dependent behavior. Module maps found via `-fmodule-map-file=` are not a problem because they are all loaded at the start of translation. Assisted-by: claude-opus-4.6 --- Patch is 33.35 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/184279.diff 11 Files Affected: - (modified) clang/docs/Modules.rst (+14) - (modified) clang/docs/ReleaseNotes.rst (+6) - (modified) clang/include/clang/Basic/DiagnosticLexKinds.td (+4) - (modified) clang/include/clang/Lex/HeaderSearch.h (+15-2) - (modified) clang/include/clang/Lex/ModuleMap.h (+6-1) - (modified) clang/include/clang/Lex/ModuleMapFile.h (+3-1) - (modified) clang/lib/Frontend/FrontendAction.cpp (+4-3) - (modified) clang/lib/Lex/HeaderSearch.cpp (+44-24) - (modified) clang/lib/Lex/ModuleMap.cpp (+38-12) - (modified) clang/lib/Lex/ModuleMapFile.cpp (+3-1) - (added) clang/test/Modules/deprecated-upwards-relative-path.m (+81) ``````````diff diff --git a/clang/docs/Modules.rst b/clang/docs/Modules.rst index bfdb05ea459c3..379f62ebf548c 100644 --- a/clang/docs/Modules.rst +++ b/clang/docs/Modules.rst @@ -668,6 +668,14 @@ token sequence within the prebuilt module representation. A header with the ``exclude`` specifier is excluded from the module. It will not be included when the module is built, nor will it be considered to be part of the module, even if an ``umbrella`` directory would otherwise make it part of the module. +.. note:: + + Header paths that use ``..`` to refer to files outside of the module + directory are deprecated in module maps that are found via implicit search + (``-fimplicit-module-maps``). Use ``-Wmodule-map-path-outside-directory`` + to warn on such paths. Future versions of Clang may reject these paths + in implicitly discovered module maps. + **Example:** A "X macro" header is an excellent candidate for a textual header, because it is can't be compiled standalone, and by itself does not contain any declarations. .. parsed-literal:: @@ -701,6 +709,12 @@ An umbrella directory declaration specifies that all of the headers in the speci The *string-literal* refers to a directory. When the module is built, all of the header files in that directory (and its subdirectories) are included in the module. +.. note:: + + Umbrella directory paths that use ``..`` to refer to directories outside of + the module directory are deprecated in implicitly discovered module maps. + See the note in `Header declaration`_ for details. + An *umbrella-dir-declaration* shall not refer to the same directory as the location of an umbrella *header-declaration*. In other words, only a single kind of umbrella can be specified for a given directory. .. note:: diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 88bdf765f858c..5c7254d5a1550 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -279,6 +279,12 @@ Improvements to Clang's diagnostics - Clang now emits ``-Wsizeof-pointer-memaccess`` when snprintf/vsnprintf use the sizeof the destination buffer(dynamically allocated) in the len parameter(#GH162366) +- Added ``-Wmodule-map-path-outside-directory`` (off by default) to warn on + header and umbrella directory paths that use ``..`` to refer outside the module + directory in module maps found via implicit search + (``-fimplicit-module-maps``). This does not affect module maps specified + explicitly via ``-fmodule-map-file=``. + Improvements to Clang's time-trace ---------------------------------- diff --git a/clang/include/clang/Basic/DiagnosticLexKinds.td b/clang/include/clang/Basic/DiagnosticLexKinds.td index 77feea9f869e9..5eceeced311f2 100644 --- a/clang/include/clang/Basic/DiagnosticLexKinds.td +++ b/clang/include/clang/Basic/DiagnosticLexKinds.td @@ -996,6 +996,10 @@ def warn_non_modular_include_in_module : Warning< def warn_module_conflict : Warning< "module '%0' conflicts with already-imported module '%1': %2">, InGroup<ModuleConflict>; +def warn_mmap_path_outside_directory : Warning< + "path refers outside of the module directory; such paths in implicitly " + "discovered module maps are deprecated">, + InGroup<DiagGroup<"module-map-path-outside-directory">>, DefaultIgnore; // C++20 modules def err_pp_module_name_is_macro : Error< diff --git a/clang/include/clang/Lex/HeaderSearch.h b/clang/include/clang/Lex/HeaderSearch.h index 25d1ca2a247fe..c566d8c96e7b2 100644 --- a/clang/include/clang/Lex/HeaderSearch.h +++ b/clang/include/clang/Lex/HeaderSearch.h @@ -755,6 +755,8 @@ class HeaderSearch { /// /// \param File The module map file. /// \param IsSystem Whether this file is in a system header directory. + /// \param ImplicitlyDiscovered Whether this file was found by module map + /// search. /// \param ID If the module map file is already mapped (perhaps as part of /// processing a preprocessed module), the ID of the file. /// \param Offset [inout] An offset within ID to start parsing. On exit, @@ -765,6 +767,7 @@ class HeaderSearch { /// building the module from preprocessed source). /// \returns true if an error occurred, false otherwise. bool parseAndLoadModuleMapFile(FileEntryRef File, bool IsSystem, + bool ImplicitlyDiscovered, FileID ID = FileID(), unsigned *Offset = nullptr, StringRef OriginalModuleMapFile = StringRef()); @@ -823,9 +826,12 @@ class HeaderSearch { /// \param IsSystem Whether the framework directory is part of the system /// frameworks. /// + /// \param ImplicitlyDiscovered Whether the framework was discovered by module + /// map search. + /// /// \returns The module, if found; otherwise, null. Module *loadFrameworkModule(StringRef Name, DirectoryEntryRef Dir, - bool IsSystem); + bool IsSystem, bool ImplicitlyDiscovered); /// Load all of the module maps within the immediate subdirectories /// of the given search directory. @@ -985,12 +991,14 @@ class HeaderSearch { ModuleMapResult parseAndLoadModuleMapFileImpl(FileEntryRef File, bool IsSystem, + bool ImplicitlyDiscovered, DirectoryEntryRef Dir, FileID ID = FileID(), unsigned *Offset = nullptr, bool DiagnosePrivMMap = false); ModuleMapResult parseModuleMapFileImpl(FileEntryRef File, bool IsSystem, + bool ImplicitlyDiscovered, DirectoryEntryRef Dir, FileID ID = FileID()); @@ -1004,6 +1012,7 @@ class HeaderSearch { /// \returns The result of attempting to load the module map file from the /// named directory. ModuleMapResult parseAndLoadModuleMapFile(StringRef DirName, bool IsSystem, + bool ImplicitlyDiscovered, bool IsFramework); /// Try to load the module map file in the given directory. @@ -1015,11 +1024,15 @@ class HeaderSearch { /// \returns The result of attempting to load the module map file from the /// named directory. ModuleMapResult parseAndLoadModuleMapFile(DirectoryEntryRef Dir, - bool IsSystem, bool IsFramework); + bool IsSystem, + bool ImplicitlyDiscovered, + bool IsFramework); ModuleMapResult parseModuleMapFile(StringRef DirName, bool IsSystem, + bool ImplicitlyDiscovered, bool IsFramework); ModuleMapResult parseModuleMapFile(DirectoryEntryRef Dir, bool IsSystem, + bool ImplicitlyDiscovered, bool IsFramework); }; diff --git a/clang/include/clang/Lex/ModuleMap.h b/clang/include/clang/Lex/ModuleMap.h index 9133422b2999f..570a68c37fac4 100644 --- a/clang/include/clang/Lex/ModuleMap.h +++ b/clang/include/clang/Lex/ModuleMap.h @@ -726,7 +726,8 @@ class ModuleMap { /// Parse a module map without creating `clang::Module` instances. bool parseModuleMapFile(FileEntryRef File, bool IsSystem, - DirectoryEntryRef Dir, FileID ID = FileID(), + bool ImplicitlyDiscovered, DirectoryEntryRef Dir, + FileID ID = FileID(), SourceLocation ExternModuleLoc = SourceLocation()); void loadAllParsedModules(); @@ -739,6 +740,9 @@ class ModuleMap { /// \param IsSystem Whether this module map file is in a system header /// directory, and therefore should be considered a system module. /// + /// \param ImplicitlyDiscovered Whether this module map file was found via + /// module map search. + /// /// \param HomeDir The directory in which relative paths within this module /// map file will be resolved. /// @@ -753,6 +757,7 @@ class ModuleMap { /// \returns true if an error occurred, false otherwise. bool parseAndLoadModuleMapFile(FileEntryRef File, bool IsSystem, + bool ImplicitlyDiscovered, DirectoryEntryRef HomeDir, FileID ID = FileID(), unsigned *Offset = nullptr, SourceLocation ExternModuleLoc = SourceLocation()); diff --git a/clang/include/clang/Lex/ModuleMapFile.h b/clang/include/clang/Lex/ModuleMapFile.h index 7d0e36e9ab86c..59389cd85a928 100644 --- a/clang/include/clang/Lex/ModuleMapFile.h +++ b/clang/include/clang/Lex/ModuleMapFile.h @@ -144,6 +144,7 @@ struct ModuleMapFile { SourceLocation Start; bool IsSystem; + bool ImplicitlyDiscovered; std::vector<TopLevelDecl> Decls; void dump(llvm::raw_ostream &out) const; @@ -163,7 +164,8 @@ struct ModuleMapFile { /// \returns The parsed ModuleMapFile if successful, std::nullopt otherwise. std::optional<ModuleMapFile> parseModuleMap(FileID ID, clang::DirectoryEntryRef Dir, SourceManager &SM, - DiagnosticsEngine &Diags, bool IsSystem, unsigned *Offset); + DiagnosticsEngine &Diags, bool IsSystem, + bool ImplicitlyDiscovered, unsigned *Offset); } // namespace modulemap } // namespace clang diff --git a/clang/lib/Frontend/FrontendAction.cpp b/clang/lib/Frontend/FrontendAction.cpp index 79e862f01be14..73f092521546f 100644 --- a/clang/lib/Frontend/FrontendAction.cpp +++ b/clang/lib/Frontend/FrontendAction.cpp @@ -706,8 +706,9 @@ static bool loadModuleMapForModuleBuild(CompilerInstance &CI, bool IsSystem, } // Load the module map file. - if (HS.parseAndLoadModuleMapFile(*ModuleMap, IsSystem, ModuleMapID, &Offset, - PresumedModuleMapFile)) + if (HS.parseAndLoadModuleMapFile(*ModuleMap, IsSystem, + /*ImplicitlyDiscovered=*/false, ModuleMapID, + &Offset, PresumedModuleMapFile)) return true; if (SrcMgr.getBufferOrFake(ModuleMapID).getBufferSize() == Offset) @@ -1176,7 +1177,7 @@ bool FrontendAction::BeginSourceFile(CompilerInstance &CI, for (const auto &Filename : CI.getFrontendOpts().ModuleMapFiles) { if (auto File = CI.getFileManager().getOptionalFileRef(Filename)) CI.getPreprocessor().getHeaderSearchInfo().parseAndLoadModuleMapFile( - *File, /*IsSystem*/ false); + *File, /*IsSystem*/ false, /*ImplicitlyDiscovered=*/false); else CI.getDiagnostics().Report(diag::err_module_map_not_found) << Filename; } diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp index 4435cb6f23d92..c6fc96785cd4c 100644 --- a/clang/lib/Lex/HeaderSearch.cpp +++ b/clang/lib/Lex/HeaderSearch.cpp @@ -342,7 +342,8 @@ Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName, if (auto FrameworkDir = FileMgr.getOptionalDirectoryRef(FrameworkDirName)) { bool IsSystem = Dir.getDirCharacteristic() != SrcMgr::C_User; - Module = loadFrameworkModule(ModuleName, *FrameworkDir, IsSystem); + Module = loadFrameworkModule(ModuleName, *FrameworkDir, IsSystem, + /*ImplicitlyDiscovered=*/true); if (Module) break; } @@ -359,7 +360,7 @@ Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName, // checked DirectoryEntryRef NormalDir = *Dir.getDirRef(); // Search for a module map file in this directory. - if (parseModuleMapFile(NormalDir, IsSystem, + if (parseModuleMapFile(NormalDir, IsSystem, /*ImplicitlyDiscovered=*/true, /*IsFramework*/ false) == MMR_NewlyProcessed) { // We just parsed a module map file; check whether the module can be // loaded now. @@ -374,6 +375,7 @@ Module *HeaderSearch::lookupModule(StringRef ModuleName, StringRef SearchName, NestedModuleMapDirName = Dir.getDirRef()->getName(); llvm::sys::path::append(NestedModuleMapDirName, ModuleName); if (parseModuleMapFile(NestedModuleMapDirName, IsSystem, + /*ImplicitlyDiscovered=*/true, /*IsFramework*/ false) == MMR_NewlyProcessed) { // If we just parsed a module map file, look for the module again. Module = ModMap.findOrLoadModule(ModuleName); @@ -1753,7 +1755,8 @@ bool HeaderSearch::hasModuleMap(StringRef FileName, // Check if it's possible that the module map for this directory can resolve // this header. - parseModuleMapFile(*Dir, IsSystem, IsFramework); + parseModuleMapFile(*Dir, IsSystem, /*ImplicitlyDiscovered=*/true, + IsFramework); auto DirState = DirectoryModuleMap.find(*Dir); if (DirState == DirectoryModuleMap.end() || !DirState->second.ModuleMapFile) continue; @@ -1940,7 +1943,8 @@ bool HeaderSearch::findUsableModuleForFrameworkHeader( // Load this framework module. If that succeeds, find the suggested module // for this header, if any. - loadFrameworkModule(ModuleName, *TopFrameworkDir, IsSystemFramework); + loadFrameworkModule(ModuleName, *TopFrameworkDir, IsSystemFramework, + /*ImplicitlyDiscovered=*/true); // FIXME: This can find a module not part of ModuleName, which is // important so that we're consistent about whether this header @@ -1977,6 +1981,7 @@ static OptionalFileEntryRef getPrivateModuleMap(FileEntryRef File, } bool HeaderSearch::parseAndLoadModuleMapFile(FileEntryRef File, bool IsSystem, + bool ImplicitlyDiscovered, FileID ID, unsigned *Offset, StringRef OriginalModuleMapFile) { // Find the directory for the module. For frameworks, that may require going @@ -2012,7 +2017,8 @@ bool HeaderSearch::parseAndLoadModuleMapFile(FileEntryRef File, bool IsSystem, } assert(Dir && "module map home directory must exist"); - switch (parseAndLoadModuleMapFileImpl(File, IsSystem, *Dir, ID, Offset, + switch (parseAndLoadModuleMapFileImpl(File, IsSystem, ImplicitlyDiscovered, + *Dir, ID, Offset, /*DiagnosePrivMMap=*/true)) { case MMR_AlreadyProcessed: case MMR_NewlyProcessed: @@ -2025,8 +2031,8 @@ bool HeaderSearch::parseAndLoadModuleMapFile(FileEntryRef File, bool IsSystem, } HeaderSearch::ModuleMapResult HeaderSearch::parseAndLoadModuleMapFileImpl( - FileEntryRef File, bool IsSystem, DirectoryEntryRef Dir, FileID ID, - unsigned *Offset, bool DiagnosePrivMMap) { + FileEntryRef File, bool IsSystem, bool ImplicitlyDiscovered, + DirectoryEntryRef Dir, FileID ID, unsigned *Offset, bool DiagnosePrivMMap) { // Check whether we've already loaded this module map, and mark it as being // loaded in case we recursively try to load it from itself. auto AddResult = LoadedModuleMaps.insert(std::make_pair(File, true)); @@ -2034,7 +2040,8 @@ HeaderSearch::ModuleMapResult HeaderSearch::parseAndLoadModuleMapFileImpl( return AddResult.first->second ? MMR_AlreadyProcessed : MMR_InvalidModuleMap; - if (ModMap.parseAndLoadModuleMapFile(File, IsSystem, Dir, ID, Offset)) { + if (ModMap.parseAndLoadModuleMapFile(File, IsSystem, ImplicitlyDiscovered, + Dir, ID, Offset)) { LoadedModuleMaps[File] = false; return MMR_InvalidModuleMap; } @@ -2042,7 +2049,8 @@ HeaderSearch::ModuleMapResult HeaderSearch::parseAndLoadModuleMapFileImpl( // Try to load a corresponding private module map. if (OptionalFileEntryRef PMMFile = getPrivateModuleMap(File, FileMgr, Diags, DiagnosePrivMMap)) { - if (ModMap.parseAndLoadModuleMapFile(*PMMFile, IsSystem, Dir)) { + if (ModMap.parseAndLoadModuleMapFile(*PMMFile, IsSystem, + ImplicitlyDiscovered, Dir)) { LoadedModuleMaps[File] = false; return MMR_InvalidModuleMap; } @@ -2054,6 +2062,7 @@ HeaderSearch::ModuleMapResult HeaderSearch::parseAndLoadModuleMapFileImpl( HeaderSearch::ModuleMapResult HeaderSearch::parseModuleMapFileImpl(FileEntryRef File, bool IsSystem, + bool ImplicitlyDiscovered, DirectoryEntryRef Dir, FileID ID) { // Check whether we've already parsed this module map, and mark it as being // parsed in case we recursively try to parse it from itself. @@ -2062,7 +2071,8 @@ HeaderSearch::parseModuleMapFileImpl(FileEntryRef File, bool IsSystem, return AddResult.first->second ? MMR_AlreadyProcessed : MMR_InvalidModuleMap; - if (ModMap.parseModuleMapFile(File, IsSystem, Dir, ID)) { + if (ModMap.parseModuleMapFile(File, IsSystem, ImplicitlyDiscovered, Dir, + ID)) { ParsedModuleMaps[File] = false; return MMR_InvalidModuleMap; } @@ -2070,7 +2080,8 @@ HeaderSearch::parseModuleMapFileImpl(FileEntryRef File, bool IsSystem, // Try to parse a corresponding private module map. if (OptionalFileEntryRef PMMFile = getPrivateModuleMap(File, FileMgr, Diags, /*Diagnose=*/false)) { - if (ModMap.parseModuleMapFile(*PMMFile, IsSystem, Dir)) { + if (ModMap.parseModuleMapFile(*PMMFile, IsSystem, ImplicitlyDiscovered, + Dir)) { ParsedModuleMaps[File] = false; return MMR_InvalidModuleMap; } @@ -2115,9 +2126,11 @@ HeaderSearch::lookupModuleMapFile(DirectoryEntryRef Dir, bool IsFramework) { } Module *HeaderSearch::loadFrameworkModule(StringRef Name, DirectoryEntryRef Dir, - bool IsSystem) { + bool IsSystem, + bool ImplicitlyDiscovered) { // Try to load a module map file. - switch (parseAndLoadModuleMapFile(Dir, IsSystem, /*IsFramework*/ true)) { + switch (parseAndLoadModuleMapFile(Dir, IsSystem, ImplicitlyDiscovered, + /*IsFramework*/ true)) { case MMR_InvalidModuleMap: // Try to infer a module map from the framework directory. if (HSOpts.ImplicitModuleMaps) @@ -2137,15 +2150,18 @@ Module *HeaderSearch::loadFrameworkModule(StringRef Name, DirectoryEntryRef Dir, HeaderSearch::ModuleMapResult HeaderSearch::parseAndLoadModuleMapFile(StringRef DirName, bool IsSystem, + bool ImplicitlyDiscovered, bool IsFramework) { if (auto Dir = FileMgr.getOptionalDirectoryRef(DirName)) - return parseAndLoadModuleMapFile(*Dir, IsSystem, IsFramework); + return parseAndLoadModuleMapFile(*Dir, IsSystem, ImplicitlyDiscovered, + IsFramework); return MMR_NoDirectory; } HeaderSearch::ModuleMapResult HeaderSearch::parseAndLoadModuleMapFile(DirectoryEntryRef Dir, bool IsSystem, + bool ImplicitlyDiscovered, bool IsFramework) { auto InsertRes = DirectoryModuleMap.insert(std::pair{ Dir, ModuleMapDirectoryState{{}, {}, ModuleMapDirectoryState::Invalid}}); @@ -2169,8 +2185,8 @@ HeaderSearch::parseAndLoadModuleMapFile(DirectoryEntryRef Dir, bool IsSystem, } if (MMState.ModuleMapFile) { - ModuleMapResult Result = - parseAndLoadModuleMapFileImpl(*MMState.ModuleMapFile, IsSystem, Dir); + ModuleMapResult Result = parseAndLoadModuleMapFileImpl( + *MMState.ModuleMapFile, IsSystem, ImplicitlyDiscovered, Dir); // Add Dir explicitly in case ModuleMapFile is in a subdirectory. // E.g. Foo.framework/Modules/module.modulemap // ^Dir ^ModuleMapFile @@ -2185,18 +2201,20 @... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/184279 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
