option to write files to memory instead of disk

[marcrasi]

Summary

This allows clients to invoke clang without writing anything to disk.

I'm not sure if this is the right design, so I'm sending this patch to
start a discussion about the right way to do this. I'll be happy to make
changes and add tests after talking about the right way to do this!

Motivation:

At Google, we run some Swift source tooling on diskless servers. The
Swift source tooling calls Clang through the ClangImporter
(https://github.com/apple/swift/blob/master/lib/ClangImporter/ClangImporter.cpp).
Clang compiles module to an on-disk module cache, which does not work on
the diskless servers. This patch lets clients ask Clang to write the
module cache to memory.

Current design:

• Adds class InMemoryOutputFileSystem : public llvm::vfs::FileSystem that supports write operations backed by memory. After a file is written, its contents are accessible through the llvm::vfs::FileSystem interface.
• Adds an InMemoryOutputFileSystem field to CompilerInstance. When this field is set, the CompilerInstance writes to the InMemoryOutputFileSystem instead of the real file system.

[marcrasi]

Ping on this. Could anyone take a look?

[sammccall]

It's great that you're looking at this! Clang tooling in general and clangd in particular a lot of use of VFS (particularly when deployed inside google), so I've got a fair bit of interest/experience in these interfaces. And we'd also looked at virtualizing module caches in particular for clangd (but haven't tackled it yet).

A couple of high-level design questions:

• if the goal is really to make module caching work without a physical disk, is virtualizing the FS underlying the cache really the right abstraction? Compared to an alternative like plugging in a ModuleCache interface, it seems like quite a complicated contract to have to maintain, and less flexible too (we've dreamed about ideas like a network-distributed cache, expiry, selectively sharing cached modules based on security context etc... easier if the objects are modules+metadata rather than filenames+bytes). Of course it's pretty generic and maybe can be used for other stuff too, but...
• if we *are* going to introduce a writable VFS concept to LLVM, I feel we must allow this to be reused for virtualizing file writes in general. (clang has several ad-hoc mechanisms for virtual file inputs that AIUI predate llvm::vfs, and they're a pain). I think at minimum we should ensure that the writable FS is an interface rather than just one implementation, and that we can implement it well for the real FS also. Does this seem reasonable?

[dexonsmith]

I think this is important; thanks for working on it!

I'm sorry I missed it before... coincidentally, I'm working in a similar problem space right now (likely I'll post a patch with an RFC next week) but my design is a bit different.

The main abstraction I came up with is in two parts:

• An OutputManager, which can support opening / closing outputs with configurable destinations (kind of like a chain of consumers, but my initial design is monolithic to simplify optional optimizations like using a WriteThroughMemoryBuffer for mirroring a just-written on-disk file in memory). The expectation is that various CompilerInstances (and other things) could share one of these. This is not a file system; it just has very simple "open" / "close" / "erase" APIs for output paths (initially it has three possible destinations: on-disk, in-memory (store in an InMemoryFileSystem), and a request-provided raw_pwrite_ostream; it can do any or all of these).
• An OutputContext, which can be instantiated to track multiple outputs in a specific context. Each CompilerInstance would have one of these, effectively replacing the std::list<OutputFile> it currently has (clearOutputFiles would call OutputContext::closeAllOutputs, which then calls OutputManager::closeOutput for each output its tracking). This can have partial configuration (partially overriding the manager's default configuration).

(Initially, I was thinking of a virtual base class OutputManager with subclasses OnDiskOutputManager and InMemoryOutputManager, and I didn't have OutputContext as a separate abstraction, but I don't think that gives us the flexibility we need for various use cases; see below.)

I also have:

• OutputConfig, configuration for a specific file; the default configuration lives on OutputManager (and is configurable).
• PartialOutputConfig, a partial configuration that overrides only some of a configuration; can be put on an OutputContext ("mirror all of my outputs in memory" or "don't write to disk, only to memory, for these outputs"), or put on an individual "open file" request ("the client needs seeking support for this output").

These are the use cases I think we should support:

• A "normal" clang invocation without implicit modules should just write to disk.
• If invoking clang from the command-line and implicit modules is turned on, the modules should be mirrored in memory (but also written to disk). My idea here is that VFS is constructed with an OverlayFileSystem at the top level and an initially-empty InMemoryFileSystem underneath in front of the "usual" one. The OutputContexts for the modules builds would be configured to mirror all outputs into the InMemoryFileSystem. (This is a step toward dropping the rather awkward InMemoryModuleCache; I think that will be easy once this lands.)
• Index services often don't want outputs written to disk. If they're happy passing in an InMemoryFileSystem to the OutputManager, they can use the same approach.
• When clang-scan-deps is used to do a modules pre-scan, its source-minimized modules should never hit the disk. The OutputManager can be configured to only fill up the InMemoryFileSystem.
• For unit tests or a -cc1 compile-in-memory API where we care about a single output file, it might be nice to be able to ignore most files (write to raw_null_ostream), but get back specific file content and/or configure certain files in some special way.
• There are lots of other outputs in clang (e.g., index-while-building files from -index-store-path, which is still being upstreamed), which all do their own temp-file dance (... or worse, they don't), and they should be able to delete code and use this as well; I think even references to errs() should go through so this could be captured in unit tests (or an in-memory compile API, or...).
• It should be easy to cleanly add more output destinations (e.g., CAS) as we find the need, and OutputContext users would not need to know about specific destinations unless they wanted some special configuration.

I'll reply here once I've posted the RFC and patch (as I said, I'm hoping next week) so you can take a look.

[dexonsmith]

In D78058#2464262, @dexonsmith wrote:

I'll reply here once I've posted the RFC and patch (as I said, I'm hoping next week) so you can take a look.

I don't quite have my patch and RFC ready (and I may not until I'm back from holidays in January), but if you're curious the current state is at https://github.com/dexonsmith/llvm-project/tree/output-manager / https://github.com/dexonsmith/llvm-project/commit/924cb118272acab5f031c04f55465ae4799be4bf.

[sammccall]

@dexonsmith thanks for sharing!
Some initial thoughts since abstracting outputs is something we're starting to care about too...

This doesn't appear to be an extension point - you can write to an InMemoryFS or to real disk, but not to anything else. If we're going to add abstractions around output, I think they should support flexible use in libraries (which doesn't need to cost complexity here: FS vs MemoryFS vs custom storage can implement the same interface).
With the right interface, I think this obviates the need for RequestedOutput and generalizes it - a caller can install an intercepting output backend that hooks certain paths and delegates the rest to the old backend.
(Of course such a backend could be provided, but it gets the complexity out of the way of the core abstractions)

I think the lifecycle and allowed operations on outputs would be clearer if an Output class was returned, with methods, rather than a stream + handle and various "free" functions (well, methods on OutputManager) than manipulate the handle.
This would again make it easier to reason about what operations are part of a storage backend: there'd be some OutputBackend interface to implement, and it would hand out Output objects which again must be implemented. (This would be reminiscent of FileManager + vfs::FileSystem + vfs::File).

[dexonsmith]

In D78058#2471735, @sammccall wrote:

@dexonsmith thanks for sharing!
Some initial thoughts since abstracting outputs is something we're starting to care about too...

Thanks for looking.

This doesn't appear to be an extension point - you can write to an InMemoryFS or to real disk, but not to anything else. If we're going to add abstractions around output, I think they should support flexible use in libraries (which doesn't need to cost complexity here: FS vs MemoryFS vs custom storage can implement the same interface).
With the right interface, I think this obviates the need for RequestedOutput and generalizes it - a caller can install an intercepting output backend that hooks certain paths and delegates the rest to the old backend.
(Of course such a backend could be provided, but it gets the complexity out of the way of the core abstractions)

I agree with your concerns with the monolithic design. My motivation for it is to get the memory optimization of using a write-through memory buffer when the output is needed both on-disk and in-memory. I imagine similar concerns if/when we add a CAS-based storage option, where it's likely more efficient to ask the CAS to materialize the on-disk file than to write it ourselves.

I'll think about whether there's a clean way to model these kinds of interactions between storage implementations without a monolithic design; let me know if you have any concrete ideas.

Another option I see is to have an abstract extension point that allows libraries to plug in custom storage, but keep the monolithic design for on-disk and in-memory (and eventually CAS).

I think the lifecycle and allowed operations on outputs would be clearer if an Output class was returned, with methods, rather than a stream + handle and various "free" functions (well, methods on OutputManager) than manipulate the handle.
This would again make it easier to reason about what operations are part of a storage backend: there'd be some OutputBackend interface to implement, and it would hand out Output objects which again must be implemented. (This would be reminiscent of FileManager + vfs::FileSystem + vfs::File).

I'll think some more about the options here. The current design is motivated by how outputs are currently used / referenced in CompilerInstance (and how the streams are passed around separately to APIs that don't know anything about outputs); i.e., the current design seems simple to land as an incremental change. But it's probably not hard to make it a bit cleaner.

[dexonsmith]

In D78058#2480411, @dexonsmith wrote:

In D78058#2471735, @sammccall wrote:

@dexonsmith thanks for sharing!
Some initial thoughts since abstracting outputs is something we're starting to care about too...

Thanks for looking.

I think I've found a fairly clean way to get the write-through memory buffer optimization with pluggable backends; still working through the other feedback, but hoping to have the patch / RFC out soonish.

[dexonsmith]

In D78058#2481050, @dexonsmith wrote:

In D78058#2480411, @dexonsmith wrote:

In D78058#2471735, @sammccall wrote:

@dexonsmith thanks for sharing!
Some initial thoughts since abstracting outputs is something we're starting to care about too...

Thanks for looking.

I think I've found a fairly clean way to get the write-through memory buffer optimization with pluggable backends; still working through the other feedback, but hoping to have the patch / RFC out soonish.

FYI, I posted an RFC here: https://lists.llvm.org/pipermail/cfe-dev/2021-January/067576.html

clang/include/clang/Basic/InMemoryOutputFileSystem.h

+//===-- InMemoryOutputFileSystem.h - Collects outputs in memory -*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_BASIC_INMEMORYOUTPUTFILESYSTEM_H_
+#define LLVM_CLANG_BASIC_INMEMORYOUTPUTFILESYSTEM_H_
+
+#include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include <memory>
+
+namespace clang {
+
+/// Collects output files in memory, and provides a `llvm::vfs::FileSystem`
+/// interface for accessing those files.
+///
+/// This class is threadsafe. Unsynchronized calls from multiple threads will
+/// not corrupt the internal state, and operations occur atomically and
+/// sequentially consistently from the point of view of all threads.
+class InMemoryOutputFileSystem : public llvm::vfs::FileSystem {
+public:
+  InMemoryOutputFileSystem()
+      : OutputFiles(new llvm::vfs::InMemoryFileSystem()) {}
+
+  /// Creates a temporary buffer that collects data for a file that may
+  /// eventually appear on the `llvm::vfs::FileSystem` interface.
+  /// `InMemoryOutputFileSystem` owns the buffer, which will not be released
+  /// until `DeleteTemporaryFile` or `FinalizeTemporaryFile` is called.
+  /// \param OutputPath the path of the file that may eventually be created.
+  /// \param TemporaryPath must be non-null. Pointee will be set to a unique
+  //         string identifying this particular temporary buffer.
+  //  \returns A stream that can be used to write to the buffer.
+  std::unique_ptr<llvm::raw_pwrite_stream>
+  CreateTemporaryBuffer(llvm::StringRef OutputPath, std::string *TemporaryPath);
+
+  /// Releases the buffer underlying the temporary file.
+  /// \param TemporaryPath the unique string from `CreateTemporaryFile`.
+  void DeleteTemporaryBuffer(llvm::StringRef TemporaryPath);
+
+  /// Makes the contents of the specified temporary buffer visible on the
+  /// `llvm::vfs::FileSystem` interface, and releases the temporary buffer. If
+  /// the file already exists on the `llvm::vfs::FileSystem` interface, then
+  /// the new contents is silently ignored.
+  /// \param OutputPath the path of the file to create.
+  /// \param TemporaryPath the unique string from `CreateTemporaryFile`.
+  void FinalizeTemporaryBuffer(llvm::StringRef OutputPath,
+                               llvm::StringRef TemporaryPath);
+
+  // MARK: - `llvm::vfs::FileSystem` overrides
+
+  llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &relpath) override {
+    std::lock_guard<std::mutex> locked(Mu);
+    return OutputFiles->status(relpath);
+  }
+
+  llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
+  openFileForRead(const llvm::Twine &relpath) override {
+    std::lock_guard<std::mutex> locked(Mu);
+    return OutputFiles->openFileForRead(relpath);
+  }
+
+  llvm::vfs::directory_iterator dir_begin(const llvm::Twine &reldir,
+                                          std::error_code &err) override {
+    std::lock_guard<std::mutex> locked(Mu);
+    return OutputFiles->dir_begin(reldir, err);
+  }
+
+  std::error_code setCurrentWorkingDirectory(const llvm::Twine &path) override {
+    std::lock_guard<std::mutex> locked(Mu);
+    return OutputFiles->setCurrentWorkingDirectory(path);
+  }
+
+  llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override {
+    std::lock_guard<std::mutex> locked(Mu);
+    return OutputFiles->getCurrentWorkingDirectory();
+  }
+
+  std::error_code
+  getRealPath(const llvm::Twine &path,
+              llvm::SmallVectorImpl<char> &output) const override {
+    std::lock_guard<std::mutex> locked(Mu);
+    return OutputFiles->getRealPath(path, output);
+  }
+
+private:
+  mutable std::mutex Mu;
+  llvm::StringMap<llvm::SmallVector<char, 0>> TemporaryBuffers;
+  llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> OutputFiles;
+};
+
+} // namespace clang
+
+#endif // LLVM_CLANG_BASIC_INMEMORYOUTPUTFILESYSTEM_H_

clang/include/clang/Frontend/CompilerInstance.h

 //===-- CompilerInstance.h - Clang Compiler Instance ------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_FRONTEND_COMPILERINSTANCE_H_
 #define LLVM_CLANG_FRONTEND_COMPILERINSTANCE_H_
 
 #include "clang/AST/ASTConsumer.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/InMemoryOutputFileSystem.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
 #include "clang/Lex/HeaderSearchOptions.h"
 #include "clang/Lex/ModuleLoader.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
class CompilerInstance : public ModuleLoader {
   std::unique_ptr<llvm::raw_fd_ostream> NonSeekStream;
 
   /// The list of active output files.
   std::list<OutputFile> OutputFiles;
 
   /// Force an output buffer.
   std::unique_ptr<llvm::raw_pwrite_stream> OutputStream;
 
+  /// If defined, outputs will be written here instead of to the real
+  /// filesystem.
+  IntrusiveRefCntPtr<InMemoryOutputFileSystem> InMemoryOutputFileSystem;
+
   CompilerInstance(const CompilerInstance &) = delete;
   void operator=(const CompilerInstance &) = delete;
 public:
   explicit CompilerInstance(
       std::shared_ptr<PCHContainerOperations> PCHContainerOps =
           std::make_shared<PCHContainerOperations>(),
       InMemoryModuleCache *SharedModuleCache = nullptr);
   ~CompilerInstance() override;
public:
 
   /// }
   /// @name Virtual File System
   /// {
 
   llvm::vfs::FileSystem &getVirtualFileSystem() const;
 
   /// }
+  /// @name In-Memory Output File System
+  /// {
+
+  IntrusiveRefCntPtr<clang::InMemoryOutputFileSystem>
+  getInMemoryOutputFileSystem() const {
+    return InMemoryOutputFileSystem;
+  }
+
+  void setInMemoryOutputFileSystem(
+      IntrusiveRefCntPtr<clang::InMemoryOutputFileSystem> FS) {
+    InMemoryOutputFileSystem = std::move(FS);
+  }
+
+  /// }
   /// @name File Manager
   /// {
 
   bool hasFileManager() const { return FileMgr != nullptr; }
 
   /// Return the current file manager to the caller.
   FileManager &getFileManager() const {
     assert(FileMgr && "Compiler instance has no file manager!");

clang/lib/Basic/CMakeLists.txt

add_clang_library(clangBasic
   Cuda.cpp
   Diagnostic.cpp
   DiagnosticIDs.cpp
   DiagnosticOptions.cpp
   FileManager.cpp
   FileSystemStatCache.cpp
   FixedPoint.cpp
   IdentifierTable.cpp
+  InMemoryOutputFileSystem.cpp
   LangOptions.cpp
   LangStandards.cpp
   Module.cpp
   ObjCRuntime.cpp
   OpenMPKinds.cpp
   OperatorPrecedence.cpp
   SanitizerBlacklist.cpp
   SanitizerSpecialCaseList.cpp

clang/lib/Basic/InMemoryOutputFileSystem.cpp

+//=== InMemoryOutputFileSystem.cpp - Collects outputs in memory -*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Basic/InMemoryOutputFileSystem.h"
+
+namespace clang {
+
+std::unique_ptr<llvm::raw_pwrite_stream>
+InMemoryOutputFileSystem::CreateTemporaryBuffer(llvm::StringRef OutputPath,
+                                                std::string *TemporaryPath) {
+  assert(TemporaryPath);
+  std::lock_guard<std::mutex> locked(Mu);
+  llvm::StringMap<llvm::SmallVector<char, 0>>::iterator it;
+  bool inserted = false;
+  unsigned suffix = 0;
+  while (!inserted) {
+    *TemporaryPath = "";
+    llvm::raw_string_ostream TemporaryPathOS(*TemporaryPath);
+    TemporaryPathOS << OutputPath << "-" << suffix;
+    TemporaryPathOS.flush();
+    auto result = TemporaryBuffers.try_emplace(*TemporaryPath);
+    it = result.first;
+    inserted = result.second;
+    suffix += 1;
+  }
+  return std::make_unique<llvm::raw_svector_ostream>(it->getValue());
+}
+
+void InMemoryOutputFileSystem::DeleteTemporaryBuffer(
+    llvm::StringRef TemporaryPath) {
+  std::lock_guard<std::mutex> locked(Mu);
+  auto it = TemporaryBuffers.find(TemporaryPath);
+  assert(it != TemporaryBuffers.end());
+  TemporaryBuffers.erase(it);
+}
+
+void InMemoryOutputFileSystem::FinalizeTemporaryBuffer(
+    llvm::StringRef OutputPath, llvm::StringRef TemporaryPath) {
+  std::lock_guard<std::mutex> locked(Mu);
+  auto it = TemporaryBuffers.find(TemporaryPath);
+  assert(it != TemporaryBuffers.end());
+  auto memoryBuffer = llvm::MemoryBuffer::getMemBufferCopy(
+      llvm::StringRef{it->getValue().data(), it->getValue().size()},
+      OutputPath);
+  OutputFiles->addFile(OutputPath, /*ModificationTime=*/0,
+                       std::move(memoryBuffer));
+  TemporaryBuffers.erase(it);
+}
+
+} // namespace clang

clang/lib/Frontend/CompilerInstance.cpp

 // Output Files
 
 void CompilerInstance::addOutputFile(OutputFile &&OutFile) {
   OutputFiles.push_back(std::move(OutFile));
 }
 
 void CompilerInstance::clearOutputFiles(bool EraseFiles) {
   for (OutputFile &OF : OutputFiles) {
+    if (InMemoryOutputFileSystem) {
+      assert(!OF.TempFilename.empty() &&
+             "InMemoryOutputFileSystem requires using temporary files");
+      if (EraseFiles) {
+        InMemoryOutputFileSystem->DeleteTemporaryBuffer(OF.TempFilename);
+      } else {
+        InMemoryOutputFileSystem->FinalizeTemporaryBuffer(OF.Filename,
+                                                          OF.TempFilename);
+      }
+      continue;
+    }
+
     if (!OF.TempFilename.empty()) {
       if (EraseFiles) {
         llvm::sys::fs::remove(OF.TempFilename);
       } else {
         SmallString<128> NewOutFile(OF.Filename);
 
         // If '-working-directory' was passed, the output filename should be
         // relative to that.
std::unique_ptr<llvm::raw_pwrite_stream> CompilerInstance::createOutputFile(
   } else if (!Extension.empty()) {
     SmallString<128> Path(InFile);
     llvm::sys::path::replace_extension(Path, Extension);
     OutFile = std::string(Path.str());
   } else {
     OutFile = "-";
   }
 
+  if (InMemoryOutputFileSystem) {
+    assert(UseTemporary &&
+           "InMemoryOutputFileSystem requires using temporary files");
+    auto stream =
+        InMemoryOutputFileSystem->CreateTemporaryBuffer(OutFile, &TempFile);
+    if (ResultPathName)
+      *ResultPathName = OutFile;
+    if (TempPathName)
+      *TempPathName = TempFile;
+    return stream;
+  }
+
   std::unique_ptr<llvm::raw_fd_ostream> OS;
   std::string OSFile;
 
   if (UseTemporary) {
     if (OutFile == "-")
       UseTemporary = false;
     else {
       llvm::sys::fs::file_status Status;
CompilerInstance Instance(ImportingInstance.getPCHContainerOperations(),
                             &ImportingInstance.getModuleCache());
   auto &Inv = *Invocation;
   Instance.setInvocation(std::move(Invocation));
 
   Instance.createDiagnostics(new ForwardingDiagnosticConsumer(
                                    ImportingInstance.getDiagnosticClient()),
                              /*ShouldOwnClient=*/true);
 
+  Instance.setInMemoryOutputFileSystem(
+      ImportingInstance.getInMemoryOutputFileSystem());
+
   // Note that this module is part of the module build stack, so that we
   // can detect cycles in the module graph.
   Instance.setFileManager(&ImportingInstance.getFileManager());
   Instance.createSourceManager(Instance.getFileManager());
   SourceManager &SourceMgr = Instance.getSourceManager();
   SourceMgr.setModuleBuildStack(
     ImportingInstance.getSourceManager().getModuleBuildStack());
   SourceMgr.pushModuleBuildStack(ModuleName,
static bool compileModuleAndReadAST(CompilerInstance &ImportingInstance,
                                     Module *Module, StringRef ModuleFileName) {
   DiagnosticsEngine &Diags = ImportingInstance.getDiagnostics();
 
   auto diagnoseBuildFailure = [&] {
     Diags.Report(ModuleNameLoc, diag::err_module_not_built)
         << Module->Name << SourceRange(ImportLoc, ModuleNameLoc);
   };
 
+  // If we're writing to an InMemoryOutputFileSystem, then immediately compile
+  // and read the module, rather than doing all the lockfile based locking logic
+  // below, because the InMemoryOutputFileSystem doesn't support lockfiles. This
+  // is okay because the locks are only necessary for performance, not
+  // correctness.
+  if (ImportingInstance.getInMemoryOutputFileSystem()) {
+    if (!compileModule(ImportingInstance, ModuleNameLoc, Module,
+                       ModuleFileName)) {
+      diagnoseBuildFailure();
+      return false;
+    }
+
+    // Try to read the module file, now that we've compiled it.
+    ASTReader::ASTReadResult ReadResult =
+        ImportingInstance.getASTReader()->ReadAST(
+            ModuleFileName, serialization::MK_ImplicitModule, ImportLoc,
+            ASTReader::ARR_None);
+
+    if (ReadResult != ASTReader::Success) {
+      diagnoseBuildFailure();
+      return false;
+    }
+
+    return true;
+  }
+
   // FIXME: have LockFileManager return an error_code so that we can
   // avoid the mkdir when the directory already exists.
   StringRef Dir = llvm::sys::path::parent_path(ModuleFileName);
   llvm::sys::fs::create_directories(Dir);
 
   while (1) {
     unsigned ModuleLoadCapabilities = ASTReader::ARR_Missing;
     llvm::LockFileManager Locked(ModuleFileName);