[libc] Add a platform independent buffered file IO data structure.

[sivachandra]

This is still WiP. Especially wrt testing, I have only added one test. Will add more and update soon.

[sivachandra]

Add more tests.

[sivachandra]

I will be making another update with either a .md document or detailed inline comments. I don't intend to extend the functionality anymore in this patch.

[lntue]

libc/include/CMakeLists.txt

126–127

I just notice that we have both *-* and *_* here. We should update to use just 1 style at some point.

libc/src/__support/File/file.cpp

171

Shouldn't you only need:

if (own_buf) free(this->buf);

libc/src/__support/File/file.h

145

comment ended early.

libc/test/src/__support/File/file_test.cpp

21

I feel like this class will be useful beyond this test and could be factored to a standalone util. Feel free to ignore this suggestion.

[michaelrj]

libc/src/__support/File/file.h

68

ownded -> owned

89–90

I think

(mode & (static_cast<ModeFlags>(OpenMode::WRITE) | static_cast<ModeFlags>(OpenMode::APPEND)));

is clearer.

libc/test/src/__support/File/CMakeLists.txt

2

this needs to be conditioned on malloc being available, which currently means if(LLVM_LIBC_INCLUDE_SCUDO OR NOT LLVM_LIBC_FULL_BUILD). I will go look into making a single variable that can encompass that in a cleanup patch.

[sivachandra]

• Address comments
• Add inline documentation on how to use the File data structure

[sivachandra]

libc/include/CMakeLists.txt

126–127

Agreed. I will make another pass to fix this.

libc/src/__support/File/file.cpp

171

The memory for the File data structure itself will be created by a malloc call so it has to be free-d at closing time. But, thanks for pointing out about the buffer. I have now added what you have suggested.

libc/src/__support/File/file.h

145

This is not it the correct place anymore, but I think I have addressed this comment.

libc/test/src/__support/File/CMakeLists.txt

2

Not required anymore after your fix from last week.

libc/test/src/__support/File/file_test.cpp

21

SGTM. I will move it to a common place as we start reusing it in other tests.

[lntue]

libc/src/__support/File/file.cpp

168

Do we need to add a check at the beginning of all other accessing methods to make sure that close has not been called?

[sivachandra]

libc/src/__support/File/file.cpp

168

Couple of things:

1. It is undefined behavior to perform file operations on closed files. So, user code should not try to close an already closed file.
2. The File data structure is not available after a close, which means it is use after free to perform any checking.

[sivachandra]

Add the file_test to libc_support_unittests suite.

libc/include/CMakeLists.txt

DEPENDS
     .llvm-libc-types.__sighandler_t
 )
 
 add_gen_header(
   stdio
   DEF_FILE stdio.h.def
   GEN_HDR stdio.h
   DEPENDS
     .llvm_libc_common_h
+    .llvm-libc-macros.stdio_macros
     .llvm-libc-types.FILE
     .llvm-libc-types.size_t
 )
 
 add_gen_header(
   stdlib
   DEF_FILE stdlib.h.def
   GEN_HDR stdlib.h

libc/include/llvm-libc-macros/CMakeLists.txt

 add_subdirectory(linux)
 
 add_header(
   fcntl_macros
   HDR
     fcntl-macros.h
   DEPENDS
     .linux.fcntl_macros
 )
+
+add_header(
+  stdio_macros
+  HDR
+    stdio-macros.h
+)

libc/include/llvm-libc-macros/stdio-macros.h

-//===-- C standard library header stdio.h ---------------------------------===//
+//===-- Definition of macros from stdio.h ---------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef LLVM_LIBC_STDIO_H
-#define LLVM_LIBC_STDIO_H
+#ifndef __LLVM_LIBC_MACROS_STDIO_MACROS_H
+#define __LLVM_LIBC_MACROS_STDIO_MACROS_H
 
-#include <__llvm-libc-common.h>
+#define SEEK_SET 0
+#define SEEK_CUR 1
+#define SEEK_END 2
 
-%%public_api()
-
-#endif // LLVM_LIBC_STDIO_H
+#endif // __LLVM_LIBC_MACROS_STDIO_MACROS_H

libc/include/stdio.h.def

 //===-- C standard library header stdio.h ---------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIBC_STDIO_H
 #define LLVM_LIBC_STDIO_H
 
 #include <__llvm-libc-common.h>
+#include <llvm-libc-macros/stdio-macros.h>
 
 %%public_api()
 
 #endif // LLVM_LIBC_STDIO_H

libc/src/__support/CMakeLists.txt

 )
 
 add_header_library(
   integer_operations
   HDRS
     integer_operations.h
 )
 
