sammccall created this revision.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous,
MaskRay, ilya-biryukov.
Herald added a project: clang.
sammccall added reviewers: ilya-biryukov, kadircet.
sammccall edited the summary of this revision.
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
Repository:
rG LLVM Github Monorepo
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
