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

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

rsmith added a comment.

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.)




Comment at: lib/Serialization/ASTReader.cpp:2094-2096
+  bool IsRelativeModuleDirectory = static_cast(Record[6]);
   R.Filename = Blob;
+  if (IsRelativeModuleDirectory) {

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D51568



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

[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 ,
+ bool ) {
   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



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

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

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D51568



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

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

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

rsmith 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.

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.)




Comment at: lib/Serialization/ASTWriter.cpp:4524-4546
+bool ASTWriter::PreparePathForOutput(SmallVectorImpl ,
+ bool ) {
   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));

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?


Repository:
  rC Clang

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

https://reviews.llvm.org/D51568



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

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

[Manman Ren via Phabricator via cfe-commits]

manmanren added a comment.

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

Manman




Comment at: include/clang/Serialization/ASTReader.h:2241
 SkipString(Record, Idx);
+Idx++;  // Relative
   }

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



Comment at: include/clang/Serialization/ASTWriter.h:642
   /// Convert a path from this build process into one that is appropriate
   /// for emission in the module file.
+  bool PreparePathForOutput(SmallVectorImpl ,

Can you add comment here on how we set IsRelativeModuleDirectory?



Comment at: include/clang/Serialization/ASTWriter.h:650
   /// Emit the current record with the given path as a blob.
-  void EmitRecordWithPath(unsigned Abbrev, RecordDataRef Record,
+  void EmitRecordWithPath(unsigned Abbrev, RecordDataImpl ,
   StringRef Path);

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D51568



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

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

[Brian Gesiak via Phabricator via cfe-commits]

modocache added subscribers: manmanren, bruno, modocache.
modocache added reviewers: manmanren, bruno.
modocache added a comment.
Herald added a subscriber: arphaman.

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?


Repository:
  rC Clang

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

https://reviews.llvm.org/D51568



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

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

[Andrew Gallagher via Phabricator via cfe-commits]

andrewjcg updated this revision to Diff 166709.
andrewjcg added a comment.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D51568

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Lex/HeaderSearchOptions.h
  include/clang/Serialization/ASTReader.h
  include/clang/Serialization/ASTWriter.h
  lib/Frontend/CompilerInvocation.cpp
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTReaderInternals.h
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/GlobalModuleIndex.cpp
  test/Modules/relocatable-modules.modulemap

Index: test/Modules/relocatable-modules.modulemap
===
--- /dev/null
+++ test/Modules/relocatable-modules.modulemap
@@ -0,0 +1,29 @@
+// Build two otherwise identical modules in two different directories and
+// verify that using `-fno-absolute-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 "" %s > main/a.modulemap
+// RUN: grep "" %s > main/a.h
+// RUN: grep "" %s > other/b.h
+// RUN: %clang_cc1 -x c++ -fmodules -emit-module -fno-absolute-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 "" %s > main/a.modulemap
+// RUN: grep "" %s > main/a.h
+// RUN: grep "" %s > other/b.h
+// RUN: %clang_cc1 -x c++ -fmodules -emit-module -fno-absolute-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" {// 
+}   // 
+
+#include "b.h"  // 
Index: lib/Serialization/GlobalModuleIndex.cpp
===
--- lib/Serialization/GlobalModuleIndex.cpp
+++ lib/Serialization/GlobalModuleIndex.cpp
@@ -628,6 +628,7 @@
 SmallString<128> ImportedFile(Record.begin() + Idx,
   Record.begin() + Idx + Length);
 Idx += Length;
+Idx++;  // Relative
 
 // Find the imported module file.
 const FileEntry *DependsOnFile
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -1327,9 +1327,14 @@
 ///
 /// \return \c true if the path was changed.
 static bool cleanPathForOutput(FileManager ,
-   SmallVectorImpl ) {
-  bool Changed = FileMgr.makeAbsolutePath(Path);
-  return Changed | llvm::sys::path::remove_dots(Path);
+   SmallVectorImpl ,
+   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
@@ -1493,7 +1498,10 @@
 
   if (WritingModule && WritingModule->Directory) {
 SmallString<128> BaseDir(WritingModule->Directory->getName());
-cleanPathForOutput(Context.getSourceManager().getFileManager(), BaseDir);
+cleanPathForOutput(Context.getSourceManager().getFileManager(), BaseDir,
+   !PP.getHeaderSearchInfo()
+.getHeaderSearchOpts()
+.NoAbsoluteModuleDirectory);
 
 // 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
@@ -1708,6 +1716,7 @@
 auto FileAbbrev = std::make_shared();
 FileAbbrev->Add(BitCodeAbbrevOp(ORIGINAL_FILE));
 FileAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // File ID
+FileAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Relative
 FileAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // File name
 unsigned FileAbbrevCode = Stream.EmitAbbrev(std::move(FileAbbrev));
 
@@ -1772,6 +1781,7 @@
   IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Overridden
   IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Transient
   IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Module map
+  IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // Relative
   IFAbbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // File name
   unsigned IFAbbrevCode =