Re: [PATCH] D19478: Remove assert mandating you can only use SPIR target with OpenCL

[Neil Henning via cfe-commits]

sheredom closed this revision.
sheredom added a comment.

Thanks!


http://reviews.llvm.org/D19478



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

[PATCH] D19478: Remove assert mandating you can only use SPIR target with OpenCL

[Neil Henning via cfe-commits]

sheredom created this revision.
sheredom added reviewers: Anastasia, yaxunl, pxli168.
sheredom added a subscriber: cfe-commits.

Remove an assert mandating that OpenCL must be used with the SPIR target. We 
need to be able to use the SPIR target with non-OpenCL inputs, which worked 
perfectly well until commit 264241 introduced an assert that requires OpenCL be 
set in LangOpts.

http://reviews.llvm.org/D19478

Files:
  lib/CodeGen/TargetInfo.cpp

Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -7390,7 +7390,6 @@
 /// Emit SPIR specific metadata: OpenCL and SPIR version.
 void SPIRTargetCodeGenInfo::emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
  CodeGen::CodeGenModule &CGM) const {
-  assert(CGM.getLangOpts().OpenCL && "SPIR is only for OpenCL");
   llvm::LLVMContext &Ctx = CGM.getModule().getContext();
   llvm::Type *Int32Ty = llvm::Type::getInt32Ty(Ctx);
   llvm::Module &M = CGM.getModule();


Index: lib/CodeGen/TargetInfo.cpp
===
--- lib/CodeGen/TargetInfo.cpp
+++ lib/CodeGen/TargetInfo.cpp
@@ -7390,7 +7390,6 @@
 /// Emit SPIR specific metadata: OpenCL and SPIR version.
 void SPIRTargetCodeGenInfo::emitTargetMD(const Decl *D, llvm::GlobalValue *GV,
  CodeGen::CodeGenModule &CGM) const {
-  assert(CGM.getLangOpts().OpenCL && "SPIR is only for OpenCL");
   llvm::LLVMContext &Ctx = CGM.getModule().getContext();
   llvm::Type *Int32Ty = llvm::Type::getInt32Ty(Ctx);
   llvm::Module &M = CGM.getModule();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19478: Remove assert mandating you can only use SPIR target with OpenCL

[Neil Henning via cfe-commits]

sheredom added a comment.

So we build a bunch of internal libraries in a mix of OpenCL and C++, and then 
link them all together to create SPIR libraries that can be fed to calls to 
clLinkProgram and linked against user kernels.

I don't have commit writes to Clang - if the patch is ok to go could one of you 
bring the patch in for me?


http://reviews.llvm.org/D19478



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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

https://github.com/sheredom updated 
https://github.com/llvm/llvm-project/pull/106577

>From 2d73b6c8463a36ec087fe66c2a48c3ae8fe5b05e Mon Sep 17 00:00:00 2001
From: Neil Henning 
Date: Thu, 29 Aug 2024 13:15:49 +0100
Subject: [PATCH] Make PCH's respect any VFS specified.

We want to be able to generate a PCH against one file-system path, and
then re-use that PCH when the file-system path is different (but the
sources are the same). We also do not know when generating the PCH what
the destination file-system path will be, so what we want to be able to
do is:

- When generating a PCH map the original directory to some fake
  directory. You could imagine `D:/Foo` being mapped to `Z:/Foo` for
  instance.
- Then when consuming a PCH, we want to be able to use a different
  mapping to map `Z:/Foo` to `D:/Some/Other/Machines/Foo` for instance.

This will let us generate and share PCHs to speed up compile time for
our users.

To enable this we've made PCH generation respect any specified
vfsoverlay, such that it will remap the paths in the PCH accordingly.

We've also made `-verify-pch` respect the
`-fvalidate-ast-input-files-content` option so that we can force
verification of inputs.
---
 clang/lib/Frontend/FrontendActions.cpp|  3 ++-
 clang/lib/Serialization/ASTWriter.cpp | 22 ++--
 clang/test/PCH/verify_no_timestamp.c  |  8 ++
 .../Inputs/vfsoverlay-directory-remap.yaml|  9 +++
 clang/test/VFS/remap-to-fake.c| 26 +++
 5 files changed, 65 insertions(+), 3 deletions(-)
 create mode 100644 clang/test/PCH/verify_no_timestamp.c
 create mode 100644 clang/test/VFS/Inputs/vfsoverlay-directory-remap.yaml
 create mode 100644 clang/test/VFS/remap-to-fake.c

diff --git a/clang/lib/Frontend/FrontendActions.cpp 
b/clang/lib/Frontend/FrontendActions.cpp
index 9f5d09e33ce244..9a60aaf880e96b 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -347,7 +347,8 @@ void VerifyPCHAction::ExecuteAction() {
   DisableValidationForModuleKind::None,
   /*AllowASTWithCompilerErrors*/ false,
   /*AllowConfigurationMismatch*/ true,
-  /*ValidateSystemInputs*/ true));
+  /*ValidateSystemInputs*/ true,
+  CI.getHeaderSearchOpts().ValidateASTInputFilesContent));
 
   Reader->ReadAST(getCurrentFile(),
   Preamble ? serialization::MK_Preamble
diff --git a/clang/lib/Serialization/ASTWriter.cpp 
b/clang/lib/Serialization/ASTWriter.cpp
index 5cfb98c2a1060a..bfbc2aae0aa61b 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -1115,13 +1115,13 @@ void ASTWriter::WriteBlockInfoBlock() {
 }
 
 /// Prepares a path for being written to an AST file by converting it
-/// to an absolute path and removing nested './'s.
+/// to an absolute path and removing nested './'s and '../'s.
 ///
 /// \return \c true if the path was changed.
 static bool cleanPathForOutput(FileManager &FileMgr,
SmallVectorImpl &Path) {
   bool Changed = FileMgr.makeAbsolutePath(Path);
-  return Changed | llvm::sys::path::remove_dots(Path);
+  return Changed | llvm::sys::path::remove_dots(Path, true);
 }
 
 /// Adjusts the given filename to only write out the portion of the
@@ -4772,6 +4772,24 @@ bool 
ASTWriter::PreparePathForOutput(SmallVectorImpl &Path) {
 Changed = true;
   }
 
+  // If we are generating a normal PCH (EG. not a C++ module).
+  if (!WritingModule) {
+// Use the vfs overlay if it exists to translate paths.
+auto &FileSys =
+Context->getSourceManager().getFileManager().getVirtualFileSystem();
+if (auto *RFS = dyn_cast(&FileSys)) {
+  if (auto Result = RFS->lookupPath(StringRef(Path.data(), Path.size( {
+if (std::optional Redirect = Result->getExternalRedirect()) 
{
+  const char *data = Redirect->data();
+  const size_t size = Redirect->size();
+  Path.resize(size);
+  Path.assign(data, data + size);
+  Changed = true;
+}
+  }
+}
+  }
+
   return Changed;
 }
 
diff --git a/clang/test/PCH/verify_no_timestamp.c 
b/clang/test/PCH/verify_no_timestamp.c
new file mode 100644
index 00..8aca76cf77c449
--- /dev/null
+++ b/clang/test/PCH/verify_no_timestamp.c
@@ -0,0 +1,8 @@
+// RUN: echo 'int SomeFunc() { return 42; }' > %t.h
+// RUN: %clang_cc1 -Werror -fno-pch-timestamp 
-fvalidate-ast-input-files-content -emit-pch -o "%t.pch" %t.h
+
+// Now change the source file, which should cause the verifier to fail with 
content mismatch.
+// RUN: echo 'int SomeFunc() { return 13; }' > %t.h
+// RUN: not %clang_cc1 -fno-pch-timestamp -fvalidate-ast-input-files-content 
-verify-pch %t.pch 2>&1 | FileCheck %s -DT=%t
+
+// CHECK: fatal error: file '[[T]].h' has been modified since the precompiled 
header '[[T]].pch' was built: content changed
diff --git a/clang/test/VFS/Inputs/vfsoverlay-directory-remap.yaml 
b/clang/test/VFS/Inputs/vfsoverlay

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

https://github.com/sheredom created 
https://github.com/llvm/llvm-project/pull/106577

We want to be able to generate a PCH against one file-system path, and then 
re-use that PCH when the file-system path is different (but the sources are the 
same). We also do not know when generating the PCH what the destination 
file-system path will be, so what we want to be able to do is:

- When generating a PCH map the original directory to some fake directory. You 
could imagine `D:/Foo` being mapped to `Z:/Foo` for instance.
- Then when consuming a PCH, we want to be able to use a different mapping to 
map `Z:/Foo` to `D:/Some/Other/Machines/Foo` for instance.

This will let us generate and share PCHs to speed up compile time for our users.

To enable this we've made PCH generation respect any specified vfsoverlay, such 
that it will remap the paths in the PCH accordingly.

We've also made `-verify-pch` respect the
`-fvalidate-ast-input-files-content` option so that we can force verification 
of inputs.

>From e8e06cef736577eb098de57f8dfae201c945d2af Mon Sep 17 00:00:00 2001
From: Neil Henning 
Date: Thu, 29 Aug 2024 13:15:49 +0100
Subject: [PATCH] Make PCH's respect any VFS specified.

We want to be able to generate a PCH against one file-system path, and
then re-use that PCH when the file-system path is different (but the
sources are the same). We also do not know when generating the PCH what
the destination file-system path will be, so what we want to be able to
do is:

- When generating a PCH map the original directory to some fake
  directory. You could imagine `D:/Foo` being mapped to `Z:/Foo` for
  instance.
- Then when consuming a PCH, we want to be able to use a different
  mapping to map `Z:/Foo` to `D:/Some/Other/Machines/Foo` for instance.

This will let us generate and share PCHs to speed up compile time for
our users.

To enable this we've made PCH generation respect any specified
vfsoverlay, such that it will remap the paths in the PCH accordingly.

We've also made `-verify-pch` respect the
`-fvalidate-ast-input-files-content` option so that we can force
verification of inputs.
---
 clang/lib/Frontend/FrontendActions.cpp|  3 ++-
 clang/lib/Serialization/ASTWriter.cpp | 19 --
 clang/test/PCH/verify_no_timestamp.c  |  8 ++
 .../Inputs/vfsoverlay-directory-remap.yaml|  9 +++
 clang/test/VFS/remap-to-fake.c| 26 +++
 5 files changed, 62 insertions(+), 3 deletions(-)
 create mode 100644 clang/test/PCH/verify_no_timestamp.c
 create mode 100644 clang/test/VFS/Inputs/vfsoverlay-directory-remap.yaml
 create mode 100644 clang/test/VFS/remap-to-fake.c

diff --git a/clang/lib/Frontend/FrontendActions.cpp 
b/clang/lib/Frontend/FrontendActions.cpp
index 9f5d09e33ce244..9a60aaf880e96b 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -347,7 +347,8 @@ void VerifyPCHAction::ExecuteAction() {
   DisableValidationForModuleKind::None,
   /*AllowASTWithCompilerErrors*/ false,
   /*AllowConfigurationMismatch*/ true,
-  /*ValidateSystemInputs*/ true));
+  /*ValidateSystemInputs*/ true,
+  CI.getHeaderSearchOpts().ValidateASTInputFilesContent));
 
   Reader->ReadAST(getCurrentFile(),
   Preamble ? serialization::MK_Preamble
diff --git a/clang/lib/Serialization/ASTWriter.cpp 
b/clang/lib/Serialization/ASTWriter.cpp
index 5cfb98c2a1060a..94cc1751cd37ec 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -1115,13 +1115,13 @@ void ASTWriter::WriteBlockInfoBlock() {
 }
 
 /// Prepares a path for being written to an AST file by converting it
-/// to an absolute path and removing nested './'s.
+/// to an absolute path and removing nested './'s and '../'s.
 ///
 /// \return \c true if the path was changed.
 static bool cleanPathForOutput(FileManager &FileMgr,
SmallVectorImpl &Path) {
   bool Changed = FileMgr.makeAbsolutePath(Path);
-  return Changed | llvm::sys::path::remove_dots(Path);
+  return Changed | llvm::sys::path::remove_dots(Path, true);
 }
 
 /// Adjusts the given filename to only write out the portion of the
@@ -4772,6 +4772,21 @@ bool 
ASTWriter::PreparePathForOutput(SmallVectorImpl &Path) {
 Changed = true;
   }
 
+  // If we are generating a normal PCH (EG. not a C++ module).
+  if (!WritingModule) {
+// Use the vfs overlay if it exists to translate paths.
+auto& FileSys = 
Context->getSourceManager().getFileManager().getVirtualFileSystem();
+if (auto *RFS = dyn_cast(&FileSys)) {
+  if (auto Result = RFS->lookupPath(PathStr)) {
+if (std::optional Redirect = Result->getExternalRedirect()) 
{
+  PathStr = *Redirect;
+  Path.assign(PathStr.data(), PathStr.data() + PathStr.size());
+  Changed = true;
+}
+  }
+}
+  }
+
   return Changed;
 }
 
diff --git a/clang/test/PCH/verify_no_timestamp.c 
b/clang/test/PCH

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

https://github.com/sheredom updated 
https://github.com/llvm/llvm-project/pull/106577

>From 774ca72fd61c98a830930dab9c60e16fcc6782a9 Mon Sep 17 00:00:00 2001
From: Neil Henning 
Date: Thu, 29 Aug 2024 13:15:49 +0100
Subject: [PATCH] Make PCH's respect any VFS specified.

We want to be able to generate a PCH against one file-system path, and
then re-use that PCH when the file-system path is different (but the
sources are the same). We also do not know when generating the PCH what
the destination file-system path will be, so what we want to be able to
do is:

- When generating a PCH map the original directory to some fake
  directory. You could imagine `D:/Foo` being mapped to `Z:/Foo` for
  instance.
- Then when consuming a PCH, we want to be able to use a different
  mapping to map `Z:/Foo` to `D:/Some/Other/Machines/Foo` for instance.

This will let us generate and share PCHs to speed up compile time for
our users.

To enable this we've made PCH generation respect any specified
vfsoverlay, such that it will remap the paths in the PCH accordingly.

We've also made `-verify-pch` respect the
`-fvalidate-ast-input-files-content` option so that we can force
verification of inputs.
---
 clang/lib/Frontend/FrontendActions.cpp|  3 ++-
 clang/lib/Serialization/ASTWriter.cpp | 21 +--
 clang/test/PCH/verify_no_timestamp.c  |  8 ++
 .../Inputs/vfsoverlay-directory-remap.yaml|  9 +++
 clang/test/VFS/remap-to-fake.c| 26 +++
 5 files changed, 64 insertions(+), 3 deletions(-)
 create mode 100644 clang/test/PCH/verify_no_timestamp.c
 create mode 100644 clang/test/VFS/Inputs/vfsoverlay-directory-remap.yaml
 create mode 100644 clang/test/VFS/remap-to-fake.c

diff --git a/clang/lib/Frontend/FrontendActions.cpp 
b/clang/lib/Frontend/FrontendActions.cpp
index 9f5d09e33ce244..9a60aaf880e96b 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -347,7 +347,8 @@ void VerifyPCHAction::ExecuteAction() {
   DisableValidationForModuleKind::None,
   /*AllowASTWithCompilerErrors*/ false,
   /*AllowConfigurationMismatch*/ true,
-  /*ValidateSystemInputs*/ true));
+  /*ValidateSystemInputs*/ true,
+  CI.getHeaderSearchOpts().ValidateASTInputFilesContent));
 
   Reader->ReadAST(getCurrentFile(),
   Preamble ? serialization::MK_Preamble
diff --git a/clang/lib/Serialization/ASTWriter.cpp 
b/clang/lib/Serialization/ASTWriter.cpp
index 5cfb98c2a1060a..bba060d01338a8 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -1115,13 +1115,13 @@ void ASTWriter::WriteBlockInfoBlock() {
 }
 
 /// Prepares a path for being written to an AST file by converting it
-/// to an absolute path and removing nested './'s.
+/// to an absolute path and removing nested './'s and '../'s.
 ///
 /// \return \c true if the path was changed.
 static bool cleanPathForOutput(FileManager &FileMgr,
SmallVectorImpl &Path) {
   bool Changed = FileMgr.makeAbsolutePath(Path);
-  return Changed | llvm::sys::path::remove_dots(Path);
+  return Changed | llvm::sys::path::remove_dots(Path, true);
 }
 
 /// Adjusts the given filename to only write out the portion of the
@@ -4772,6 +4772,23 @@ bool 
ASTWriter::PreparePathForOutput(SmallVectorImpl &Path) {
 Changed = true;
   }
 
+  // If we are generating a normal PCH (EG. not a C++ module).
+  if (!WritingModule) {
+// Use the vfs overlay if it exists to translate paths.
+auto &FileSys =
+Context->getSourceManager().getFileManager().getVirtualFileSystem();
+
+if (auto *RFS = dyn_cast(&FileSys)) {
+  if (auto Result = RFS->lookupPath(PathStr)) {
+if (std::optional Redirect = Result->getExternalRedirect()) 
{
+  PathStr = *Redirect;
+  Path.assign(PathStr.data(), PathStr.data() + PathStr.size());
+  Changed = true;
+}
+  }
+}
+  }
+
   return Changed;
 }
 
