Bigcheese created this revision. Bigcheese added reviewers: dexonsmith, vsapsai, rdhindsa. Bigcheese added a project: clang. Herald added a subscriber: hiraditya.
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. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D77772 Files: clang/include/clang/Basic/FileManager.h clang/lib/Basic/FileManager.cpp llvm/lib/Support/MemoryBuffer.cpp Index: llvm/lib/Support/MemoryBuffer.cpp =================================================================== --- llvm/lib/Support/MemoryBuffer.cpp +++ llvm/lib/Support/MemoryBuffer.cpp @@ -329,7 +329,7 @@ // 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 Index: clang/lib/Basic/FileManager.cpp =================================================================== --- clang/lib/Basic/FileManager.cpp +++ clang/lib/Basic/FileManager.cpp @@ -458,7 +458,8 @@ } 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 @@ 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, Index: clang/include/clang/Basic/FileManager.h =================================================================== --- clang/include/clang/Basic/FileManager.h +++ clang/include/clang/Basic/FileManager.h @@ -378,15 +378,19 @@ /// 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.
Index: llvm/lib/Support/MemoryBuffer.cpp =================================================================== --- llvm/lib/Support/MemoryBuffer.cpp +++ llvm/lib/Support/MemoryBuffer.cpp @@ -329,7 +329,7 @@ // 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 Index: clang/lib/Basic/FileManager.cpp =================================================================== --- clang/lib/Basic/FileManager.cpp +++ clang/lib/Basic/FileManager.cpp @@ -458,7 +458,8 @@ } 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 @@ 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, Index: clang/include/clang/Basic/FileManager.h =================================================================== --- clang/include/clang/Basic/FileManager.h +++ clang/include/clang/Basic/FileManager.h @@ -378,15 +378,19 @@ /// 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.
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits