Remove F_modify flag from FileOutputBuffer.

[ruiu]

Summary

This code is dead. There is no use of the feature in the entire LLVM codebase.

[zturner]

I think we will need to add this back at some point in the future, because it's necessary for incremental updates. But if you want to remove it in the meantime since there's no existing use, I suppose I can't object.

[ruiu]

I was trying to find a cause of a bug in this file, and although this code was innocent, it took some time to understand and verify it, and then I noticed that there's no use of the feature after all. Let's remove this for now to reduce the maintaining cost.

llvm/include/llvm/Support/FileOutputBuffer.h

 /// If the FileOutputBuffer is committed, the target file's content will become
 /// the buffer content at the time of the commit.  If the FileOutputBuffer is
 /// not committed, the file will be deleted in the FileOutputBuffer destructor.
 class FileOutputBuffer {
 public:
   enum {
     /// set the 'x' bit on the resulting file
     F_executable = 1,
-
-    /// the contents of the new file are initialized from the file that exists
-    /// at the location (if present).  This allows in-place modification of an
-    /// existing file.
-    F_modify = 2
   };
 
   /// Factory method to create an OutputBuffer object which manages a read/write
   /// buffer of the specified size. When committed, the buffer will be written
   /// to the file at the specified path.
   ///
   /// When F_modify is specified and \p FilePath refers to an existing on-disk
   /// file \p Size may be set to -1, in which case the entire file is used.

llvm/lib/Support/FileOutputBuffer.cpp

createInMemoryBuffer(StringRef Path, size_t Size, unsigned Mode) {
   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);
 }
 
 static Expected<std::unique_ptr<OnDiskBuffer>>
-createOnDiskBuffer(StringRef Path, size_t Size, bool InitExisting,
-                   unsigned Mode) {
+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);
 
-  if (InitExisting) {
-    if (auto EC = sys::fs::copy_file(Path, File.FD))
-      return errorCodeToError(EC);
-  } else {
 #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);
   if (EC) {
     consumeError(File.discard());
     return errorCodeToError(EC);
   }
   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) {
   unsigned Mode = fs::all_read | fs::all_write;
   if (Flags & F_executable)
     Mode |= fs::all_exe;
 
   fs::file_status Stat;
   fs::status(Path, Stat);
 
-  if ((Flags & F_modify) && Size == size_t(-1)) {
-    if (Stat.type() == fs::file_type::regular_file)
-      Size = Stat.getSize();
-    else if (Stat.type() == fs::file_type::file_not_found)
-      return errorCodeToError(errc::no_such_file_or_directory);
-    else
-      return errorCodeToError(errc::invalid_argument);
-  }
-
   // Usually, we want to create OnDiskBuffer to create a temporary file in
   // the same directory as the destination file and atomically replaces it
   // by rename(2).
   //
   // However, if the destination file is a special file, we don't want to
   // use rename (e.g. we don't want to replace /dev/null with a regular
   // file.) If that's the case, we create an in-memory buffer, open the
   // destination file and write to it on commit().
   switch (Stat.type()) {
   case fs::file_type::directory_file:
     return errorCodeToError(errc::is_a_directory);
   case fs::file_type::regular_file:
   case fs::file_type::file_not_found:
   case fs::file_type::status_error:
-    return createOnDiskBuffer(Path, Size, !!(Flags & F_modify), Mode);
+    return createOnDiskBuffer(Path, Size, Mode);
   default:
     return createInMemoryBuffer(Path, Size, Mode);
   }
 }

llvm/unittests/Support/FileOutputBufferTest.cpp

TEST(FileOutputBuffer, Test) {
   ASSERT_NO_ERROR(fs::status(Twine(File4), Status));
   bool IsExecutable = (Status.permissions() & fs::owner_exe);
   EXPECT_TRUE(IsExecutable);
   ASSERT_NO_ERROR(fs::remove(File4.str()));
 
   // Clean up.
   ASSERT_NO_ERROR(fs::remove(TestDirectory.str()));
 }
-
-TEST(FileOutputBuffer, TestModify) {
-  // Create unique temporary directory for these tests
-  SmallString<128> TestDirectory;
-  {
-    ASSERT_NO_ERROR(
-      fs::createUniqueDirectory("FileOutputBuffer-modify", TestDirectory));
-  }
-
-  SmallString<128> File1(TestDirectory);
-  File1.append("/file");
-  // First write some data.
-  {
-    Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
-      FileOutputBuffer::create(File1, 10);
-    ASSERT_NO_ERROR(errorToErrorCode(BufferOrErr.takeError()));
-    std::unique_ptr<FileOutputBuffer> &Buffer = *BufferOrErr;
-    memcpy(Buffer->getBufferStart(), "AAAAAAAAAA", 10);
-    ASSERT_NO_ERROR(errorToErrorCode(Buffer->commit()));
-  }
-
-  // Then re-open the file for modify and change only some bytes.
-  {
-    Expected<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
-        FileOutputBuffer::create(File1, size_t(-1), FileOutputBuffer::F_modify);
-    ASSERT_NO_ERROR(errorToErrorCode(BufferOrErr.takeError()));
-    std::unique_ptr<FileOutputBuffer> &Buffer = *BufferOrErr;
-    ASSERT_EQ(10U, Buffer->getBufferSize());
-    uint8_t *Data = Buffer->getBufferStart();
-    Data[0] = 'X';
-    Data[9] = 'X';
-    ASSERT_NO_ERROR(errorToErrorCode(Buffer->commit()));
-  }
-
-  // Finally, re-open the file for read and verify that it has the modified
-  // contents.
-  {
-    ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr = MemoryBuffer::getFile(File1);
-    ASSERT_NO_ERROR(BufferOrErr.getError());
-    std::unique_ptr<MemoryBuffer> Buffer = std::move(*BufferOrErr);
-    ASSERT_EQ(10U, Buffer->getBufferSize());
-    EXPECT_EQ(StringRef("XAAAAAAAAX"), Buffer->getBuffer());
-  }
-
-  // Clean up.
-  ASSERT_NO_ERROR(fs::remove(File1));
-  ASSERT_NO_ERROR(fs::remove(TestDirectory));
-}
-
 } // anonymous namespace