Re: [PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Richard Smith via cfe-commits]

On 30 August 2017 at 11:52, Boris Kolpackov via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Victor Leschuk  writes:
>
> > Hello Boris, looks like this revision broke tests on our win10 builder:
> > http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_
> 64-scei-ps4-windows10pro-fast/builds/11760
> >
> > Clang :: CXX/modules-ts/basic/basic.link/module-declaration.cpp
> >
> > I had to revert this revision. Could you please take a look?
>
> Sorry about that.
>
> I took a look and it seems the problem is with the test's path regex:
>
> '{{.*}}/x.pcm'
>
> Which, on Windows, is being matched against a path like this:
>
> ...\module-declaration.cpp.tmp\x.pcm
>
> It looks like the "canonical" fix for this is to use [/\\].
>
> What is the protocol in this situation? Should I fix the test and then
> re-apply my patch?


Yes, please.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via cfe-commits]

Victor Leschuk  writes:

> Hello Boris, looks like this revision broke tests on our win10 builder:
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/11760
> 
> Clang :: CXX/modules-ts/basic/basic.link/module-declaration.cpp
> 
> I had to revert this revision. Could you please take a look?

Sorry about that.

I took a look and it seems the problem is with the test's path regex:

'{{.*}}/x.pcm'

Which, on Windows, is being matched against a path like this:

...\module-declaration.cpp.tmp\x.pcm

It looks like the "canonical" fix for this is to use [/\\].

What is the protocol in this situation? Should I fix the test and then
re-apply my patch?

Thanks,
Boris
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Victor Leschuk via cfe-commits]

Hello Boris, looks like this revision broke tests on our win10 builder:
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/11760

Clang :: CXX/modules-ts/basic/basic.link/module-declaration.cpp

I had to revert this revision. Could you please take a look?


On 08/30/2017 11:47 AM, Phabricator via Phabricator via cfe-commits wrote:
> This revision was automatically updated to reflect the committed changes.
> Closed by commit rL312105: [modules] Add ability to specify module name to 
> module file mapping (authored by borisk).
>
> Changed prior to commit:
>   https://reviews.llvm.org/D35020?vs=111826&id=113207#toc
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D35020
>
> Files:
>   cfe/trunk/docs/Modules.rst
>   cfe/trunk/include/clang/Driver/Options.td
>   cfe/trunk/include/clang/Lex/HeaderSearch.h
>   cfe/trunk/include/clang/Lex/HeaderSearchOptions.h
>   cfe/trunk/include/clang/Serialization/ASTReader.h
>   cfe/trunk/include/clang/Serialization/ModuleManager.h
>   cfe/trunk/lib/Driver/ToolChains/Clang.cpp
>   cfe/trunk/lib/Frontend/CompilerInstance.cpp
>   cfe/trunk/lib/Frontend/CompilerInvocation.cpp
>   cfe/trunk/lib/Frontend/FrontendActions.cpp
>   cfe/trunk/lib/Lex/HeaderSearch.cpp
>   cfe/trunk/lib/Serialization/ASTReader.cpp
>   cfe/trunk/lib/Serialization/ASTWriter.cpp
>   cfe/trunk/lib/Serialization/GlobalModuleIndex.cpp
>   cfe/trunk/lib/Serialization/ModuleManager.cpp
>   cfe/trunk/test/CXX/modules-ts/basic/basic.search/module-import.cpp
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

-- 
Best Regards,

Victor Leschuk | Software Engineer |Access Softek

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

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL312105: [modules] Add ability to specify module name to 
module file mapping (authored by borisk).

Changed prior to commit:
  https://reviews.llvm.org/D35020?vs=111826&id=113207#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D35020

Files:
  cfe/trunk/docs/Modules.rst
  cfe/trunk/include/clang/Driver/Options.td
  cfe/trunk/include/clang/Lex/HeaderSearch.h
  cfe/trunk/include/clang/Lex/HeaderSearchOptions.h
  cfe/trunk/include/clang/Serialization/ASTReader.h
  cfe/trunk/include/clang/Serialization/ModuleManager.h
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/lib/Frontend/CompilerInstance.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/lib/Frontend/FrontendActions.cpp
  cfe/trunk/lib/Lex/HeaderSearch.cpp
  cfe/trunk/lib/Serialization/ASTReader.cpp
  cfe/trunk/lib/Serialization/ASTWriter.cpp
  cfe/trunk/lib/Serialization/GlobalModuleIndex.cpp
  cfe/trunk/lib/Serialization/ModuleManager.cpp
  cfe/trunk/test/CXX/modules-ts/basic/basic.search/module-import.cpp

