[clangd] Don't mmap source files on all platforms --> don't crash on git checkout

[sammccall]

Summary

Previously we mmapped on unix and not on windows: on windows mmap takes
an exclusive lock on the file and prevents the user saving changes!

The failure mode on linux is a bit more subtle: if the file is changed on disk
but the SourceManager sticks around, then subsequent operations on the
SourceManager will fail as invariants are violated (e.g. null-termination).

This commonly manifests as crashes after switching git branches with many files
open in clangd.

Nominally mmap is for performance here, and we should be willing to give some
up to stop crashing. Measurements on my system (linux+desktop+SSD) at least
show no measurable regression on an a fairly IO-heavy workload: drop disk caches,
open SemaOverload.cpp, wait for first diagnostics.

for i in `seq 100`; do
  for variant in mmap volatile; do
    echo 3 | sudo tee /proc/sys/vm/drop_caches
    /usr/bin/time --append --quiet -o ~/timings -f "%C %E" \
    bin/clangd.$variant -sync -background-index=0 < /tmp/mirror > /dev/null
  done
done

bin/clangd.mmap -sync -background-index=0 0:07.60
bin/clangd.volatile -sync -background-index=0 0:07.89
bin/clangd.mmap -sync -background-index=0 0:07.44
bin/clangd.volatile -sync -background-index=0 0:07.89
bin/clangd.mmap -sync -background-index=0 0:07.42
bin/clangd.volatile -sync -background-index=0 0:07.50
bin/clangd.mmap -sync -background-index=0 0:07.90
bin/clangd.volatile -sync -background-index=0 0:07.53
bin/clangd.mmap -sync -background-index=0 0:07.64
bin/clangd.volatile -sync -background-index=0 0:07.55
bin/clangd.mmap -sync -background-index=0 0:07.75
bin/clangd.volatile -sync -background-index=0 0:07.47
bin/clangd.mmap -sync -background-index=0 0:07.90
bin/clangd.volatile -sync -background-index=0 0:07.50
bin/clangd.mmap -sync -background-index=0 0:07.81
bin/clangd.volatile -sync -background-index=0 0:07.95
bin/clangd.mmap -sync -background-index=0 0:07.55
bin/clangd.volatile -sync -background-index=0 0:07.65
bin/clangd.mmap -sync -background-index=0 0:08.15
bin/clangd.volatile -sync -background-index=0 0:07.54
bin/clangd.mmap -sync -background-index=0 0:07.78
bin/clangd.volatile -sync -background-index=0 0:07.61
bin/clangd.mmap -sync -background-index=0 0:07.78
bin/clangd.volatile -sync -background-index=0 0:07.55
bin/clangd.mmap -sync -background-index=0 0:07.41
bin/clangd.volatile -sync -background-index=0 0:07.40
bin/clangd.mmap -sync -background-index=0 0:07.54
bin/clangd.volatile -sync -background-index=0 0:07.42
bin/clangd.mmap -sync -background-index=0 0:07.45
bin/clangd.volatile -sync -background-index=0 0:07.49
bin/clangd.mmap -sync -background-index=0 0:07.95
bin/clangd.volatile -sync -background-index=0 0:07.66
bin/clangd.mmap -sync -background-index=0 0:08.04

[merge_guards_bot]

Unit tests: pass. 62284 tests passed, 0 failed and 831 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[sammccall]

Also I'd like to put this onto the 10 release branch because this is a big crasher, fix seems safe, we've run this code on windows for a while.

[ilya-biryukov]

LGTM!

clang-tools-extra/clangd/FSProvider.h

public:
   /// such as addDocument(), and will also be propagated to result callbacks.
   /// Embedders may use this to isolate filesystem accesses.
   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;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/clangd/FSProvider.cpp

 #include "llvm/Support/VirtualFileSystem.h"
 #include <memory>
 
 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.
+/// won't be memory-mapped. Memory-mapping isn't desirable for clangd:
+///   - edits to the underlying files change contents MemoryBuffers owned by
+//      SourceManager, breaking its invariants and leading to crashes
+///   - it locks files on windows, preventing edits
 class VolatileFileSystem : public llvm::vfs::ProxyFileSystem {
 public:
   explicit VolatileFileSystem(llvm::IntrusiveRefCntPtr<FileSystem> FS)
       : ProxyFileSystem(std::move(FS)) {}
 
   llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
   openFileForRead(const llvm::Twine &InPath) override {
     llvm::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.
+    // clangd has exclusive access to those and nothing else should touch them.
     llvm::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 llvm::vfs::File {
private:
   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.
+  // Avoid using memory-mapped files.
   // FIXME: Try to use a similar approach in Sema instead of relying on
   //        propagation of the 'isVolatile' flag through all layers.
-#ifdef _WIN32
   return new VolatileFileSystem(
       llvm::vfs::createPhysicalFileSystem().release());
-#else
-  return llvm::vfs::createPhysicalFileSystem().release();
-#endif
 }
 } // namespace clangd
 } // namespace clang