Author: Michael Spencer Date: 2026-04-09T14:36:42-07:00 New Revision: 67ff7695616f8411abc538c634e14a0aa7141dc3
URL: https://github.com/llvm/llvm-project/commit/67ff7695616f8411abc538c634e14a0aa7141dc3 DIFF: https://github.com/llvm/llvm-project/commit/67ff7695616f8411abc538c634e14a0aa7141dc3.diff LOG: [clang][modules] Add warning for symlinks to modular headers (#188059) Symlinks that are not covered by a module that point to a header owned by a module create situations where if a header is owned by a module depends on which headers were included prior. This adds a diagnostic for such cases when they can be detected, and informs the user to use a textual forwarding header instead. This bypasses Clang's FileManager and VFS as they don't know about symlinks. The diagnostic is worded as "may" because of this. Added: clang/test/Modules/symlink-to-modular-header.c Modified: clang/include/clang/Basic/DiagnosticLexKinds.td clang/include/clang/Lex/HeaderSearch.h clang/lib/Lex/HeaderSearch.cpp clang/lib/Lex/ModuleMap.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Basic/DiagnosticLexKinds.td b/clang/include/clang/Basic/DiagnosticLexKinds.td index 07afc69793499..30efb0d90c124 100644 --- a/clang/include/clang/Basic/DiagnosticLexKinds.td +++ b/clang/include/clang/Basic/DiagnosticLexKinds.td @@ -1005,6 +1005,13 @@ 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; +def warn_mmap_deprecated_symlink_to_modular_header : Warning< + "header '%0' may be a symlink to '%1' owned by module '%2'; " + "symlinks to modular headers are deprecated, replace with a textual " + "forwarding header">, + InGroup<DiagGroup<"mmap-deprecated-symlink-to-modular-header">>, + DefaultIgnore; +def note_mmap_module_defined_here : Note<"module defined here">; // 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 48dd58ceed090..0baf07e95ef83 100644 --- a/clang/include/clang/Lex/HeaderSearch.h +++ b/clang/include/clang/Lex/HeaderSearch.h @@ -410,6 +410,12 @@ class HeaderSearch { StringRef PathPrefix, ModuleMapDirectoryState &MMState); + /// Check if a relative path would be covered by the module map index. + /// Returns the module names that would cover this path. + SmallVector<StringRef, 1> + findMatchingModulesInIndex(StringRef RelativePath, + const ModuleMapDirectoryState &MMState) const; + public: HeaderSearch(const HeaderSearchOptions &HSOpts, SourceManager &SourceMgr, DiagnosticsEngine &Diags, const LangOptions &LangOpts, @@ -844,6 +850,11 @@ class HeaderSearch { /// of the given search directory. void loadSubdirectoryModuleMaps(DirectoryLookup &SearchDir); + /// Diagnose headers that are a symlink and not covered by a module map. + void diagnoseUncoveredSymlink(FileEntryRef File, + ModuleMap::KnownHeader &Module, + const DirectoryEntry *Root); + /// Find and suggest a usable module for the given file. /// /// \return \c true if the file can be used, \c false if we are not permitted to diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp index 3eaad16d8ba04..5cc2c04a68077 100644 --- a/clang/lib/Lex/HeaderSearch.cpp +++ b/clang/lib/Lex/HeaderSearch.cpp @@ -1733,6 +1733,52 @@ void HeaderSearch::processModuleMapForIndex(const modulemap::ModuleMapFile &MMF, } } +/// Compute relative path from DirPath to FileName by stripping the DirPath +/// prefix. DirPath should be derived from FileName (e.g. via parent_path) to +/// ensure consistent path separators. Returns empty if FileName doesn't start +/// with DirPath. +static StringRef computeRelativePath(StringRef FileName, StringRef DirPath) { + if (!FileName.starts_with(DirPath)) + return {}; + StringRef RelativePath = FileName.substr(DirPath.size()); + while (!RelativePath.empty() && + llvm::sys::path::is_separator(RelativePath.front())) + RelativePath = RelativePath.substr(1); + return RelativePath; +} + +SmallVector<StringRef, 1> HeaderSearch::findMatchingModulesInIndex( + StringRef RelativePath, const ModuleMapDirectoryState &MMState) const { + SmallVector<StringRef, 1> Modules; + + // Check for exact matches in cache. + auto CachedMods = MMState.HeaderToModules.find(RelativePath); + if (CachedMods != MMState.HeaderToModules.end()) + Modules.append(CachedMods->second.begin(), CachedMods->second.end()); + + // Check umbrella directories. + for (const auto &UmbrellaDir : MMState.UmbrellaDirModules) { + if (RelativePath.starts_with(UmbrellaDir.first) || UmbrellaDir.first == ".") + Modules.push_back(UmbrellaDir.second); + } + + // Add all modules corresponding to an umbrella header. We don't know which + // other headers these umbrella headers include, so it's possible any one of + // them includes the file. `ModuleMap::findModuleForHeader` will select the + // correct module, accounting for any already known headers from other module + // maps or loaded PCMs. + // + // TODO: Clang should strictly enforce that umbrella headers include the + // other headers in their directory, or that they are referenced in + // the module map. The current behavior can be order of include/import + // dependent. This would allow treating umbrella headers the same as + // umbrella directories here. + Modules.append(MMState.UmbrellaHeaderModules.begin(), + MMState.UmbrellaHeaderModules.end()); + + return Modules; +} + bool HeaderSearch::hasModuleMap(StringRef FileName, const DirectoryEntry *Root, bool IsSystem) { @@ -1764,7 +1810,9 @@ bool HeaderSearch::hasModuleMap(StringRef FileName, if (DirState == DirectoryModuleMap.end() || !DirState->second.ModuleMapFile) continue; - if (!HSOpts.LazyLoadModuleMaps) + if (!HSOpts.LazyLoadModuleMaps && + Diags.isIgnored(diag::warn_mmap_deprecated_symlink_to_modular_header, + SourceLocation())) return true; auto &MMState = DirState->second; @@ -1775,49 +1823,21 @@ bool HeaderSearch::hasModuleMap(StringRef FileName, buildModuleMapIndex(*Dir, MMState); } + // The header cache is needed for the symlink diagnostic. + if (!HSOpts.LazyLoadModuleMaps) + return true; + // Compute relative path from directory to the file. Use DirName (which // we computed via parent_path) rather than Dir->getName() to ensure // consistent path separators. - StringRef RelativePath = FileName.substr(DirName.size()); - // Strip leading separator - while (!RelativePath.empty() && - llvm::sys::path::is_separator(RelativePath.front())) - RelativePath = RelativePath.substr(1); + StringRef RelativePath = computeRelativePath(FileName, DirName); SmallString<128> RelativePathNative(RelativePath); llvm::sys::path::native(RelativePathNative); - RelativePath = RelativePathNative; - - // Check for exact matches in index - llvm::SmallVector<StringRef, 4> ModulesToLoad; - auto CachedMods = MMState.HeaderToModules.find(RelativePath); - if (CachedMods != MMState.HeaderToModules.end()) { - ModulesToLoad.append(CachedMods->second.begin(), - CachedMods->second.end()); - } - // Check umbrella directories - for (const auto &UmbrellaDir : MMState.UmbrellaDirModules) { - if (RelativePath.starts_with(UmbrellaDir.first) || - UmbrellaDir.first == ".") { - ModulesToLoad.push_back(UmbrellaDir.second); - } - } + auto ModulesToLoad = + findMatchingModulesInIndex(RelativePathNative, MMState); - // Add all modules corresponding to an umbrella header. We don't know which - // other headers these umbrella headers include, so it's possible any one of - // them includes `FileName`. `ModuleMap::findModuleForHeader` will select - // the correct module, accounting for any already known headers from other - // module maps or loaded PCMs. - // - // TODO: Clang should strictly enforce that umbrella headers include the - // other headers in their directory, or that they are referenced in - // the module map. The current behavior can be order of include/import - // dependent. This would allow treating umbrella headers the same as - // umbrella directories here. - ModulesToLoad.append(MMState.UmbrellaHeaderModules.begin(), - MMState.UmbrellaHeaderModules.end()); - - // Load all matching modules + // Load all matching modules. bool LoadedAny = false; for (StringRef ModName : ModulesToLoad) { if (ModMap.findOrLoadModule(ModName)) { @@ -1896,6 +1916,77 @@ static bool suggestModule(HeaderSearch &HS, ModuleMap::KnownHeader Module, return true; } +void HeaderSearch::diagnoseUncoveredSymlink(FileEntryRef File, + ModuleMap::KnownHeader &Module, + const DirectoryEntry *Root) { + if (!Module) + return; + + if (!HSOpts.ImplicitModuleMaps || Module.getModule()->isPartOfFramework() || + !Module.getModule()->isModuleMapModule()) + return; + + if (Diags.isIgnored(diag::warn_mmap_deprecated_symlink_to_modular_header, + Module.getModule()->DefinitionLoc)) + return; + + if (File.isDeviceFile() || File.isNamedPipe()) + return; + + llvm::SmallString<128> AbsPath(File.getName()); + FileMgr.makeAbsolutePath(AbsPath); + llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true); + + // NOTE: This path may be redirected, LLVM's VFS does not model symlinks, so + // it's possible this fails. The diagnostic is worded as such. + llvm::SmallString<128> LinkTarget; + if (llvm::sys::fs::readlink(AbsPath, LinkTarget)) + return; + + // We know this file is a symlink and resolved to a module. Check that there's + // a module map that would be discoverable that covers this header. This uses + // VFS paths as that's how module map search works. + StringRef FileName = File.getNameAsRequested(); + StringRef DirName = FileName; + const DirectoryEntry *CurDir = nullptr; + + // Walk up the directory tree looking for a module map that covers this path. + do { + DirName = llvm::sys::path::parent_path(DirName); + if (DirName.empty()) + break; + + auto Dir = FileMgr.getOptionalDirectoryRef(DirName); + if (!Dir) + break; + CurDir = *Dir; + + auto DirState = DirectoryModuleMap.find(*Dir); + if (DirState == DirectoryModuleMap.end() || !DirState->second.ModuleMapFile) + continue; + + // Use DirName (from parent_path) rather than Dir->getName() to ensure + // consistent path separators. + StringRef RelativePath = computeRelativePath(FileName, DirName); + if (RelativePath.empty()) + continue; + SmallString<128> RelativePathNative(RelativePath); + llvm::sys::path::native(RelativePathNative); + + auto MatchingModules = + findMatchingModulesInIndex(RelativePathNative, DirState->second); + if (!MatchingModules.empty()) + return; // Symlink path is covered, no diagnostic needed. + } while (CurDir != Root); + + // The symlink path is not covered by any module map. + Diags.Report(diag::warn_mmap_deprecated_symlink_to_modular_header) + << File.getName() << LinkTarget + << Module.getModule()->getFullModuleName(); + Diags.Report(Module.getModule()->DefinitionLoc, + diag::note_mmap_module_defined_here); +} + bool HeaderSearch::findUsableModuleForHeader( FileEntryRef File, const DirectoryEntry *Root, Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule, bool IsSystemHeaderDir) { @@ -1908,6 +1999,7 @@ bool HeaderSearch::findUsableModuleForHeader( hasModuleMap(File.getNameAsRequested(), Root, IsSystemHeaderDir); ModuleMap::KnownHeader Module = findModuleForHeader(File, /*AllowTextual=*/true); + diagnoseUncoveredSymlink(File, Module, Root); return suggestModule(*this, Module, File, RequestingModule, SuggestedModule); } @@ -1923,7 +2015,7 @@ bool HeaderSearch::findUsableModuleForHeader( // map data from PCMs. Module = ModMap.findModuleForHeader(File, /*AllowTextual=*/true); } - + diagnoseUncoveredSymlink(File, Module, Root); return suggestModule(*this, Module, File, RequestingModule, SuggestedModule); } diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp index c9c90d41ee55f..8a8b2ec4a775a 100644 --- a/clang/lib/Lex/ModuleMap.cpp +++ b/clang/lib/Lex/ModuleMap.cpp @@ -2346,6 +2346,17 @@ bool ModuleMap::parseAndLoadModuleMapFile(FileEntryRef File, bool IsSystem, ModuleMapLoader Loader(SourceMgr, Diags, *this, ID, Dir, IsSystem, ImplicitlyDiscovered); Result = Loader.parseAndLoadModuleMapFile(*MMF); + + // Also record that this was parsed if it wasn't previously. This is used + // for diagnostics. + llvm::DenseMap<const FileEntry *, + const modulemap::ModuleMapFile *>::iterator PKnown = + ParsedModuleMap.find(File); + if (PKnown == ParsedModuleMap.end()) { + ParsedModuleMaps.push_back( + std::make_unique<modulemap::ModuleMapFile>(std::move(*MMF))); + ParsedModuleMap[File] = &*ParsedModuleMaps.back(); + } } LoadedModuleMap[File] = Result; diff --git a/clang/test/Modules/symlink-to-modular-header.c b/clang/test/Modules/symlink-to-modular-header.c new file mode 100644 index 0000000000000..79e3ea3a0f15a --- /dev/null +++ b/clang/test/Modules/symlink-to-modular-header.c @@ -0,0 +1,98 @@ +// REQUIRES: symlinks + +// Test that we warn when including a symlink to a modular header that isn't +// covered by a module map at the symlink's location. + +// RUN: rm -rf %t +// RUN: split-file %s %t + +// Create the symlink directories and symlinks. +// RUN: mkdir -p %t/covered %t/uncovered +// RUN: ln -s %t/ModuleA/A.h %t/covered/A.h +// RUN: ln -s %t/ModuleA/A.h %t/uncovered/A.h + +// Including the symlink that IS covered should not warn. +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -I%t/covered -I%t/ModuleA \ +// RUN: -fmodules-cache-path=%t/cache -fsyntax-only %t/tu-covered.m \ +// RUN: -Wmmap-deprecated-symlink-to-modular-header -verify + +// Including the symlink that is NOT covered should warn. +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -I%t/uncovered -I%t/ModuleA \ +// RUN: -fmodules-cache-path=%t/cache -fsyntax-only %t/tu-uncovered.m \ +// RUN: -Wmmap-deprecated-symlink-to-modular-header -verify + +// Same test with lazy module map loading. +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -I%t/uncovered -I%t/ModuleA \ +// RUN: -fmodules-cache-path=%t/cache2 -fmodules-lazy-load-module-maps \ +// RUN: -fsyntax-only %t/tu-uncovered.m \ +// RUN: -Wmmap-deprecated-symlink-to-modular-header -verify + +// Test the diagnostic when the module is loaded from a PCM. Import B which +// transitively builds A into a PCM, then include A.h via the uncovered +// symlink so A is loaded from the PCM. +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -I%t \ +// RUN: -fmodules-cache-path=%t/cache3 -fsyntax-only %t/tu-uncovered-pcm.m \ +// RUN: -Wmmap-deprecated-symlink-to-modular-header \ +// RUN: -fno-modules-check-relocated -Rmodule-map -verify + +//--- ModuleA/module.modulemap +module A { + header "A.h" +} + +//--- ModuleA/A.h + +//--- ModuleB/module.modulemap +module B { + header "B.h" + export * +} + +//--- ModuleB/B.h +#include "ModuleA/A.h" + +//--- covered/module.modulemap +module SymlinkA { + header "A.h" +} + +//--- tu-covered.m +// expected-no-diagnostics +#include "A.h" + +//--- tu-uncovered.m +#pragma clang __debug module_lookup A +#include "A.h" + +// expected-warning-re@* {{may be a symlink to '{{.*}}' owned by module 'A'}} +// expected-note@ModuleA/module.modulemap:1 {{module defined here}} + +//--- tu-uncovered-pcm.m +#include "ModuleB/B.h" +#include "uncovered/A.h" + +// expected-warning-re@* {{may be a symlink to '{{.*}}' owned by module 'A'}} +// expected-note@ModuleA/module.modulemap:1 {{module defined here}} +// expected-remark-re@* {{loading modulemap '{{.*}}ModuleB/module.modulemap'}} +// expected-remark-re@* {{loading modulemap '{{.*}}module.modulemap'}} +// expected-remark-re@* {{loading modulemap '{{.*}}pthread.modulemap'}} + +// RUN: ln -s pthread/pthread.h %t/pthread.h +// RUN: %clang_cc1 -fmodules-cache-path=%t/modules -fmodules -fimplicit-module-maps -I %t %t/tu.c -fsyntax-only \ +// RUN: -Wmmap-deprecated-symlink-to-modular-header -verify + +//--- module.modulemap +extern module pthread "pthread.modulemap" + +//--- pthread.modulemap +module pthread { header "pthread/pthread.h" } + +//--- pthread/pthread.h +typedef int p; + +//--- tu.c +#include <pthread.h> +p P = 4; + +// expected-warning-re@* {{may be a symlink to '{{.*}}' owned by module 'pthread'}} +// [email protected]:1 {{module defined here}} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