+add_subdirectory(File)
 add_subdirectory(FPUtil)
 add_subdirectory(OSUtil)

libc/src/__support/File/CMakeLists.txt

+add_object_library(
+  file
+  SRCS
+    file.cpp
+  HDRS
+    file.h
+)

libc/src/__support/File/file.h

+//===--- A platform independent file data structure -------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_SUPPORT_OSUTIL_FILE_H
+#define LLVM_LIBC_SRC_SUPPORT_OSUTIL_FILE_H
+
+#include <stddef.h>
+#include <stdint.h>
+
+namespace __llvm_libc {
+
+// This a generic base class to encapsulate a platform independent file data
+// structure. Platform specific specializations should create a subclass as
+// suitable for their platform.
+class File {
+public:
+  using LockFunc = void(File *);
+  using UnlockFunc = void(File *);
+
+  using WriteFunc = size_t(File *, const char *, size_t);
+  using ReadFunc = size_t(File *, char *, size_t);
+  using SeekFunc = int(File *, long, int);
+  using CloseFunc = int(File *);
+  using FlushFunc = int(File *);
+
+  using ModeFlags = uint32_t;
+
+  enum class OpenMode : ModeFlags {
+    READ = 0x1,
+    WRITE = 0x2,
+    APPEND = 0x4,
+  };
+
+  enum class ContentType : ModeFlags {
+    BINARY = 0x10,
+  };
+
+  enum class CreateType : ModeFlags {
+    EXCLUSIVE = 0x100,
+  };
+
+private:
+  enum class FileOp : uint8_t { NONE, READ, WRITE, SEEK };
+
+  // Platfrom specific functions which create new file objects should initialize
+  // these fields suitably via the constructor. Typically, they should be simple
+  // syscall wrappers for the corresponding functionality.
+  WriteFunc *platform_write;
+  ReadFunc *platform_read;
+  SeekFunc *platform_seek;
+  CloseFunc *platform_close;
+  FlushFunc *platform_flush;
+  LockFunc *platform_lock;
+  UnlockFunc *platform_unlock;
+
+  char *buf;      // Pointer to the stream buffer for buffered streams
+  size_t bufsize; // Size of the buffer pointed to by |buf|.
+
+  // Buffering mode to used to buffer.
+  int bufmode;
+
+  // If own_buf is true, the |buf| is ownded by the stream and will be
+  // free-ed when close method is called on the stream.
+  bool own_buf;
+
+  ModeFlags mode;
+
+  // Current read or write pointers.
+  size_t pos;
+
+  // Represents the previous operation that was performed.
+  FileOp prev_op;
+
+  // When the buffer is used as a read buffer, read_limit is the upper limit
+  // of the index to which the buffer can be read until
+  size_t read_limit;
+
+  bool eof;
+  bool err;
+
+protected:
+  bool write_allowed() const {
+    return (mode & static_cast<ModeFlags>(OpenMode::WRITE)) ||
+           (mode & static_cast<ModeFlags>(OpenMode::APPEND));
+  }
+
+  bool read_allowed() const {
+    return mode & static_cast<ModeFlags>(OpenMode::READ);
+  }
+
+public:
+  // We want this constructor to be constexpr so that global file objects
+  // like stdout do not require invocation of the constructor which can
+  // potentially lead to static initialization order fiasco.
+  constexpr File(WriteFunc *wf, ReadFunc *rf, SeekFunc *sf, CloseFunc *cf,
+                 FlushFunc *ff, LockFunc *lf, UnlockFunc *ulf, char *buffer,
+                 size_t buffer_size, int buffer_mode, bool owned,
+                 ModeFlags modeflags)
+      : platform_write(wf), platform_read(rf), platform_seek(sf),
+        platform_close(cf), platform_flush(ff), platform_lock(lf),
+        platform_unlock(ulf), buf(buffer), bufsize(buffer_size),
+        bufmode(buffer_mode), own_buf(owned), mode(modeflags), pos(0),
+        prev_op(FileOp::NONE), read_limit(0), eof(false), err(false) {}
+
+  static void init(File *f, WriteFunc *wf, ReadFunc *rf, SeekFunc *sf,
+                   CloseFunc *cf, FlushFunc *ff, LockFunc *lf, UnlockFunc *ulf,
+                   char *buffer, size_t buffer_size, int buffer_mode,
+                   bool owned, ModeFlags modeflags) {
+    f->platform_write = wf;
+    f->platform_read = rf;
+    f->platform_seek = sf;
+    f->platform_close = cf;
+    f->platform_flush = ff;
+    f->platform_lock = lf;
+    f->platform_unlock = ulf;
+    f->buf = buffer;
+    f->bufsize = buffer_size;
+    f->bufmode = buffer_mode;
+    f->own_buf = owned;
+    f->mode = modeflags;
+
+    f->prev_op = FileOp::NONE;
+    f->read_limit = f->pos = 0;
+    f->eof = f->err = false;
+  }
+
+  // Buffered write of |len| bytes from |data|.
+  size_t write(const char *data, size_t len);
+
+  // Buffered read of |len| bytes into |data|.
+  size_t read(char *data, size_t len);
+
+  int seek(long offset, int whence);
+
+  // 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();
+
+  // Sets the internal buffer to |buffer| with buffering mode |mode|.
+  // |size| is the size of |buffer|. This new |buffer| is not owned by the
+  void set_buffer(char *buffer, size_t size, bool owned);
+
+  // Closes the file stream and frees up all resources.
+  int close();
+
+  void lock() { platform_lock(this); }
+  void unlock() { platform_unlock(this); }
+
+  bool error() const { return err; }
+  void clearerr() { err = false; }
+  bool iseof() const { return eof; }
+
+  // Returns an bit map of flags corresponding to enumerations of
+  // OpenMode, ContentType and CreateType.
+  static ModeFlags mode_flags(const char *mode);
+};
+
+// This is a convenience RAII class to lock and unlock file objects.
+class FileLock {
+  File *file;
+
+public:
+  explicit FileLock(File *f) : file(f) { file->lock(); }
+
+  ~FileLock() { file->unlock(); }
+
+  FileLock(const FileLock &) = delete;
+  FileLock(FileLock &&) = delete;
+};
+
+} // namespace __llvm_libc
+
+#endif // LLVM_LIBC_SRC_SUPPORT_OSUTIL_FILE_H

