[clangd] Avoid memory-mapping files on Windows

[ilya-biryukov]

Summary

Memory-mapping files on Windows leads to them being locked and prevents
editors from saving changes to those files on disk. This is fine for the
compiler, but not acceptable for an interactive tool like clangd.
Therefore, we choose to avoid using memory-mapped files on Windows.

[malaperle]

Hi Ilya. Does this apply to compile_commands.json too? I've seen that problem for that file as well. If not, I understand it can be another patch, just curious.

[nik]

Please fix this also for libclang clients. I think it's safe to assume that files might be edited once CXTranslationUnit_PrecompiledPreamble or CXTranslationUnit_CacheCompletionResults is set as flag - that's what clang_defaultEditingTranslationUnitOptions() returns.

[kadircet]

LGTM

clangd/FSProvider.cpp

34

make_unique?

35

can we move once ?

[ilya-biryukov]

We need to keep mmapping the PCH files.

In D55139#1315944, @malaperle wrote:

Hi Ilya. Does this apply to compile_commands.json too? I've seen that problem for that file as well. If not, I understand it can be another patch, just curious.

Yeah, this applies to all files, including compiler_commands.json.
I've discussed this with a few smart folks and they pointed out doing it for all files is not ok, most problematic are PCH files, which are (1) huge, (2) produced by clangd itself and never modified after being produced.
(1) means it's very wasteful to fully load them into memory, (2) means it can easily be avoided. That complicated things, though, and the current approach to fix this in libclang D54995 (passing an explicit flag when we know the file should be memory-mapped) starts to look more attractive.
I'll patch it up in this patch by looking at file name and avoiding that for PCH files. There are still PCM files, which are very similar, my hopes are they're not widely used on Windows these days so we should be fine for now, but that would definitely bite us in the future.

Please fix this also for libclang clients. I think it's safe to assume that files might be edited once CXTranslationUnit_PrecompiledPreamble or CXTranslationUnit_CacheCompletionResults is set as flag - that's what clang_defaultEditingTranslationUnitOptions() returns.

libclang clients should be covered by D54995 and previous changes around it. As I mentioned in the previous comment, passing a flag when file cannot be memory-mapped seems like a proper solution anyway.

[ilya-biryukov]

• Keep using memory-mapped files for PCHs
• Move once

[ilya-biryukov]

clangd/FSProvider.cpp

34

Unfortunately it doesn't work here - VolatileFile is private in this class.

35

We need moar moves!!! xD

Thanks for noticing, fixed.

[ilya-biryukov]

• s/VolatileFSProvider/VolatileFileSystem

clangd/CMakeLists.txt

add_clang_library(clangDaemon
   Compiler.cpp
   Context.cpp
   Diagnostics.cpp
   DraftStore.cpp
   ExpectedTypes.cpp
   FindSymbols.cpp
   FileDistance.cpp
   FS.cpp
+  FSProvider.cpp
   FuzzyMatch.cpp
   GlobalCompilationDatabase.cpp
   Headers.cpp
   JSONTransport.cpp
   Logger.cpp
   Protocol.cpp
   Quality.cpp
   RIFF.cpp

clangd/FSProvider.h

public:
   virtual llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
   getFileSystem() const = 0;
 };
 
 class RealFileSystemProvider : public FileSystemProvider {
 public:
   // FIXME: returns the single real FS instance, which is not threadsafe.
   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
-  getFileSystem() const override {
-    return llvm::vfs::getRealFileSystem();
-  }
+  getFileSystem() const override;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clangd/FSProvider.cpp

+//===--- FSProvider.cpp - VFS provider for ClangdServer -------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "FSProvider.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include <memory>
+
+using namespace llvm;
+
+namespace clang {
+namespace clangd {
+
+namespace {
+/// Always opens files in the underlying filesystem as "volatile", meaning they
+/// won't be memory-mapped. This avoid locking the files on Windows.
+class VolatileFSProvider : public llvm::vfs::ProxyFileSystem {
+public:
+  explicit VolatileFSProvider(llvm::IntrusiveRefCntPtr<FileSystem> FS)
+      : ProxyFileSystem(std::move(FS)) {}
+
+  llvm::ErrorOr<std::unique_ptr<vfs::File>>
+  openFileForRead(const Twine &InPath) override {
+    SmallString<128> Path;
+    InPath.toVector(Path);
+
+    auto File = getUnderlyingFS().openFileForRead(Path);
+    if (!File)
+      return File;
+    // Try to guess preamble files, they can be memory-mapped even on Windows as
+    // clangd has exclusive access to those.
+    StringRef FileName = llvm::sys::path::filename(Path);
+    if (FileName.startswith("preamble-") && FileName.endswith(".pch"))
+      return File;
+    return std::unique_ptr<VolatileFile>(
+        new VolatileFile(std::move(*File)));
+  }
+
+private:
+  class VolatileFile : public vfs::File {
+  public:
+    VolatileFile(std::unique_ptr<vfs::File> Wrapped)
+        : Wrapped(std::move(Wrapped)) {
+      assert(this->Wrapped);
+    }
+
+    virtual llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
+    getBuffer(const Twine &Name, int64_t FileSize, bool RequiresNullTerminator,
+              bool /*IsVolatile*/) override {
+      return Wrapped->getBuffer(Name, FileSize, RequiresNullTerminator,
+                                /*IsVolatile=*/true);
+    }
+
+    llvm::ErrorOr<vfs::Status> status() override { return Wrapped->status(); }
+    llvm::ErrorOr<std::string> getName() override { return Wrapped->getName(); }
+    std::error_code close() override { return Wrapped->close(); }
+
+  private:
+    std::unique_ptr<File> Wrapped;
+  };
+};
+} // namespace
+
+llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
+clang::clangd::RealFileSystemProvider::getFileSystem() const {
+// Avoid using memory-mapped files on Windows, they cause file locking issues.
+// FIXME: Try to use a similar approach in Sema instead of relying on
+//        error-prone propagation of the 'isVolatile' flag through all layers.
+#ifdef _WIN32
+  return new VolatileFSProvider(vfs::getRealFileSystem());
+#else
+  return vfs::getRealFileSystem();
+#endif
+}
+} // namespace clangd
+} // namespace clang