[libc] add buffering to FILE writes
authorMichael Jones <michaelrj@google.com>
Wed, 1 Jun 2022 21:42:57 +0000 (14:42 -0700)
committerMichael Jones <michaelrj@google.com>
Fri, 10 Jun 2022 16:58:46 +0000 (09:58 -0700)
Previously all FILE objects were fully buffered, this patch adds line
buffering and unbuffered output, as well as applying them to stdout and
stderr.

Reviewed By: sivachandra

Differential Revision: https://reviews.llvm.org/D126829

libc/config/linux/api.td
libc/spec/stdc.td
libc/src/__support/File/file.cpp
libc/src/__support/File/file.h
libc/src/__support/File/linux_file.cpp
libc/test/src/__support/File/file_test.cpp

index 3ab2f3a..10b23d3 100644 (file)
@@ -150,6 +150,9 @@ def StdIOAPI : PublicAPI<"stdio.h"> {
   let Macros = [
     SimpleMacroDef<"stderr", "stderr">,
     SimpleMacroDef<"stdout", "stdout">,
+    SimpleMacroDef<"_IOFBF", "0">,
+    SimpleMacroDef<"_IOLBF", "1">,
+    SimpleMacroDef<"_IONBF", "2">,
   ];
   let Types = ["size_t", "FILE", "cookie_io_functions_t"];
 }