libc/src/__support/File/file.cpp

+//===--- Implementation of a platform independent file data structure -----===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "file.h"
+#include <errno.h>
+#include <stdlib.h>
+
+namespace __llvm_libc {
+
+size_t File::write(const char *data, size_t len) {
+  if (!write_allowed()) {
+    errno = EBADF;
+    err = true;
+    return 0;
+  }
+
+  FileLock lock(this);
+  prev_op = FileOp::WRITE;
+
+  const size_t used = pos;
+  const size_t bufspace = bufsize - pos;
+  const size_t write_size = bufspace > len ? len : bufspace;
+  // TODO: Replace the for loop below with a call to internal memcpy.
+  for (size_t i = 0; i < write_size; ++i)
+    buf[pos + i] = data[i];
+  pos += write_size;
+  if (len < bufspace)
+    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);
+  pos = 0; // Buffer is now empty so reset pos to the beginning.
+  if (bytes_written < bufsize) {
+    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;
+  }
+
+  // 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) {
+    // TODO: Replace the for loop below with a call to internal memcpy.
+    for (size_t i = 0; i < remaining; ++i)
+      buf[i] = data[i];
+    pos += remaining;
+    return len;
+  }
+
+  size_t transferred = platform_write(this, data + write_size, remaining);
+  if (transferred < remaining) {
+    err = true;
+    return write_size + transferred;
+  }
+  return len;
+}
+
+size_t File::read(char *data, size_t len) {
+  if (!read_allowed()) {
+    errno = EBADF;
+    err = true;
+    return 0;
+  }
+
+  FileLock lock(this);
+  prev_op = FileOp::READ;
+
+  // Because read_limit is always greater than equal to pos,
+  // available_data is never a wrapped around value.
+  size_t available_data = read_limit - pos;
+  if (len <= available_data) {
+    // TODO: Replace the for loop below with a call to internal memcpy.
+    for (size_t i = 0; i < len; ++i)
+      data[i] = buf[i + pos];
+    pos += len;
+    return len;
+  }
+
+  // Copy all of the available data.
+  // TODO: Replace the for loop with a call to internal memcpy.
+  for (size_t i = 0; i < available_data; ++i)
+    data[i] = buf[i + pos];
+  read_limit = pos = 0; // Reset the pointers.
+
+  size_t to_fetch = len - available_data;
+  if (to_fetch > bufsize) {
+    size_t fetched_size = platform_read(this, data, to_fetch);
+    if (fetched_size < to_fetch) {
+      if (errno == 0)
+        eof = true;
+      else
+        err = true;
+      return available_data + fetched_size;
+    }
+    return len;
+  }
+
+  // Fetch and buffer another buffer worth of data.
+  size_t fetched_size = platform_read(this, buf, bufsize);
+  read_limit += fetched_size;
+  size_t transfer_size = fetched_size >= to_fetch ? to_fetch : fetched_size;
+  for (size_t i = 0; i < transfer_size; ++i)
+    data[i] = buf[i];
+  pos += transfer_size;
+  if (fetched_size < to_fetch) {
+    if (errno == 0)
+      eof = true;
+    else
+      err = true;
+  }
+  return transfer_size + available_data;
+}
+
+int File::seek(long offset, int whence) {
+  FileLock lock(this);
+  if (prev_op == FileOp::WRITE && pos > 0) {
+    size_t transferred_size = platform_write(this, buf, pos);
+    if (transferred_size < pos) {
+      err = true;
+      return -1;
+    }
+  }
+  pos = read_limit = 0;
+  prev_op = FileOp::SEEK;
+  // Reset the eof flag as a seek might move the file positon to some place
+  // readable.
+  eof = false;
+  return platform_seek(this, offset, whence);
+}
+
+int File::flush() {
+  FileLock lock(this);
+  if (prev_op == FileOp::WRITE && pos > 0) {
+    size_t transferred_size = platform_write(this, buf, pos);
+    if (transferred_size < pos) {
+      err = true;
+      return -1;
+    }
+    pos = 0;
+    return platform_flush(this);
+  }
+  return 0;
+}
+
+int File::close() {
+  {
+    FileLock lock(this);
+    if (prev_op == FileOp::WRITE && pos > 0) {
+      size_t transferred_size = platform_write(this, buf, pos);
+      if (transferred_size < pos) {
+        err = true;
+        return -1;
+      }
+    }
+    if (platform_close(this) != 0)
+      return -1;
+  }
+  free(this);
+  return 0;
+}
+
+File::ModeFlags File::mode_flags(const char *mode) {
+  // First character in |mode| should be 'a', 'r' or 'w'.
+  if (*mode != 'a' && *mode != 'r' && *mode != 'w')
+    return 0;
+
+  // There should be exaclty one main mode ('a', 'r' or 'w') character.
+  // If there are more than one main mode characters listed, then
+  // we will consider |mode| as incorrect and return 0;
+  int main_mode_count = 0;
+
+  ModeFlags flags = 0;
+  for (; *mode != '\0'; ++mode) {
+    switch (*mode) {
+    case 'r':
+      flags |= static_cast<ModeFlags>(OpenMode::READ);
+      ++main_mode_count;
+      break;
+    case 'w':
+      flags |= static_cast<ModeFlags>(OpenMode::WRITE);
+      ++main_mode_count;
+      break;
+    case '+':
+      flags |= (static_cast<ModeFlags>(OpenMode::WRITE) |
+                static_cast<ModeFlags>(OpenMode::READ));
+      break;
+    case 'b':
+      flags |= static_cast<ModeFlags>(ContentType::BINARY);
+      break;
+    case 'a':
+      flags |= static_cast<ModeFlags>(OpenMode::APPEND);
+      ++main_mode_count;
+      break;
+    case 'x':
+      flags |= static_cast<ModeFlags>(CreateType::EXCLUSIVE);
+      break;
+    default:
+      return 0;
+    }
+  }
+
+  if (main_mode_count != 1)
+    return 0;
+
+  return flags;
+}
+
+} // namespace __llvm_libc

