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

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb500c49cd4f8: [clangd] Don't mmap source files on all platforms --> don't crash on git… (authored by sammccall).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73617/new/

https://reviews.llvm.org/D73617

Files:
  clang-tools-extra/clangd/FSProvider.cpp
  clang-tools-extra/clangd/FSProvider.h


Index: clang-tools-extra/clangd/FSProvider.h
===================================================================
--- clang-tools-extra/clangd/FSProvider.h
+++ clang-tools-extra/clangd/FSProvider.h
@@ -30,7 +30,6 @@
 
 class RealFileSystemProvider : public FileSystemProvider {
 public:
-  // FIXME: returns the single real FS instance, which is not threadsafe.
   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>
   getFileSystem() const override;
 };
Index: clang-tools-extra/clangd/FSProvider.cpp
===================================================================
--- clang-tools-extra/clangd/FSProvider.cpp
+++ clang-tools-extra/clangd/FSProvider.cpp
@@ -19,7 +19,10 @@
 
 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)
@@ -34,7 +37,7 @@
     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;
@@ -70,15 +73,11 @@
 
 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
-//        propagation of the 'isVolatile' flag through all layers.
-#ifdef _WIN32
+  // 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.
   return new VolatileFileSystem(
       llvm::vfs::createPhysicalFileSystem().release());
-#else
-  return llvm::vfs::createPhysicalFileSystem().release();
-#endif
 }
 } // namespace clangd
 } // namespace clang


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D73617.241222.patch
Type: text/x-patch
Size: 2442 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200129/4b2edd87/attachment.bin>