[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 397902.
jansvoboda11 added a comment.

Rebase on top of extracted patches.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113676/new/

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,40 @@
+// 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-both.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-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/sp1 -I %t/sp2 -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
@@ -3907,7 +3907,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  = 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
@@ -107,8 +107,13 @@
 Module *ModuleMap::makeModule(StringRef Name, SourceLocation DefinitionLoc,
   Module *Parent, bool IsFramework, bool IsExplicit,
   unsigned VisibilityID) {
-  return new Module(Name, DefinitionLoc, Parent, IsFramework, IsExplicit,
-VisibilityID);
+  Module *Result = new Module(Name, DefinitionLoc, Parent, IsFramework,
+  IsExplicit, VisibilityID);
+
+  for (const auto  : Callbacks)
+Callback->moduleMapModuleCreated(Result);
+
+  return Result;
 }
 
 Module::ExportDecl
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ 

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D113676#3168219 , @Bigcheese wrote:

> Can we not consider a modulemap used when we load an AST that depends on that 
> modulemap? It's possible this breaks if the module only includes textual 
> headers though.

I don't think that would be 100% correct. I think it's possible for `Module` 
instances (created by parsing a modulemap file) to have semantic meaning even 
without their AST being loaded. One example (that I just made up but sounds 
plausible enough to me) is that we could have a fixit for an `@import` of 
non-existent module that suggests importing another available module 
(discovered through header search) with the most similar name. We don't need 
the AST file for that, but removing search paths could change the diagnostic 
output.

> It really feels like we should have a single place where we actually know if 
> a module is used or not. Long term I would really like to separate modulemap 
> parsing from `Module` creation, which would be a great place to actually do 
> this.

Agreed. But I'm afraid that's a big undertaking. Do you have any other ideas on 
how to detect module usage with the current state of things?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113676/new/

https://reviews.llvm.org/D113676

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

Can we not consider a modulemap used when we load an AST that depends on that 
modulemap? It's possible this breaks if the module only includes textual 
headers though. It really feels like we should have a single place where we 
actually know if a module is used or not. Long term I would really like to 
separate modulemap parsing from `Module` creation, which would be a great place 
to actually do this.

For now we just need to make sure we report every modulemap search path that 
wouldn't change semantics if removed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113676/new/

https://reviews.llvm.org/D113676

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added reviewers: Bigcheese, dexonsmith, vsapsai.
jansvoboda11 added a comment.

Still a WIP, but might be get some feedback on the direction.

I'm not 100% sure when to consider a search path that triggered the creation of 
`Module` as used. Doing so on `Module` creation is not useful, since we do that 
(somewhat) eagerly. The current approach marks search path as used whenever 
it's responsible for the creation of a `Module` that gets returned by 
`HeaderSearch::lookupModule`. That's not correct in situations when a `Module` 
is created via header search (but not actually returned) and then gets queried 
directly from `ModuleMap` instead of `HeaderSearch`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113676/new/

https://reviews.llvm.org/D113676

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added inline comments.



Comment at: clang/lib/Lex/HeaderSearch.cpp:95
+  if (HS.CurrentSearchPathIdx != ~0U)
+HS.ModuleToSearchDirIdx[M] = HS.CurrentSearchPathIdx;
+}