Index: cfe/trunk/lib/Serialization/ASTWriter.cpp
===
--- cfe/trunk/lib/Serialization/ASTWriter.cpp
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp
@@ -1505,6 +1505,7 @@
   for (auto I : M.Signature)
 Record.push_back(I);
 
+  AddString(M.ModuleName, Record);
   AddPath(M.FileName, Record);
 }
 Stream.EmitRecord(IMPORTS, Record);
@@ -4779,7 +4780,8 @@
 // each of those modules were mapped into our own offset/ID space, so that
 // the reader can build the appropriate mapping to its own offset/ID space.
 // The map consists solely of a blob with the following format:
-// *(module-name-len:i16 module-name:len*i8
+// *(module-kind:i8
+//   module-name-len:i16 module-name:len*i8
 //   source-location-offset:i32
 //   identifier-id:i32
 //   preprocessed-entity-id:i32
@@ -4790,6 +4792,10 @@
 //   c++-base-specifiers-id:i32
 //   type-id:i32)
 // 
+// module-kind is the ModuleKind enum value. If it is MK_PrebuiltModule or
+// MK_ExplicitModule, then the module-name is the module name. Otherwise,
+// it is the module file name.
+//
 auto Abbrev = std::make_shared();
 Abbrev->Add(BitCodeAbbrevOp(MODULE_OFFSET_MAP));
 Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob));
@@ -4800,9 +4806,13 @@
   for (ModuleFile &M : Chain->ModuleMgr) {
 using namespace llvm::support;
 endian::Writer LE(Out);
-StringRef FileName = M.FileName;
-LE.write(FileName.size());
-Out.write(FileName.data(), FileName.size());
+LE.write(static_cast(M.Kind));
+StringRef Name =
+  M.Kind == MK_PrebuiltModule || M.Kind == MK_ExplicitModule
+  ? M.ModuleName
+  : M.FileName;
+LE.write(Name.size());
+Out.write(Name.data(), Name.size());
 
 // Note: if a base ID was uint max, it would not be possible to load
 // another module after it or have more than one entity inside it.
Index: cfe/trunk/lib/Serialization/ModuleManager.cpp
===
--- cfe/trunk/lib/Serialization/ModuleManager.cpp
+++ cfe/trunk/lib/Serialization/ModuleManager.cpp
@@ -28,15 +28,23 @@
 using namespace clang;
 using namespace serialization;
 
-ModuleFile *ModuleManager::lookup(StringRef Name) const {
+ModuleFile *ModuleManager::lookupByFileName(StringRef Name) const {
   const FileEntry *Entry = FileMgr.getFile(Name, /*openFile=*/false,
/*cacheFailure=*/false);
   if (Entry)
 return lookup(Entry);
 
   return nullptr;
 }
 
+ModuleFile *ModuleManager::lookupByModuleName(StringRef Name) const {
+  if (const Module *Mod = HeaderSearchInfo.getModuleMap().findModule(Name))
+if (const FileEntry *File = Mod->getASTFile())
+  return lookup(File);
+
+  return nullptr;
+}
+
 ModuleFile *ModuleManager::lookup(const FileEntry *File) const {
   auto Known = Modules.find(File);
   if (Known == Modules.end())
@@ -306,9 +314,11 @@
 }
 
 ModuleManager::ModuleManager(FileManager &FileMgr, MemoryBufferCache &PCMCache,
- const PCHContainerReader &PCHContainerRdr)
+ const PCHContainerReader &PCHContainerRdr,
+ const HeaderSearch& HeaderSearchInfo)
 : FileMgr(FileMgr), PCMCache(&PCMCache), PCHContainerRdr(PCHContainerRdr),
-  GlobalIndex(), FirstVisitState(nullptr) {}
+  HeaderSearchInfo (HeaderSearchInfo), GlobalIndex(),
+  FirstVisitState(nullptr) {}
 
 ModuleManager::~ModuleManager() { delete FirstVisitState; }
 
Index: cfe/trunk/lib/Serialization/ASTReader.cpp
===
--- cfe/trunk/lib/Serialization/ASTReader.cpp
+++ cfe/trunk/lib/Serialization/ASTReader.cpp
@@ -2485,7 +2485,23 @@

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

boris marked 2 inline comments as done.
boris added inline comments.



Comment at: lib/Frontend/CompilerInvocation.cpp:986
+if (Val.find('=') == StringRef::npos)
+  Opts.ExtraDeps.push_back(Val);
+  }

