[clang] 92e8af0 - [Clang] Expose RequiresNullTerminator in FileManager.
Michael Spencer via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 15 14:18:08 PDT 2020
Author: Michael Spencer
Date: 2020-04-15T14:17:51-07:00
New Revision: 92e8af0ecbe7eb36bc03a211afa9151c81b7b531
URL: https://github.com/llvm/llvm-project/commit/92e8af0ecbe7eb36bc03a211afa9151c81b7b531
DIFF: https://github.com/llvm/llvm-project/commit/92e8af0ecbe7eb36bc03a211afa9151c81b7b531.diff
LOG: [Clang] Expose RequiresNullTerminator in FileManager.
This is needed to fix the reason
0a2be46cfdb698fe (Modules: Invalidate out-of-date PCMs as they're
discovered) and 5b44a4b07fc1d ([modules] Do not cache invalid state for
modules that we attempted to load.) were reverted.
These patches changed Clang to use `isVolatile` when loading modules.
This had the side effect of not using mmap when loading modules, and
thus greatly increased memory usage.
The reason it wasn't using mmap is because `MemoryBuffer` plays some
games with file size when you request null termination, and it has to
disable these when `isVolatile` is set as the size may change by the
time it's mmapped. Clang by default passes
`RequiresNullTerminator = true`, and `shouldUseMmap` ignored if
`RequiresNullTerminator` was even requested.
This patch adds `RequiresNullTerminator` to the `FileManager` interface
so Clang can use it when loading modules, and changes `shouldUseMmap` to
only take volatility into account if `RequiresNullTerminator` is true.
This is fine as both `mmap` and a `read` loop are vulnerable to
modifying the file while reading, but are immune to the rename Clang
does when replacing a module file.
Differential Revision: https://reviews.llvm.org/D77772
Added:
Modified:
clang/include/clang/Basic/FileManager.h
clang/lib/Basic/FileManager.cpp
llvm/lib/Support/MemoryBuffer.cpp
llvm/unittests/Support/MemoryBufferTest.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h
index fed43786d410..b481f5846936 100644
--- a/clang/include/clang/Basic/FileManager.h
+++ b/clang/include/clang/Basic/FileManager.h
@@ -378,15 +378,19 @@ class FileManager : public RefCountedBase<FileManager> {
/// Open the specified file as a MemoryBuffer, returning a new
/// MemoryBuffer if successful, otherwise returning null.
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
- getBufferForFile(const FileEntry *Entry, bool isVolatile = false);
+ getBufferForFile(const FileEntry *Entry, bool isVolatile = false,
+ bool RequiresNullTerminator = true);
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
- getBufferForFile(StringRef Filename, bool isVolatile = false) {
- return getBufferForFileImpl(Filename, /*FileSize=*/-1, isVolatile);
+ getBufferForFile(StringRef Filename, bool isVolatile = false,
+ bool RequiresNullTerminator = true) {
+ return getBufferForFileImpl(Filename, /*FileSize=*/-1, isVolatile,
+ RequiresNullTerminator);
}
private:
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
- getBufferForFileImpl(StringRef Filename, int64_t FileSize, bool isVolatile);
+ getBufferForFileImpl(StringRef Filename, int64_t FileSize, bool isVolatile,
+ bool RequiresNullTerminator);
public:
/// Get the 'stat' information for the given \p Path.
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index ac8af8fcaf4a..e92e9d5911c0 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -458,7 +458,8 @@ void FileManager::fillRealPathName(FileEntry *UFE, llvm::StringRef FileName) {
}
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
-FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile) {
+FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile,
+ bool RequiresNullTerminator) {
uint64_t FileSize = Entry->getSize();
// If there's a high enough chance that the file have changed since we
// got its size, force a stat before opening it.
@@ -468,28 +469,29 @@ FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile) {
StringRef Filename = Entry->getName();
// If the file is already open, use the open file descriptor.
if (Entry->File) {
- auto Result =
- Entry->File->getBuffer(Filename, FileSize,
- /*RequiresNullTerminator=*/true, isVolatile);
+ auto Result = Entry->File->getBuffer(Filename, FileSize,
+ RequiresNullTerminator, isVolatile);
Entry->closeFile();
return Result;
}
// Otherwise, open the file.
- return getBufferForFileImpl(Filename, FileSize, isVolatile);
+ return getBufferForFileImpl(Filename, FileSize, isVolatile,
+ RequiresNullTerminator);
}
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
FileManager::getBufferForFileImpl(StringRef Filename, int64_t FileSize,
- bool isVolatile) {
+ bool isVolatile,
+ bool RequiresNullTerminator) {
if (FileSystemOpts.WorkingDir.empty())
- return FS->getBufferForFile(Filename, FileSize,
- /*RequiresNullTerminator=*/true, isVolatile);
+ return FS->getBufferForFile(Filename, FileSize, RequiresNullTerminator,
+ isVolatile);
SmallString<128> FilePath(Filename);
FixupRelativePath(FilePath);
- return FS->getBufferForFile(FilePath, FileSize,
- /*RequiresNullTerminator=*/true, isVolatile);
+ return FS->getBufferForFile(FilePath, FileSize, RequiresNullTerminator,
+ isVolatile);
}
/// getStatValue - Get the 'stat' information for the specified path,
diff --git a/llvm/lib/Support/MemoryBuffer.cpp b/llvm/lib/Support/MemoryBuffer.cpp
index e467daf42a33..248fb72c4968 100644
--- a/llvm/lib/Support/MemoryBuffer.cpp
+++ b/llvm/lib/Support/MemoryBuffer.cpp
@@ -329,7 +329,7 @@ static bool shouldUseMmap(sys::fs::file_t FD,
// mmap may leave the buffer without null terminator if the file size changed
// by the time the last page is mapped in, so avoid it if the file size is
// likely to change.
- if (IsVolatile)
+ if (IsVolatile && RequiresNullTerminator)
return false;
// We don't use mmap for small files because this can severely fragment our
diff --git a/llvm/unittests/Support/MemoryBufferTest.cpp b/llvm/unittests/Support/MemoryBufferTest.cpp
index 1b72aadebf47..1a424f66ad95 100644
--- a/llvm/unittests/Support/MemoryBufferTest.cpp
+++ b/llvm/unittests/Support/MemoryBufferTest.cpp
@@ -380,4 +380,32 @@ TEST_F(MemoryBufferTest, writeThroughFile) {
ASSERT_EQ(16u, MB.getBufferSize());
EXPECT_EQ("xxxxxxxxxxxxxxxx", MB.getBuffer());
}
+
+TEST_F(MemoryBufferTest, mmapVolatileNoNull) {
+ // Verify that `MemoryBuffer::getOpenFile` will use mmap when
+ // `RequiresNullTerminator = false`, `IsVolatile = true`, and the file is
+ // large enough to use mmap.
+ //
+ // This is done because Clang should use this mode to open module files, and
+ // falling back to malloc for them causes a huge memory usage increase.
+
+ int FD;
+ SmallString<64> TestPath;
+ ASSERT_NO_ERROR(sys::fs::createTemporaryFile(
+ "MemoryBufferTest_mmapVolatileNoNull", "temp", FD, TestPath));
+ FileRemover Cleanup(TestPath);
+ raw_fd_ostream OF(FD, true);
+ // Create a file large enough to mmap. A 32KiB file should be enough.
+ for (unsigned i = 0; i < 0x1000; ++i)
+ OF << "01234567";
+ OF.flush();
+
+ auto MBOrError = MemoryBuffer::getOpenFile(FD, TestPath,
+ /*FileSize=*/-1, /*RequiresNullTerminator=*/false, /*IsVolatile=*/true);
+ ASSERT_NO_ERROR(MBOrError.getError())
+ OwningBuffer MB = std::move(*MBOrError);
+ EXPECT_EQ(MB->getBufferKind(), MemoryBuffer::MemoryBuffer_MMap);
+ EXPECT_EQ(MB->getBufferSize(), std::size_t(0x8000));
+ EXPECT_TRUE(MB->getBuffer().startswith("01234567"));
+}
}
More information about the cfe-commits
mailing list