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
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
