[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 <https://reviews.llvm.org/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<bool>(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