[modules] Accept equivalent module caches from different symlink

[ellishg]

Use VFS.equivalent(), which follows symlinks, to check if two module cache paths are equivalent. This prevents a PCH error when building from a different path that is a symlink of the original.

error: PCH was compiled with module cache path '/home/foo/blah/ModuleCache/2IBP1TNT8OR8D', but the path is currently '/data/users/foo/blah/ModuleCache/2IBP1TNT8OR8D'
1 error generated.

[llvmbot]

@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Ellis Hoag (ellishg)

Changes

Use fs::equivalent(), which follows symlinks, to check if two module cache paths are equivalent. This prevents a PCH error when building from a different path that is a symlink of the original.

error: PCH was compiled with module cache path '/home/foo/blah/ModuleCache/2IBP1TNT8OR8D', but the path is currently '/data/users/foo/blah/ModuleCache/2IBP1TNT8OR8D'
1 error generated.

---

Full diff: https://github.com/llvm/llvm-project/pull/90925.diff

2 Files Affected:

• (modified) clang/lib/Serialization/ASTReader.cpp (+9-11)
• (added) clang/test/Modules/module-symlink.m (+11)

diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 0ef57a3ea804ef..c20ead8b865692 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -839,17 +839,15 @@ static bool checkHeaderSearchOptions(const HeaderSearchOptions &HSOpts,
                                      DiagnosticsEngine *Diags,
                                      const LangOptions &LangOpts,
                                      const PreprocessorOptions &PPOpts) {
-  if (LangOpts.Modules) {
-    if (SpecificModuleCachePath != ExistingModuleCachePath &&
-        !PPOpts.AllowPCHWithDifferentModulesCachePath) {
-      if (Diags)
-        Diags->Report(diag::err_pch_modulecache_mismatch)
-          << SpecificModuleCachePath << ExistingModuleCachePath;
-      return true;
-    }
-  }
-
-  return false;
+  if (!LangOpts.Modules || PPOpts.AllowPCHWithDifferentModulesCachePath ||
+      SpecificModuleCachePath == ExistingModuleCachePath ||
+      llvm::sys::fs::equivalent(SpecificModuleCachePath,
+                                ExistingModuleCachePath))
+    return false;
+  if (Diags)
+    Diags->Report(diag::err_pch_modulecache_mismatch)
+        << SpecificModuleCachePath << ExistingModuleCachePath;
+  return true;
 }
 
 bool PCHValidator::ReadHeaderSearchOptions(const HeaderSearchOptions &HSOpts,
diff --git a/clang/test/Modules/module-symlink.m b/clang/test/Modules/module-symlink.m
new file mode 100644
index 00000000000000..be447449a0e81e
--- /dev/null
+++ b/clang/test/Modules/module-symlink.m
@@ -0,0 +1,11 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules-cache-path=%t/modules -fmodules -fimplicit-module-maps -I %S/Inputs -emit-pch -o %t.pch %s -verify
+
+// RUN: ln -s %t/modules %t/modules.symlink
+// RUN: %clang_cc1 -fmodules-cache-path=%t/modules.symlink -fmodules -fimplicit-module-maps -I %S/Inputs -include-pch %t.pch %s -verify
+
+// expected-no-diagnostics
+
+@import ignored_macros;
+
+struct Point p;

[benlangmuir]

This should go through the VirtualFileSystem to be consistent with the code that reads module files. The callers of this API should both have easy access to a VFS to pass in: for SimplePCHValidator it's FileMgr.getVirtualFileSystem() and for PCHValidator it's Reader.getFileManager().getVirtualFileSystem().

You could also add a helper method to VirtualFileSystem for equivalent that does status(Path1).equivalent(status(Path2)) (plus error handling).

[ellishg]

Thanks for the advice. I realized that this does not handle the case where both paths are equal, but do not exist. In that case, no error will be thrown. Is that acceptable? I guess this was the case before this pr.

[benlangmuir]

That's fine, it will fail when it tries to read anything from the cache.

[ellishg]

@benlangmuir do you have other concerns? Can I get a stamp?

[benlangmuir]

LGTM if we rename the checkHeaderSearchOptions function.

[benlangmuir]

I realize you're not changing the behaviour here, but now that we don't even pass in a HeaderSearchOptions we should probably rename this. Maybe checkModuleCachePath?