[PCH] Allow VFS to be used for tests that generate PCH files

[cameron314]

Summary

When using a virtual file-system (VFS) and a preamble file (PCH) is generated, it is generated on-disk in the real file-system instead of in the VFS (which I guess makes sense, since the VFS is read-only). However, when subsequently reading the generated PCH, the frontend passes through the VFS it has been given -- resulting in an error and a failed parse (since the VFS doesn't contain the PCH; the real filesystem does).

This patch fixes that by detecting when a VFS is being used for a parse that needs to work with a PCH file, and creating an overlay VFS that includes the real file-system underneath. Since the PCH is hard-coded to always be on the real file-system, I believe this is the cleanest approach.

This allows tests to be written which make use of both PCH files and a VFS, like the one I've included here.

Note: This was originally part of the code to test the bug fixed in D20338, but while languishing in review it has since been fixed by somebody else in D27810. However, I feel it's still important to be able to test the frontend preamble code while at the same time making use of a VFS, so I've rebased that part of the patch (and my test to go with it).

[ilya-biryukov]

Could we fix this in tests instead by providing an overlay over RealFS there instead of doing that in ASTUnit?

If I'm passing vfs::FileSystem, I specifically do not want any file accesses to silently go through RealFS, ignoring the vfs::FileSystem that I passed.

We do overlays in clangd and we also have a vfs::FileSystem implementation that only reads certain WhitelistedDirs from RealFS to ensure we don't read other files from RealFS.
See https://reviews.llvm.org/diffusion/L/browse/clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp;312537$34.

[cameron314]

I suppose we could do the overlay manually in all the tests that need it; I guess it depends on what the goal of the VFS support is. To my mind, it doesn't make sense that a file is placed in the RealFS in the first place if a VFS is used, but this is quite entrenched in the current design and difficult to change. In which case, I would argue that parsing a file with a preamble and a VFS should not give a cryptic error without an explicit overlay, but rather "just work". But I agree my current patch is a little crude in that it allows access to the entire RealFS via the VFS.

I could change the automatic overlay to *only* support access to the generated PCH file, similar to how the FilteredFileSystem you've pointed out works, but for a single file only. Would that be acceptable?

[ilya-biryukov]

In D37474#861206, @cameron314 wrote:

I suppose we could do the overlay manually in all the tests that need it; I guess it depends on what the goal of the VFS support is. To my mind, it doesn't make sense that a file is placed in the RealFS in the first place if a VFS is used, but this is quite entrenched in the current design and difficult to change. In which case, I would argue that parsing a file with a preamble and a VFS should not give a cryptic error without an explicit overlay, but rather "just work". But I agree my current patch is a little crude in that it allows access to the entire RealFS via the VFS.

I could change the automatic overlay to *only* support access to the generated PCH file, similar to how the FilteredFileSystem you've pointed out works, but for a single file only. Would that be acceptable?

I totally agree, the current behavior is definitely confusing for clients using vfs::FileSystem without proper filesystem access on top of ASTUnit.
Having an overlay that provides only the PCH file sounds like a good middle ground.

Maybe we could even read the PCH contents using file APIs instead of vfs::FileSystem and add them into PreprocessorOption::RemappedFileBuffers? Do you think that would also work?
Since ASTUnit is responsible writes PCHs to disk, it also makes sense for it to read them from disk, but accesses to all other files would go through vfs::FileSystem.

[cameron314]

I'll change the overlay to only allow access to the PCH.

I agree that it would be nice to only read the PCH using real filesystem APIs in order to be symmetrical, but this seems non-trivial. It also feels like a step in the wrong direction -- ideally the VFS would hold the PCH too instead of putting it on disk, in my opinion.

I'm not sure adding a remapped file buffer would work, since it's unclear to me exactly at which point the PCH is written relative to the multiple places it's read. Additionally, there's code that does directory traversal in the current VFS to find a PCH when given a directory name instead of a file name; this only works if the VFS is aware of the PCH, or if the code is rewritten to use the underlying file system.

[cameron314]

Here's an updated patch that allows only the PCH to be accessed from the real FS when a VSF is present. Tests still pass.

[ilya-biryukov]

In D37474#862187, @cameron314 wrote:

I'll change the overlay to only allow access to the PCH.

I agree that it would be nice to only read the PCH using real filesystem APIs in order to be symmetrical, but this seems non-trivial. It also feels like a step in the wrong direction -- ideally the VFS would hold the PCH too instead of putting it on disk, in my opinion.

Unfortunately vfs::FileSystem is a read-only API, and AFAIK this is by design. So we should not hope that would ever happen.

I'm not sure adding a remapped file buffer would work, since it's unclear to me exactly at which point the PCH is written relative to the multiple places it's read. Additionally, there's code that does directory traversal in the current VFS to find a PCH when given a directory name instead of a file name; this only works if the VFS is aware of the PCH, or if the code is rewritten to use the underlying file system.

Fair point, the directory traversal code would only work with vfs.
We can use assumptions about our input, though. ASTUnit controls PCH creation and AFAIK it in our case pch will always point to a file. Moreover, the path seems to PCH seems to always be absolute, so we should not care about working directory of vfs either.

Using remapped buffers would make the code simpler, would not require custom vfs implementations and you're also right that it would look more symmetrical since we'll use filesystem API for both writes and reads of PCH.
Maybe we could try this approach?

[cameron314]

Looking at the way remapped buffers are handled, I just remembered that they must exist on the file system (at the very least, in a directory that exists) or the remapping is not taken into account. So that pretty much rules out the other approach, I think.

[ilya-biryukov]

In D37474#863335, @cameron314 wrote:

Looking at the way remapped buffers are handled, I just remembered that they must exist on the file system (at the very least, in a directory that exists) or the remapping is not taken into account. So that pretty much rules out the other approach, I think.

You are right, thanks for pointing this out. Fiddling with VFS seems like an only option.

lib/Frontend/ASTUnit.cpp

1013

Maybe create a combination of InMemoryFileSystem and OverlayFileSystem instead of custom filtering implementation?
We really need to read only a single file given that ASTUnit never creates directory PCHs.
I bet it would make the code simpler and less error-prone.

1053

Maybe create a PCH overlay only when !VFS->exists(/*PreamblePath...*/)?
This seems like a good enough indication that RealFS is underneath the vfs::FileSystem instance, even if pointer equality does not hold (i.e. in case of overlays over RealFS).

unittests/Frontend/CMakeLists.txt

9

Maybe rename to PCHPreambleTest?
LLVM code generally capitalizes all letters in abbreviations.

[cameron314]

lib/Frontend/ASTUnit.cpp

1013

I hadn't thought of that. Yes, that makes sense and is more concise.

1053

Makes sense.

unittests/Frontend/CMakeLists.txt

9

Oops, overlooked this one. Will do!

[cameron314]

The latest patch. I think this one should do the trick :-)

