From: Michael Spencer Date: Thu, 9 Apr 2020 03:29:39 +0000 (-0700) Subject: [Clang] Expose RequiresNullTerminator in FileManager. X-Git-Tag: llvmorg-12-init~8905 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=92e8af0ecbe7eb36bc03a211afa9151c81b7b531;p=platform%2Fupstream%2Fllvm.git [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 --- diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h index fed4378..b481f58 100644 --- a/clang/include/clang/Basic/FileManager.h +++ b/clang/include/clang/Basic/FileManager.h @@ -378,15 +378,19 @@ public: /// Open the specified file as a MemoryBuffer, returning a new /// MemoryBuffer if successful, otherwise returning null. llvm::ErrorOr> - getBufferForFile(const FileEntry *Entry, bool isVolatile = false); + getBufferForFile(const FileEntry *Entry, bool isVolatile = false, + bool RequiresNullTerminator = true); llvm::ErrorOr> - 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> - 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 ac8af8f..e92e9d5 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> -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> 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 e467daf..248fb72 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 1b72aad..1a424f6 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")); +} }