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

[andrewjcg]

Summary

Currently, modules built using module map files embed the path to the directory
that houses their inputs (e.g. headers and module map file) into the output
module file. This path is embedded as an absolute path and the various input
files are either relativized to this path (if they are underneath it) or also
embedded as an absolute path.

This adds a flag which disables making the module directory an absolute path.
To support relative module directory paths, this diff also updates the serialized
AST to write out an extra bit with serialized paths to indicate whether they are
relative to the module directory or not (before this diff, these were handled by
absolutify paths that weren't relative the module directory).

[andrewjcg]

I'm not sure this is the best approach, but I wasn't sure of a better one (to support module files w/o absolute paths). Another approach I tried, was relativizing the other input files (from outside the module directory) using chains of ../ (e.g. ../../../../../other/modules/module.modulemap), but this causes issues when symlinks appear in the module directory path. Another potential option could be to add a bit to the serialized module format for each input, to allow some inputs to mark themselves as relative to the CWD, rather than the module directory.

[andrewjcg]

fix umbrella writing

[andrewjcg]

Dropping the module directory entirely and fully resolving paths on serialization
broke some things during deserialization, specifically when the deserializer wanted
to update paths to use an alternate module directory.

This switches to a different strategy of only relativizing the paths that are
actually under the module home dir, and adding a bit to the serialized paths to
indiciate this. This bit is read on deserializiation to determine whether the
path is resolved against the module directory or not.

[modocache]

Friendly ping! Could someone recommend a reviewer for this? Or is there something wrong with the patch?

Looking at the blame I can see @manmanren; I believe you're not actively working in this part of the codebase, but could you review, or recommend someone who could? Or maybe @bruno?

[manmanren]

I am not sure if this is the best approach, but the implementation looks okay to me. @bruno @rsmith What do you think?

Manman

include/clang/Serialization/ASTReader.h

2241 ↗

(On Diff #166709)

Can you expand on this comment? Skip the relative bit?

include/clang/Serialization/ASTWriter.h

642 ↗

(On Diff #166709)

Can you add comment here on how we set IsRelativeModuleDirectory?

650 ↗

(On Diff #166709)

Can you update the comment to say that record is updated with the extra bit?

[rsmith]

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.

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. (For an implicitly built module, we probably need to track the absolute path so we can detect whether we found the right PCM file.)

lib/Serialization/ASTWriter.cpp

4537–4558

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?

[bruno]

Thanks for working on this @andrewjcg

In D51568#1313717, @rsmith wrote:

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. (For an implicitly built module, we probably need to track the absolute path so we can detect whether we found the right PCM file.)

+1

[andrewjcg]

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.

lib/Serialization/ASTWriter.cpp

4537–4558

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?

[rsmith]

In D51568#1314004, @andrewjcg wrote:

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.

Ah, of course, that's exactly what I was missing :) I guess I live in a too--fmodule-map-file-home-is-cwd-centric world where the two are always the same (even when modules are relocated).

Following the 0-1-infinity principle, I think it might make sense to have a 'catalog' of prefixes for paths (relative to the module, relative to the compiler's CWD, relative to the compiler's resource directory, relative to the sysroot) that we try stripping off, with different markers to say which one we removed. (Right now, if you relocate the compiler, you invalidate any .pcm file that references files in its resource directory, for instance.) But this seems like a step in a good direction.

Just checking that we're on the same page here... suppose I do this:

• compile module described in foo/module.modulemap (with no -fmodule-map-file-home-is-cwd, so module-relative paths have the foo/ prefix stripped off them)
• use -Ibar/ to find some textual headers

Then, if I relocate the foo/ module to elsewhere/foo and pass the corresponding pcm file to a compilation using that module, I will still expect to find the bar/ files referenced by the pcm file relative to the compiler's working directory, not in elsewhere/bar. Is that what you're expecting, or would you expect the file paths to be emitted as module-relative ../bar/... paths? (Note that the latter can break if foo is a symlink.)

lib/Serialization/ASTReader.cpp

2094–2096 ↗

(On Diff #166709)

This'd be more readable (throughout the patch) spelled as IsRelativeToModuleDirectory. (I was wondering "What is a relative module directory?")

include/clang/Driver/CC1Options.td

 //===----------------------------------------------------------------------===//
 
 def nostdsysteminc : Flag<["-"], "nostdsysteminc">,
   HelpText<"Disable standard system #include directories">;
 def fdisable_module_hash : Flag<["-"], "fdisable-module-hash">,
   HelpText<"Disable the module hash">;
 def fmodules_hash_content : Flag<["-"], "fmodules-hash-content">,
   HelpText<"Enable hashing the content of a module file">;
+def fdisable_module_directory : Flag<["-"], "fdisable-module-directory">,
+  HelpText<"Do not relativize paths to the module home directory or absolutify "
+           "paths that are not relative to the module home directory.">;
 def c_isystem : JoinedOrSeparate<["-"], "c-isystem">, MetaVarName<"<directory>">,
   HelpText<"Add directory to the C SYSTEM include search path">;
 def objc_isystem : JoinedOrSeparate<["-"], "objc-isystem">,
   MetaVarName<"<directory>">,
   HelpText<"Add directory to the ObjC SYSTEM include search path">;
 def objcxx_isystem : JoinedOrSeparate<["-"], "objcxx-isystem">,
   MetaVarName<"<directory>">,
   HelpText<"Add directory to the ObjC++ SYSTEM include search path">;

include/clang/Lex/HeaderSearchOptions.h

public:
 
   /// Whether the module includes debug information (-gmodules).
   unsigned UseDebugInfo : 1;
 
   unsigned ModulesValidateDiagnosticOptions : 1;
 
   unsigned ModulesHashContent : 1;
 
+  /// Do not relativize paths to the module directory or absolutify non-module-
+  /// directory-relative paths.
+  unsigned DisableModuleDirectory : 1;
+
   HeaderSearchOptions(StringRef _Sysroot = "/")
       : Sysroot(_Sysroot), ModuleFormat("raw"), DisableModuleHash(false),
         ImplicitModuleMaps(false), ModuleMapFileHomeIsCwd(false),
         UseBuiltinIncludes(true), UseStandardSystemIncludes(true),
         UseStandardCXXIncludes(true), UseLibcxx(false), Verbose(false),
         ModulesValidateOncePerBuildSession(false),
         ModulesValidateSystemHeaders(false), UseDebugInfo(false),
-        ModulesValidateDiagnosticOptions(true), ModulesHashContent(false) {}
+        ModulesValidateDiagnosticOptions(true), ModulesHashContent(false),
+        DisableModuleDirectory(true) {}
 
   /// AddPath - Add the \p Path path to the specified \p Group list.
   void AddPath(StringRef Path, frontend::IncludeDirGroup Group,
                bool IsFramework, bool IgnoreSysRoot) {
     UserEntries.emplace_back(Path, Group, IsFramework, IgnoreSysRoot);
   }
 
   /// AddSystemHeaderPrefix - Override whether \#include directives naming a

lib/Frontend/CompilerInvocation.cpp

for (const auto *A : Args.filtered(OPT_fmodule_file)) {
     StringRef Val = A->getValue();
     if (Val.find('=') != StringRef::npos)
       Opts.PrebuiltModuleFiles.insert(Val.split('='));
   }
   for (const auto *A : Args.filtered(OPT_fprebuilt_module_path))
     Opts.AddPrebuiltModulePath(A->getValue());
   Opts.DisableModuleHash = Args.hasArg(OPT_fdisable_module_hash);
   Opts.ModulesHashContent = Args.hasArg(OPT_fmodules_hash_content);
+  Opts.DisableModuleDirectory = Args.hasArg(OPT_fdisable_module_directory);
   Opts.ModulesValidateDiagnosticOptions =
       !Args.hasArg(OPT_fmodules_disable_diagnostic_validation);
   Opts.ImplicitModuleMaps = Args.hasArg(OPT_fimplicit_module_maps);
   Opts.ModuleMapFileHomeIsCwd = Args.hasArg(OPT_fmodule_map_file_home_is_cwd);
   Opts.ModuleCachePruneInterval =
       getLastArgIntValue(Args, OPT_fmodules_prune_interval, 7 * 24 * 60 * 60);
   Opts.ModuleCachePruneAfter =
       getLastArgIntValue(Args, OPT_fmodules_prune_after, 31 * 24 * 60 * 60);

lib/Serialization/ASTWriter.cpp

#undef BLOCK
   Stream.ExitBlock();
 }
 
 /// Prepares a path for being written to an AST file by converting it
 /// to an absolute path and removing nested './'s.
 ///
 /// \return \c true if the path was changed.
 static bool cleanPathForOutput(FileManager &FileMgr,
-                               SmallVectorImpl<char> &Path) {
-  bool Changed = FileMgr.makeAbsolutePath(Path);
-  return Changed | llvm::sys::path::remove_dots(Path);
+                               SmallVectorImpl<char> &Path,
+                               bool MakeAbsolute = true) {
+  bool Changed = false;
+  if (MakeAbsolute) {
+    Changed |= FileMgr.makeAbsolutePath(Path);
+  }
+  Changed |= llvm::sys::path::remove_dots(Path);
+  return Changed;
 }
 
 /// Adjusts the given filename to only write out the portion of the
 /// filename that is not part of the system root directory.
 ///
 /// \param Filename the file name to adjust.
 ///
 /// \param BaseDir When non-NULL, the PCH file is a relocatable AST file and
if (WritingModule) {
     auto Abbrev = std::make_shared<BitCodeAbbrev>();
     Abbrev->Add(BitCodeAbbrevOp(MODULE_NAME));
     Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // Name
     unsigned AbbrevCode = Stream.EmitAbbrev(std::move(Abbrev));
     RecordData::value_type Record[] = {MODULE_NAME};
     Stream.EmitRecordWithBlob(AbbrevCode, Record, WritingModule->Name);
   }
 
-  if (WritingModule && WritingModule->Directory) {
+  if (WritingModule &&
+      WritingModule->Directory &&
+      !PP.getHeaderSearchInfo()
+          .getHeaderSearchOpts()
+          .DisableModuleDirectory) {
     SmallString<128> BaseDir(WritingModule->Directory->getName());
     cleanPathForOutput(Context.getSourceManager().getFileManager(), BaseDir);
 
     // If the home of the module is the current working directory, then we
     // want to pick up the cwd of the build process loading the module, not
     // our cwd, when we load this module.
     if (!PP.getHeaderSearchInfo()
              .getHeaderSearchOpts()
while (!Q.empty()) {
     for (const auto &R : Mod->Requirements) {
       RecordData::value_type Record[] = {SUBMODULE_REQUIRES, R.second};
       Stream.EmitRecordWithBlob(RequiresAbbrev, Record, R.first);
     }
 
     // Emit the umbrella header, if there is one.
     if (auto UmbrellaHeader = Mod->getUmbrellaHeader()) {
       RecordData::value_type Record[] = {SUBMODULE_UMBRELLA_HEADER};
-      Stream.EmitRecordWithBlob(UmbrellaAbbrev, Record,
-                                UmbrellaHeader.NameAsWritten);
+      SmallString<128> Buffer;
+      if (PP->getHeaderSearchInfo()
+              .getHeaderSearchOpts()
+              .DisableModuleDirectory) {
+        llvm::sys::path::append(Buffer, Mod->Directory->getName());
+      }
+      llvm::sys::path::append(Buffer, UmbrellaHeader.NameAsWritten);
+      llvm::sys::path::remove_dots(Buffer);
+      Stream.EmitRecordWithBlob(UmbrellaAbbrev, Record, Buffer);
     } else if (auto UmbrellaDir = Mod->getUmbrellaDir()) {
       RecordData::value_type Record[] = {SUBMODULE_UMBRELLA_DIR};
-      Stream.EmitRecordWithBlob(UmbrellaDirAbbrev, Record,
-                                UmbrellaDir.NameAsWritten);
+      SmallString<128> Buffer;
+      if (PP->getHeaderSearchInfo()
+              .getHeaderSearchOpts()
+              .DisableModuleDirectory) {
+        llvm::sys::path::append(Buffer, Mod->Directory->getName());
+      }
+      llvm::sys::path::append(Buffer, UmbrellaDir.NameAsWritten);
+      llvm::sys::path::remove_dots(Buffer);
+      Stream.EmitRecordWithBlob(UmbrellaDirAbbrev, Record, Buffer);
     }
 
     // Emit the headers.
     struct {
       unsigned RecordKind;
       unsigned Abbrev;
       Module::HeaderKind HeaderKind;
     } HeaderLists[] = {
void ASTWriter::AddToken(const Token &Tok, RecordDataImpl &Record) {
   Record.push_back(Tok.getFlags());
 }
 
 void ASTWriter::AddString(StringRef Str, RecordDataImpl &Record) {
   Record.push_back(Str.size());
   Record.insert(Record.end(), Str.begin(), Str.end());
 }
 
 bool ASTWriter::PreparePathForOutput(SmallVectorImpl<char> &Path) {
   assert(Context && "should have context when outputting path");
 
   bool Changed =
-      cleanPathForOutput(Context->getSourceManager().getFileManager(), Path);
+      cleanPathForOutput(Context->getSourceManager().getFileManager(),
+                         Path,
+                         !PP->getHeaderSearchInfo()
+                             .getHeaderSearchOpts()
+                             .DisableModuleDirectory);
 
   // Remove a prefix to make the path relative, if relevant.
   const char *PathBegin = Path.data();
   const char *PathPtr =
       adjustFilenameForRelocatableAST(PathBegin, BaseDirectory);
   if (PathPtr != PathBegin) {
     Path.erase(Path.begin(), Path.begin() + (PathPtr - PathBegin));
     Changed = true;
   }
 
   return Changed;
 }
 
 void ASTWriter::AddPath(StringRef Path, RecordDataImpl &Record) {
   SmallString<128> FilePath(Path);
   PreparePathForOutput(FilePath);
   AddString(FilePath, Record);
 }
 
 void ASTWriter::EmitRecordWithPath(unsigned Abbrev, RecordDataRef Record,
                                    StringRef Path) {

test/Modules/relocatable-modules.modulemap

+// Build two otherwise identical modules in two different directories and
+// verify that using `-fdisable-module-directory` makes them identical.
+//
+// RUN: rm -rf %t
+//
+// RUN: mkdir -p %t/p1
+// RUN: cd %t/p1
+// RUN: mkdir -p main other
+// RUN: grep "<AM>" %s > main/a.modulemap
+// RUN: grep "<AH>" %s > main/a.h
+// RUN: grep "<BH>" %s > other/b.h
+// RUN: %clang_cc1 -x c++ -fmodules -emit-module -fdisable-module-directory \
+// RUN:   -fmodule-name="a" -Imain -I. -o - main/a.modulemap > a.pcm
+//
+// RUN: mkdir -p %t/p2
+// RUN: cd %t/p2
+// RUN: mkdir -p main other
+// RUN: grep "<AM>" %s > main/a.modulemap
+// RUN: grep "<AH>" %s > main/a.h
+// RUN: grep "<BH>" %s > other/b.h
+// RUN: %clang_cc1 -x c++ -fmodules -emit-module -fdisable-module-directory \
+// RUN:   -fmodule-name="a" -Imain -I. -o - main/a.modulemap > a.pcm
+//
+// RUN: diff %t/p1/a.pcm %t/p2/a.pcm
+
+module "a" {                // <AM>
+}                           // <AM>
+
+#include "b.h"              // <AH>