libc/test/src/__support/CMakeLists.txt

 
 add_custom_command(TARGET libc_str_to_float_comparison_test
                    POST_BUILD
                    COMMAND $<TARGET_FILE:libc_str_to_float_comparison_test> ${float_test_file}
                    DEPENDS ${float_test_file}
                    COMMENT "Test the strtof and strtod implementations against precomputed results." 
                    VERBATIM)
 
+add_subdirectory(File)
 add_subdirectory(OSUtil)

libc/test/src/__support/File/CMakeLists.txt

+add_libc_unittest(
+  file_test
+  SRCS
+    file_test.cpp
+  DEPENDS
+    libc.include.stdio
+    libc.src.__support.File.file
+)

libc/test/src/__support/File/file_test.cpp

+//===-- Unittests for platform independent file class ---------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/__support/File/file.h"
+#include "utils/UnitTest/Test.h"
+
+#include <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+using ModeFlags = __llvm_libc::File::ModeFlags;
+
+class StringFile : public __llvm_libc::File {
+  static constexpr size_t SIZE = 512;
+  size_t pos;
+  char str[SIZE] = {0};
+
+  static size_t str_read(__llvm_libc::File *f, char *data, size_t len);
+  static size_t str_write(__llvm_libc::File *f, const char *data, size_t len);
+  static int str_seek(__llvm_libc::File *f, long offset, int whence);
+  static int str_close(__llvm_libc::File *f) { return 0; }
+  static int str_flush(__llvm_libc::File *f) { return 0; }
+
+  // TODO: Add a proper locking system and tests around that.
+  static void str_lock(__llvm_libc::File *f) {}
+  static void str_unlock(__llvm_libc::File *f) {}
+
+public:
+  explicit StringFile(char *buffer, size_t buflen, int bufmode, bool owned,
+                      ModeFlags modeflags)
+      : __llvm_libc::File(&str_write, &str_read, &str_seek, &str_close,
+                          &str_flush, &str_lock, &str_unlock, buffer, buflen,
+                          bufmode, owned, modeflags),
+        pos(0) {}
+
+  void init(char *buffer, size_t buflen, int bufmode, bool owned,
+            ModeFlags modeflags) {
+    File::init(this, &str_write, &str_read, &str_seek, &str_close, &str_flush,
+               &str_lock, &str_unlock, buffer, buflen, bufmode, owned,
+               modeflags);
+    pos = 0;
+  }
+
+  void reset() { pos = 0; }
+  size_t get_pos() const { return pos; }
+  const char *get_str() { return str; }
+};
+
+size_t StringFile::str_read(__llvm_libc::File *f, char *data, size_t len) {
+  StringFile *sf = static_cast<StringFile *>(f);
+  if (sf->pos >= SIZE)
+    return 0;
+  size_t i = 0;
+  for (i = 0; i < len; ++i)
+    data[i] = sf->str[sf->pos + i];
+  sf->pos += i;
+  return i;
+}
+
+size_t StringFile::str_write(__llvm_libc::File *f, const char *data,
+                             size_t len) {
+  StringFile *sf = static_cast<StringFile *>(f);
+  if (sf->pos >= SIZE)
+    return 0;
+  size_t i = 0;
+  for (i = 0; i < len; ++i)
+    sf->str[sf->pos + i] = data[i];
+  sf->pos += i;
+  return i;
+}
+
+int StringFile::str_seek(__llvm_libc::File *f, long offset, int whence) {
+  StringFile *sf = static_cast<StringFile *>(f);
+  if (whence == SEEK_SET)
+    sf->pos = offset;
+  if (whence == SEEK_CUR)
+    sf->pos += offset;
+  if (whence == SEEK_END)
+    sf->pos = SIZE + offset;
+  return 0;
+}
+
+StringFile *new_string_file(char *buffer, size_t buflen, int bufmode,
+                            bool owned, const char *mode) {
+  StringFile *f = reinterpret_cast<StringFile *>(malloc(sizeof(StringFile)));
+  f->init(buffer, buflen, bufmode, owned, __llvm_libc::File::mode_flags(mode));
+  return f;
+}
+
+TEST(LlvmLibcFileTest, SimpleWrite) {
+  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");
+
+  ASSERT_EQ(sizeof(data), f->write(data, sizeof(data)));
+  EXPECT_EQ(f->get_pos(), size_t(0)); // Data is buffered in the file stream
+  ASSERT_EQ(f->flush(), 0);
+  EXPECT_EQ(f->get_pos(), sizeof(data)); // Data should now be available
+  EXPECT_STREQ(f->get_str(), data);
+
+  f->reset();
+  ASSERT_EQ(f->get_pos(), size_t(0));
+  ASSERT_EQ(sizeof(data), f->write(data, sizeof(data)));
+  EXPECT_EQ(f->get_pos(), size_t(0)); // Data is buffered in the file stream
+  // The second write should trigger a buffer flush.
+  ASSERT_EQ(sizeof(data), f->write(data, sizeof(data)));
+  EXPECT_GE(f->get_pos(), size_t(0));
+
+  ASSERT_EQ(f->close(), 0);
+}