jansvoboda11 created this revision.
jansvoboda11 added a reviewer: ahoppen.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
When looking for module, `HeaderSearch::lookupModule` iterates through search
paths, parses modulemaps, eagerly creates `Module` instances and caches them in
`ModuleMap`. When first called, it will correctly note usage of the search path
that discovered the returned module. On subsequent calls, however, it can pull
previously created `Module` from `ModuleMap` without noting the search path was
used.
This patch fixes that. This requires `HeaderSearch` to be able to go from
`Module` to the index of corresponding search path. This is achieved by adding
a callback to the modulemap parser, intercepting creations of `Module`
instances and pairing them with the current search path index.
The outlined solution doesn't handle all corner cases, though. It's possible
for clients to use `HeaderSearch` to lookup module `B`, which could deserialize
unrelated modulemap and create instance of module `A`. The client can then look
up module `A` directly in `ModuleMap`, avoiding the logic in `HeaderSearch`
that makes `-Rsearch-path-usage` work.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D113676
Files:
clang/include/clang/Lex/HeaderSearch.h
clang/include/clang/Lex/ModuleMap.h
clang/lib/Lex/HeaderSearch.cpp
clang/lib/Lex/ModuleMap.cpp
clang/lib/Serialization/ASTReader.cpp
clang/test/Preprocessor/search-path-usage-modules.m
clang/test/Preprocessor/search-path-usage.m
Index: clang/test/Preprocessor/search-path-usage.m
===================================================================
--- clang/test/Preprocessor/search-path-usage.m
+++ clang/test/Preprocessor/search-path-usage.m
@@ -128,19 +128,3 @@
// expected-remark-re {{search path used: '{{.*}}/b-missing.hmap'}}
#endif
#endif
-
-// Check that search paths with module maps are reported.
-//
-// RUN: mkdir %t/modulemap_abs
-// RUN: sed "s|DIR|%/S/Inputs/search-path-usage|g" \
-// RUN: %S/Inputs/search-path-usage/modulemap_abs/module.modulemap.template \
-// RUN: > %t/modulemap_abs/module.modulemap
-// RUN: %clang_cc1 -Eonly %s -Rsearch-path-usage \
-// RUN: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules \
-// RUN: -I %t/modulemap_abs \
-// RUN: -I %S/Inputs/search-path-usage/a \
-// RUN: -DMODMAP_ABS -verify
-#ifdef MODMAP_ABS
-@import b; // \
-// expected-remark-re {{search path used: '{{.*}}/modulemap_abs'}}
-#endif
Index: clang/test/Preprocessor/search-path-usage-modules.m
===================================================================
--- /dev/null
+++ clang/test/Preprocessor/search-path-usage-modules.m
@@ -0,0 +1,37 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -Eonly %t/test-both.m -I %t/sp1 -I %t/sp2 -Rsearch-path-usage \
+// RUN: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache 2>&1 | FileCheck %t/test-both.m
+// RUN: %clang_cc1 -Eonly %t/test-first.m -I %t/sp2 -I %t/sp1 -Rsearch-path-usage \
+// RUN: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache 2>&1 | FileCheck %t/test-first.m
+// RUN: %clang_cc1 -Eonly %t/test-second.m -I %t/sp2 -I %t/sp1 -Rsearch-path-usage \
+// RUN: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache 2>&1 | FileCheck %t/test-second.m
+
+//--- sp1/module.modulemap
+module mod1 { header "mod1.h" }
+//--- sp1/mod1.h
+int module1();
+
+//--- sp2/module.modulemap
+module mod2 { header "mod2.h" }
+//--- sp2/mod2.h
+int module2();
+
+//--- test-both.m
+@import mod1;
+@import mod2;
+// CHECK: search path used: '{{.*}}/sp1'
+// CHECK: search path used: '{{.*}}/sp2'
+
+//--- test-first.m
+@import mod1;
+// CHECK-NOT: search path used: '{{.*}}/sp2'
+// CHECK: search path used: '{{.*}}/sp1'
+// CHECK-NOT: search path used: '{{.*}}/sp2'
+
+//--- test-second.m
+@import mod2;
+// CHECK-NOT: search path used: '{{.*}}/sp1'
+// CHECK: search path used: '{{.*}}/sp2'
+// CHECK-NOT: search path used: '{{.*}}/sp1'
Index: clang/lib/Serialization/ASTReader.cpp
===================================================================
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -3908,7 +3908,7 @@
// An implicitly-loaded module file should have its module listed in some
// module map file that we've already loaded.
Module *M =
- PP.getHeaderSearchInfo().lookupModule(F.ModuleName, F.ImportLoc);
+ PP.getHeaderSearchInfo().lookupModule(F.ModuleName, SourceLocation());
auto &Map = PP.getHeaderSearchInfo().getModuleMap();
const FileEntry *ModMap = M ? Map.getModuleMapFileForUniquing(M) : nullptr;
// Don't emit module relocation error if we have -fno-validate-pch
Index: clang/lib/Lex/ModuleMap.cpp
===================================================================
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -824,6 +824,8 @@
// Create a new module with this name.
Module *Result = new Module(Name, SourceLocation(), Parent, IsFramework,
IsExplicit, NumCreatedModules++);
+ for (const auto& Callback : Callbacks)
+ Callback->moduleMapModuleCreated(Result);
if (!Parent) {
if (LangOpts.CurrentModule == Name)
SourceModule = Result;
@@ -1023,6 +1025,8 @@
Module *Result = new Module(ModuleName, SourceLocation(), Parent,
/*IsFramework=*/true, /*IsExplicit=*/false,
NumCreatedModules++);
+ for (const auto& Callback : Callbacks)
+ Callback->moduleMapModuleCreated(Result);
InferredModuleAllowedBy[Result] = ModuleMapFile;
Result->IsInferred = true;
if (!Parent) {
@@ -1117,6 +1121,8 @@
/*IsExplicit=*/false, NumCreatedModules++);
Result->ShadowingModule = ShadowingModule;
Result->markUnavailable(/*Unimportable*/true);
+ for (const auto& Callback : Callbacks)
+ Callback->moduleMapModuleCreated(Result);
ModuleScopeIDs[Result] = CurrentModuleScopeID;
ShadowModules.push_back(Result);
Index: clang/lib/Lex/HeaderSearch.cpp
===================================================================
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -36,6 +36,7 @@
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/SaveAndRestore.h"
#include <algorithm>
#include <cassert>
#include <cstddef>
@@ -83,8 +84,19 @@
const LangOptions &LangOpts,
const TargetInfo *Target)
: HSOpts(std::move(HSOpts)), Diags(Diags),
- FileMgr(SourceMgr.getFileManager()), FrameworkMap(64),
- ModMap(SourceMgr, Diags, LangOpts, Target, *this) {}
+ FileMgr(SourceMgr.getFileManager()), CurrentSearchPathIdx(~0U),
+ FrameworkMap(64), ModMap(SourceMgr, Diags, LangOpts, Target, *this) {
+ struct MMCallback : ModuleMapCallbacks {
+ HeaderSearch &HS;
+ MMCallback(HeaderSearch &HS) : HS(HS) {}
+ void moduleMapModuleCreated(Module *M) override {
+ // Module map parsing initiated by header search.
+ if (HS.CurrentSearchPathIdx != ~0U)
+ HS.ModuleToSearchDirIdx[M] = HS.CurrentSearchPathIdx;
+ }
+ };
+ ModMap.addModuleMapCallbacks(std::make_unique<MMCallback>(*this));
+}
void HeaderSearch::PrintStats() {
llvm::errs() << "\n*** HeaderSearch Stats:\n"
@@ -248,8 +260,10 @@
bool AllowExtraModuleMapSearch) {
// Look in the module map to determine if there is a module by this name.
Module *Module = ModMap.findModule(ModuleName);
- if (Module || !AllowSearch || !HSOpts->ImplicitModuleMaps)
+ if (Module || !AllowSearch || !HSOpts->ImplicitModuleMaps) {
+ noteModuleLookupUsage(Module, ImportLoc);
return Module;
+ }
StringRef SearchName = ModuleName;
Module = lookupModule(ModuleName, SearchName, ImportLoc,
@@ -276,11 +290,12 @@
SourceLocation ImportLoc,
bool AllowExtraModuleMapSearch) {
Module *Module = nullptr;
- unsigned Idx;
// Look through the various header search paths to load any available module
// maps, searching for a module map that describes this module.
- for (Idx = 0; Idx != SearchDirs.size(); ++Idx) {
+ for (unsigned Idx = 0; Idx != SearchDirs.size(); ++Idx) {
+ llvm::SaveAndRestore<unsigned> X(CurrentSearchPathIdx, Idx);
+
if (SearchDirs[Idx].isFramework()) {
// Search for or infer a module map for a framework. Here we use
// SearchName rather than ModuleName, to permit finding private modules
@@ -344,7 +359,7 @@
}
if (Module)
- noteLookupUsage(Idx, ImportLoc);
+ noteModuleLookupUsage(Module, ImportLoc);
return Module;
}
@@ -681,11 +696,18 @@
noteLookupUsage(HitIdx, Loc);
}
+void HeaderSearch::noteModuleLookupUsage(Module *M, SourceLocation Loc) {
+ auto It = ModuleToSearchDirIdx.find(M);
+ if (It != ModuleToSearchDirIdx.end())
+ noteLookupUsage(It->second, Loc);
+}
+
void HeaderSearch::noteLookupUsage(unsigned HitIdx, SourceLocation Loc) {
SearchDirsUsage[HitIdx] = true;
auto UserEntryIdxIt = SearchDirToHSEntry.find(HitIdx);
- if (UserEntryIdxIt != SearchDirToHSEntry.end())
+ // TODO: Avoid emitting duplicates.
+ if (UserEntryIdxIt != SearchDirToHSEntry.end() && Loc.isValid())
Diags.Report(Loc, diag::remark_pp_search_path_usage)
<< HSOpts->UserEntries[UserEntryIdxIt->second].Path;
}
@@ -1732,6 +1754,8 @@
if (HSOpts->ImplicitModuleMaps) {
// Load module maps for each of the header search directories.
for (unsigned Idx = 0, N = SearchDirs.size(); Idx != N; ++Idx) {
+ llvm::SaveAndRestore<unsigned> X(CurrentSearchPathIdx, Idx);
+
bool IsSystem = SearchDirs[Idx].isSystemHeaderDirectory();
if (SearchDirs[Idx].isFramework()) {
std::error_code EC;
@@ -1787,6 +1811,8 @@
// Load module maps for each of the header search directories.
for (unsigned Idx = 0, N = SearchDirs.size(); Idx != N; ++Idx) {
+ llvm::SaveAndRestore<unsigned> X(CurrentSearchPathIdx, Idx);
+
// We only care about normal header directories.
if (!SearchDirs[Idx].isNormalDir()) {
continue;
Index: clang/include/clang/Lex/ModuleMap.h
===================================================================
--- clang/include/clang/Lex/ModuleMap.h
+++ clang/include/clang/Lex/ModuleMap.h
@@ -70,6 +70,11 @@
/// \param Header The umbrella header to collect.
virtual void moduleMapAddUmbrellaHeader(FileManager *FileMgr,
const FileEntry *Header) {}
+
+ /// Called when new module is created.
+ ///
+ /// \param M The newly created module.
+ virtual void moduleMapModuleCreated(Module *M) {}
};
class ModuleMap {
Index: clang/include/clang/Lex/HeaderSearch.h
===================================================================
--- clang/include/clang/Lex/HeaderSearch.h
+++ clang/include/clang/Lex/HeaderSearch.h
@@ -188,6 +188,12 @@
/// Whether the DirectoryLookup at the corresponding index in SearchDirs has
/// been successfully used to lookup a file.
std::vector<bool> SearchDirsUsage;
+ /// When iterating through `SearchDirs`, the current index.
+ unsigned CurrentSearchPathIdx;
+ /// Mapping from module to the search directory index that discovered its
+ /// module map file.
+ llvm::DenseMap<Module *, unsigned> ModuleToSearchDirIdx;
+
unsigned AngledDirIdx = 0;
unsigned SystemDirIdx = 0;
bool NoCurDirSearch = false;
@@ -731,6 +737,7 @@
/// using the search path at index `HitIdx`.
void cacheLookupSuccess(LookupFileCacheInfo &CacheLookup, unsigned HitIdx,
SourceLocation IncludeLoc);
+ void noteModuleLookupUsage(Module *M, SourceLocation Loc);
/// Note that a lookup at the given include location was successful using the
/// search path at index `HitIdx`.
void noteLookupUsage(unsigned HitIdx, SourceLocation IncludeLoc);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