diff --git a/clang/test/PCH/verify_no_timestamp.c 
b/clang/test/PCH/verify_no_timestamp.c
new file mode 100644
index 00..8aca76cf77c449
--- /dev/null
+++ b/clang/test/PCH/verify_no_timestamp.c
@@ -0,0 +1,8 @@
+// RUN: echo 'int SomeFunc() { return 42; }' > %t.h
+// RUN: %clang_cc1 -Werror -fno-pch-timestamp 
-fvalidate-ast-input-files-content -emit-pch -o "%t.pch" %t.h
+
+// Now change the source file, which should cause the verifier to fail with 
content mismatch.
+// RUN: echo 'int SomeFunc() { return 13; }' > %t.h
+// RUN: not %clang_cc1 -fno-pch-timestamp -fvalidate-ast-input-files-content 
-verify-pch %t.pch 2>&1 | FileCheck %s -DT=%t
+
+// CHECK: fatal error: file '[[T]].h' has been modified since the precompiled 
header '[[T]].pch' was built: content changed
diff --git a/clang/test/VFS/Inputs/vfsoverlay-directory-remap.yaml 
b/clang/test/VFS/Inputs/vfsoverlay-directory-remap.yaml
new file mode 100644
index 00..3cb279a08fae14
--- /dev/nul

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