rsmith wrote:
> Does a module file specified via `-fmodule-file=foo=foo.pcm` get included in 
> the dep file if it is used? (If not, I'm happy for that to be fixed in a 
> separate change, but it does need to be fixed for depfile-based build 
> systems.)
No, currently it does not and neither a module file that is discovered via 
-fprebuilt-module-path. I think if we are doing this, then we should do for 
both cases. What do you think?


https://reviews.llvm.org/D35020



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

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Only typographical nits and post-commit suggestions, so please go ahead. Thanks!




Comment at: include/clang/Serialization/ModuleManager.h:63
+  /// \brief Preprocessor's HeaderSearchInfo containing the module map.
+  const HeaderSearch& HeaderSearchInfo;
+

` &`, not `& `, please.



Comment at: lib/Frontend/CompilerInvocation.cpp:986
+if (Val.find('=') == StringRef::npos)
+  Opts.ExtraDeps.push_back(Val);
+  }

Does a module file specified via `-fmodule-file=foo=foo.pcm` get included in 
the dep file if it is used? (If not, I'm happy for that to be fixed in a 
separate change, but it does need to be fixed for depfile-based build systems.)



Comment at: lib/Frontend/CompilerInvocation.cpp:982
+StringRef Val = A->getValue();
+if (Val.find('=') == StringRef::npos)
+  Opts.ExtraDeps.push_back(Val);

boris wrote:
> rsmith wrote:
> > There should be some way to specify a module file that contains a `=` in 
> > its file name. Ideally, that way would be compatible with our 
> > currently-supported form of `-fmodule-name=filename`. I don't see a way to 
> > support that other than `stat`ing every value we're given and checking to 
> > see whether it exists before attempting to split on a `=`... thoughts?
> > 
> > One somewhat hackish alternative would be to only recognize an `=` if there 
> > is no preceding `/`. (So `-fmodule-file=foo=bar.pcm` specifies a mapping, 
> > and `-fmodule-file=./foo=bar.pcm` specifies the file `./foo=bar.pcm`.)
> > 
> > (FWIW, we also support module names containing `=` characters.)
> A couple of thoughts:
> 
> 1. Using stat() is also not without issues: I may have an unrelated file with 
> a name that matches the whole value -- this will be fun to debug.
> 
> 2. GCC seems to have given up on trying to support paths with '=' since 
> AFAIK, none of their key=value options (e.g., -fdebug-prefix-map) support any 
> kind of escaping. I think the implicit assumption is that if you are using 
> paths with '=' in them, then you've asked for it.
> 
> 3. The '/' idea will work but will get a bit hairier if we also want to 
> support Windows (will need to check for both slashes).
> 
> 4. Another option is to reserve empty module name as a way to escape '=': 
> -fmodule-file==foo=bar.pcm. Not backwards compatible (but neither is ./) and 
> looks a bit weird but is the simplest to implement.
> 
> My preference order is (2), (4), (3), (1). Let me know which way you want to 
> go.
> 
> 
We've had a while to think about this, and haven't found a completely 
satisfactory answer, so let's go with (2) for now, without prejudice to future 
alternatives. It's fully GCC-compatible, and happens to be what this patch 
already does :)



Comment at: lib/Serialization/ASTReader.cpp:10180
+  ModuleMgr(PP.getFileManager(), PP.getPCMCache(), PCHContainerRdr,
+PP.getHeaderSearchInfo ()),
   PCMCache(PP.getPCMCache()), DummyIdResolver(PP),

No space before `()`.



Comment at: lib/Serialization/ASTReader.cpp:4145-4146
+// by-name lookup.
+if (Type == MK_PrebuiltModule || Type == MK_ExplicitModule)
+  ModuleMgr.registerPrebuilt(F);
 break;

boris wrote:
> rsmith wrote:
> > I'm worried by this: the `Type` can be different for different imports of 
> > the same .pcm file, and you'll only get here for the *first* import edge. 
> > So you can fail to add the module to the "prebuilt modules" map here, and 
> > then later attempt to look it up there (when processing the module offset 
> > map) and fail to find it.
> > 
> > Instead of keeping a separate lookup structure on the module manager, could 
> > you look up the `Module`, and then use its `ASTFile` to find the 
> > corresponding `ModuleFile`?
> > 
> > (If you go that way, the changes to module lookup when reading the module 
> > offset map could be generalized to apply to all `MK_*Module` types.)
> Yes, I was not entirely happy with this register prebuilt business so thanks 
> for the hint. I've implemented this and all tests pass (both Clang's and my 
> own). Though I cannot say I fully understand all the possible scenarios (like 
> when can ASTFile be NULL).
> 
> 
> 
> > (If you go that way, the changes to module lookup when reading the module 
> > offset map could be generalized to apply to all MK_*Module types.)
> 
> Are you suggesting that we switch to storing module names instead of module 
> files for all the module types in the offset map? If so, I think I've tried 
> that at some point but it didn't go well (existing tests were failing if I 
> remember correctly). I can try again if you think this should work. 
> 
> 
Yes, that's what I was suggesting, but don't block this commit on it. 
Ultimately, we should probably 

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.


