Make PCH's respect any VFS specified. #106577
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: Neil Henning (sheredom) ChangesWe 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:
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 Full diff: https://github.com/llvm/llvm-project/pull/106577.diff 5 Files Affected:
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<char> &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<char> &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<llvm::vfs::RedirectingFileSystem>(&FileSys)) {
+ if (auto Result = RFS->lookupPath(PathStr)) {
+ if (std::optional<StringRef> 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 00000000000000..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 00000000000000..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 00000000000000..fbf14117c7e8cc
--- /dev/null
+++ b/clang/test/VFS/remap-to-fake.c
@@ -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
+// RUN: echo 'int SomeFunc() { return 13; }' >> %t/From/B.h
+// RUN: echo '#pragma once' > %t/To/B.h
+// RUN: echo 'int SomeFunc() { return 13; }' >> %t/To/B.h
+// RUN: sed -e "s@FROM_DIR@%{/t:regex_replacement}/From@g" -e "s@TO_DIR@%{/t:regex_replacement}/Fake@g" %S/Inputs/vfsoverlay-directory-remap.yaml > %t/to-fake.yaml
+// RUN: sed -e "s@FROM_DIR@%{/t:regex_replacement}/Fake@g" -e "s@TO_DIR@%{/t:regex_replacement}/To@g" %S/Inputs/vfsoverlay-directory-remap.yaml > %t/from-fake.yaml
+
+// RUN: %clang_cc1 -Werror -fno-pch-timestamp -fvalidate-ast-input-files-content -ivfsoverlay %t/to-fake.yaml -emit-pch -o "%t.pch" %t/From/B.h
+
+// Remove the `From` directory as we don't want to accidentally find that source if the PCH hasn't remapped using the VFS!
+// RUN: rm -rf %t/From
+
+// The PCH will be invalid because the `Fake` directory does not exist.
+// RUN: not %clang_cc1 -fno-pch-timestamp -fvalidate-ast-input-files-content -verify-pch %t.pch
+
+// But if we specify the correct VFS overlay it'll verify clean.
+// RUN: %clang_cc1 -fno-pch-timestamp -fvalidate-ast-input-files-content -verify-pch -ivfsoverlay %t/from-fake.yaml %t.pch
+
+// RUN: %clang_cc1 -fno-pch-timestamp -fvalidate-ast-input-files-content -Werror -I %t/To -ivfsoverlay %t/from-fake.yaml -include-pch "%t.pch" -emit-llvm -C %s -o %t.o
+
+#include "B.h"
+
+int UseSomeFunc() { return SomeFunc(); }
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
sheredom
force-pushed
the
ivfsoverlay_heart_pch
branch
from
e8e06ce to
774ca72
Compare
sheredom
force-pushed
the
ivfsoverlay_heart_pch
branch
from
774ca72 to
2d73b6c
Compare
jansvoboda11
left a comment
jansvoboda11
left a comment
There was a problem hiding this comment.
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:
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? |
| } | ||
|
|
||
| // If we are generating a normal PCH (EG. not a C++ module). | ||
| if (!WritingModule) { |
There was a problem hiding this comment.
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
2d73b6c to
588d45e
Compare
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.
sheredom
force-pushed
the
ivfsoverlay_heart_pch
branch
from
588d45e to
bbf433a
Compare
|
Any other comments or can I land this then? 😄 |
benlangmuir
left a comment
benlangmuir
left a comment
There was a problem hiding this comment.
Sorry for being slow to look at this in more detail:
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?).
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 theRedirectingFileSystem 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.
No bother!
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. |
|
It looks like it almost works: if I create a module in a virtual path and use E.g. Compiled with The references to t.h and module.modulemap seem to be stored with However, I can't remap the prefix header itself 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: So it hasn't remapped the paths correctly with |
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? |
|
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:
We need the PCH as created itself to never have If need be I can make the ASTWriter only do this remapping if we also had |
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! |
|
I'm working with Neil and I've just tried changing our system to work the way where all our rsp etc use virtual paths and then create a vfsoverlay that points to the "right" places... it almost works for portable pch. What is not working is "#include_next".. that gets resolved to a non-virtual path which is written into the .pch and .d files. For example this file: Unix\LibCxx\include\c++\v1\stddef.h has "#include_next <stddef.h>" Are there any options to control that? |
|
So we've been digging a bit further - I think its the |
|
Ok by specifying Do you think we could add an overload for |
|
Adding to @sheredom ... the precompiled headers are actually portable now (hurray!).. the single last non-virtual path we see is in the object files (.o). And it is related to the working dir as mentioned. |
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!). |
|
@sheredom I think I'm missing something. What's the actual failure? Setting |
|
So you can see in Driver.cpp:1292 that it will pass on any set |
|
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. |
|
Ok already need some advice after hacking!
Is the correct fix to basically defer all the actions that could refer to paths until we have definitely set the VFS? |
|
We ended up running into other issues where more than PCHs wouldn't respect the VFS, and had to use system level directory remapping instead. I'll close this out. |
6 participants
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:
D:/Foobeing mapped toZ:/Foofor instance.Z:/FootoD:/Some/Other/Machines/Foofor 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-pchrespect the-fvalidate-ast-input-files-contentoption so that we can force verification of inputs.