[ilya-biryukov]

Looks good, just a few minor style comments.

include/clang/Frontend/PrecompiledPreamble.h

100 ↗

(On Diff #114414)

Not introduced by this change, but could you also add a full stop here for constistency?

103 ↗

(On Diff #114414)

NIT: comment should end with a full stop.

lib/Frontend/ASTUnit.cpp

1021

Maybe return original VFS if PCHFilename could not be read and not create any empty overlays?

1053

The check && RealFS is redundant and can be removed.

[cameron314]

Final diff, will commit soon. Thanks!

include/clang/Frontend/PrecompiledPreamble.h

100 ↗

(On Diff #114414)

Sure.

103 ↗

(On Diff #114414)

OK!

lib/Frontend/ASTUnit.cpp

1021

Makes sense, will do.

1053

It's not redundant, but looking at the implementation of vfs::getRealFileSystem it will always return a non-null pointer.

lib/Frontend/ASTUnit.cpp

static void checkAndSanitizeDiags(SmallVectorImpl<StoredDiagnostic> &
   for (StoredDiagnostic &SD : StoredDiagnostics) {
     if (SD.getLocation().isValid()) {
       FullSourceLoc Loc(SD.getLocation(), SM);
       SD.setLocation(Loc);
     }
   }
 }
 
+static IntrusiveRefCntPtr<vfs::FileSystem> createPreambleVFSOverlay(
+  IntrusiveRefCntPtr<vfs::FileSystem> RawPreambleVFS,
+  IntrusiveRefCntPtr<vfs::FileSystem> CustomVFS) {
+  IntrusiveRefCntPtr<vfs::OverlayFileSystem>
+    Overlay(new vfs::OverlayFileSystem(RawPreambleVFS));
+  llvm::ErrorOr<std::string> PrevWorkingDir =
+    CustomVFS->getCurrentWorkingDirectory();
+  Overlay->pushOverlay(CustomVFS);
+  // pushOverlay changes the working directory, but we can't assume that the
+  // two filesystems are compatible that way. We only underlay the other
+  // filesystem so that the PCH file can be written/read from it anyway,
+  // and for that we use an absolute path in the first place.
+  if (PrevWorkingDir)
+    CustomVFS->setCurrentWorkingDirectory(PrevWorkingDir.get());
+  return Overlay;
+}
+
 /// Parse the source file into a translation unit using the given compiler
 /// invocation, replacing the current translation unit.
 ///
 /// \returns True if a failure occurred that causes the ASTUnit not to
 /// contain any translation-unit information, false otherwise.
 bool ASTUnit::Parse(std::shared_ptr<PCHContainerOperations> PCHContainerOps,
                     std::unique_ptr<llvm::MemoryBuffer> OverrideMainBuffer,
                     IntrusiveRefCntPtr<vfs::FileSystem> VFS) {
   if (!Invocation)
     return true;
 
   // Create the compiler instance to use for building the AST.
   std::unique_ptr<CompilerInstance> Clang(
       new CompilerInstance(std::move(PCHContainerOps)));
   if (FileMgr && VFS) {
     assert(VFS == FileMgr->getVirtualFileSystem() &&
            "VFS passed to Parse and VFS in FileMgr are different");
   } else if (VFS) {
     Clang->setVirtualFileSystem(VFS);
   }
 
+  // Make sure we can access the PCH file even if we're using a VFS
+  if (!VFS && FileMgr)
+    VFS = FileMgr->getVirtualFileSystem();
+  IntrusiveRefCntPtr<vfs::FileSystem> RealFS = vfs::getRealFileSystem();
+  if (OverrideMainBuffer && VFS && RealFS && VFS != RealFS) {
+    // We have a slight inconsistency here -- we're using the VFS to
+    // read files, but the PCH was generated in the real file system.
+    // No choice but to overlay the two.
+    VFS = createPreambleVFSOverlay(RealFS, VFS);
+    if (FileMgr) {
+      FileMgr = new FileManager(FileMgr->getFileSystemOpts(), VFS);
+      Clang->setFileManager(FileMgr.get());
+    }
+    else {
+      Clang->setVirtualFileSystem(VFS);
+    }
+  }
+
   // Recover resources if we crash before exiting this method.
   llvm::CrashRecoveryContextCleanupRegistrar<CompilerInstance>
     CICleanup(Clang.get());
 
   Clang->setInvocation(std::make_shared<CompilerInvocation>(*Invocation));
   OriginalSourceFile = Clang->getFrontendOpts().Inputs[0].getFile();
     
   // Set up diagnostics, capturing any diagnostics that would

unittests/Frontend/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_unittest(FrontendTests
   ASTUnitTest.cpp
   FrontendActionTest.cpp
   CodeGenActionTest.cpp
+  PchPreambleTest.cpp
   )
 target_link_libraries(FrontendTests
   clangAST
   clangBasic
   clangFrontend
   clangLex
   clangSema
   clangCodeGen
   )

unittests/Frontend/PchPreambleTest.cpp

+//====-- unittests/Frontend/PchPreambleTest.cpp - FrontendAction tests ---====//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+class ReadCountingInMemoryFileSystem : public vfs::InMemoryFileSystem
+{
+  std::map<std::string, unsigned> ReadCounts;
+
+public:
+  ErrorOr<std::unique_ptr<vfs::File>> openFileForRead(const Twine &Path) override
+  {
+    SmallVector<char, 128> PathVec;
+    Path.toVector(PathVec);
+    llvm::sys::path::remove_dots(PathVec, true);
+    ++ReadCounts[std::string(PathVec.begin(), PathVec.end())];
+    return InMemoryFileSystem::openFileForRead(Path);
+  }
+
+  unsigned GetReadCount(const Twine &Path) const
+  {
+    auto it = ReadCounts.find(Path.str());
+    return it == ReadCounts.end() ? 0 : it->second;
+  }
+};
+
+class PchPreambleTest : public ::testing::Test {
+  IntrusiveRefCntPtr<ReadCountingInMemoryFileSystem> VFS;
+  StringMap<std::string> RemappedFiles;
+  std::shared_ptr<PCHContainerOperations> PCHContainerOpts;
+  FileSystemOptions FSOpts;
+
+public:
+  void SetUp() override {
+    VFS = new ReadCountingInMemoryFileSystem();
+    // We need the working directory to be set to something absolute,
+    // otherwise it ends up being inadvertently set to the current
+    // working directory in the real file system due to a series of
+    // unfortunate conditions interacting badly.
+    // What's more, this path *must* be absolute on all (real)
+    // filesystems, so just '/' won't work (e.g. on Win32).
+    VFS->setCurrentWorkingDirectory("//./");
+  }
+
+  void TearDown() override {
+  }
+
+  void AddFile(const std::string &Filename, const std::string &Contents) {
+    ::time_t now;
+    ::time(&now);
+    VFS->addFile(Filename, now, MemoryBuffer::getMemBufferCopy(Contents, Filename));
+  }
+
+  void RemapFile(const std::string &Filename, const std::string &Contents) {
+    RemappedFiles[Filename] = Contents;
+  }
+
+  std::unique_ptr<ASTUnit> ParseAST(const std::string &EntryFile) {
+    PCHContainerOpts = std::make_shared<PCHContainerOperations>();
+    std::shared_ptr<CompilerInvocation> CI(new CompilerInvocation);
+    CI->getFrontendOpts().Inputs.push_back(
+      FrontendInputFile(EntryFile, FrontendOptions::getInputKindForExtension(
+        llvm::sys::path::extension(EntryFile).substr(1))));
+
+    CI->getTargetOpts().Triple = "i386-unknown-linux-gnu";
+
+    CI->getPreprocessorOpts().RemappedFileBuffers = GetRemappedFiles();
+
+    PreprocessorOptions &PPOpts = CI->getPreprocessorOpts();
+    PPOpts.RemappedFilesKeepOriginalName = true;
+
+    IntrusiveRefCntPtr<DiagnosticsEngine>
+      Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions, new DiagnosticConsumer));
+
+    FileManager *FileMgr = new FileManager(FSOpts, VFS);
+
+    std::unique_ptr<ASTUnit> AST = ASTUnit::LoadFromCompilerInvocation(
+      CI, PCHContainerOpts, Diags, FileMgr, false, false,
+      /*PrecompilePreambleAfterNParses=*/1);
+    return AST;
+  }
+
+  bool ReparseAST(const std::unique_ptr<ASTUnit> &AST) {
+    bool reparseFailed = AST->Reparse(PCHContainerOpts, GetRemappedFiles(), VFS);
+    return !reparseFailed;
+  }
+
+  unsigned GetFileReadCount(const std::string &Filename) const {
+    return VFS->GetReadCount(Filename);
+  }
+
+private:
+  std::vector<std::pair<std::string, llvm::MemoryBuffer *>>
+  GetRemappedFiles() const {
+    std::vector<std::pair<std::string, llvm::MemoryBuffer *>> Remapped;
+    for (const auto &RemappedFile : RemappedFiles) {
+      std::unique_ptr<MemoryBuffer> buf = MemoryBuffer::getMemBufferCopy(
+        RemappedFile.second, RemappedFile.first());
+      Remapped.emplace_back(RemappedFile.first(), buf.release());
+    }
+    return Remapped;
+  }
+};
+
+TEST_F(PchPreambleTest, ReparseWithOverriddenFileDoesNotInvalidatePreamble) {
+  std::string Header1 = "//./header1.h";
+  std::string Header2 = "//./header2.h";
+  std::string MainName = "//./main.cpp";
+  AddFile(Header1, "");
+  AddFile(Header2, "#pragma once");
+  AddFile(MainName,
+    "#include \"//./foo/../header1.h\"\n"
+    "#include \"//./foo/../header2.h\"\n"
+    "int main() { return ZERO; }");
+  RemapFile(Header1, "static const int ZERO = 0;\n");
+
+  std::unique_ptr<ASTUnit> AST(ParseAST(MainName));
+  ASSERT_TRUE(AST.get());
+  ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+  unsigned initialCounts[] = {
+    GetFileReadCount(MainName),
+    GetFileReadCount(Header1),
+    GetFileReadCount(Header2)
+  };
+
+  ASSERT_TRUE(ReparseAST(AST));
+
+  ASSERT_NE(initialCounts[0], GetFileReadCount(MainName));
+  ASSERT_EQ(initialCounts[1], GetFileReadCount(Header1));
+  ASSERT_EQ(initialCounts[2], GetFileReadCount(Header2));
+}
+
+} // anonymous namespace