https://reviews.llvm.org/D35020



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

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

boris updated this revision to Diff 111826.
boris added a comment.

New revision of the patch that I believe addresses all the issues except for 
the '=' escaping.


https://reviews.llvm.org/D35020

Files:
  docs/Modules.rst
  include/clang/Driver/Options.td
  include/clang/Lex/HeaderSearch.h
  include/clang/Lex/HeaderSearchOptions.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ModuleManager.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GlobalModuleIndex.cpp
  lib/Serialization/ModuleManager.cpp
  test/CXX/modules-ts/basic/basic.search/module-import.cpp

Index: test/CXX/modules-ts/basic/basic.search/module-import.cpp
===
--- /dev/null
+++ test/CXX/modules-ts/basic/basic.search/module-import.cpp
@@ -0,0 +1,39 @@
+// Tests for imported module search.
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'export module x; int a, b;' > %t/x.cppm
+// RUN: echo 'export module y; import x; int c;' > %t/y.cppm
+// RUN: echo 'export module z; import y; int d;' > %t/z.cppm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface %t/x.cppm -o %t/x.pcm
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=%t/x.pcm %t/y.cppm -o %t/y.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: mv %t/x.pcm %t/a.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm %t/z.cppm -o %t/z.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=z=%t/z.pcm -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=z
+//
+
+import MODULE_NAME;
+
+// expected-no-diagnostics
Index: lib/Serialization/ModuleManager.cpp
===
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -28,15 +28,23 @@
 using namespace clang;
 using namespace serialization;
 
-ModuleFile *ModuleManager::lookup(StringRef Name) const {
+ModuleFile *ModuleManager::lookupByFileName(StringRef Name) const {
   const FileEntry *Entry = FileMgr.getFile(Name, /*openFile=*/false,
/*cacheFailure=*/false);
   if (Entry)
 return lookup(Entry);
 
   return nullptr;
 }
 
+ModuleFile *ModuleManager::lookupByModuleName(StringRef Name) const {
+  if (const Module *Mod = HeaderSearchInfo.getModuleMap().findModule(Name))
+if (const FileEntry *File = Mod->getASTFile())
+  return lookup(File);
+
+  return nullptr;
+}
+
 ModuleFile *ModuleManager::lookup(const FileEntry *File) const {
   auto Known = Modules.find(File);
   if (Known == Modules.end())
@@ -306,9 +314,11 @@
 }
 
 ModuleManager::ModuleManager(FileManager &FileMgr, MemoryBufferCache &PCMCache,
- const PCHContainerReader &PCHContainerRdr)
+ const PCHContainerReader &PCHContainerRdr,
+ const HeaderSearch& HeaderSearchInfo)
 : FileMgr(FileMgr), PCMCache(&PCMCache), PCHContainerRdr(PCHContainerRdr),
-  GlobalIndex(), FirstVisitState(nullptr) {}
+  HeaderSearchInfo (HeaderSearchInfo), GlobalIndex(),
+  FirstVisitState(nullptr) {}
 
 ModuleManager::~ModuleManager() { delete FirstVisitState; }
 
Index: lib/Serialization/GlobalModuleIndex.cpp
===
--- lib/Serialization/GlobalModuleIndex.cpp
+++ lib/Serialization/GlobalModuleIndex.cpp
@@ -619,6 +619,10 @@
   (uint32_t)Record[Idx++], (uint32_t)Record[Idx++],
   (uint32_t)Record[Idx++]}}};
 
+// Skip the module name (currently this is only used for prebuilt
+// modules while here we are only dealing with cached).
+Idx += Record[Idx] + 1;
+
 // Retrieve the imported file name.
 unsigned Length 

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

boris added inline comments.



Comment at: lib/Serialization/ASTReader.cpp:4145-4146
+// by-name lookup.
+if (Type == MK_PrebuiltModule || Type == MK_ExplicitModule)
+  ModuleMgr.registerPrebuilt(F);
 break;