sheredom wrote:

> Instead of using VFS overlays to make the AST file relocatable, have you 
> considered making use of `adjustFilenameForRelocatableAST()` (i.e. storing 
> relative paths to the AST file) and then setting the CWD accordingly when 
> loading?

The problem we've got is that while we've only got a single path being 
relocated in the example, in practice we're actually going to remap multiple of 
these. We'll at least be relocating:

- The location of the checked out unreal engine folder.
- The location of the Visual Studio.
- The location of the Windows SDK.
- Maybe some proprietary platform toolchain stuff too.

And to make this work we'd have to provide different path relocations for all 
of them.

I'm not sure relocatable PCH would work for that case?

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

@@ -4772,6 +4772,23 @@ bool 
ASTWriter::PreparePathForOutput(SmallVectorImpl &Path) {
 Changed = true;
   }
 
+  // If we are generating a normal PCH (EG. not a C++ module).
+  if (!WritingModule) {

sheredom wrote:

Note that without this guard the following tests would fail:

```
Failed Tests (2):
  Clang :: ClangScanDeps/modules-file-name-as-requested.m
  Clang :: ClangScanDeps/modules-implementation-vfs.m
```

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

@@ -1115,13 +1115,13 @@ void ASTWriter::WriteBlockInfoBlock() {
 }
 
 /// Prepares a path for being written to an AST file by converting it
-/// to an absolute path and removing nested './'s.
+/// to an absolute path and removing nested './'s and '../'s.
 ///
 /// \return \c true if the path was changed.
 static bool cleanPathForOutput(FileManager &FileMgr,
SmallVectorImpl &Path) {
   bool Changed = FileMgr.makeAbsolutePath(Path);
-  return Changed | llvm::sys::path::remove_dots(Path);
+  return Changed | llvm::sys::path::remove_dots(Path, true);

sheredom wrote:

Ah good shout missed that!

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

@@ -1115,13 +1115,13 @@ void ASTWriter::WriteBlockInfoBlock() {
 }
 
 /// Prepares a path for being written to an AST file by converting it
-/// to an absolute path and removing nested './'s.
+/// to an absolute path and removing nested './'s and '../'s.
 ///
 /// \return \c true if the path was changed.
 static bool cleanPathForOutput(FileManager &FileMgr,
SmallVectorImpl &Path) {
   bool Changed = FileMgr.makeAbsolutePath(Path);
-  return Changed | llvm::sys::path::remove_dots(Path);
+  return Changed | llvm::sys::path::remove_dots(Path, true);

sheredom wrote:

Hm I am not sure actually!

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

@@ -4772,6 +4772,23 @@ bool 
ASTWriter::PreparePathForOutput(SmallVectorImpl &Path) {
 Changed = true;
   }
 
+  // If we are generating a normal PCH (EG. not a C++ module).
+  if (!WritingModule) {
+// Use the vfs overlay if it exists to translate paths.
+auto &FileSys =
+Context->getSourceManager().getFileManager().getVirtualFileSystem();
+
+if (auto *RFS = dyn_cast(&FileSys)) {

sheredom wrote:

Lemme try!

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

@@ -0,0 +1,26 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/From
+// RUN: mkdir -p %t/To
+// RUN: echo '#pragma once' > %t/From/B.h

sheredom wrote:

Never knew that existed and haven't used it before. But happy to port over for 
sure!

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

@@ -1115,13 +1115,13 @@ void ASTWriter::WriteBlockInfoBlock() {
 }
 
 /// Prepares a path for being written to an AST file by converting it
-/// to an absolute path and removing nested './'s.
+/// to an absolute path and removing nested './'s and '../'s.
 ///
 /// \return \c true if the path was changed.
 static bool cleanPathForOutput(FileManager &FileMgr,
SmallVectorImpl &Path) {
   bool Changed = FileMgr.makeAbsolutePath(Path);
-  return Changed | llvm::sys::path::remove_dots(Path);
+  return Changed | llvm::sys::path::remove_dots(Path, true);

sheredom wrote:

Added 
https://github.com/llvm/llvm-project/pull/106577/files#diff-3056397628ccde0c171064fdbedcdf91ff1ae3691f8dd1133150fc2cebf92277R11
 which actually uncovered a bug in the impl and fixed that too (was reusing 
`PathStr` wrongly).

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

@@ -4772,6 +4772,23 @@ bool 
ASTWriter::PreparePathForOutput(SmallVectorImpl &Path) {
 Changed = true;
   }
 
+  // If we are generating a normal PCH (EG. not a C++ module).
+  if (!WritingModule) {
+// Use the vfs overlay if it exists to translate paths.
+auto &FileSys =
+Context->getSourceManager().getFileManager().getVirtualFileSystem();
+
+if (auto *RFS = dyn_cast(&FileSys)) {

sheredom wrote:

So tried this, and there are problems. `getRealPath` requires that the remapped 
path already exists, and so will fail (returning the original path again) 
because the remapped location doesn't exist (because we are wanting to remap to 
some fake directory!).

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

@@ -1115,13 +1115,13 @@ void ASTWriter::WriteBlockInfoBlock() {
 }
 
 /// Prepares a path for being written to an AST file by converting it
-/// to an absolute path and removing nested './'s.
+/// to an absolute path and removing nested './'s and '../'s.
 ///
 /// \return \c true if the path was changed.
 static bool cleanPathForOutput(FileManager &FileMgr,
SmallVectorImpl &Path) {
   bool Changed = FileMgr.makeAbsolutePath(Path);
-  return Changed | llvm::sys::path::remove_dots(Path);
+  return Changed | llvm::sys::path::remove_dots(Path, true);

sheredom wrote:

Yeah thinking more closely - I think you are right. But at least for our 
usecase we are using `..` and needing to remap them, but we are *not* using 
symlinks.

Could I hide this behind an option?

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

@@ -1115,13 +1115,13 @@ void ASTWriter::WriteBlockInfoBlock() {
 }
 
 /// Prepares a path for being written to an AST file by converting it
-/// to an absolute path and removing nested './'s.
+/// to an absolute path and removing nested './'s and '../'s.
 ///
 /// \return \c true if the path was changed.
 static bool cleanPathForOutput(FileManager &FileMgr,
SmallVectorImpl &Path) {
   bool Changed = FileMgr.makeAbsolutePath(Path);
-  return Changed | llvm::sys::path::remove_dots(Path);
+  return Changed | llvm::sys::path::remove_dots(Path, true);

sheredom wrote:

You are right. We must have done this before using the VFS and not revisited. 
Lemme undo this .. addition!

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

[clang] [llvm] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

https://github.com/sheredom updated 
https://github.com/llvm/llvm-project/pull/106577

>From 588d45ef291997bc6df487ce9562768d92bff14d Mon Sep 17 00:00:00 2001
From: Neil Henning 
Date: Thu, 29 Aug 2024 13:15:49 +0100
Subject: [PATCH] Make PCH's respect any VFS specified.

We want to be able to generate a PCH against one file-system path, and
then re-use that PCH when the file-system path is different (but the
sources are the same). We also do not know when generating the PCH what
the destination file-system path will be, so what we want to be able to
do is:

- When generating a PCH map the original directory to some fake
  directory. You could imagine `D:/Foo` being mapped to `Z:/Foo` for
  instance.
- Then when consuming a PCH, we want to be able to use a different
  mapping to map `Z:/Foo` to `D:/Some/Other/Machines/Foo` for instance.

This will let us generate and share PCHs to speed up compile time for
our users.

To enable this we've made PCH generation respect any specified
vfsoverlay, such that it will remap the paths in the PCH accordingly.

We've also made `-verify-pch` respect the
`-fvalidate-ast-input-files-content` option so that we can force
verification of inputs.
---
 clang/lib/Frontend/FrontendActions.cpp|  3 ++-
 clang/lib/Serialization/ASTWriter.cpp | 18 +
 clang/test/PCH/verify_no_timestamp.c  |  8 ++
 .../Inputs/vfsoverlay-directory-remap.yaml|  9 +++
 clang/test/VFS/remap-to-fake.c| 27 +++
 llvm/cmake/config-ix.cmake|  1 +
 6 files changed, 65 insertions(+), 1 deletion(-)
 create mode 100644 clang/test/PCH/verify_no_timestamp.c
 create mode 100644 clang/test/VFS/Inputs/vfsoverlay-directory-remap.yaml
 create mode 100644 clang/test/VFS/remap-to-fake.c

diff --git a/clang/lib/Frontend/FrontendActions.cpp 
b/clang/lib/Frontend/FrontendActions.cpp
index 64f90c493c1055..0fec806f714d83 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -348,7 +348,8 @@ void VerifyPCHAction::ExecuteAction() {
   DisableValidationForModuleKind::None,
   /*AllowASTWithCompilerErrors*/ false,
   /*AllowConfigurationMismatch*/ true,
-  /*ValidateSystemInputs*/ true));
+  /*ValidateSystemInputs*/ true,
+  CI.getHeaderSearchOpts().ValidateASTInputFilesContent));
 
   Reader->ReadAST(getCurrentFile(),
   Preamble ? serialization::MK_Preamble
diff --git a/clang/lib/Serialization/ASTWriter.cpp 
b/clang/lib/Serialization/ASTWriter.cpp
index 008bf571f847dc..28787f3ee88281 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -4781,6 +4781,24 @@ bool 
ASTWriter::PreparePathForOutput(SmallVectorImpl &Path) {
 Changed = true;
   }
 
+  // If we are generating a normal PCH (EG. not a C++ module).
+  if (!WritingModule) {
+// Use the vfs overlay if it exists to translate paths.
+auto &FileSys =
+Context->getSourceManager().getFileManager().getVirtualFileSystem();
+if (auto *RFS = dyn_cast(&FileSys)) {
+  if (auto Result = RFS->lookupPath(StringRef(Path.data(), Path.size( {
+if (std::optional Redirect = Result->getExternalRedirect()) 
{
+  const char *data = Redirect->data();
+  const size_t size = Redirect->size();
+  Path.resize(size);
+  Path.assign(data, data + size);
+  Changed = true;
+}
+  }
+}
+  }
+
   return Changed;
 }
 
diff --git a/clang/test/PCH/verify_no_timestamp.c 
b/clang/test/PCH/verify_no_timestamp.c
new file mode 100644
index 00..8aca76cf77c449
--- /dev/null
+++ b/clang/test/PCH/verify_no_timestamp.c
@@ -0,0 +1,8 @@
+// RUN: echo 'int SomeFunc() { return 42; }' > %t.h
+// RUN: %clang_cc1 -Werror -fno-pch-timestamp 
-fvalidate-ast-input-files-content -emit-pch -o "%t.pch" %t.h
+
+// Now change the source file, which should cause the verifier to fail with 
content mismatch.
+// RUN: echo 'int SomeFunc() { return 13; }' > %t.h
+// RUN: not %clang_cc1 -fno-pch-timestamp -fvalidate-ast-input-files-content 
-verify-pch %t.pch 2>&1 | FileCheck %s -DT=%t
+
+// CHECK: fatal error: file '[[T]].h' has been modified since the precompiled 
header '[[T]].pch' was built: content changed
diff --git a/clang/test/VFS/Inputs/vfsoverlay-directory-remap.yaml 
b/clang/test/VFS/Inputs/vfsoverlay-directory-remap.yaml
new file mode 100644
index 00..3cb279a08fae14
--- /dev/null
+++ b/clang/test/VFS/Inputs/vfsoverlay-directory-remap.yaml
@@ -0,0 +1,9 @@
+{
+  'version': 0,
+  'roots': [
+{ 'name': 'FROM_DIR',
+  'type': 'directory-remap',
+  'external-contents': 'TO_DIR'
+}
+  ]
+}
diff --git a/clang/test/VFS/remap-to-fake.c b/clang/test/VFS/remap-to-fake.c
new file mode 100644
index 00..85572e0120ae60
--- /dev/null
+++ b/clang/test/VFS/remap-to-fake.c
@@ -0,0 +1,27 @@
+// RUN: split-file %s %t
+// RUN: sed -e "s@FROM_DIR@%{/t:regex_replacement}/From@g" -e 

[clang] [llvm] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

@@ -0,0 +1,26 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/From
+// RUN: mkdir -p %t/To
+// RUN: echo '#pragma once' > %t/From/B.h

sheredom wrote:

Done!

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

[clang] [llvm] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

@@ -1115,13 +1115,13 @@ void ASTWriter::WriteBlockInfoBlock() {
 }
 
 /// Prepares a path for being written to an AST file by converting it
-/// to an absolute path and removing nested './'s.
+/// to an absolute path and removing nested './'s and '../'s.
 ///
 /// \return \c true if the path was changed.
 static bool cleanPathForOutput(FileManager &FileMgr,
SmallVectorImpl &Path) {
   bool Changed = FileMgr.makeAbsolutePath(Path);
-  return Changed | llvm::sys::path::remove_dots(Path);
+  return Changed | llvm::sys::path::remove_dots(Path, true);

sheredom wrote:

Removed!

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

https://github.com/sheredom updated 
https://github.com/llvm/llvm-project/pull/106577

>From bbf433a5f56db6375e132b27bd4f7cd58feafe7b Mon Sep 17 00:00:00 2001
From: Neil Henning 
Date: Thu, 29 Aug 2024 13:15:49 +0100
Subject: [PATCH] Make PCH's respect any VFS specified.

We want to be able to generate a PCH against one file-system path, and
then re-use that PCH when the file-system path is different (but the
sources are the same). We also do not know when generating the PCH what
the destination file-system path will be, so what we want to be able to
do is:

- When generating a PCH map the original directory to some fake
  directory. You could imagine `D:/Foo` being mapped to `Z:/Foo` for
  instance.
- Then when consuming a PCH, we want to be able to use a different
  mapping to map `Z:/Foo` to `D:/Some/Other/Machines/Foo` for instance.

This will let us generate and share PCHs to speed up compile time for
our users.

To enable this we've made PCH generation respect any specified
vfsoverlay, such that it will remap the paths in the PCH accordingly.

We've also made `-verify-pch` respect the
`-fvalidate-ast-input-files-content` option so that we can force
verification of inputs.
---
 clang/lib/Frontend/FrontendActions.cpp|  3 ++-
 clang/lib/Serialization/ASTWriter.cpp | 18 +
 clang/test/PCH/verify_no_timestamp.c  |  8 ++
 .../Inputs/vfsoverlay-directory-remap.yaml|  9 +++
 clang/test/VFS/remap-to-fake.c| 27 +++
 5 files changed, 64 insertions(+), 1 deletion(-)
 create mode 100644 clang/test/PCH/verify_no_timestamp.c
 create mode 100644 clang/test/VFS/Inputs/vfsoverlay-directory-remap.yaml
 create mode 100644 clang/test/VFS/remap-to-fake.c

diff --git a/clang/lib/Frontend/FrontendActions.cpp 
b/clang/lib/Frontend/FrontendActions.cpp
index 64f90c493c1055..0fec806f714d83 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -348,7 +348,8 @@ void VerifyPCHAction::ExecuteAction() {
   DisableValidationForModuleKind::None,
   /*AllowASTWithCompilerErrors*/ false,
   /*AllowConfigurationMismatch*/ true,
-  /*ValidateSystemInputs*/ true));
+  /*ValidateSystemInputs*/ true,
+  CI.getHeaderSearchOpts().ValidateASTInputFilesContent));
 
   Reader->ReadAST(getCurrentFile(),
   Preamble ? serialization::MK_Preamble
diff --git a/clang/lib/Serialization/ASTWriter.cpp 
b/clang/lib/Serialization/ASTWriter.cpp
index 008bf571f847dc..28787f3ee88281 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -4781,6 +4781,24 @@ bool 
ASTWriter::PreparePathForOutput(SmallVectorImpl &Path) {
 Changed = true;
   }
 
+  // If we are generating a normal PCH (EG. not a C++ module).
+  if (!WritingModule) {
+// Use the vfs overlay if it exists to translate paths.
+auto &FileSys =
+Context->getSourceManager().getFileManager().getVirtualFileSystem();
+if (auto *RFS = dyn_cast(&FileSys)) {
+  if (auto Result = RFS->lookupPath(StringRef(Path.data(), Path.size( {
+if (std::optional Redirect = Result->getExternalRedirect()) 
{
+  const char *data = Redirect->data();
+  const size_t size = Redirect->size();
+  Path.resize(size);
+  Path.assign(data, data + size);
+  Changed = true;
+}
+  }
+}
+  }
+
   return Changed;
 }
 
diff --git a/clang/test/PCH/verify_no_timestamp.c 
b/clang/test/PCH/verify_no_timestamp.c
new file mode 100644
index 00..8aca76cf77c449
--- /dev/null
+++ b/clang/test/PCH/verify_no_timestamp.c
@@ -0,0 +1,8 @@
+// RUN: echo 'int SomeFunc() { return 42; }' > %t.h
+// RUN: %clang_cc1 -Werror -fno-pch-timestamp 
-fvalidate-ast-input-files-content -emit-pch -o "%t.pch" %t.h
+
+// Now change the source file, which should cause the verifier to fail with 
content mismatch.
+// RUN: echo 'int SomeFunc() { return 13; }' > %t.h
+// RUN: not %clang_cc1 -fno-pch-timestamp -fvalidate-ast-input-files-content 
-verify-pch %t.pch 2>&1 | FileCheck %s -DT=%t
+
+// CHECK: fatal error: file '[[T]].h' has been modified since the precompiled 
header '[[T]].pch' was built: content changed
diff --git a/clang/test/VFS/Inputs/vfsoverlay-directory-remap.yaml 
b/clang/test/VFS/Inputs/vfsoverlay-directory-remap.yaml
new file mode 100644
index 00..3cb279a08fae14
--- /dev/null
+++ b/clang/test/VFS/Inputs/vfsoverlay-directory-remap.yaml
@@ -0,0 +1,9 @@
+{
+  'version': 0,
+  'roots': [
+{ 'name': 'FROM_DIR',
+  'type': 'directory-remap',
+  'external-contents': 'TO_DIR'
+}
+  ]
+}
diff --git a/clang/test/VFS/remap-to-fake.c b/clang/test/VFS/remap-to-fake.c
new file mode 100644
index 00..85572e0120ae60
--- /dev/null
+++ b/clang/test/VFS/remap-to-fake.c
@@ -0,0 +1,27 @@
+// RUN: split-file %s %t
+// RUN: sed -e "s@FROM_DIR@%{/t:regex_replacement}/From@g" -e 
"s@TO_DIR@%{/t:regex_replacement}/Fake@g" 
%S/Inputs/

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

sheredom wrote:

Ok already need some advice after hacking!

- `-working-directory` is only used by the **non** `-cc1` clang driver path.
- But the VFS is only initialized way deep once you are in the `-cc1` path.
- The problem is that just after `-working-directory` is set, we then use it to 
query the VFS to ensure that include directories are correct - which actually 
seems like it might be a bug too because these could also be paths that will 
get remapped via a vfs-overlay!

Is the correct fix to basically defer all the actions that could refer to paths 
until we have definitely set the VFS?

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

sheredom wrote:

> Right, that's what I meant by "Is it trying to set it too early before the 
> VFS is created or something?". We could make the driver setup the VFS like 
> the frontend would and do that before checking the -working-directory; not 
> sure if there's a reason we don't do that already. Or maybe we could ignore 
> the failure to set the working directory in the driver and check it in the 
> frontend? That might cause some poor diagnostics if the driver is accessing 
> relative paths and the working directory wasn't set successfully

Yeah for sure! I'll mock up a solution and see how it fares with the rest of 
the test suite and get back to you.

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

sheredom wrote:

> > We tried setting -working-directory=Z:/working, but the VFS requires that 
> > this is a real path.
> 
> What error are you seeing if it's not? Is it trying to set it too early 
> before the VFS is created or something?

The VFS doesn't actually override the set current working directory, so it goes 
down to the base file system class and that will check that the path exists on 
the real filesystem (which it doesn't, cause it is fake!).

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

sheredom wrote:

So you can see [in 
Driver.cpp:1292](https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/Driver.cpp#L1292)
 that it will pass on any set `-working-directory` option to the VFS, but at 
this point the VFS is a `RealFileSystem`, which means it gets to 
[VirtualFileSystem.cpp:350](https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/VirtualFileSystem.cpp#L350)
 and fails because the folder isn't on the filesystem (because it is a fake 
path!).

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

sheredom wrote:

> > `-emit-pch -o "%t.pch" %t/From/../From/B.h`
> 
> > `'could not find file 
> > 'D:\llvm-project\build\tools\clang\test\VFS\Output\remap-to-fake.c.tmp\From\..\From\B.h'`
> 
> Isn't that the same issue I mentioned, that it doesn't appear to work for the 
> main PCH input file? If you remove everything else except this file does it 
> make progress? If so, fixing whatever is going wrong with the main input 
> seems promising.

Oh sorry I wasn't grokking what you were meaning. So just to be super duper 
clear - you mean that I should, when using this external names stuff, make it 
rewrite that path in the PCH production to respect the VFS?

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

sheredom wrote:

> > Build machine A with a path E:/foo/header.h which wants to make a PCH from 
> > it. We want it to remap E:/foo -> Z:/fake.
> 
> What I have in mind is that you have a VFS on build machine A that maps 
> Z:/fake (virtual path) -> E:/foo (external path), with use-external-names: 
> false (EDIT: and your compiler invocation is also written in terms of 
> Z:/fake). In principle, this should give you a PCH (+ Modules) that only 
> refers to Z:/fake, the virtual path. Another way to think about this is 
> except for the VFS layer, clang would not even know those external paths 
> exist, only about Z:/fake.

Oh - the penny is finally dropping. Lemme give that a try!

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

sheredom wrote:

Ok by specifying `-resource-dir=Z:/resource` and adding it a VFS made that 
work. But we are seeing a single stray original path in the source files that 
we think is related to the working directory. We tried setting 
`-working-directory=Z:/working`, but the VFS requires that this is a real path.

Do you think we could add an overload for `setCurrentWorkingDirectory` on the 
actual `llvm::VirtualFileSystem` so that it will try lookup the overlay first?

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

sheredom wrote:

So we've been digging a bit further - I think its the `ResourceDir` path that 
is getting messed up. Even with the VFS it is using the location of clang to 
get the clang headers, which don't then get remapped. Will try and make a test 
for it.

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

sheredom wrote:

> I think it's something much earlier in clang is not using the VFS at all when 
> looking up the main input path, so then it only knows the external path for 
> that case. My guess is that if that were fixed then the ASTWriter/Reader 
> would do the right thing here.

I don't think that would fix our problem though? We have:

- Build machine A with a path `E:/foo/header.h` which wants to make a PCH from 
it. We want it to remap `E:/foo` -> `Z:/fake`.
- Build machine B that *has no clue* where the original `header.h` file was on 
build machine A's filesystem. But we can tell it that the file is at 
`Z:/fake/header.h`. We can also remap that to our local version of the same 
file at `E:/bar/header.h`.
- This lets us share PCHs to significantly speed up our builds with Unreal 
Build Accelerator.

We need the PCH as created itself to *never* have `E:/foo` in any of the paths 
if we've specified this in the VFS. So we need the ASTWriter to do this 
remapping when it is creating the PCH.

If need be I can make the ASTWriter only do this remapping if we also had 
`"use-external-names": false` if that would make this easier?

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

sheredom wrote:

> > If the goal is to put virtual paths in the PCH so that you can map them 
> > somewhere else in the consuming compiler's VFS, does 
> > the`RedirectingFileSystem` setting `'use-external-names': false` do what 
> > you need? The idea behind that setting is that we would use the virtual 
> > paths everywhere in the compiler instead of translating them to the 
> > external/on-disk path. I don't have a lot of experience with enabling that 
> > in modules/pch so maybe there's something that it doesn't handle.
> 
> I'll have a look at that and get back to you!

Ok we tested this and it doesn't work as expected - the generated PCH still has 
the original paths baked into it.

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

sheredom wrote:

Any other comments or can I land this then? 😄 

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

sheredom wrote:

> Sorry for being slow to look at this in more detail:

No bother!


> > When generating a PCH map the original directory to some fake directory. 
> > You could imagine D:/Foo being mapped to Z:/Foo for instance.
> 
> Can you clarify what this means? Is `Z:/Foo` a virtual-only path or does it 
> actually exist on disk? In your test case both directories exist, which I 
> assume wouldn't normally be the case (or else why not build in the desired 
> location directly?).

`Z:/Foo` is virtual - a fake path. We want to remap all paths on build machines 
to various fake root paths + generate the PCHs, then on other build machines / 
dev machines remap the fake root paths back to wherever that user has those 
paths installed (they might install things in different drives, folders, etc).

> If the goal is to put virtual paths in the PCH so that you can map them 
> somewhere else in the consuming compiler's VFS, does 
> the`RedirectingFileSystem` setting `'use-external-names': false` do what you 
> need? The idea behind that setting is that we would use the virtual paths 
> everywhere in the compiler instead of translating them to the 
> external/on-disk path. I don't have a lot of experience with enabling that in 
> modules/pch so maybe there's something that it doesn't handle.

I'll have a look at that and get back to you!

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

[clang] Make PCH's respect any VFS specified. (PR #106577)

[Neil Henning via cfe-commits]

sheredom wrote:

> It looks like it almost works: if I create a module in a virtual path and use 
> `use-external-names: false`, then the module stores the virtual path for its 
> input files.
> 
> E.g.
> 
> ```
> {
>   "version": 0,
>   "use-external-names": false,
>   "roots": [
> {
>   "contents": [
> {
>   "external-contents": "/tmp/a/t.h",
>   "name": "t.h",
>   "type": "file"
> },
> {
>   "external-contents": "/tmp/b/prefix.h",
>   "name": "prefix.h",
>   "type": "file"
> },
> {
>   "external-contents": "/tmp/a/module.modulemap",
>   "name": "module.modulemap",
>   "type": "file"
> }
>   ],
>   "name": "/root",
>   "type": "directory"
> },
>   ]
> }
> ```
> 
> Compiled with
> 
> ```
> clang -x c-header /tmp/b/prefix.h -I/root -ivfsoverlay vfs -fmodules -o 
> prefix.h.pch
> ```
> 
> The references to t.h and module.modulemap seem to be stored with `/root` 
> (the virtual path).
> 
> However, I can't remap the prefix header itself
> 
> ```
> clang -x c-header /root/prefix.h -I/root -ivfsoverlay vfs -fmodules -o 
> prefix.h.pch
> clang: error: no such file or directory: '/root/prefix.h'
> clang: error: no input files
> ```
> 
> I guess it's resolving that path outside the VFS. Maybe that's fixable though?

Ok I tried to change my test over like you suggested, and it fails with:

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