index f173e4b..e9b9a2d 100644 (file)
@@ -474,6 +474,9 @@ def StdC : StandardSpec<"stdc"> {
       [
           Macro<"stderr">,
           Macro<"stdout">,
+          Macro<"_IOFBF">,
+          Macro<"_IOLBF">,
+          Macro<"_IONBF">,
       ], // Macros
       [ // Types
           SizeTType,
index 31b960b..7a204c8 100644 (file)
@@ -25,52 +25,155 @@ size_t File::write_unlocked(const void *data, size_t len) {
 
   prev_op = FileOp::WRITE;
 
-  cpp::ArrayRef<uint8_t> dataref(data, len);
-  cpp::MutableArrayRef<uint8_t> bufref(buf, bufsize);
+  if (bufmode == _IOFBF) { // fully buffered
+    return write_unlocked_fbf(data, len);
+  } else if (bufmode == _IOLBF) { // line buffered
+    return write_unlocked_lbf(data, len);
+  } else /*if (bufmode == _IONBF) */ { // unbuffered
+    size_t ret_val = write_unlocked_nbf(data, len);
+    flush_unlocked();
+    return ret_val;
+  }
+}
 
-  const size_t used = pos;
+size_t File::write_unlocked_nbf(const void *data, size_t len) {
+  if (pos > 0) { // If the buffer is not empty
+    // Flush the buffer
+    const size_t write_size = pos;
+    size_t bytes_written = platform_write(this, buf, write_size);
+    pos = 0; // Buffer is now empty so reset pos to the beginning.
+    // If less bytes were written than expected, then an error occurred.
+    if (bytes_written < write_size) {
+      err = true;
+      return 0; // No bytes from data were written, so return 0.
+    }
+  }
+
+  size_t written = platform_write(this, data, len);
+  if (written < len)
+    err = true;
+  return written;
+}
+
+size_t File::write_unlocked_fbf(const void *data, size_t len) {
+  const size_t init_pos = pos;
   const size_t bufspace = bufsize - pos;
-  const size_t write_size = bufspace > len ? len : bufspace;
+
+  // If data is too large to be buffered at all, then just write it unbuffered.
+  if (len > bufspace + bufsize)
+    return write_unlocked_nbf(data, len);
+
+  // we split |data| (conceptually) using the split point. Then we handle the
+  // two pieces separately.
+  const size_t split_point = len < bufspace ? len : bufspace;
+
+  // The primary piece is the piece of |data| we want to write to the buffer
+  // before flushing. It will always fit into the buffer, since the split point
+  // is defined as being min(len, bufspace), and it will always exist if len is
+  // non-zero.
+  cpp::ArrayRef<uint8_t> primary(data, split_point);
+
+  // The second piece is the remainder of |data|. It is written to the buffer if
+  // it fits, or written directly to the output if it doesn't. If the primary
+  // piece fits entirely in the buffer, the remainder may be nothing.
+  cpp::ArrayRef<uint8_t> remainder(
+      static_cast<const uint8_t *>(data) + split_point, len - split_point);
+
+  cpp::MutableArrayRef<uint8_t> bufref(buf, bufsize);
+
+  // Copy the first piece into the buffer.
   // TODO: Replace the for loop below with a call to internal memcpy.
-  for (size_t i = 0; i < write_size; ++i)
-    bufref[pos + i] = dataref[i];
-  pos += write_size;
-  if (len < bufspace)
+  for (size_t i = 0; i < primary.size(); ++i)
+    bufref[pos + i] = primary[i];
+  pos += primary.size();
+
+  // If there is no remainder, we can return early, since the first piece has
+  // fit completely into the buffer.
+  if (remainder.size() == 0)
     return len;
 
-  // If the control reaches beyond this point, it means that |data|
-  // is more than what can be accomodated in the buffer. So, we first
-  // flush out the buffer.
-  size_t bytes_written = platform_write(this, buf, bufsize);
+  // We need to flush the buffer now, since there is still data and the buffer
+  // is full.
+  const size_t write_size = pos;
+  size_t bytes_written = platform_write(this, buf, write_size);
   pos = 0; // Buffer is now empty so reset pos to the beginning.
-  if (bytes_written < bufsize) {
+  // If less bytes were written than expected, then an error occurred. Return
+  // the number of bytes that have been written from |data|.
+  if (bytes_written < write_size) {
     err = true;
-    // If less bytes were written than expected, then there are two
-    // possibilities.
-    // 1. None of the bytes from |data| were flushed out.
-    if (bytes_written <= used)
-      return 0;
-    // 2. Some of the bytes from |data| were written
-    return bytes_written - used;
+    return bytes_written <= init_pos ? 0 : bytes_written - init_pos;
   }
 
-  // If the remaining bytes from |data| can fit in the buffer, write
-  // into it. Else, write it directly to the platform stream.
-  size_t remaining = len - write_size;
-  if (remaining <= len) {
+  // The second piece is handled basically the same as the first, although we
+  // know that if the second piece has data in it then the buffer has been
+  // flushed, meaning that pos is always 0.
+  if (remainder.size() < bufsize) {
     // TODO: Replace the for loop below with a call to internal memcpy.
-    for (size_t i = 0; i < remaining; ++i)
-      bufref[i] = dataref[i];
-    pos += remaining;
-    return len;
+    for (size_t i = 0; i < remainder.size(); ++i)
+      bufref[i] = remainder[i];
+    pos = remainder.size();
+  } else {
+    size_t bytes_written =
+        platform_write(this, remainder.data(), remainder.size());
+
+    // If less bytes were written than expected, then an error occurred. Return
+    // the number of bytes that have been written from |data|.
+    if (bytes_written < remainder.size()) {
+      err = true;
+      return primary.size() + bytes_written;
+    }
+  }
+
+  return len;
+}
+
+size_t File::write_unlocked_lbf(const void *data, size_t len) {
+  const size_t init_pos = pos;
+  const size_t bufspace = bufsize - pos;
+
+  constexpr char NEWLINE_CHAR = '\n';
+  size_t last_newline = len;
+  for (size_t i = len - 1; i > 0; --i) {
+    if (static_cast<const char *>(data)[i] == NEWLINE_CHAR) {
+      last_newline = i;
+      break;
+    }
+  }
+
+  // If there is no newline, treat this as fully buffered.
+  if (last_newline == len) {
+    return write_unlocked_fbf(data, len);
   }
 
-  size_t transferred =
-      platform_write(this, dataref.data() + write_size, remaining);
-  if (transferred < remaining) {
+  // we split |data| (conceptually) using the split point. Then we handle the
+  // two pieces separately.
+  const size_t split_point = last_newline + 1;
+
+  // The primary piece is everything in |data| up to the newline. It's written
+  // unbuffered to the output.
+  cpp::ArrayRef<uint8_t> primary(data, split_point);
+
+  // The second piece is the remainder of |data|. It is written fully buffered,
+  // meaning it may stay in the buffer if it fits.
+  cpp::ArrayRef<uint8_t> remainder(
+      static_cast<const uint8_t *>(data) + split_point, len - split_point);
+
+  size_t written = 0;
+
+  written = write_unlocked_nbf(primary.data(), primary.size());
+  if (written < primary.size()) {
     err = true;
-    return write_size + transferred;
+    return written;
   }
+
+  flush_unlocked();
+
+  written += write_unlocked_fbf(remainder.data(), remainder.size());
+  if (written < primary.size() + remainder.size()) {
+    err = true;
+    return written;
+  }
+
   return len;
 }
 
@@ -154,8 +257,7 @@ int File::seek(long offset, int whence) {
   return platform_seek(this, offset, whence);
 }
 
-int File::flush() {
-  FileLock lock(this);
+int File::flush_unlocked() {
   if (prev_op == FileOp::WRITE && pos > 0) {
     size_t transferred_size = platform_write(this, buf, pos);
     if (transferred_size < pos) {
index 49a5779..f657f9f 100644 (file)
@@ -180,7 +180,12 @@ public:
 
   // If buffer has data written to it, flush it out. Does nothing if the
   // buffer is currently being used as a read buffer.
-  int flush();
+  int flush() {
+    FileLock lock(this);
+    return flush_unlocked();
+  }
+
+  int flush_unlocked();
 
   // Sets the internal buffer to |buffer| with buffering mode |mode|.
   // |size| is the size of |buffer|. This new |buffer| is owned by the
@@ -217,6 +222,11 @@ public:
   // Returns an bit map of flags corresponding to enumerations of
   // OpenMode, ContentType and CreateType.
   static ModeFlags mode_flags(const char *mode);
+
+private:
+  size_t write_unlocked_lbf(const void *data, size_t len);
+  size_t write_unlocked_fbf(const void *data, size_t len);
+  size_t write_unlocked_nbf(const void *data, size_t len);
 };
 
 // The implementaiton of this function is provided by the platfrom_file
index fc706c2..eb2a90c 100644 (file)
@@ -11,7 +11,8 @@
 #include "src/__support/OSUtil/syscall.h" // For internal syscall function.
 
 #include <errno.h>
-#include <fcntl.h>       // For mode_t and other flags to the open syscall
+#include <fcntl.h> // For mode_t and other flags to the open syscall
+#include <stdio.h>
 #include <stdlib.h>      // For malloc
 #include <sys/syscall.h> // For syscall numbers
 
@@ -158,24 +159,19 @@ File *openfile(const char *path, const char *mode) {
 
   void *buffer = malloc(File::DEFAULT_BUFFER_SIZE);
   auto *file = reinterpret_cast<LinuxFile *>(malloc(sizeof(LinuxFile)));
-  LinuxFile::init(
-      file, fd, buffer, File::DEFAULT_BUFFER_SIZE,
-      0, // TODO: Set the correct buffer mode when buffer mode is available.
-      true, modeflags);
+  LinuxFile::init(file, fd, buffer, File::DEFAULT_BUFFER_SIZE, _IOFBF, true,
+                  modeflags);
   return file;
 }
 
-// TODO: Use the appropriate buffering modes for the standard streams below
-// the different buffering modes are available.
 constexpr size_t STDOUT_BUFFER_SIZE = 1024;
 char stdout_buffer[STDOUT_BUFFER_SIZE];
-static LinuxFile StdOut(1, stdout_buffer, STDOUT_BUFFER_SIZE, 0, false,
+static LinuxFile StdOut(1, stdout_buffer, STDOUT_BUFFER_SIZE, _IOLBF, false,
                         File::ModeFlags(File::OpenMode::APPEND));
 File *stdout = &StdOut;
 
-constexpr size_t STDERR_BUFFER_SIZE = 1024;
-char stderr_buffer[STDERR_BUFFER_SIZE];
-static LinuxFile StdErr(2, stderr_buffer, STDERR_BUFFER_SIZE, 0, false,
+constexpr size_t STDERR_BUFFER_SIZE = 0;
+static LinuxFile StdErr(2, nullptr, STDERR_BUFFER_SIZE, _IONBF, false,
                         File::ModeFlags(File::OpenMode::APPEND));
 File *stderr = &StdErr;
 
index d7bdeb7..e41295b 100644 (file)
@@ -116,7 +116,8 @@ TEST(LlvmLibcFileTest, WriteOnly) {
   const char data[] = "hello, file";
   constexpr size_t FILE_BUFFER_SIZE = sizeof(data) * 3 / 2;
   char file_buffer[FILE_BUFFER_SIZE];
-  StringFile *f = new_string_file(file_buffer, FILE_BUFFER_SIZE, 0, false, "w");
+  StringFile *f =
+      new_string_file(file_buffer, FILE_BUFFER_SIZE, _IOFBF, false, "w");
 
   ASSERT_EQ(sizeof(data), f->write(data, sizeof(data)));
   EXPECT_EQ(f->get_pos(), size_t(0)); // Data is buffered in the file stream
@@ -133,6 +134,9 @@ TEST(LlvmLibcFileTest, WriteOnly) {
   EXPECT_GE(f->get_pos(), size_t(0));
   ASSERT_EQ(f->flush(), 0);
   EXPECT_EQ(f->get_pos(), 2 * sizeof(data));
+  MemoryView src1("hello, file\0hello, file", sizeof(data) * 2),
+      dst1(f->get_str(), sizeof(data) * 2);
+  EXPECT_MEM_EQ(src1, dst1);
 
   char read_data[sizeof(data)];
   // This is not a readable file.
@@ -144,11 +148,87 @@ TEST(LlvmLibcFileTest, WriteOnly) {
   ASSERT_EQ(f->close(), 0);
 }
 
+TEST(LlvmLibcFileTest, WriteLineBuffered) {
+  const char data[] = "hello\n file";
+  constexpr size_t FILE_BUFFER_SIZE = sizeof(data) * 3 / 2;
+
+  char file_buffer_line[FILE_BUFFER_SIZE];
+  char file_buffer_full[FILE_BUFFER_SIZE];
+
+  StringFile *f_line =
+      new_string_file(file_buffer_line, FILE_BUFFER_SIZE, _IOLBF, false, "w");
+  // We also initialize a fully buffered file we'll do the same writes to for
+  // comparison.
+  StringFile *f_full =
+      new_string_file(file_buffer_full, FILE_BUFFER_SIZE, _IOFBF, false, "w");
+
+  ASSERT_EQ(sizeof(data), f_line->write(data, sizeof(data)));
+  ASSERT_EQ(sizeof(data), f_full->write(data, sizeof(data)));
+
+  EXPECT_EQ(f_line->get_pos(), size_t(6)); // buffer after the newline
+  EXPECT_EQ(f_full->get_pos(), size_t(0)); // buffer all of data
+
+  MemoryView src1("hello\n", 6), dst1(f_line->get_str(), 6);
+  EXPECT_MEM_EQ(src1, dst1);
+
+  // We can't check the data in f_full, since no data has been written.
+
+  const char data2[] = "longer for an \n overflow";
+
+  ASSERT_EQ(sizeof(data2), f_line->write(data2, sizeof(data2)));
+  // The line buffer's initial contents should be " file\0"
+  // Writing data2 should write up until the newline, even though that doesn't
+  // all fit in the buffer.
+
+  ASSERT_EQ(sizeof(data2), f_full->write(data2, sizeof(data2)));
+  // The full buffer's initial contents should be "hello\n file\0"
+  // Writing data2 should cause a flush of the buffer, as well as the remainder
+  // to be written directly since it doesn't fit in the buffer.
+
+  EXPECT_EQ(f_line->get_pos(), size_t(27));
+  EXPECT_EQ(f_full->get_pos(), sizeof(data) + sizeof(data2));
+
+  MemoryView src2("hello\n file\0longer for an \n", 27),
+      dst2(f_line->get_str(), 27);
+  EXPECT_MEM_EQ(src2, dst2);
+
+  MemoryView src3("hello\n file\0longer for an \n overflow", 37),
+      dst_full_final(f_full->get_str(), 37);
+  EXPECT_MEM_EQ(src3, dst_full_final);
+
+  ASSERT_EQ(f_line->flush(), 0);
+  ASSERT_EQ(f_full->flush(), 0);
+
+  EXPECT_EQ(f_line->get_pos(), sizeof(data) + sizeof(data2));
+  MemoryView dst_line_final(f_line->get_str(), 37);
+  EXPECT_MEM_EQ(src3, dst_line_final);
+  EXPECT_MEM_EQ(src3, dst_full_final);
+
+  ASSERT_EQ(f_line->close(), 0);
+  ASSERT_EQ(f_full->close(), 0);
+}
+
+TEST(LlvmLibcFileTest, WriteUnbuffered) {
+  const char data[] = "written immediately";
+  constexpr size_t FILE_BUFFER_SIZE = sizeof(data) + 1;
+  char file_buffer[FILE_BUFFER_SIZE];
+  StringFile *f =
+      new_string_file(file_buffer, FILE_BUFFER_SIZE, _IONBF, false, "w");
+
+  ASSERT_EQ(sizeof(data), f->write(data, sizeof(data)));
+  EXPECT_EQ(f->get_pos(),
+            sizeof(data)); // no buffering means this is written immediately.
+  EXPECT_STREQ(f->get_str(), data);
+
+  ASSERT_EQ(f->close(), 0);
+}
+
 TEST(LlvmLibcFileTest, ReadOnly) {
   const char initial_content[] = "1234567890987654321";
   constexpr size_t FILE_BUFFER_SIZE = sizeof(initial_content);
   char file_buffer[FILE_BUFFER_SIZE];
-  StringFile *f = new_string_file(file_buffer, FILE_BUFFER_SIZE, 0, false, "r");
+  StringFile *f =
+      new_string_file(file_buffer, FILE_BUFFER_SIZE, _IOFBF, false, "r");
   f->reset_and_fill(initial_content, sizeof(initial_content));
 
   constexpr size_t READ_SIZE = sizeof(initial_content) / 2;
@@ -198,7 +278,8 @@ TEST(LlvmLibcFileTest, ReadSeekCurAndRead) {
   const char initial_content[] = "1234567890987654321";
   constexpr size_t FILE_BUFFER_SIZE = sizeof(initial_content);
   char file_buffer[FILE_BUFFER_SIZE];
-  StringFile *f = new_string_file(file_buffer, FILE_BUFFER_SIZE, 0, false, "r");
+  StringFile *f =
+      new_string_file(file_buffer, FILE_BUFFER_SIZE, _IOFBF, false, "r");
   f->reset_and_fill(initial_content, sizeof(initial_content));
 
   constexpr size_t READ_SIZE = 5;
@@ -220,7 +301,8 @@ TEST(LlvmLibcFileTest, AppendOnly) {
   const char write_data[] = "append";
   constexpr size_t FILE_BUFFER_SIZE = sizeof(write_data) * 3 / 2;
   char file_buffer[FILE_BUFFER_SIZE];
-  StringFile *f = new_string_file(file_buffer, FILE_BUFFER_SIZE, 0, false, "a");
+  StringFile *f =
+      new_string_file(file_buffer, FILE_BUFFER_SIZE, _IOFBF, false, "a");
   f->reset_and_fill(initial_content, sizeof(initial_content));
 
   constexpr size_t READ_SIZE = 5;
@@ -246,7 +328,7 @@ TEST(LlvmLibcFileTest, WriteUpdate) {
   constexpr size_t FILE_BUFFER_SIZE = sizeof(data) * 3 / 2;
   char file_buffer[FILE_BUFFER_SIZE];
   StringFile *f =
-      new_string_file(file_buffer, FILE_BUFFER_SIZE, 0, false, "w+");
+      new_string_file(file_buffer, FILE_BUFFER_SIZE, _IOFBF, false, "w+");
 
   ASSERT_EQ(sizeof(data), f->write(data, sizeof(data)));
   EXPECT_EQ(f->get_pos(), size_t(0)); // Data is buffered in the file stream
@@ -266,7 +348,7 @@ TEST(LlvmLibcFileTest, ReadUpdate) {
   constexpr size_t FILE_BUFFER_SIZE = sizeof(initial_content);
   char file_buffer[FILE_BUFFER_SIZE];
   StringFile *f =
-      new_string_file(file_buffer, FILE_BUFFER_SIZE, 0, false, "r+");
+      new_string_file(file_buffer, FILE_BUFFER_SIZE, _IOFBF, false, "r+");
   f->reset_and_fill(initial_content, sizeof(initial_content));
 
   constexpr size_t READ_SIZE = sizeof(initial_content) / 2;
@@ -300,7 +382,7 @@ TEST(LlvmLibcFileTest, AppendUpdate) {
   constexpr size_t FILE_BUFFER_SIZE = sizeof(data) * 3 / 2;
   char file_buffer[FILE_BUFFER_SIZE];
   StringFile *f =
-      new_string_file(file_buffer, FILE_BUFFER_SIZE, 0, false, "a+");
+      new_string_file(file_buffer, FILE_BUFFER_SIZE, _IOFBF, false, "a+");
   f->reset_and_fill(initial_content, sizeof(initial_content));
 
   ASSERT_EQ(sizeof(data), f->write(data, sizeof(data)));
@@ -335,3 +417,44 @@ TEST(LlvmLibcFileTest, AppendUpdate) {
 
   ASSERT_EQ(f->close(), 0);
 }
+
+TEST(LlvmLibcFileTest, SmallBuffer) {
+  const char WRITE_DATA[] = "small buffer";
+  constexpr size_t WRITE_SIZE = sizeof(WRITE_DATA);
+  constexpr size_t FILE_BUFFER_SIZE = sizeof(WRITE_DATA) / 2 - 1;
+  char file_buffer[FILE_BUFFER_SIZE];
+  StringFile *f =
+      new_string_file(file_buffer, FILE_BUFFER_SIZE, _IOFBF, false, "w");
+
+  ASSERT_EQ(WRITE_SIZE, f->write(WRITE_DATA, WRITE_SIZE));
+  // Since data much larger than the buffer is being written, all of it should
+  // be available in the file without a flush operation.
+  EXPECT_EQ(f->get_pos(), sizeof(WRITE_DATA));
+  ASSERT_STREQ(f->get_str(), WRITE_DATA);
+
+  ASSERT_EQ(f->close(), 0);
+}
+
+TEST(LlvmLibcFileTest, ZeroLengthBuffer) {
+  const char WRITE_DATA[] = "small buffer";
+  constexpr size_t WRITE_SIZE = sizeof(WRITE_DATA);
+  StringFile *f_fbf = new_string_file(nullptr, 0, _IOFBF, true, "w");
+  StringFile *f_lbf = new_string_file(nullptr, 0, _IOLBF, true, "w");
+  StringFile *f_nbf = new_string_file(nullptr, 0, _IONBF, true, "w");
+
+  ASSERT_EQ(WRITE_SIZE, f_fbf->write(WRITE_DATA, WRITE_SIZE));
+  ASSERT_EQ(WRITE_SIZE, f_lbf->write(WRITE_DATA, WRITE_SIZE));
+  ASSERT_EQ(WRITE_SIZE, f_nbf->write(WRITE_DATA, WRITE_SIZE));
+  // Since there is no buffer space, all of the written data should
+  // be available in the file without a flush operation.
+  EXPECT_EQ(f_fbf->get_pos(), sizeof(WRITE_DATA));
+  EXPECT_EQ(f_lbf->get_pos(), sizeof(WRITE_DATA));
+  EXPECT_EQ(f_nbf->get_pos(), sizeof(WRITE_DATA));
+  ASSERT_STREQ(f_fbf->get_str(), WRITE_DATA);
+  ASSERT_STREQ(f_lbf->get_str(), WRITE_DATA);
+  ASSERT_STREQ(f_nbf->get_str(), WRITE_DATA);
+
+  ASSERT_EQ(f_fbf->close(), 0);
+  ASSERT_EQ(f_lbf->close(), 0);
+  ASSERT_EQ(f_nbf->close(), 0);
+}