rsmith wrote:
> I'm worried by this: the `Type` can be different for different imports of the 
> same .pcm file, and you'll only get here for the *first* import edge. So you 
> can fail to add the module to the "prebuilt modules" map here, and then later 
> attempt to look it up there (when processing the module offset map) and fail 
> to find it.
> 
> Instead of keeping a separate lookup structure on the module manager, could 
> you look up the `Module`, and then use its `ASTFile` to find the 
> corresponding `ModuleFile`?
> 
> (If you go that way, the changes to module lookup when reading the module 
> offset map could be generalized to apply to all `MK_*Module` types.)
Yes, I was not entirely happy with this register prebuilt business so thanks 
for the hint. I've implemented this and all tests pass (both Clang's and my 
own). Though I cannot say I fully understand all the possible scenarios (like 
when can ASTFile be NULL).



> (If you go that way, the changes to module lookup when reading the module 
> offset map could be generalized to apply to all MK_*Module types.)

Are you suggesting that we switch to storing module names instead of module 
files for all the module types in the offset map? If so, I think I've tried 
that at some point but it didn't go well (existing tests were failing if I 
remember correctly). I can try again if you think this should work. 




https://reviews.llvm.org/D35020



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

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

boris marked 6 inline comments as done.
boris added a comment.

I've marked as "done" items that I have resolved in my local revision (not yet 
uploaded) and have added one comment for further feedback.




Comment at: lib/Frontend/CompilerInvocation.cpp:982
+StringRef Val = A->getValue();
+if (Val.find('=') == StringRef::npos)
+  Opts.ExtraDeps.push_back(Val);

rsmith wrote:
> There should be some way to specify a module file that contains a `=` in its 
> file name. Ideally, that way would be compatible with our currently-supported 
> form of `-fmodule-name=filename`. I don't see a way to support that other 
> than `stat`ing every value we're given and checking to see whether it exists 
> before attempting to split on a `=`... thoughts?
> 
> One somewhat hackish alternative would be to only recognize an `=` if there 
> is no preceding `/`. (So `-fmodule-file=foo=bar.pcm` specifies a mapping, and 
> `-fmodule-file=./foo=bar.pcm` specifies the file `./foo=bar.pcm`.)
> 
> (FWIW, we also support module names containing `=` characters.)
A couple of thoughts:

1. Using stat() is also not without issues: I may have an unrelated file with a 
name that matches the whole value -- this will be fun to debug.

2. GCC seems to have given up on trying to support paths with '=' since AFAIK, 
none of their key=value options (e.g., -fdebug-prefix-map) support any kind of 
escaping. I think the implicit assumption is that if you are using paths with 
'=' in them, then you've asked for it.

3. The '/' idea will work but will get a bit hairier if we also want to support 
Windows (will need to check for both slashes).

4. Another option is to reserve empty module name as a way to escape '=': 
-fmodule-file==foo=bar.pcm. Not backwards compatible (but neither is ./) and 
looks a bit weird but is the simplest to implement.

My preference order is (2), (4), (3), (1). Let me know which way you want to go.




https://reviews.llvm.org/D35020



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

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: docs/ClangCommandLineReference.rst:3-4
   ---
   NOTE: This file is automatically generated by running clang-tblgen
   -gen-opt-docs. Do not edit this file by hand!!
   ---

Please revert the change to this file; we can regenerate it after this lands 
(as a separate commit).



Comment at: include/clang/Driver/Options.td:1105
+  Group, Flags<[DriverOption,CC1Option]>, 
MetaVarName<"[=]">,
+  HelpText<"Specify the mapping of module name to precompiled module file 
loading it if name is omitted.">;
 def fmodules_ignore_macro : Joined<["-"], "fmodules-ignore-macro=">, 
Group, Flags<[CC1Option]>,

How about "Specify the mapping of module name to precompiled module file, or 
load a module file if name is omitted."?



Comment at: include/clang/Serialization/ModuleManager.h:52-53
 
+  /// \brief All loaded prebuilt/explicit modules, indexed by module name.
+  std::map PrebuiltModules;
+

`llvm::StringMap` would be preferable here.



