[PATCH] D51568: [modules] Add `-fno-absolute-module-directory` flag for relocatable modules

[Andrew Gallagher via Phabricator via cfe-commits]

andrewjcg marked 2 inline comments as done.
andrewjcg added a comment.

> I don't think we need to change the serialization format for this: a serialized path beginning with / is already treated as absolute and any other path is already treated as relative, so we don't need a flag to carry that information.

But I think we need this since we now have two types of relative paths -- a CWD-relative path and a module-home-relative path -- and we use this flag to discern them for the AST reader.  Previously, because `cleanPathForOutput` would always absolutify input paths, we didn't need this flag -- any relative path was relative the module home and all other paths were absolute.

I attempted another take on this diff which just made all paths relative the CWD (and did away with module home relative paths), but this didn't work since the reader substitutes in a new module home.

> I'm also not convinced we need to put this behind a flag. It would seem reasonable to me to simply always emit the MODULE_DIRECTORY as relative (if we found the module via a relative path), at least for an explicitly-built module.

Yeah, makes sense.  Will fix this.



================
Comment at: lib/Serialization/ASTWriter.cpp:4524-4546
+bool ASTWriter::PreparePathForOutput(SmallVectorImpl<char> &Path,
+                                     bool &IsRelativeModuleDirectory) {
   assert(Context && "should have context when outputting path");
 
   bool Changed =
-      cleanPathForOutput(Context->getSourceManager().getFileManager(), Path);
+      cleanPathForOutput(Context->getSourceManager().getFileManager(), Path,
+                         llvm::sys::path::is_absolute(BaseDirectory));
----------------
rsmith wrote:
> The intent of this function is to use an absolute form of both the file path and `BaseDirectory` so that we can strip off `BaseDirectory` even if the file path ends up absolute somehow.  Rather than changing `BaseDirectory` above and then changing the code here to compensate, could you only change the code that writes out the `MODULE_DIRECTORY` record to write a relative path?
So we also several non-module-home relative paths get processed here, so just I think we'd have to throw away the output of `cleanPathForOutput` if the `Changed == false`, to preserve the input relative path when non-module-home-relative?


Repository:
  rC Clang

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

https://reviews.llvm.org/D51568