[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 be using something like an index into the `IMPORTS` list instead (but it's not quite as easy as that since we also have module offsets for imports of imports and so on).


https://reviews.llvm.org/D35020