Comment at: lib/Driver/ToolChains/Clang.cpp:3613-3622
+  // The -fmodule-file= form can be used to load files containing
+  // precompiled modules.
+  for (const Arg *A : Args.filtered(options::OPT_fmodule_file)) {
+StringRef Val = A->getValue();
+if (Val.find('=') == StringRef::npos) {
   if (HaveAnyModules)
+CmdArgs.push_back(Args.MakeArgString("-fmodule-file=" + Val));

What's the purpose of splitting this into two separate loops? I see that you're 
passing on all `-fmodule-file=name=value` flags even if modules is not enabled. 
Is that useful?



Comment at: lib/Frontend/CompilerInstance.cpp:1623-1630
+/// FIXME: perhaps we should (a) look for a module using the module name
+//  to file map (PrebuiltModuleFiles) and (b) diagnose if still not found?
+//if (Module == nullptr) {
+//  getDiagnostics().Report(ModuleNameLoc, diag::err_module_not_found)
+//<< ModuleName;
+//  ModuleBuildFailed = true;
+//  return ModuleLoadResult();

This doesn't make sense for header modules (defined by a module map). For the 
case of a Modules TS module implementation unit, we should skip this 
`CurrentModule` check, but let's leave that for a separate patch.



Comment at: lib/Frontend/CompilerInvocation.cpp:982
+StringRef Val = A->getValue();
+if (Val.find('=') == StringRef::npos)
+  Opts.ExtraDeps.push_back(Val);

There should be some way to specify a module file that contains a `=` in its 
file name. Ideally, that way would be compatible with our currently-supported 
form of `-fmodule-name=filename`. I don't see a way to support that other than 
`stat`ing every value we're given and checking to see whether it exists before 
attempting to split on a `=`... thoughts?

One somewhat hackish alternative would be to only recognize an `=` if there is 
no preceding `/`. (So `-fmodule-file=foo=bar.pcm` specifies a mapping, and 
`-fmodule-file=./foo=bar.pcm` specifies the file `./foo=bar.pcm`.)

(FWIW, we also support module names containing `=` characters.)



Comment at: lib/Lex/HeaderSearch.cpp:131
 
 std::string HeaderSearch::getModuleFileName(Module *Module) {
   const FileEntry *ModuleMap =

I like the renaming you've done here. Should this one also be renamed to 
`getCachedModuleFileName`?



Comment at: lib/Serialization/ASTReader.cpp:4145-4146
+// by-name lookup.
+if (Type == MK_PrebuiltModule || Type == MK_ExplicitModule)
+  ModuleMgr.registerPrebuilt(F);
 break;

I'm worried by this: the `Type` can be different for different imports of the 
same .pcm file, and you'll only get here for the *first* import edge. So you 
can fail to add the module to the "prebuilt modules" map here, and then later 
attempt to look it up there (when processing the module offset map) and fail to 
find it.

Instead of keeping a separate lookup structure on the module manager, could you 
look up the `Module`, and then use its `ASTFile` to find the corresponding 
`ModuleFile`?

(If you go that way, the changes to module lookup when reading the module 
offset map could be generalized to apply to all `MK_*Module` types.)


https://reviews.llvm.org/D35020



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

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.


https://reviews.llvm.org/D35020



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

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.


https://reviews.llvm.org/D35020



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

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.

FWIW, I went ahead and implemented this functionality in GCC. It has been 
merged into its c++-modules branch.


https://reviews.llvm.org/D35020



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

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.

I am holding off on proposing the same functionality to GCC because I want to 
make sure the command line interface is the same for both compilers (GCC has 
less baggage in this area, option-name-wise). So confirming that at least the 
naming/semantics are acceptable would be very helpful.


https://reviews.llvm.org/D35020



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

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

boris updated this revision to Diff 107017.
boris added a comment.

Rebase on latest HEAD.


https://reviews.llvm.org/D35020

Files:
  docs/ClangCommandLineReference.rst
  docs/Modules.rst
  include/clang/Driver/Options.td
  include/clang/Lex/HeaderSearch.h
  include/clang/Lex/HeaderSearchOptions.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ModuleManager.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GlobalModuleIndex.cpp
  lib/Serialization/ModuleManager.cpp
  test/CXX/modules-ts/basic/basic.search/module-import.cpp

Index: test/CXX/modules-ts/basic/basic.search/module-import.cpp
===
--- /dev/null
+++ test/CXX/modules-ts/basic/basic.search/module-import.cpp
@@ -0,0 +1,39 @@
+// Tests for imported module search.
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'export module x; int a, b;' > %t/x.cppm
+// RUN: echo 'export module y; import x; int c;' > %t/y.cppm
+// RUN: echo 'export module z; import y; int d;' > %t/z.cppm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface %t/x.cppm -o %t/x.pcm
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=%t/x.pcm %t/y.cppm -o %t/y.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: mv %t/x.pcm %t/a.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm %t/z.cppm -o %t/z.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=z=%t/z.pcm -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=z
+//
+
+import MODULE_NAME;
+
+// expected-no-diagnostics
Index: lib/Serialization/ModuleManager.cpp
===
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -45,6 +45,14 @@
   return Known->second;
 }
 
+ModuleFile *ModuleManager::lookupPrebuilt(StringRef Name) const {
+  auto Known = PrebuiltModules.find(Name);
+  if (Known == PrebuiltModules.end())
+return nullptr;
+
+  return Known->second;
+}
+
 std::unique_ptr
 ModuleManager::lookupBuffer(StringRef Name) {
   const FileEntry *Entry = FileMgr.getFile(Name, /*openFile=*/false,
@@ -192,6 +200,11 @@
   return NewlyLoaded;
 }
 
+void ModuleManager::registerPrebuilt (ModuleFile &M) {
+  assert (!M.ModuleName.empty());
+  PrebuiltModules[M.ModuleName] = &M;
+}
+
 void ModuleManager::removeModules(
 ModuleIterator First,
 llvm::SmallPtrSetImpl &LoadedSuccessfully,
@@ -232,6 +245,8 @@
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
 Modules.erase(victim->File);
+if (!victim->ModuleName.empty())
+  PrebuiltModules.erase(victim->ModuleName);
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
Index: lib/Serialization/GlobalModuleIndex.cpp
===
--- lib/Serialization/GlobalModuleIndex.cpp
+++ lib/Serialization/GlobalModuleIndex.cpp
@@ -619,6 +619,10 @@
   (uint32_t)Record[Idx++], (uint32_t)Record[Idx++],
   (uint32_t)Record[Idx++]}}};
 
+// Skip the module name (currently this is only used for prebuilt
+// modules while here we are only dealing with cached).
+Idx += Record[Idx] + 1;
+
 // Retrieve the imported file name.
 unsigned Length = Record[Idx++];
 SmallString<128> ImportedFile(Record.begin() + Idx,
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -1505,6 +1505,7 @@
   for (auto I : M.Signature)
 Record.push_back(I);
 
+  AddString(M.ModuleName, Record);
   AddPath(M.FileName, Rec

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

boris added a comment.

Ping.


https://reviews.llvm.org/D35020



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

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

boris updated this revision to Diff 106041.
boris added a comment.

Rebase on latest HEAD.


https://reviews.llvm.org/D35020

Files:
  docs/ClangCommandLineReference.rst
  docs/Modules.rst
  include/clang/Driver/Options.td
  include/clang/Lex/HeaderSearch.h
  include/clang/Lex/HeaderSearchOptions.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ModuleManager.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GlobalModuleIndex.cpp
  lib/Serialization/ModuleManager.cpp
  test/CXX/modules-ts/basic/basic.search/module-import.cpp

Index: test/CXX/modules-ts/basic/basic.search/module-import.cpp
===
--- /dev/null
+++ test/CXX/modules-ts/basic/basic.search/module-import.cpp
@@ -0,0 +1,39 @@
+// Tests for imported module search.
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'export module x; int a, b;' > %t/x.cppm
+// RUN: echo 'export module y; import x; int c;' > %t/y.cppm
+// RUN: echo 'export module z; import y; int d;' > %t/z.cppm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface %t/x.cppm -o %t/x.pcm
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=%t/x.pcm %t/y.cppm -o %t/y.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: mv %t/x.pcm %t/a.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm %t/z.cppm -o %t/z.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=z=%t/z.pcm -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=z
+//
+
+import MODULE_NAME;
+
+// expected-no-diagnostics
Index: lib/Serialization/ModuleManager.cpp
===
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -45,6 +45,14 @@
   return Known->second;
 }
 
+ModuleFile *ModuleManager::lookupPrebuilt(StringRef Name) const {
+  auto Known = PrebuiltModules.find(Name);
+  if (Known == PrebuiltModules.end())
+return nullptr;
+
+  return Known->second;
+}
+
 std::unique_ptr
 ModuleManager::lookupBuffer(StringRef Name) {
   const FileEntry *Entry = FileMgr.getFile(Name, /*openFile=*/false,
@@ -192,6 +200,11 @@
   return NewlyLoaded;
 }
 
+void ModuleManager::registerPrebuilt (ModuleFile &M) {
+  assert (!M.ModuleName.empty());
+  PrebuiltModules[M.ModuleName] = &M;
+}
+
 void ModuleManager::removeModules(
 ModuleIterator First,
 llvm::SmallPtrSetImpl &LoadedSuccessfully,
@@ -232,6 +245,8 @@
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
 Modules.erase(victim->File);
+if (!victim->ModuleName.empty())
+  PrebuiltModules.erase(victim->ModuleName);
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
Index: lib/Serialization/GlobalModuleIndex.cpp
===
--- lib/Serialization/GlobalModuleIndex.cpp
+++ lib/Serialization/GlobalModuleIndex.cpp
@@ -619,6 +619,10 @@
   (uint32_t)Record[Idx++], (uint32_t)Record[Idx++],
   (uint32_t)Record[Idx++]}}};
 
+// Skip the module name (currently this is only used for prebuilt
+// modules while here we are only dealing with cached).
+Idx += Record[Idx] + 1;
+
 // Retrieve the imported file name.
 unsigned Length = Record[Idx++];
 SmallString<128> ImportedFile(Record.begin() + Idx,
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -1505,6 +1505,7 @@
   for (auto I : M.Signature)
 Record.push_back(I);
 
+  AddString(M.ModuleName, Record);
   AddPath(M.FileName, Rec

[PATCH] D35020: [Modules] Add ability to specify module name to module file mapping

[Boris Kolpackov via Phabricator via cfe-commits]

boris created this revision.

Extend the -fmodule-file option to support the [=] value format.
If the name is omitted, then the old semantics is preserved (the module file
is loaded whether needed or not). If the name is specified, then the mapping
is treated as just another prebuilt module search mechanism, similar to
-fprebuilt-module-path, and the module file is only loaded if actually
used (e.g., via import). With one exception: this mapping also overrides
module file references embedded in other modules (which can be useful if
module files are moved/renamed as often happens during remote compilation).

This override semantics requires some extra work: we now store the module
name in addition to the file name in the serialized AST representation as
well as keep track of already loaded prebuilt modules by-name in addition
to by-file.

Patch by Boris Kolpackov

---

Additional notes:

1. Based on this mailing list discussion:

  http://lists.llvm.org/pipermail/cfe-dev/2017-June/054431.html

2. The need to change the serialized AST representation was a bit more than 
what I hoped for my first patch but based on this FIXME comment I recon this is 
moving in right direction:

  ASTReader::ReadModuleOffsetMap():

  // FIXME: Looking up dependency modules by filename is horrible.

  So now, at least for prebuilt modules, we look up by the module name.

3. Overloading -fmodule-file for this admittedly fairly different semantics 
might look like a bad idea and source of some unnecessary complexity. The 
problem with a separate option approach is the difficulty of finding a decent 
name that is not already used (e.g., -fmodule-map is out because of 
-fmodule-map-file; more details in the mailing list thread). But I am still 
open to changing this to a separate option if there are strong feelings (and 
good name suggestions ;-)).

4. I plan to implement a companion option that will read this mapping from a 
file. I will submit it as a separate patch once the general validity of the 
approach is confirmed.


https://reviews.llvm.org/D35020

Files:
  docs/ClangCommandLineReference.rst
  docs/Modules.rst
  include/clang/Driver/Options.td
  include/clang/Lex/HeaderSearch.h
  include/clang/Lex/HeaderSearchOptions.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ModuleManager.h
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInstance.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Lex/HeaderSearch.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GlobalModuleIndex.cpp
  lib/Serialization/ModuleManager.cpp
  test/CXX/modules-ts/basic/basic.search/module-import.cpp

Index: test/CXX/modules-ts/basic/basic.search/module-import.cpp
===
--- /dev/null
+++ test/CXX/modules-ts/basic/basic.search/module-import.cpp
@@ -0,0 +1,39 @@
+// Tests for imported module search.
+//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: echo 'export module x; int a, b;' > %t/x.cppm
+// RUN: echo 'export module y; import x; int c;' > %t/y.cppm
+// RUN: echo 'export module z; import y; int d;' > %t/z.cppm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface %t/x.cppm -o %t/x.pcm
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=%t/x.pcm %t/y.cppm -o %t/y.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/x.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: mv %t/x.pcm %t/a.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=x
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=y
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -emit-module-interface -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm %t/z.cppm -o %t/z.pcm
+//
+// RUN: %clang_cc1 -std=c++1z -fmodules-ts -I%t -fmodule-file=z=%t/z.pcm -fmodule-file=y=%t/y.pcm -fmodule-file=x=%t/a.pcm -verify %s \
+// RUN:-DMODULE_NAME=z
+//
+
+import MODULE_NAME;
+
+// expected-no-diagnostics
Index: lib/Serialization/ModuleManager.cpp
===
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -45,6 +45,14 @@
   return Known->second;
 }
 
+ModuleFile *ModuleM