[PATCH] D39842: Allow to store precompiled preambles in memory.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D39842#927578, @mgorny wrote:

> Please revert this commit. You've just broken all the stand-alone builds of 
> clang.


Sorry about that. Should be fixed in https://reviews.llvm.org/rL318514


Repository:
  rL LLVM

https://reviews.llvm.org/D39842



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

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

Please revert this commit. You've just broken all the stand-alone builds of 
clang.




Comment at: cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp:27
 #include "llvm/ADT/StringSet.h"
+#include "llvm/Config/config.h"
 #include "llvm/Support/CrashRecoveryContext.h"

This is private LLVM header which is not available when doing stand-alone 
builds.


Repository:
  rL LLVM

https://reviews.llvm.org/D39842



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

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL318411: Allow to store precompiled preambles in memory. 
(authored by ibiryukov).

Repository:
  rL LLVM

https://reviews.llvm.org/D39842

Files:
  cfe/trunk/include/clang/Frontend/FrontendActions.h
  cfe/trunk/include/clang/Frontend/PrecompiledPreamble.h
  cfe/trunk/lib/Frontend/ASTUnit.cpp
  cfe/trunk/lib/Frontend/FrontendActions.cpp
  cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp

Index: cfe/trunk/lib/Frontend/FrontendActions.cpp
===
--- cfe/trunk/lib/Frontend/FrontendActions.cpp
+++ cfe/trunk/lib/Frontend/FrontendActions.cpp
@@ -80,9 +80,12 @@
 std::unique_ptr
 GeneratePCHAction::CreateASTConsumer(CompilerInstance , StringRef InFile) {
   std::string Sysroot;
+  if (!ComputeASTConsumerArguments(CI, /*ref*/ Sysroot))
+return nullptr;
+
   std::string OutputFile;
   std::unique_ptr OS =
-  ComputeASTConsumerArguments(CI, InFile, Sysroot, OutputFile);
+  CreateOutputFile(CI, InFile, /*ref*/ OutputFile);
   if (!OS)
 return nullptr;
 
@@ -103,17 +106,20 @@
   return llvm::make_unique(std::move(Consumers));
 }
 
-std::unique_ptr
-GeneratePCHAction::ComputeASTConsumerArguments(CompilerInstance ,
-   StringRef InFile,
-   std::string ,
-   std::string ) {
+bool GeneratePCHAction::ComputeASTConsumerArguments(CompilerInstance ,
+std::string ) {
   Sysroot = CI.getHeaderSearchOpts().Sysroot;
   if (CI.getFrontendOpts().RelocatablePCH && Sysroot.empty()) {
 CI.getDiagnostics().Report(diag::err_relocatable_without_isysroot);
-return nullptr;
+return false;
   }
 
+  return true;
+}
+
+std::unique_ptr
+GeneratePCHAction::CreateOutputFile(CompilerInstance , StringRef InFile,
+std::string ) {
   // We use createOutputFile here because this is exposed via libclang, and we
   // must disable the RemoveFileOnSignal behavior.
   // We use a temporary to avoid race conditions.
Index: cfe/trunk/lib/Frontend/ASTUnit.cpp
===
--- cfe/trunk/lib/Frontend/ASTUnit.cpp
+++ cfe/trunk/lib/Frontend/ASTUnit.cpp
@@ -1013,24 +1013,6 @@
   }
 }
 
-static IntrusiveRefCntPtr createVFSOverlayForPreamblePCH(
-StringRef PCHFilename,
-IntrusiveRefCntPtr RealFS,
-IntrusiveRefCntPtr VFS) {
-  // We want only the PCH file from the real filesystem to be available,
-  // so we create an in-memory VFS with just that and overlay it on top.
-  auto Buf = RealFS->getBufferForFile(PCHFilename);
-  if (!Buf)
-return VFS;
-  IntrusiveRefCntPtr
-  PCHFS(new vfs::InMemoryFileSystem());
-  PCHFS->addFile(PCHFilename, 0, std::move(*Buf));
-  IntrusiveRefCntPtr
-  Overlay(new vfs::OverlayFileSystem(VFS));
-  Overlay->pushOverlay(PCHFS);
-  return Overlay;
-}
-
 /// Parse the source file into a translation unit using the given compiler
 /// invocation, replacing the current translation unit.
 ///
@@ -1042,6 +1024,19 @@
   if (!Invocation)
 return true;
 
+  auto CCInvocation = std::make_shared(*Invocation);
+  if (OverrideMainBuffer) {
+assert(Preamble &&
+   "No preamble was built, but OverrideMainBuffer is not null");
+IntrusiveRefCntPtr OldVFS = VFS;
+Preamble->AddImplicitPreamble(*CCInvocation, VFS, OverrideMainBuffer.get());
+if (OldVFS != VFS && FileMgr) {
+  assert(OldVFS == FileMgr->getVirtualFileSystem() &&
+ "VFS passed to Parse and VFS in FileMgr are different");
+  FileMgr = new FileManager(FileMgr->getFileSystemOpts(), VFS);
+}
+  }
+
   // Create the compiler instance to use for building the AST.
   std::unique_ptr Clang(
   new CompilerInstance(std::move(PCHContainerOps)));
@@ -1052,29 +1047,11 @@
 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 RealFS = vfs::getRealFileSystem();
