FileOutputBuffer: handle mmap(2) failure

[ruiu]

Summary

If the underlying filesystem does not support mmap system call,
FileOutputBuffer may fail when it attempts to mmap an output temporary
file. This patch handles such situation.

Unfortunately, it looks like it is very hard to test this functionality
without a filesystem that doesn't support mmap using llvm-lit. I tested
this locally by passing an invalid parameter to mmap so that it fails and
falls back to the in-memory buffer. Maybe that's all what we can do.
I believe it is reasonable to submit this without a test.

[sbc100]

Nice!

When I was thinking about doing this I also had a hard time envisioning how to test (Presumably it would require some kind of mock for fs::mapped_file_region).

I wonder if there are other places where this fallback is already performed at a higher level that we could presumably now remove some code? It seems like other tools such as clang were already handling the lack of mmap since the bug reporter didn't hit an issue until link time.

[ruiu]

I wonder if there are other places where this fallback is already performed at a higher level that we could presumably now remove some code? It seems like other tools such as clang were already handling the lack of mmap since the bug reporter didn't hit an issue until link time.

I don't think testing whether this fallback will be triggered or not can be done at a higher level prior to actually creating a file and call mmap(2) on the file, as the condition of the failure depends on the filesystem. Even on the same system and possibly with the same pathname, it may or may not fail, depending on the filesystem of the destination directory.

[sbc100]

In D56949#1366922, @ruiu wrote:

I wonder if there are other places where this fallback is already performed at a higher level that we could presumably now remove some code? It seems like other tools such as clang were already handling the lack of mmap since the bug reporter didn't hit an issue until link time.

I don't think testing whether this fallback will be triggered or not can be done at a higher level prior to actually creating a file and call mmap(2) on the file, as the condition of the failure depends on the filesystem. Even on the same system and possibly with the same pathname, it may or may not fail, depending on the filesystem of the destination directory.

Perhaps one way to test this would be allow an implementation of fs (or just mapped_file_region) to be injected at runtime so that a unit test could run with mock fs. That would require a bunch of refactoring to allow for such dependency injection and I'm not sure how much precedent these is in llvm for such things.

llvm/lib/Support/FileOutputBuffer.cpp

createInMemoryBuffer(StringRef Path, size_t Size, unsigned Mode) {
   std::error_code EC;
   MemoryBlock MB = Memory::allocateMappedMemory(
       Size, nullptr, sys::Memory::MF_READ | sys::Memory::MF_WRITE, EC);
   if (EC)
     return errorCodeToError(EC);
   return llvm::make_unique<InMemoryBuffer>(Path, MB, Mode, Size);
 }
 
-static Expected<std::unique_ptr<OnDiskBuffer>>
+static Expected<std::unique_ptr<FileOutputBuffer>>
 createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode) {
   Expected<fs::TempFile> FileOrErr =
       fs::TempFile::create(Path + ".tmp%%%%%%%", Mode);
   if (!FileOrErr)
     return FileOrErr.takeError();
   fs::TempFile File = std::move(*FileOrErr);
 
 #ifndef _WIN32
   // On Windows, CreateFileMapping (the mmap function on Windows)
   // automatically extends the underlying file. We don't need to
   // extend the file beforehand. _chsize (ftruncate on Windows) is
   // pretty slow just like it writes specified amount of bytes,
   // so we should avoid calling that function.
   if (auto EC = fs::resize_file(File.FD, Size)) {
     consumeError(File.discard());
     return errorCodeToError(EC);
   }
 #endif
 
   // Mmap it.
   std::error_code EC;
   auto MappedFile = llvm::make_unique<fs::mapped_file_region>(
       File.FD, fs::mapped_file_region::readwrite, Size, 0, EC);
+
+  // mmap(2) can fail if the underlying filesystem does not support it.
+  // If that happens, we fall back to in-memory buffer as the last resort.
   if (EC) {
     consumeError(File.discard());
-    return errorCodeToError(EC);
+    return createInMemoryBuffer(Path, Size, Mode);
   }
+
   return llvm::make_unique<OnDiskBuffer>(Path, std::move(File),
                                          std::move(MappedFile));
 }
 
 // Create an instance of FileOutputBuffer.
 Expected<std::unique_ptr<FileOutputBuffer>>
 FileOutputBuffer::create(StringRef Path, size_t Size, unsigned Flags) {
   // Handle "-" as stdout just like llvm::raw_ostream does.