These indices will be out of date if the search paths are changed via 
`HeaderSearch::SetSearchPaths` or `HeaderSearch::AddSearchPath` after the first 
module map has been loaded. One could probably adjust the indices in 
`AddSearchPath` but maybe it’s easier to not use the index into `SearchDirs` as 
the key. An alternative suggestion would be to use 
`std::shared_ptr` instead, changing some of the data 
structures as follows:
```
- std::vector SearchDirs
+ std::vector> SearchDirs

- std::vector SearchDirsUsage;
+ std::map, bool> SearchDirsUsage; 

- llvm::DenseMap ModuleToSearchDirIdx;
+ llvm::DenseMap> 
ModuleToSearchDirIdx;

- llvm::DenseMap SearchDirToHSEntry;
+ llvm::DenseMap, unsigned> 
SearchDirToHSEntry; 
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113676/new/

https://reviews.llvm.org/D113676

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: clang/lib/Lex/HeaderSearch.cpp:94
+  // Module map parsing initiated by header search.
+  if (HS.CurrentSearchPathIdx != ~0U)
+HS.ModuleToSearchDirIdx[M] = HS.CurrentSearchPathIdx;

ahoppen wrote:
> jansvoboda11 wrote:
> > ahoppen wrote:
> > > When would the `moduleMapModuleCreated` be called while 
> > > `CurrentSearchPathIdx == ~0U`? Could this be an `assert` instead?
> > This happens whenever any of the `ModuleMap` member functions that create 
> > new `Module` instances are called outside of `HeaderSearch`.
> > 
> > The `MMCallback` callback is basically "global" (present for the whole 
> > lifetime of `ModuleMap`), so that we don't have to repeatedly 
> > register/deregister it in `HeaderSearch::lookupModule`.
> Is there any reasonable case where module maps would be created without 
> `HeaderSearch` triggering the creation?
I think parsing of module maps (and therefore creation of contained modules) 
should be triggered through `HeaderSearch`.

However, there are also C++20 modules and explicit Clang modules that are not 
discovered by the header search mechanisms or modulemap parsing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113676/new/

https://reviews.llvm.org/D113676

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added inline comments.



Comment at: clang/lib/Lex/HeaderSearch.cpp:94
+  // Module map parsing initiated by header search.
+  if (HS.CurrentSearchPathIdx != ~0U)
+HS.ModuleToSearchDirIdx[M] = HS.CurrentSearchPathIdx;

jansvoboda11 wrote:
> ahoppen wrote:
> > When would the `moduleMapModuleCreated` be called while 
> > `CurrentSearchPathIdx == ~0U`? Could this be an `assert` instead?
> This happens whenever any of the `ModuleMap` member functions that create new 
> `Module` instances are called outside of `HeaderSearch`.
> 
> The `MMCallback` callback is basically "global" (present for the whole 
> lifetime of `ModuleMap`), so that we don't have to repeatedly 
> register/deregister it in `HeaderSearch::lookupModule`.
Is there any reasonable case where module maps would be created without 
`HeaderSearch` triggering the creation?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113676/new/

https://reviews.llvm.org/D113676

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

Thanks for the feedback, I'll try to clean this up a bit.




Comment at: clang/lib/Lex/HeaderSearch.cpp:94
+  // Module map parsing initiated by header search.
+  if (HS.CurrentSearchPathIdx != ~0U)
+HS.ModuleToSearchDirIdx[M] = HS.CurrentSearchPathIdx;

ahoppen wrote:
> When would the `moduleMapModuleCreated` be called while `CurrentSearchPathIdx 
> == ~0U`? Could this be an `assert` instead?
This happens whenever any of the `ModuleMap` member functions that create new 
`Module` instances are called outside of `HeaderSearch`.

The `MMCallback` callback is basically "global" (present for the whole lifetime 
of `ModuleMap`), so that we don't have to repeatedly register/deregister it in 
`HeaderSearch::lookupModule`.



Comment at: clang/lib/Lex/HeaderSearch.cpp:264
+  if (Module || !AllowSearch || !HSOpts->ImplicitModuleMaps) {
+noteModuleLookupUsage(Module, ImportLoc);
 return Module;

ahoppen wrote:
> Just a thought: Could we move `noteModuleLookupUsage` into `findModule`? IIUC 
> that would guarantee that we’re catching all cases and we wouldn’t need to 
> call `noteModuleLookupUsage ` from both overloads of `lookupModule`.
That would clean up `HeaderSearch` nicely. I'll look into creating new 
`HeaderSearch::findModule` function that would wrap `ModuleMap::findModule` and 
note usage.



Comment at: clang/lib/Lex/ModuleMap.cpp:851
   new Module("", Loc, Parent, /*IsFramework*/ false,
  /*IsExplicit*/ true, NumCreatedModules++);
   Result->Kind = Module::PrivateModuleFragment;

ahoppen wrote:
> Maybe a stupid question but why don’t we need to call the callback e.g. here 
> (or on the other `new Module`) calls in this file?
This is related to C++20 modules, so it wasn't necessary for the test case I 
had in mind.

But I agree we should handle this as well. I think more robust solution would 
be to add factory function to `ModuleMap` through which all `Module` instances 
get created, and invoke the callback there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113676/new/

https://reviews.llvm.org/D113676

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added a comment.

LGTM overall. I’ve got a few questions/remarks inline.




Comment at: clang/lib/Lex/HeaderSearch.cpp:94
+  // Module map parsing initiated by header search.
+  if (HS.CurrentSearchPathIdx != ~0U)
+HS.ModuleToSearchDirIdx[M] = HS.CurrentSearchPathIdx;

When would the `moduleMapModuleCreated` be called while `CurrentSearchPathIdx 
== ~0U`? Could this be an `assert` instead?



Comment at: clang/lib/Lex/HeaderSearch.cpp:264
+  if (Module || !AllowSearch || !HSOpts->ImplicitModuleMaps) {
+noteModuleLookupUsage(Module, ImportLoc);
 return Module;

Just a thought: Could we move `noteModuleLookupUsage` into `findModule`? IIUC 
that would guarantee that we’re catching all cases and we wouldn’t need to call 
`noteModuleLookupUsage ` from both overloads of `lookupModule`.



Comment at: clang/lib/Lex/ModuleMap.cpp:851
   new Module("", Loc, Parent, /*IsFramework*/ false,
  /*IsExplicit*/ true, NumCreatedModules++);
   Result->Kind = Module::PrivateModuleFragment;

Maybe a stupid question but why don’t we need to call the callback e.g. here 
(or on the other `new Module`) calls in this file?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113676/new/

https://reviews.llvm.org/D113676

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 386749.
jansvoboda11 added a comment.

Add extra test case


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113676/new/

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,40 @@
+// 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-both.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-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/sp1 -I %t/sp2 -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  = 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  : 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  : Callbacks)
+Callback->moduleMapModuleCreated(Result);
   InferredModuleAllowedBy[Result] = ModuleMapFile;
   Result->IsInferred = true;
   if (!Parent) {

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 386541.
jansvoboda11 added a comment.

Apply clang-format.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113676/new/

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  = 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  : 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  : 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 

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: clang/lib/Lex/HeaderSearch.cpp:264
+  if (Module || !AllowSearch || !HSOpts->ImplicitModuleMaps) {
+noteModuleLookupUsage(Module, ImportLoc);
 return Module;

This is the important change.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113676/new/

https://reviews.llvm.org/D113676

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113676: WIP: [clang][lex] Fix search path usage remark with modules

[Jan Svoboda via Phabricator via cfe-commits]

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  = 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