[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)

2024-05-07 Thread Ellis Hoag via cfe-commits 

https://github.com/ellishg closed 
https://github.com/llvm/llvm-project/pull/90925
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)

2024-05-07 Thread Ellis Hoag via cfe-commits 

https://github.com/ellishg edited 
https://github.com/llvm/llvm-project/pull/90925
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)

2024-05-07 Thread Ellis Hoag via cfe-commits 

https://github.com/ellishg updated 
https://github.com/llvm/llvm-project/pull/90925

>From 4760ebce0ff7725f4bb75f5107f551d867e4db6d Mon Sep 17 00:00:00 2001
From: Ellis Hoag 
Date: Thu, 2 May 2024 17:47:38 -0700
Subject: [PATCH 1/4] [modules] Accept equivalent module caches from different
 symlink

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.
```
---
 clang/lib/Serialization/ASTReader.cpp | 20 +---
 clang/test/Modules/module-symlink.m   | 11 +++
 2 files changed, 20 insertions(+), 11 deletions(-)
 create mode 100644 clang/test/Modules/module-symlink.m

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 ,
  DiagnosticsEngine *Diags,
  const LangOptions ,
  const PreprocessorOptions ) {
-  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 ,
diff --git a/clang/test/Modules/module-symlink.m 
b/clang/test/Modules/module-symlink.m
new file mode 100644
index 00..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;

>From 490eefe98e3dd020ff3e51c7f817ec2b3d3a2663 Mon Sep 17 00:00:00 2001
From: Ellis Hoag 
Date: Fri, 3 May 2024 09:50:11 -0700
Subject: [PATCH 2/4] Require shell to fix windows test

---
 clang/test/Modules/module-symlink.m | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/clang/test/Modules/module-symlink.m 
b/clang/test/Modules/module-symlink.m
index be447449a0e81e..9a69186c5ea28f 100644
--- a/clang/test/Modules/module-symlink.m
+++ b/clang/test/Modules/module-symlink.m
@@ -1,3 +1,5 @@
+// REQUIRES: shell
+
 // 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
 

>From 6e58177107f854f42d3cdc70e796c425a1797798 Mon Sep 17 00:00:00 2001
From: Ellis Hoag 
Date: Fri, 3 May 2024 10:34:35 -0700
Subject: [PATCH 3/4] Use VFS to check if files are equal

---
 clang/lib/Serialization/ASTReader.cpp | 25 +++
 clang/test/Modules/module-symlink.m   |  1 +
 llvm/include/llvm/Support/VirtualFileSystem.h |  4 +++
 llvm/lib/Support/VirtualFileSystem.cpp| 10 
 4 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/clang/lib/Serialization/ASTReader.cpp 
b/clang/lib/Serialization/ASTReader.cpp
index c20ead8b865692..d35c870926f96e 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -833,16 +833,18 @@ bool SimpleASTReaderListener::ReadPreprocessorOptions(
 /// against the header search options in an existing preprocessor.
 ///
 /// \param Diags If non-null, produce diagnostics for any mismatches incurred.
-static bool checkHeaderSearchOptions(const HeaderSearchOptions ,
+static bool checkHeaderSearchOptions(llvm::vfs::FileSystem ,
  StringRef SpecificModuleCachePath,
  StringRef ExistingModuleCachePath,
  DiagnosticsEngine *Diags,
  const LangOptions ,
  const PreprocessorOptions ) {
   if

[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)

2024-05-07 Thread Matt Arsenault via cfe-commits 

https://github.com/arsenm updated 
https://github.com/llvm/llvm-project/pull/90925

>From 4760ebce0ff7725f4bb75f5107f551d867e4db6d Mon Sep 17 00:00:00 2001
From: Ellis Hoag 
Date: Thu, 2 May 2024 17:47:38 -0700
Subject: [PATCH 1/4] [modules] Accept equivalent module caches from different
 symlink

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.
```
---
 clang/lib/Serialization/ASTReader.cpp | 20 +---
 clang/test/Modules/module-symlink.m   | 11 +++
 2 files changed, 20 insertions(+), 11 deletions(-)
 create mode 100644 clang/test/Modules/module-symlink.m

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 ,
  DiagnosticsEngine *Diags,
  const LangOptions ,
  const PreprocessorOptions ) {
-  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 ,
diff --git a/clang/test/Modules/module-symlink.m 
b/clang/test/Modules/module-symlink.m
new file mode 100644
index 00..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;

>From 490eefe98e3dd020ff3e51c7f817ec2b3d3a2663 Mon Sep 17 00:00:00 2001
From: Ellis Hoag 
Date: Fri, 3 May 2024 09:50:11 -0700
Subject: [PATCH 2/4] Require shell to fix windows test

---
 clang/test/Modules/module-symlink.m | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/clang/test/Modules/module-symlink.m 
b/clang/test/Modules/module-symlink.m
index be447449a0e81e..9a69186c5ea28f 100644
--- a/clang/test/Modules/module-symlink.m
+++ b/clang/test/Modules/module-symlink.m
@@ -1,3 +1,5 @@
+// REQUIRES: shell
+
 // 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
 

>From 6e58177107f854f42d3cdc70e796c425a1797798 Mon Sep 17 00:00:00 2001
From: Ellis Hoag 
Date: Fri, 3 May 2024 10:34:35 -0700
Subject: [PATCH 3/4] Use VFS to check if files are equal

---
 clang/lib/Serialization/ASTReader.cpp | 25 +++
 clang/test/Modules/module-symlink.m   |  1 +
 llvm/include/llvm/Support/VirtualFileSystem.h |  4 +++
 llvm/lib/Support/VirtualFileSystem.cpp| 10 
 4 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/clang/lib/Serialization/ASTReader.cpp 
b/clang/lib/Serialization/ASTReader.cpp
index c20ead8b865692..d35c870926f96e 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -833,16 +833,18 @@ bool SimpleASTReaderListener::ReadPreprocessorOptions(
 /// against the header search options in an existing preprocessor.
 ///
 /// \param Diags If non-null, produce diagnostics for any mismatches incurred.
-static bool checkHeaderSearchOptions(const HeaderSearchOptions ,
+static bool checkHeaderSearchOptions(llvm::vfs::FileSystem ,
  StringRef SpecificModuleCachePath,
  StringRef ExistingModuleCachePath,
  DiagnosticsEngine *Diags,
  const LangOptions ,
  const PreprocessorOptions ) {
   if

[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)

2024-05-07 Thread Ben Langmuir via cfe-commits 

https://github.com/benlangmuir edited 
https://github.com/llvm/llvm-project/pull/90925
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)

2024-05-07 Thread Ben Langmuir via cfe-commits 

https://github.com/benlangmuir approved this pull request.

LGTM if we rename the checkHeaderSearchOptions function.

https://github.com/llvm/llvm-project/pull/90925
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)

2024-05-07 Thread Ben Langmuir via cfe-commits 


@@ -833,32 +833,33 @@ bool SimpleASTReaderListener::ReadPreprocessorOptions(
 /// against the header search options in an existing preprocessor.
 ///
 /// \param Diags If non-null, produce diagnostics for any mismatches incurred.
-static bool checkHeaderSearchOptions(const HeaderSearchOptions ,
+static bool checkHeaderSearchOptions(llvm::vfs::FileSystem ,

benlangmuir wrote:

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`?

https://github.com/llvm/llvm-project/pull/90925
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)

2024-05-07 Thread Ellis Hoag via cfe-commits 

ellishg wrote:

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

https://github.com/llvm/llvm-project/pull/90925
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)

2024-05-03 Thread Ben Langmuir via cfe-commits 


@@ -839,17 +839,15 @@ static bool checkHeaderSearchOptions(const 
HeaderSearchOptions ,
  DiagnosticsEngine *Diags,
  const LangOptions ,
  const PreprocessorOptions ) {
-  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,

benlangmuir wrote:

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

https://github.com/llvm/llvm-project/pull/90925
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)

2024-05-03 Thread Ellis Hoag via cfe-commits 


@@ -839,17 +839,15 @@ static bool checkHeaderSearchOptions(const 
HeaderSearchOptions ,
  DiagnosticsEngine *Diags,
  const LangOptions ,
  const PreprocessorOptions ) {
-  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,

ellishg wrote:

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.

https://github.com/llvm/llvm-project/pull/90925
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [modules] Accept equivalent module caches from different symlink (PR #90925)

2024-05-03 Thread Ellis Hoag via cfe-commits 

https://github.com/ellishg updated 
https://github.com/llvm/llvm-project/pull/90925

>From 4760ebce0ff7725f4bb75f5107f551d867e4db6d Mon Sep 17 00:00:00 2001
From: Ellis Hoag 
Date: Thu, 2 May 2024 17:47:38 -0700
Subject: [PATCH 1/3] [modules] Accept equivalent module caches from different
 symlink

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.
```
---
 clang/lib/Serialization/ASTReader.cpp | 20 +---
 clang/test/Modules/module-symlink.m   | 11 +++
 2 files changed, 20 insertions(+), 11 deletions(-)
 create mode 100644 clang/test/Modules/module-symlink.m

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 ,
  DiagnosticsEngine *Diags,
  const LangOptions ,
  const PreprocessorOptions ) {
-  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 ,
diff --git a/clang/test/Modules/module-symlink.m 
b/clang/test/Modules/module-symlink.m
new file mode 100644
index 00..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;

>From 490eefe98e3dd020ff3e51c7f817ec2b3d3a2663 Mon Sep 17 00:00:00 2001
From: Ellis Hoag 
Date: Fri, 3 May 2024 09:50:11 -0700
Subject: [PATCH 2/3] Require shell to fix windows test

---
 clang/test/Modules/module-symlink.m | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/clang/test/Modules/module-symlink.m 
b/clang/test/Modules/module-symlink.m
index be447449a0e81e..9a69186c5ea28f 100644
--- a/clang/test/Modules/module-symlink.m
+++ b/clang/test/Modules/module-symlink.m
@@ -1,3 +1,5 @@
+// REQUIRES: shell
+
 // 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
 

>From 6e58177107f854f42d3cdc70e796c425a1797798 Mon Sep 17 00:00:00 2001
From: Ellis Hoag 
Date: Fri, 3 May 2024 10:34:35 -0700
Subject: [PATCH 3/3] Use VFS to check if files are equal

---
 clang/lib/Serialization/ASTReader.cpp | 25 +++
 clang/test/Modules/module-symlink.m   |  1 +
 llvm/include/llvm/Support/VirtualFileSystem.h |  4 +++
 llvm/lib/Support/VirtualFileSystem.cpp| 10 
 4 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/clang/lib/Serialization/ASTReader.cpp 
b/clang/lib/Serialization/ASTReader.cpp
index c20ead8b865692..d35c870926f96e 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -833,16 +833,18 @@ bool SimpleASTReaderListener::ReadPreprocessorOptions(
 /// against the header search options in an existing preprocessor.
 ///
 /// \param Diags If non-null, produce diagnostics for any mismatches incurred.
-static bool checkHeaderSearchOptions(const HeaderSearchOptions ,
+static bool checkHeaderSearchOptions(llvm::vfs::FileSystem ,
  StringRef SpecificModuleCachePath,
  StringRef ExistingModuleCachePath,
  DiagnosticsEngine *Diags,
  const LangOptions ,
  const PreprocessorOptions ) {
   if