[Clang] Expose RequiresNullTerminator in FileManager.

[Bigcheese]

Summary

This is needed to fix the reason
0a2be46cfdb698fe (Modules: Invalidate out-of-date PCMs as they're
discovered) and rG5b44a4b07fc1 ([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.

[dexonsmith]

Code change LGTM.

Is there a way to add a MemoryBuffer unit test for the change to shouldUseMmap?

[Bigcheese]

Not really. It's a static function in MemoryBuffer.cpp, and the MemoryBuffer class doesn't have a Kind member so we can't check for MemoryBufferMMapFile.

[dexonsmith]

In D77772#1973250, @Bigcheese wrote:

Not really. It's a static function in MemoryBuffer.cpp, and the MemoryBuffer class doesn't have a Kind member so we can't check for MemoryBufferMMapFile.

Ah, but there's a vtable :), and it looks like we already have a virtual function called MemoryBuffer::getBufferKind() to expose this.

Maybe use MemoryBuffer::getFile() API to test this?

[Bigcheese]

Added a test.

[dexonsmith]

LGTM.

llvm/unittests/Support/MemoryBufferTest.cpp

396

Maybe add named arguments in comments for the last three? They're a bit inscrutable, and the whole point is to test IsVolatile.

clang/include/clang/Basic/FileManager.h

public:
   /// bypasses all mapping and uniquing, blindly creating a new FileEntry.
   /// There is no attempt to deduplicate these; if you bypass the same file
   /// twice, you get two new file entries.
   llvm::Optional<FileEntryRef> getBypassFile(FileEntryRef VFE);
 
   /// 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.
   ///
   /// If the path is relative, it will be resolved against the WorkingDir of the
   /// FileManager's FileSystemOptions.
   ///
   /// \returns a \c std::error_code describing an error, if there was one

clang/lib/Basic/FileManager.cpp

void FileManager::fillRealPathName(FileEntry *UFE, llvm::StringRef FileName) {
   // FIXME: the semantic of RealPathName is unclear, and the name might be
   // misleading. We need to clean up the interface here.
   makeAbsolutePath(AbsPath);
   llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
   UFE->RealPathName = std::string(AbsPath.str());
 }
 
 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.
   if (isVolatile)
     FileSize = -1;
 
   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,
 /// using the cache to accelerate it if possible.  This returns true
 /// if the path points to a virtual file or does not exist, or returns
 /// false if it's an existent real file.  If FileDescriptor is NULL,
 /// do directory look-up instead of file look-up.
 std::error_code

llvm/lib/Support/MemoryBuffer.cpp

static bool shouldUseMmap(sys::fs::file_t FD,
                           size_t MapSize,
                           off_t Offset,
                           bool RequiresNullTerminator,
                           int PageSize,
                           bool IsVolatile) {
   // 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
   // address space.
   if (MapSize < 4 * 4096 || MapSize < (unsigned)PageSize)
     return false;
 
   if (!RequiresNullTerminator)

llvm/unittests/Support/MemoryBufferTest.cpp

TEST_F(MemoryBufferTest, writeThroughFile) {
   }
 
   auto MBOrError = MemoryBuffer::getFile(TestPath);
   ASSERT_FALSE(MBOrError.getError());
   auto &MB = **MBOrError;
   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"));
+}
 }