-  if (OverrideMainBuffer && VFS && RealFS && VFS != RealFS &&
-  !VFS->exists(Preamble->GetPCHPath())) {
-// We have a slight inconsistency here -- we're using the VFS to
-// read files, but the PCH was generated in the real file system.
-VFS = createVFSOverlayForPreamblePCH(Preamble->GetPCHPath(), 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
 CICleanup(Clang.get());
 
-  Clang->setInvocation(std::make_shared(*Invocation));
+  Clang->setInvocation(CCInvocation);
   

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:44
+SmallString<64> Path;
+llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/true, Path);
+llvm::sys::path::append(Path, "___clang_inmemory_preamble___");

ilya-biryukov wrote:
> klimek wrote:
> > ilya-biryukov wrote:
> > > klimek wrote:
> > > > ilya-biryukov wrote:
> > > > > klimek wrote:
> > > > > > erasedOnReboot seems weird for something that we don't want to have 
> > > > > > on the actual file system. Why do we even want a temp directory?
> > > > > Yeah, `erasedOnReboot` seems weird here.
> > > > > What I really wanted is a filepath that's valid on all platforms and 
> > > > > is on an existing drive for Windows, so the value of `erasedOnReboot` 
> > > > > doesn't really matter. `system_temp_directory` might be the wrong 
> > > > > thing to use for that in the first place. Is there a better 
> > > > > alternative I'm missing?
> > > > Why does it need to get an existing path / existing drive?
> > > Oh, maybe it doesn't have to be. But I expect that a probability of 
> > > something breaking is lower if we use an existing path.
> > > Maybe I'm wrong and it will all work out just fine, but that seems to be 
> > > a rather good compromise adding an extra bit of confidence that things 
> > > will be working in the long-term.
> > I'd on the contrary say that this is more likely to fail. If we don't have 
> > a real filesystem, or have a read-only view of a real file system, getting 
> > a temp dir might well fail, while we can easily overlay any path we want in 
> > memory.
> That's true in general, but LLVM's `system_temp_directory` will return the 
> default (`/var/tmp` or `/tmp` on Linux, `C:\Temp` on Windows) if it fails to 
> find the real temp dir.
> So in the very worst case we'll be using hard-coded paths.
> 
> On the other hand, if that's one of the possible behaviors, we'd probably 
> want to not break in that case anyway. I'll update the patch to follow your 
> suggestions. Thanks!
This should do the trick.


https://reviews.llvm.org/D39842



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

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 123172.
ilya-biryukov added a comment.

- Removed redundant #include.


https://reviews.llvm.org/D39842

Files:
  include/clang/Frontend/FrontendActions.h
  include/clang/Frontend/PrecompiledPreamble.h
  lib/Frontend/ASTUnit.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Frontend/PrecompiledPreamble.cpp

Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -24,16 +24,45 @@
 #include "clang/Serialization/ASTWriter.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSet.h"
+#include "llvm/Config/config.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/MutexGuard.h"
 #include "llvm/Support/Process.h"
 
+#include 
+
 using namespace clang;
 
 namespace {
 
+StringRef getInMemoryPreamblePath() {
+#if defined(LLVM_ON_UNIX)
+  return "/__clang_tmp/___clang_inmemory_preamble___";
+#elif defined(LLVM_ON_WIN32)
+  return "C:\\__clang_tmp\\___clang_inmemory_preamble___";
+#else
+#warning "Unknown platform. Defaulting to UNIX-style paths for in-memory PCHs"
+  return "/__clang_tmp/___clang_inmemory_preamble___";
+#endif
+}
+
+IntrusiveRefCntPtr
+createVFSOverlayForPreamblePCH(StringRef PCHFilename,
+   std::unique_ptr PCHBuffer,
+   IntrusiveRefCntPtr VFS) {
+  // We want only the PCH file from the real filesystem to be available,
+  // so we create an in-memory VFS with just that and overlay it on top.
+  IntrusiveRefCntPtr PCHFS(
+  new vfs::InMemoryFileSystem());
+  PCHFS->addFile(PCHFilename, 0, std::move(PCHBuffer));
+  IntrusiveRefCntPtr Overlay(
+  new vfs::OverlayFileSystem(VFS));
+  Overlay->pushOverlay(PCHFS);
+  return Overlay;
+}
+
 /// Keeps a track of files to be deleted in destructor.
 class TemporaryFiles {
 public:
@@ -101,8 +130,9 @@
 
 class PrecompilePreambleAction : public ASTFrontendAction {
 public:
-  PrecompilePreambleAction(PreambleCallbacks )
-  : Callbacks(Callbacks) {}
+  PrecompilePreambleAction(std::string *InMemStorage,
+   PreambleCallbacks )
+  : InMemStorage(InMemStorage), Callbacks(Callbacks) {}
 
   std::unique_ptr CreateASTConsumer(CompilerInstance ,
  StringRef InFile) override;
@@ -123,6 +153,7 @@
   friend class PrecompilePreambleConsumer;
 
   bool HasEmittedPreamblePCH = false;
+  std::string *InMemStorage;
   PreambleCallbacks 
 };
 
@@ -164,13 +195,18 @@
 
 std::unique_ptr
 PrecompilePreambleAction::CreateASTConsumer(CompilerInstance ,
-
 StringRef InFile) {
   std::string Sysroot;
-  std::string OutputFile;
-  std::unique_ptr OS =
-  GeneratePCHAction::ComputeASTConsumerArguments(CI, InFile, Sysroot,
- OutputFile);
+  if (!GeneratePCHAction::ComputeASTConsumerArguments(CI, Sysroot))
+return nullptr;
+
+  std::unique_ptr OS;
+  if (InMemStorage) {
+OS = llvm::make_unique(*InMemStorage);
+  } else {
+std::string OutputFile;
+OS = GeneratePCHAction::CreateOutputFile(CI, InFile, OutputFile);
+  }
   if (!OS)
 return nullptr;
 
@@ -202,7 +238,7 @@
 const CompilerInvocation ,
 const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds Bounds,
 DiagnosticsEngine , IntrusiveRefCntPtr VFS,
-std::shared_ptr PCHContainerOps,
+std::shared_ptr PCHContainerOps, bool StoreInMemory,
 PreambleCallbacks ) {
   assert(VFS && "VFS is null");
 
@@ -214,12 +250,19 @@
   PreprocessorOptions  =
   PreambleInvocation->getPreprocessorOpts();
 
-  // Create a temporary file for the precompiled preamble. In rare
-  // circumstances, this can fail.
-  llvm::ErrorOr PreamblePCHFile =
-  PrecompiledPreamble::TempPCHFile::CreateNewPreamblePCHFile();
-  if (!PreamblePCHFile)
-return BuildPreambleError::CouldntCreateTempFile;
+  llvm::Optional TempFile;
+  if (!StoreInMemory) {
+// Create a temporary file for the precompiled preamble. In rare
+// circumstances, this can fail.
+llvm::ErrorOr PreamblePCHFile =
+PrecompiledPreamble::TempPCHFile::CreateNewPreamblePCHFile();
+if (!PreamblePCHFile)
+  return BuildPreambleError::CouldntCreateTempFile;
+TempFile = std::move(*PreamblePCHFile);
+  }
+
+  PCHStorage Storage = StoreInMemory ? PCHStorage(InMemoryPreamble())
+ : PCHStorage(std::move(*TempFile));
 
   // Save the preamble text for later; we'll need to compare against it for
   // subsequent reparses.
@@ -230,8 +273,8 @@
 
   // Tell the compiler invocation to generate a temporary precompiled header.
   FrontendOpts.ProgramAction = frontend::GeneratePCH;
-  // FIXME: Generate the precompiled header into memory?
-  FrontendOpts.OutputFile = 

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 123171.
ilya-biryukov added a comment.

- Use a hard-coded virtual path for in-memory PCHs instead of 
system_temp_directory.


https://reviews.llvm.org/D39842

Files:
  include/clang/Frontend/FrontendActions.h
  include/clang/Frontend/PrecompiledPreamble.h
  lib/Frontend/ASTUnit.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Frontend/PrecompiledPreamble.cpp

Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -24,16 +24,46 @@
 #include "clang/Serialization/ASTWriter.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSet.h"
+#include "llvm/Config/config.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/MutexGuard.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 
+#include 
+
 using namespace clang;
 
 namespace {
 
+StringRef getInMemoryPreamblePath() {
+#if defined(LLVM_ON_UNIX)
+  return "/__clang_tmp/___clang_inmemory_preamble___";
+#elif defined(LLVM_ON_WIN32)
+  return "C:\\__clang_tmp\\___clang_inmemory_preamble___";
+#else
+#warning "Unknown platform. Defaulting to UNIX-style paths for in-memory PCHs"
+  return "/__clang_tmp/___clang_inmemory_preamble___";
+#endif
+}
+
+IntrusiveRefCntPtr
+createVFSOverlayForPreamblePCH(StringRef PCHFilename,
+   std::unique_ptr PCHBuffer,
+   IntrusiveRefCntPtr VFS) {
+  // We want only the PCH file from the real filesystem to be available,
+  // so we create an in-memory VFS with just that and overlay it on top.
+  IntrusiveRefCntPtr PCHFS(
+  new vfs::InMemoryFileSystem());
+  PCHFS->addFile(PCHFilename, 0, std::move(PCHBuffer));
+  IntrusiveRefCntPtr Overlay(
+  new vfs::OverlayFileSystem(VFS));
+  Overlay->pushOverlay(PCHFS);
+  return Overlay;
+}
+
 /// Keeps a track of files to be deleted in destructor.
 class TemporaryFiles {
 public:
@@ -101,8 +131,9 @@
 
 class PrecompilePreambleAction : public ASTFrontendAction {
 public:
-  PrecompilePreambleAction(PreambleCallbacks )
-  : Callbacks(Callbacks) {}
+  PrecompilePreambleAction(std::string *InMemStorage,
+   PreambleCallbacks )
+  : InMemStorage(InMemStorage), Callbacks(Callbacks) {}
 
   std::unique_ptr CreateASTConsumer(CompilerInstance ,
  StringRef InFile) override;
@@ -123,6 +154,7 @@
   friend class PrecompilePreambleConsumer;
 
   bool HasEmittedPreamblePCH = false;
+  std::string *InMemStorage;
   PreambleCallbacks 
 };
 
@@ -164,13 +196,18 @@
 
 std::unique_ptr
 PrecompilePreambleAction::CreateASTConsumer(CompilerInstance ,
-
 StringRef InFile) {
   std::string Sysroot;
-  std::string OutputFile;
-  std::unique_ptr OS =
-  GeneratePCHAction::ComputeASTConsumerArguments(CI, InFile, Sysroot,
- OutputFile);
+  if (!GeneratePCHAction::ComputeASTConsumerArguments(CI, Sysroot))
+return nullptr;
+
+  std::unique_ptr OS;
+  if (InMemStorage) {
+OS = llvm::make_unique(*InMemStorage);
+  } else {
+std::string OutputFile;
+OS = GeneratePCHAction::CreateOutputFile(CI, InFile, OutputFile);
+  }
   if (!OS)
 return nullptr;
 
@@ -202,7 +239,7 @@
 const CompilerInvocation ,
 const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds Bounds,
 DiagnosticsEngine , IntrusiveRefCntPtr VFS,
-std::shared_ptr PCHContainerOps,
+std::shared_ptr PCHContainerOps, bool StoreInMemory,
 PreambleCallbacks ) {
   assert(VFS && "VFS is null");
 
@@ -214,12 +251,19 @@
   PreprocessorOptions  =
   PreambleInvocation->getPreprocessorOpts();
 
-  // Create a temporary file for the precompiled preamble. In rare
-  // circumstances, this can fail.
-  llvm::ErrorOr PreamblePCHFile =
-  PrecompiledPreamble::TempPCHFile::CreateNewPreamblePCHFile();
-  if (!PreamblePCHFile)
-return BuildPreambleError::CouldntCreateTempFile;
+  llvm::Optional TempFile;
+  if (!StoreInMemory) {
+// Create a temporary file for the precompiled preamble. In rare
+// circumstances, this can fail.
+llvm::ErrorOr PreamblePCHFile =
+PrecompiledPreamble::TempPCHFile::CreateNewPreamblePCHFile();
+if (!PreamblePCHFile)
+  return BuildPreambleError::CouldntCreateTempFile;
+TempFile = std::move(*PreamblePCHFile);
+  }
+
+  PCHStorage Storage = StoreInMemory ? PCHStorage(InMemoryPreamble())
+ : PCHStorage(std::move(*TempFile));
 
   // Save the preamble text for later; we'll need to compare against it for
   // subsequent reparses.
@@ -230,8 +274,8 @@
 
   // Tell the compiler invocation to generate a temporary precompiled header.
   FrontendOpts.ProgramAction = frontend::GeneratePCH;
-  // FIXME: 

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:44
+SmallString<64> Path;
+llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/true, Path);
+llvm::sys::path::append(Path, "___clang_inmemory_preamble___");

klimek wrote:
> ilya-biryukov wrote:
> > klimek wrote:
> > > ilya-biryukov wrote:
> > > > klimek wrote:
> > > > > erasedOnReboot seems weird for something that we don't want to have 
> > > > > on the actual file system. Why do we even want a temp directory?
> > > > Yeah, `erasedOnReboot` seems weird here.
> > > > What I really wanted is a filepath that's valid on all platforms and is 
> > > > on an existing drive for Windows, so the value of `erasedOnReboot` 
> > > > doesn't really matter. `system_temp_directory` might be the wrong thing 
> > > > to use for that in the first place. Is there a better alternative I'm 
> > > > missing?
> > > Why does it need to get an existing path / existing drive?
> > Oh, maybe it doesn't have to be. But I expect that a probability of 
> > something breaking is lower if we use an existing path.
> > Maybe I'm wrong and it will all work out just fine, but that seems to be a 
> > rather good compromise adding an extra bit of confidence that things will 
> > be working in the long-term.
> I'd on the contrary say that this is more likely to fail. If we don't have a 
> real filesystem, or have a read-only view of a real file system, getting a 
> temp dir might well fail, while we can easily overlay any path we want in 
> memory.
That's true in general, but LLVM's `system_temp_directory` will return the 
default (`/var/tmp` or `/tmp` on Linux, `C:\Temp` on Windows) if it fails to 
find the real temp dir.
So in the very worst case we'll be using hard-coded paths.

On the other hand, if that's one of the possible behaviors, we'd probably want 
to not break in that case anyway. I'll update the patch to follow your 
suggestions. Thanks!


https://reviews.llvm.org/D39842



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

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:44
+SmallString<64> Path;
+llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/true, Path);
+llvm::sys::path::append(Path, "___clang_inmemory_preamble___");

ilya-biryukov wrote:
> klimek wrote:
> > ilya-biryukov wrote:
> > > klimek wrote:
> > > > erasedOnReboot seems weird for something that we don't want to have on 
> > > > the actual file system. Why do we even want a temp directory?
> > > Yeah, `erasedOnReboot` seems weird here.
> > > What I really wanted is a filepath that's valid on all platforms and is 
> > > on an existing drive for Windows, so the value of `erasedOnReboot` 
> > > doesn't really matter. `system_temp_directory` might be the wrong thing 
> > > to use for that in the first place. Is there a better alternative I'm 
> > > missing?
> > Why does it need to get an existing path / existing drive?
> Oh, maybe it doesn't have to be. But I expect that a probability of something 
> breaking is lower if we use an existing path.
> Maybe I'm wrong and it will all work out just fine, but that seems to be a 
> rather good compromise adding an extra bit of confidence that things will be 
> working in the long-term.
I'd on the contrary say that this is more likely to fail. If we don't have a 
real filesystem, or have a read-only view of a real file system, getting a temp 
dir might well fail, while we can easily overlay any path we want in memory.


https://reviews.llvm.org/D39842



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

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:44
+SmallString<64> Path;
+llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/true, Path);
+llvm::sys::path::append(Path, "___clang_inmemory_preamble___");

klimek wrote:
> ilya-biryukov wrote:
> > klimek wrote:
> > > erasedOnReboot seems weird for something that we don't want to have on 
> > > the actual file system. Why do we even want a temp directory?
> > Yeah, `erasedOnReboot` seems weird here.
> > What I really wanted is a filepath that's valid on all platforms and is on 
> > an existing drive for Windows, so the value of `erasedOnReboot` doesn't 
> > really matter. `system_temp_directory` might be the wrong thing to use for 
> > that in the first place. Is there a better alternative I'm missing?
> Why does it need to get an existing path / existing drive?
Oh, maybe it doesn't have to be. But I expect that a probability of something 
breaking is lower if we use an existing path.
Maybe I'm wrong and it will all work out just fine, but that seems to be a 
rather good compromise adding an extra bit of confidence that things will be 
working in the long-term.


https://reviews.llvm.org/D39842



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

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:44
+SmallString<64> Path;
+llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/true, Path);
+llvm::sys::path::append(Path, "___clang_inmemory_preamble___");

ilya-biryukov wrote:
> klimek wrote:
> > erasedOnReboot seems weird for something that we don't want to have on the 
> > actual file system. Why do we even want a temp directory?
> Yeah, `erasedOnReboot` seems weird here.
> What I really wanted is a filepath that's valid on all platforms and is on an 
> existing drive for Windows, so the value of `erasedOnReboot` doesn't really 
> matter. `system_temp_directory` might be the wrong thing to use for that in 
> the first place. Is there a better alternative I'm missing?
Why does it need to get an existing path / existing drive?


https://reviews.llvm.org/D39842



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

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:44
+SmallString<64> Path;
+llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/true, Path);
+llvm::sys::path::append(Path, "___clang_inmemory_preamble___");

klimek wrote:
> erasedOnReboot seems weird for something that we don't want to have on the 
> actual file system. Why do we even want a temp directory?
Yeah, `erasedOnReboot` seems weird here.
What I really wanted is a filepath that's valid on all platforms and is on an 
existing drive for Windows, so the value of `erasedOnReboot` doesn't really 
matter. `system_temp_directory` might be the wrong thing to use for that in the 
first place. Is there a better alternative I'm missing?


https://reviews.llvm.org/D39842



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

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Manuel Klimek via Phabricator via cfe-commits]

klimek accepted this revision.
klimek added a comment.
This revision is now accepted and ready to land.

LG




Comment at: lib/Frontend/PrecompiledPreamble.cpp:490
   PreprocessorOpts.DisablePCHValidation = true;
+  if (Storage.getKind() == PCHStorage::Kind::TempFile) {
+const TempPCHFile  = Storage.asFile();

ilya-biryukov wrote:
> klimek wrote:
> > ilya-biryukov wrote:
> > > klimek wrote:
> > > > This looks a bit like we should push it into the PCHStorage.
> > > I've extracted a function here to make the code read simpler.
> > > However, I placed it directly into the `PrecompiledPreamble` instead of 
> > > `PCHStorage` to keep `PCHStorage` responsible for just one thing: 
> > > managing the `variant`-like union.
> > It being called PCHStorage makes it sound like it handles the abstraction 
> > for how the preamble is stored. Given that the variant-like union is 
> > basically an interface with an implementation switch, I think all switching 
> > on it is also the responsibility of that class. Otherwise we'd need another 
> > abstraction on top of it?
> Abstraction on top of it is `PrecompiledPreamble` itself. And `PCHStorage` is 
> just an implementation detail.
> Does that sound reasonable?
That makes more sense now, thx for explaining. It still seems a bit off to me, 
but I can't come up with a better solution, so LG.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:44
+SmallString<64> Path;
+llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/true, Path);
+llvm::sys::path::append(Path, "___clang_inmemory_preamble___");

erasedOnReboot seems weird for something that we don't want to have on the 
actual file system. Why do we even want a temp directory?


https://reviews.llvm.org/D39842



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

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:206
+  std::unique_ptr Storage;
+  if (InMemStorage) {
+OS = llvm::make_unique(*InMemStorage);

klimek wrote:
> ilya-biryukov wrote:
> > klimek wrote:
> > > It looks like we should pass in the output stream, not the storage?
> > We're not actually using the `Storage` variable, it's a leftover from 
> > previous versions. Removed it.
> > 
> > Or did you mean that we should pass in the output stream directly to 
> > `PrecompilePreambleAction`'s constructor?
> Yes, I'm generally looking at things that might be better to decide at a 
> higher abstraction level and pass in, rather than having switches for 
> behavior (like InMemStorage) all over the place. Generally, I think we should 
> have a storage (PCHStorage sounds like it was the right abstraction, but 
> perhaps that one currently has a bad name) and the things dealing with that 
> storage shouldn't care whether it's in memory or on the file system.
It's a bit easier to share code with `GeneratePCHAction` this way in a way that 
would obviously works (as we call the same functions at the same points in the 
compilation pipeline, that is in `CreateASTConsumer`). 

`PCHStorage` is not a public interface and `PrecompiledPreamble` is exactly the 
interface that let's you use it without caring whether PCHs are stored in 
memory or on disk. It also feels ok for it to have the storage-dependent code 
as part of its own implementation.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:490
   PreprocessorOpts.DisablePCHValidation = true;
+  if (Storage.getKind() == PCHStorage::Kind::TempFile) {
+const TempPCHFile  = Storage.asFile();

klimek wrote:
> ilya-biryukov wrote:
> > klimek wrote:
> > > This looks a bit like we should push it into the PCHStorage.
> > I've extracted a function here to make the code read simpler.
> > However, I placed it directly into the `PrecompiledPreamble` instead of 
> > `PCHStorage` to keep `PCHStorage` responsible for just one thing: managing 
> > the `variant`-like union.
> It being called PCHStorage makes it sound like it handles the abstraction for 
> how the preamble is stored. Given that the variant-like union is basically an 
> interface with an implementation switch, I think all switching on it is also 
> the responsibility of that class. Otherwise we'd need another abstraction on 
> top of it?
Abstraction on top of it is `PrecompiledPreamble` itself. And `PCHStorage` is 
just an implementation detail.
Does that sound reasonable?


https://reviews.llvm.org/D39842



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

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:206
+  std::unique_ptr Storage;
+  if (InMemStorage) {
+OS = llvm::make_unique(*InMemStorage);

ilya-biryukov wrote:
> klimek wrote:
> > It looks like we should pass in the output stream, not the storage?
> We're not actually using the `Storage` variable, it's a leftover from 
> previous versions. Removed it.
> 
> Or did you mean that we should pass in the output stream directly to 
> `PrecompilePreambleAction`'s constructor?
Yes, I'm generally looking at things that might be better to decide at a higher 
abstraction level and pass in, rather than having switches for behavior (like 
InMemStorage) all over the place. Generally, I think we should have a storage 
(PCHStorage sounds like it was the right abstraction, but perhaps that one 
currently has a bad name) and the things dealing with that storage shouldn't 
care whether it's in memory or on the file system.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:490
   PreprocessorOpts.DisablePCHValidation = true;
+  if (Storage.getKind() == PCHStorage::Kind::TempFile) {
+const TempPCHFile  = Storage.asFile();

ilya-biryukov wrote:
> klimek wrote:
> > This looks a bit like we should push it into the PCHStorage.
> I've extracted a function here to make the code read simpler.
> However, I placed it directly into the `PrecompiledPreamble` instead of 
> `PCHStorage` to keep `PCHStorage` responsible for just one thing: managing 
> the `variant`-like union.
It being called PCHStorage makes it sound like it handles the abstraction for 
how the preamble is stored. Given that the variant-like union is basically an 
interface with an implementation switch, I think all switching on it is also 
the responsibility of that class. Otherwise we'd need another abstraction on 
top of it?


https://reviews.llvm.org/D39842



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

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: lib/Frontend/ASTUnit.cpp:1028
+IntrusiveRefCntPtr OldVFS = VFS;
+Preamble->AddImplicitPreamble(*CCInvocation, /*ref*/ VFS,
+  OverrideMainBuffer.get());

klimek wrote:
> Since when are we using the /*ref*/ annotation?
Oh, sorry, that's my thing. Slipped into this change.
Removed those.



Comment at: lib/Frontend/PrecompiledPreamble.cpp:206
+  std::unique_ptr Storage;
+  if (InMemStorage) {
+OS = llvm::make_unique(*InMemStorage);

klimek wrote:
> It looks like we should pass in the output stream, not the storage?
We're not actually using the `Storage` variable, it's a leftover from previous 
versions. Removed it.

Or did you mean that we should pass in the output stream directly to 
`PrecompilePreambleAction`'s constructor?



Comment at: lib/Frontend/PrecompiledPreamble.cpp:490
   PreprocessorOpts.DisablePCHValidation = true;
+  if (Storage.getKind() == PCHStorage::Kind::TempFile) {
+const TempPCHFile  = Storage.asFile();

klimek wrote:
> This looks a bit like we should push it into the PCHStorage.
I've extracted a function here to make the code read simpler.
However, I placed it directly into the `PrecompiledPreamble` instead of 
`PCHStorage` to keep `PCHStorage` responsible for just one thing: managing the 
`variant`-like union.


https://reviews.llvm.org/D39842



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

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 122838.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.

- Fixed comments.
- Removed /*ref*/ annotations.
- Removed unused "Storage" variable.
- Extract a helper function that properly sets up VFS to access the PCHStorage.


https://reviews.llvm.org/D39842

Files:
  include/clang/Frontend/FrontendActions.h
  include/clang/Frontend/PrecompiledPreamble.h
  lib/Frontend/ASTUnit.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Frontend/PrecompiledPreamble.cpp

Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -28,12 +28,41 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/MutexGuard.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 
+#include 
+
 using namespace clang;
 
 namespace {
 
+StringRef getInMemoryPreamblePath() {
+  // Note that the lambda is called inline to initialize the variable.
+  static auto PreambleName = []() {
+SmallString<64> Path;
+llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/true, Path);
+llvm::sys::path::append(Path, "___clang_inmemory_preamble___");
+return Path;
+  }();
+  return PreambleName;
+}
+
+IntrusiveRefCntPtr
+createVFSOverlayForPreamblePCH(StringRef PCHFilename,
+   std::unique_ptr PCHBuffer,
+   IntrusiveRefCntPtr VFS) {
+  // We want only the PCH file from the real filesystem to be available,
+  // so we create an in-memory VFS with just that and overlay it on top.
+  IntrusiveRefCntPtr PCHFS(
+  new vfs::InMemoryFileSystem());
+  PCHFS->addFile(PCHFilename, 0, std::move(PCHBuffer));
+  IntrusiveRefCntPtr Overlay(
+  new vfs::OverlayFileSystem(VFS));
+  Overlay->pushOverlay(PCHFS);
+  return Overlay;
+}
+
 /// Keeps a track of files to be deleted in destructor.
 class TemporaryFiles {
 public:
@@ -101,8 +130,9 @@
 
 class PrecompilePreambleAction : public ASTFrontendAction {
 public:
-  PrecompilePreambleAction(PreambleCallbacks )
-  : Callbacks(Callbacks) {}
+  PrecompilePreambleAction(std::string *InMemStorage,
+   PreambleCallbacks )
+  : InMemStorage(InMemStorage), Callbacks(Callbacks) {}
 
   std::unique_ptr CreateASTConsumer(CompilerInstance ,
  StringRef InFile) override;
@@ -123,6 +153,7 @@
   friend class PrecompilePreambleConsumer;
 
   bool HasEmittedPreamblePCH = false;
+  std::string *InMemStorage;
   PreambleCallbacks 
 };
 
@@ -164,13 +195,18 @@
 
 std::unique_ptr
 PrecompilePreambleAction::CreateASTConsumer(CompilerInstance ,
-
 StringRef InFile) {
   std::string Sysroot;
-  std::string OutputFile;
-  std::unique_ptr OS =
-  GeneratePCHAction::ComputeASTConsumerArguments(CI, InFile, Sysroot,
- OutputFile);
+  if (!GeneratePCHAction::ComputeASTConsumerArguments(CI, Sysroot))
+return nullptr;
+
+  std::unique_ptr OS;
+  if (InMemStorage) {
+OS = llvm::make_unique(*InMemStorage);
+  } else {
+std::string OutputFile;
+OS = GeneratePCHAction::CreateOutputFile(CI, InFile, OutputFile);
+  }
   if (!OS)
 return nullptr;
 
@@ -202,7 +238,7 @@
 const CompilerInvocation ,
 const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds Bounds,
 DiagnosticsEngine , IntrusiveRefCntPtr VFS,
-std::shared_ptr PCHContainerOps,
+std::shared_ptr PCHContainerOps, bool StoreInMemory,
 PreambleCallbacks ) {
   assert(VFS && "VFS is null");
 
@@ -214,12 +250,19 @@
   PreprocessorOptions  =
   PreambleInvocation->getPreprocessorOpts();
 
-  // Create a temporary file for the precompiled preamble. In rare
-  // circumstances, this can fail.
-  llvm::ErrorOr PreamblePCHFile =
-  PrecompiledPreamble::TempPCHFile::CreateNewPreamblePCHFile();
-  if (!PreamblePCHFile)
-return BuildPreambleError::CouldntCreateTempFile;
+  llvm::Optional TempFile;
+  if (!StoreInMemory) {
+// Create a temporary file for the precompiled preamble. In rare
+// circumstances, this can fail.
+llvm::ErrorOr PreamblePCHFile =
+PrecompiledPreamble::TempPCHFile::CreateNewPreamblePCHFile();
+if (!PreamblePCHFile)
+  return BuildPreambleError::CouldntCreateTempFile;
+TempFile = std::move(*PreamblePCHFile);
+  }
+
+  PCHStorage Storage = StoreInMemory ? PCHStorage(InMemoryPreamble())
+ : PCHStorage(std::move(*TempFile));
 
   // Save the preamble text for later; we'll need to compare against it for
   // subsequent reparses.
@@ -230,8 +273,8 @@
 
   // Tell the compiler invocation to generate a temporary precompiled header.
   FrontendOpts.ProgramAction = frontend::GeneratePCH;
-  // FIXME: Generate the precompiled header into memory?
-  

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.



Comment at: include/clang/Frontend/PrecompiledPreamble.h:99
   /// Changes options inside \p CI to use PCH from this preamble. Also remaps
-  /// main file to \p MainFileBuffer.
+  /// main file to \p MainFileBuffer and updates \p VFS to ensure preamble is
+  /// accessible.

...ensure *the* preamble...



Comment at: lib/Frontend/ASTUnit.cpp:1028
+IntrusiveRefCntPtr OldVFS = VFS;
+Preamble->AddImplicitPreamble(*CCInvocation, /*ref*/ VFS,
+  OverrideMainBuffer.get());

Since when are we using the /*ref*/ annotation?



Comment at: lib/Frontend/PrecompiledPreamble.cpp:206
+  std::unique_ptr Storage;
+  if (InMemStorage) {
+OS = llvm::make_unique(*InMemStorage);

It looks like we should pass in the output stream, not the storage?



Comment at: lib/Frontend/PrecompiledPreamble.cpp:490
   PreprocessorOpts.DisablePCHValidation = true;
+  if (Storage.getKind() == PCHStorage::Kind::TempFile) {
+const TempPCHFile  = Storage.asFile();

This looks a bit like we should push it into the PCHStorage.


https://reviews.llvm.org/D39842



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

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I'm really happy you've made this work! I don't understand it enough to do a 
meaningful review (keen to learn if you have time for a walkthrough when back 
in the office).


https://reviews.llvm.org/D39842



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

[PATCH] D39842: Allow to store precompiled preambles in memory.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.

These preambles are built by ASTUnit and clangd. Previously, preambles
were always stored on disk.

In-memory preambles are routed back to the compiler as virtual files in
a custom VFS.

Interface of ASTUnit does not allow to use in-memory preambles, as
ASTUnit::CodeComplete receives FileManager as a parameter, so we can't
change VFS used by the compiler inside the CodeComplete method.

A follow-up commit will update clangd in clang-tools-extra to use
in-memory preambles.

Proper variant-like storage for Preambles.

Final fixes and adjustments.


https://reviews.llvm.org/D39842

Files:
  include/clang/Frontend/FrontendActions.h
  include/clang/Frontend/PrecompiledPreamble.h
  lib/Frontend/ASTUnit.cpp
  lib/Frontend/FrontendActions.cpp
  lib/Frontend/PrecompiledPreamble.cpp

Index: lib/Frontend/PrecompiledPreamble.cpp
===
--- lib/Frontend/PrecompiledPreamble.cpp
+++ lib/Frontend/PrecompiledPreamble.cpp
@@ -28,12 +28,42 @@
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/MutexGuard.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 
+#include 
+
 using namespace clang;
 
 namespace {
 
+StringRef getInMemoryPreamblePath() {
+  // Note that the lambda is called inline to initialize the variable.
+  static auto PreambleName = []() {
+SmallString<64> Path;
+llvm::sys::path::system_temp_directory(/*erasedOnReboot=*/true,
+   /*ref*/ Path);
+llvm::sys::path::append(Path, "___clang_inmemory_preamble___");
+return Path;
+  }();
+  return PreambleName;
+}
+
+IntrusiveRefCntPtr
+createVFSOverlayForPreamblePCH(StringRef PCHFilename,
+   std::unique_ptr PCHBuffer,
+   IntrusiveRefCntPtr VFS) {
+  // We want only the PCH file from the real filesystem to be available,
+  // so we create an in-memory VFS with just that and overlay it on top.
+  IntrusiveRefCntPtr PCHFS(
+  new vfs::InMemoryFileSystem());
+  PCHFS->addFile(PCHFilename, 0, std::move(PCHBuffer));
+  IntrusiveRefCntPtr Overlay(
+  new vfs::OverlayFileSystem(VFS));
+  Overlay->pushOverlay(PCHFS);
+  return Overlay;
+}
+
 /// Keeps a track of files to be deleted in destructor.
 class TemporaryFiles {
 public:
@@ -101,8 +131,9 @@
 
 class PrecompilePreambleAction : public ASTFrontendAction {
 public:
-  PrecompilePreambleAction(PreambleCallbacks )
-  : Callbacks(Callbacks) {}
+  PrecompilePreambleAction(std::string *InMemStorage,
+   PreambleCallbacks )
+  : InMemStorage(InMemStorage), Callbacks(Callbacks) {}
 
   std::unique_ptr CreateASTConsumer(CompilerInstance ,
  StringRef InFile) override;
@@ -123,6 +154,7 @@
   friend class PrecompilePreambleConsumer;
 
   bool HasEmittedPreamblePCH = false;
+  std::string *InMemStorage;
   PreambleCallbacks 
 };
 
@@ -164,13 +196,19 @@
 
 std::unique_ptr
 PrecompilePreambleAction::CreateASTConsumer(CompilerInstance ,
-
 StringRef InFile) {
   std::string Sysroot;
-  std::string OutputFile;
-  std::unique_ptr OS =
-  GeneratePCHAction::ComputeASTConsumerArguments(CI, InFile, Sysroot,
- OutputFile);
+  if (!GeneratePCHAction::ComputeASTConsumerArguments(CI, /*ref*/ Sysroot))
+return nullptr;
+
+  std::unique_ptr OS;
+  std::unique_ptr Storage;
+  if (InMemStorage) {
+OS = llvm::make_unique(*InMemStorage);
+  } else {
+std::string OutputFile;
+OS = GeneratePCHAction::CreateOutputFile(CI, InFile, /*ref*/ OutputFile);
+  }
   if (!OS)
 return nullptr;
 
@@ -202,7 +240,7 @@
 const CompilerInvocation ,
 const llvm::MemoryBuffer *MainFileBuffer, PreambleBounds Bounds,
 DiagnosticsEngine , IntrusiveRefCntPtr VFS,
-std::shared_ptr PCHContainerOps,
+std::shared_ptr PCHContainerOps, bool StoreInMemory,
 PreambleCallbacks ) {
   assert(VFS && "VFS is null");
 
@@ -214,12 +252,19 @@
   PreprocessorOptions  =
   PreambleInvocation->getPreprocessorOpts();
 
-  // Create a temporary file for the precompiled preamble. In rare
-  // circumstances, this can fail.
-  llvm::ErrorOr PreamblePCHFile =
-  PrecompiledPreamble::TempPCHFile::CreateNewPreamblePCHFile();
-  if (!PreamblePCHFile)
-return BuildPreambleError::CouldntCreateTempFile;
+  llvm::Optional TempFile;
+  if (!StoreInMemory) {
+// Create a temporary file for the precompiled preamble. In rare
+// circumstances, this can fail.
+llvm::ErrorOr PreamblePCHFile =
+PrecompiledPreamble::TempPCHFile::CreateNewPreamblePCHFile();
+if (!PreamblePCHFile)
+  return BuildPreambleError::CouldntCreateTempFile;
+TempFile = std::move(*PreamblePCHFile);
+  }
+
+  PCHStorage Storage = StoreInMemory ? PCHStorage(InMemoryPreamble())
+