llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Michael Spencer (Bigcheese) <details> <summary>Changes</summary> Add -Wduplicate-header-ownership, an off by default warning that fires at include time when a header is owned by multiple top-level modules. This helps catch overlapping module maps that can cause confusing module resolution. Assisted-by: claude-opus-4.6 --- Full diff: https://github.com/llvm/llvm-project/pull/188538.diff 5 Files Affected: - (modified) clang/include/clang/Basic/DiagnosticLexKinds.td (+7) - (modified) clang/include/clang/Basic/Module.h (+3) - (modified) clang/include/clang/Lex/ModuleMap.h (+23-3) - (modified) clang/lib/Lex/ModuleMap.cpp (+106-12) - (added) clang/test/Modules/duplicate-header-ownership.c (+138) ``````````diff diff --git a/clang/include/clang/Basic/DiagnosticLexKinds.td b/clang/include/clang/Basic/DiagnosticLexKinds.td index 5eceeced311f2..291d6391dfb1f 100644 --- a/clang/include/clang/Basic/DiagnosticLexKinds.td +++ b/clang/include/clang/Basic/DiagnosticLexKinds.td @@ -875,6 +875,13 @@ def err_mmap_module_redefinition : Error< def note_mmap_prev_definition : Note<"previously defined here">; def err_mmap_umbrella_clash : Error< "umbrella for module '%0' already covers this directory">; +def warn_mmap_duplicate_header_ownership : Warning< + "header '%0' is owned by multiple modules">, + InGroup<DiagGroup<"duplicate-header-ownership">>, DefaultIgnore; +def note_mmap_header_owned_by : Note< + "header owned by module '%0' here">; +def note_mmap_header_covered_by_umbrella : Note< + "header covered by umbrella for module '%0' here">; def err_mmap_module_id : Error< "expected a module name or '*'">; def err_mmap_expected_library_name : Error< diff --git a/clang/include/clang/Basic/Module.h b/clang/include/clang/Basic/Module.h index 016cc2ce684b8..94eb877d0a4e9 100644 --- a/clang/include/clang/Basic/Module.h +++ b/clang/include/clang/Basic/Module.h @@ -311,6 +311,9 @@ class alignas(8) Module { /// The umbrella header or directory. std::variant<std::monostate, FileEntryRef, DirectoryEntryRef> Umbrella; + /// The location of the umbrella header or directory declaration. + SourceLocation UmbrellaDeclLoc; + /// The module signature. ASTFileSignature Signature; diff --git a/clang/include/clang/Lex/ModuleMap.h b/clang/include/clang/Lex/ModuleMap.h index 570a68c37fac4..46655c3aa6565 100644 --- a/clang/include/clang/Lex/ModuleMap.h +++ b/clang/include/clang/Lex/ModuleMap.h @@ -219,6 +219,18 @@ class ModuleMap { /// header. llvm::DenseMap<const DirectoryEntry *, Module *> UmbrellaDirs; + /// Mapping from (header, (sub)module) pairs to the source location where + /// the header was added to the module (the header directive location). + /// TODO: Consider moving this into Module::Header and serializing it into + /// PCMs so that locations are available for headers deserialized from + /// modules. Need to evaluate size/perf overhead of adding a SourceLocation + /// to the serialization format for this diagnostic. + llvm::DenseMap<std::pair<const FileEntry *, const Module *>, SourceLocation> + HeaderOwnerLocs; + + /// Headers for which we've already diagnosed duplicate ownership. + llvm::DenseSet<const FileEntry *> DiagnosedDuplicateHeaders; + /// A generation counter that is used to test whether modules of the /// same name may shadow or are illegal redefinitions. /// @@ -355,6 +367,11 @@ class ModuleMap { /// associated with a specific module (e.g. in /usr/include). HeadersMap::iterator findKnownHeader(FileEntryRef File); + /// Warn if a header is owned by multiple top-level modules. + void diagnoseDuplicateHeaderOwnership(SourceLocation FilenameLoc, + StringRef Filename, FileEntryRef File, + HeadersMap::iterator Known); + /// Searches for a module whose umbrella directory contains \p File. /// /// \param File The header to search for. @@ -712,17 +729,20 @@ class ModuleMap { void setUmbrellaHeaderAsWritten(Module *Mod, FileEntryRef UmbrellaHeader, const Twine &NameAsWritten, - const Twine &PathRelativeToRootModuleDirectory); + const Twine &PathRelativeToRootModuleDirectory, + SourceLocation Loc = SourceLocation()); /// Sets the umbrella directory of the given module to the given directory. void setUmbrellaDirAsWritten(Module *Mod, DirectoryEntryRef UmbrellaDir, const Twine &NameAsWritten, - const Twine &PathRelativeToRootModuleDirectory); + const Twine &PathRelativeToRootModuleDirectory, + SourceLocation Loc = SourceLocation()); /// Adds this header to the given module. /// \param Role The role of the header wrt the module. void addHeader(Module *Mod, Module::Header Header, - ModuleHeaderRole Role, bool Imported = false); + ModuleHeaderRole Role, bool Imported = false, + SourceLocation Loc = SourceLocation()); /// Parse a module map without creating `clang::Module` instances. bool parseModuleMapFile(FileEntryRef File, bool IsSystem, diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp index 6c991430cb08b..a8016cf8afa1d 100644 --- a/clang/lib/Lex/ModuleMap.cpp +++ b/clang/lib/Lex/ModuleMap.cpp @@ -301,11 +301,13 @@ void ModuleMap::resolveHeader(Module *Mod, else // Record this umbrella header. setUmbrellaHeaderAsWritten(Mod, *File, Header.FileName, - RelativePathName.str()); + RelativePathName.str(), + Header.FileNameLoc); } else { Module::Header H = {Header.FileName, std::string(RelativePathName), *File}; - addHeader(Mod, H, headerKindToRole(Header.Kind)); + addHeader(Mod, H, headerKindToRole(Header.Kind), /*Imported=*/false, + Header.FileNameLoc); } } else if (Header.HasBuiltinHeader && !Header.Size && !Header.ModTime) { // There's a builtin header but no corresponding on-disk header. Assume @@ -486,21 +488,24 @@ void ModuleMap::diagnoseHeaderInclusion(Module *RequestingModule, bool RequestingModuleIsModuleInterface, SourceLocation FilenameLoc, StringRef Filename, FileEntryRef File) { - // No errors for indirect modules. This may be a bit of a problem for modules - // with no source files. - if (getTopLevelOrNull(RequestingModule) != getTopLevelOrNull(SourceModule)) - return; - if (RequestingModule) { resolveUses(RequestingModule, /*Complain=*/false); resolveHeaderDirectives(RequestingModule, /*File=*/std::nullopt); } + HeadersMap::iterator Known = findKnownHeader(File); + + diagnoseDuplicateHeaderOwnership(FilenameLoc, Filename, File, Known); + + // No errors for indirect modules. This may be a bit of a problem for modules + // with no source files. + if (getTopLevelOrNull(RequestingModule) != getTopLevelOrNull(SourceModule)) + return; + bool Excluded = false; Module *Private = nullptr; Module *NotUsed = nullptr; - HeadersMap::iterator Known = findKnownHeader(File); if (Known != Headers.end()) { for (const KnownHeader &Header : Known->second) { // Excluded headers don't really belong to a module. @@ -564,6 +569,86 @@ void ModuleMap::diagnoseHeaderInclusion(Module *RequestingModule, } } +void ModuleMap::diagnoseDuplicateHeaderOwnership(SourceLocation FilenameLoc, + StringRef Filename, + FileEntryRef File, + HeadersMap::iterator Known) { + if (Known == Headers.end()) + return; + + if (Diags.isIgnored(diag::warn_mmap_duplicate_header_ownership, + FilenameLoc)) + return; + + // Only diagnose each header once. + if (!DiagnosedDuplicateHeaders.insert(&File.getFileEntry()).second) + return; + + struct OwnerInfo { + Module *Mod; + SourceLocation Loc; + bool IsUmbrella; + }; + + // Collect distinct top-level modules that explicitly own this header with + // a modular (non-textual, non-excluded) role. + SmallVector<OwnerInfo, 2> OwningModules; + llvm::SmallPtrSet<Module *, 2> SeenTopLevel; + for (const KnownHeader &H : Known->second) { + if (!isModular(H.getRole())) + continue; + Module *TopLevel = H.getModule()->getTopLevelModule(); + if (!SeenTopLevel.insert(TopLevel).second) + continue; + auto It = HeaderOwnerLocs.find({&File.getFileEntry(), H.getModule()}); + SourceLocation OwnerLoc = + It != HeaderOwnerLocs.end() ? It->second : SourceLocation(); + OwningModules.push_back({TopLevel, OwnerLoc, /*IsUmbrella=*/false}); + } + + // Need at least one explicit owner for there to be a conflict, since + // umbrella coverage can only add one more. + if (OwningModules.empty()) + return; + + // Also check umbrella directory coverage for additional owners from different + // top-level modules — but only if the header isn't excluded from the umbrella + // module. Explicit headers take precedence over umbrella dirs in module + // resolution, but a header owned by one module that another module's umbrella + // covers can still create problems. + SmallVector<DirectoryEntryRef, 2> IntermediateDirs; + if (KnownHeader UmbrellaOwner = + findHeaderInUmbrellaDirs(File, IntermediateDirs)) { + Module *TopLevel = UmbrellaOwner.getModule()->getTopLevelModule(); + // Only add if it's a different top-level module and the header isn't + // excluded from the umbrella module. + if (SeenTopLevel.insert(TopLevel).second) { + // Check that the header isn't excluded in the umbrella module. + bool IsExcluded = llvm::any_of(Known->second, [TopLevel](const KnownHeader &H) { + return H.getModule()->getTopLevelModule() == TopLevel && + H.getRole() == ExcludedHeader; + }); + if (!IsExcluded) { + OwningModules.push_back( + {TopLevel, UmbrellaOwner.getModule()->UmbrellaDeclLoc, + /*IsUmbrella=*/true}); + } + } + } + + if (OwningModules.size() < 2) + return; + + Diags.Report(FilenameLoc, diag::warn_mmap_duplicate_header_ownership) + << Filename; + for (const auto &Owner : OwningModules) { + unsigned NoteID = Owner.IsUmbrella + ? diag::note_mmap_header_covered_by_umbrella + : diag::note_mmap_header_owned_by; + Diags.Report(Owner.Loc, NoteID) << Owner.Mod->getFullModuleName(); + } +} + static bool isBetterKnownHeader(const ModuleMap::KnownHeader &New, const ModuleMap::KnownHeader &Old) { // Prefer available modules. @@ -1212,9 +1297,12 @@ Module *ModuleMap::createShadowedModule(StringRef Name, bool IsFramework, void ModuleMap::setUmbrellaHeaderAsWritten( Module *Mod, FileEntryRef UmbrellaHeader, const Twine &NameAsWritten, - const Twine &PathRelativeToRootModuleDirectory) { + const Twine &PathRelativeToRootModuleDirectory, SourceLocation Loc) { Headers[UmbrellaHeader].push_back(KnownHeader(Mod, NormalHeader)); + if (Loc.isValid()) + HeaderOwnerLocs[{&UmbrellaHeader.getFileEntry(), Mod}] = Loc; Mod->Umbrella = UmbrellaHeader; + Mod->UmbrellaDeclLoc = Loc; Mod->UmbrellaAsWritten = NameAsWritten.str(); Mod->UmbrellaRelativeToRootModuleDirectory = PathRelativeToRootModuleDirectory.str(); @@ -1227,8 +1315,9 @@ void ModuleMap::setUmbrellaHeaderAsWritten( void ModuleMap::setUmbrellaDirAsWritten( Module *Mod, DirectoryEntryRef UmbrellaDir, const Twine &NameAsWritten, - const Twine &PathRelativeToRootModuleDirectory) { + const Twine &PathRelativeToRootModuleDirectory, SourceLocation Loc) { Mod->Umbrella = UmbrellaDir; + Mod->UmbrellaDeclLoc = Loc; Mod->UmbrellaAsWritten = NameAsWritten.str(); Mod->UmbrellaRelativeToRootModuleDirectory = PathRelativeToRootModuleDirectory.str(); @@ -1307,7 +1396,8 @@ void ModuleMap::resolveHeaderDirectives( } void ModuleMap::addHeader(Module *Mod, Module::Header Header, - ModuleHeaderRole Role, bool Imported) { + ModuleHeaderRole Role, bool Imported, + SourceLocation Loc) { KnownHeader KH(Mod, Role); FileEntryRef HeaderEntry = Header.Entry; @@ -1319,6 +1409,9 @@ void ModuleMap::addHeader(Module *Mod, Module::Header Header, if (llvm::is_contained(HeaderList, KH)) return; + if (Loc.isValid()) + HeaderOwnerLocs[{&HeaderEntry.getFileEntry(), Mod}] = Loc; + HeaderList.push_back(KH); Mod->addHeader(headerRoleToKind(Role), std::move(Header)); @@ -2092,7 +2185,8 @@ void ModuleMapLoader::handleUmbrellaDirDecl( } // Record this umbrella directory. - Map.setUmbrellaDirAsWritten(ActiveModule, *Dir, DirNameAsWritten, DirName); + Map.setUmbrellaDirAsWritten(ActiveModule, *Dir, DirNameAsWritten, DirName, + UDD.Location); } void ModuleMapLoader::handleExportDecl(const modulemap::ExportDecl &ED) { diff --git a/clang/test/Modules/duplicate-header-ownership.c b/clang/test/Modules/duplicate-header-ownership.c new file mode 100644 index 0000000000000..26bad14f4bc44 --- /dev/null +++ b/clang/test/Modules/duplicate-header-ownership.c @@ -0,0 +1,138 @@ +// RUN: rm -rf %t +// RUN: split-file %s %t + +// Test that we warn when two modules own the same header (at include time). +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -I%t/headers \ +// RUN: -fmodules-cache-path=%t/cache %t/tu.c -fsyntax-only \ +// RUN: -Wduplicate-header-ownership -verify + +// Test that the warning is off by default. +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -I%t/headers \ +// RUN: -fmodules-cache-path=%t/cache2 %t/tu-no-warn.c -fsyntax-only -verify + +// Test that no warning fires if the conflicting header is not included. +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -I%t/headers \ +// RUN: -fmodules-cache-path=%t/cache3 %t/tu-no-include.c -fsyntax-only \ +// RUN: -Wduplicate-header-ownership -verify + +// Test umbrella header in different module conflicts with explicit header. +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -I%t/umbrella \ +// RUN: -fmodules-cache-path=%t/cache4 %t/tu-umbrella.c -fsyntax-only \ +// RUN: -Wduplicate-header-ownership -verify + +// Test umbrella dir in different module conflicts with explicit header. +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -I%t/umbrella-dir \ +// RUN: -fmodules-cache-path=%t/cache5 %t/tu-umbrella-dir.c -fsyntax-only \ +// RUN: -Wduplicate-header-ownership -verify + +// Test that explicit header + umbrella in the SAME module is fine. +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -I%t/same-module \ +// RUN: -fmodules-cache-path=%t/cache6 %t/tu-same-module.c -fsyntax-only \ +// RUN: -Wduplicate-header-ownership -verify + +// Test that excluded header under another module's umbrella is fine. +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -I%t/excluded \ +// RUN: -fmodules-cache-path=%t/cache7 %t/tu-excluded.c -fsyntax-only \ +// RUN: -Wduplicate-header-ownership -verify + +//--- headers/a.h +void a(void); + +//--- headers/b.h +void b(void); + +//--- headers/module.modulemap +module A { + header "a.h" +} + +module B { + header "a.h" +} + +module C { + header "b.h" +} + +//--- tu.c +#include "a.h" +// expected-warning@-1 {{header 'a.h' is owned by multiple modules}} +// expected-note@* {{header owned by module 'A' here}} +// expected-note@* {{header owned by module 'B' here}} + +//--- tu-no-warn.c +// expected-no-diagnostics +#include "a.h" + +//--- tu-no-include.c +// expected-no-diagnostics +#include "b.h" + +//--- umbrella/umbrella.h +#include "d.h" + +//--- umbrella/d.h +void d(void); + +//--- umbrella/module.modulemap +module D { + umbrella header "umbrella.h" +} + +module E { + header "d.h" +} + +//--- tu-umbrella.c +#include "d.h" +// expected-warning@-1 {{header 'd.h' is owned by multiple modules}} +// expected-note@* {{header owned by module 'E' here}} +// expected-note@* {{header covered by umbrella for module 'D' here}} + +//--- umbrella-dir/sub/e.h +void e(void); + +//--- umbrella-dir/module.modulemap +module F { + umbrella "sub" +} + +module G { + header "sub/e.h" +} + +//--- tu-umbrella-dir.c +#include "sub/e.h" +// expected-warning@-1 {{header 'sub/e.h' is owned by multiple modules}} +// expected-note@* {{header owned by module 'G' here}} +// expected-note@* {{header covered by umbrella for module 'F' here}} + +//--- same-module/sub/f.h +void f(void); + +//--- same-module/module.modulemap +module H { + umbrella "sub" + header "sub/f.h" +} + +//--- tu-same-module.c +// expected-no-diagnostics +#include "sub/f.h" + +//--- excluded/sub/g.h +void g(void); + +//--- excluded/module.modulemap +module I { + umbrella "sub" + exclude header "sub/g.h" +} + +module J { + header "sub/g.h" +} + +//--- tu-excluded.c +// expected-no-diagnostics +#include "sub/g.h" `````````` </details> https://github.com/llvm/llvm-project/pull/188538 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
