[flang] Fix output buffering bug

[klausler]

Summary

The I/O runtime library code was failing to retain data in a buffer
from the current output record when flushing the buffer; this is
fatally wrong when the corresponding file cannot be repositioned,
as in the case of standard output to the console. So refine the
Flush() member function to retain a specified number of bytes,
rearrange the data as necessary (using existing code for read frame
management after moving it into a new member function), and add
a big comment to the head of the file to clarify the roles of the
various data members in the management of contiguous frames in
circular buffers.

[Leporacanthicus]

I'm wondering if it's possible to construct a test-case for this "shuffle it around" situation, so that some newbie (like me ...) doesn't break it accidentally when making changes?

[klausler]

In D99198#2645322, @Leporacanthicus wrote:

I'm wondering if it's possible to construct a test-case for this "shuffle it around" situation, so that some newbie (like me ...) doesn't break it accidentally when making changes?

It gets exercised pretty thoroughly by the I/O tests in the FCVS Fortran '77 suite, especially by codes doing direct and sequential I/O, and the flang/unittests/Runtime/external-io.cpp test really shook it out on the read path (and still passes with this change now that it's used on the write path). But it would be nice to be able to make the code work harder by e.g. using an artificially small buffer size. I've done that in the past during development (but obviously not for console output, sorry) but I'm not sure how it could be made part of a unit testing framework.

[Leporacanthicus]

In D99198#2645402, @klausler wrote:

In D99198#2645322, @Leporacanthicus wrote:

I'm wondering if it's possible to construct a test-case for this "shuffle it around" situation, so that some newbie (like me ...) doesn't break it accidentally when making changes?

It gets exercised pretty thoroughly by the I/O tests in the FCVS Fortran '77 suite, especially by codes doing direct and sequential I/O, and the flang/unittests/Runtime/external-io.cpp test really shook it out on the read path (and still passes with this change now that it's used on the write path). But it would be nice to be able to make the code work harder by e.g. using an artificially small buffer size. I've done that in the past during development (but obviously not for console output, sorry) but I'm not sure how it could be made part of a unit testing framework.

Yes, I am asking without fully understanding the overall infrastructure. A way to test without having to write 64KB+ of data would be nice - but not sure how easy that is to achieve either.
From what I can tell from a quick look, the buffer functionality itself isn't being tested, it is the IO functionality that is layered on top of it that gets tested - so it wouldn't be very easy to implement a dummy STORE class that tracks the actual output and expects to see certain strings - even if we could make the buffer size smaller.
Maybe the right thing would be to actually add a small test-suite for the buffer itself, along with some kind of variable setting for the buffer-size?

These are just my thoughts, as a newbie to this project.

[klausler]

In D99198#2645631, @Leporacanthicus wrote:

In D99198#2645402, @klausler wrote:

In D99198#2645322, @Leporacanthicus wrote:

I'm wondering if it's possible to construct a test-case for this "shuffle it around" situation, so that some newbie (like me ...) doesn't break it accidentally when making changes?

It gets exercised pretty thoroughly by the I/O tests in the FCVS Fortran '77 suite, especially by codes doing direct and sequential I/O, and the flang/unittests/Runtime/external-io.cpp test really shook it out on the read path (and still passes with this change now that it's used on the write path). But it would be nice to be able to make the code work harder by e.g. using an artificially small buffer size. I've done that in the past during development (but obviously not for console output, sorry) but I'm not sure how it could be made part of a unit testing framework.

Yes, I am asking without fully understanding the overall infrastructure. A way to test without having to write 64KB+ of data would be nice - but not sure how easy that is to achieve either.
From what I can tell from a quick look, the buffer functionality itself isn't being tested, it is the IO functionality that is layered on top of it that gets tested - so it wouldn't be very easy to implement a dummy STORE class that tracks the actual output and expects to see certain strings - even if we could make the buffer size smaller.
Maybe the right thing would be to actually add a small test-suite for the buffer itself, along with some kind of variable setting for the buffer-size?

These are just my thoughts, as a newbie to this project.

That's a fine idea, and I encourage you to take a whack at it. I am swamped.

[PeteSteinfeld]

All builds, tests, and looks good.

[kiranchandramohan]

LGTM. Thanks Peter for fixing this quickly.

flang/runtime/buffer.h

31

This detailed explanation is helpful.

177

Nice function and inline documentation.

[klausler]

Added a unit test.

[Leporacanthicus]

Nice test, thanks for adding that!

[Leporacanthicus]

LGTM

[klausler]

In D99198#2646917, @Leporacanthicus wrote:

LGTM

I have verified that the new test fails without the patch to buffer.h. I will run the FCVS I/O tests with the new patch before merging.

flang/runtime/buffer.h

 void LeftShiftBufferCircularly(char *, std::size_t bytes, std::size_t shift);
 
 // Maintains a view of a contiguous region of a file in a memory buffer.
 // The valid data in the buffer may be circular, but any active frame
 // will also be contiguous in memory.  The requirement stems from the need to
 // preserve read data that may be reused by means of Tn/TLn edit descriptors
 // without needing to position the file (which may not always be possible,
 // e.g. a socket) and a general desire to reduce system call counts.
-template <typename STORE> class FileFrame {
+//
+// Possible scenario with a tiny 32-byte buffer after a ReadFrame or
+// WriteFrame with a file offset of 103 to access "DEF":
+//
+//    fileOffset_ 100 --+  +-+ frame of interest (103:105)
+//   file:  ............ABCDEFGHIJKLMNOPQRSTUVWXYZ....
+// buffer: [NOPQRSTUVWXYZ......ABCDEFGHIJKLM]   (size_ == 32)
+//                             |  +-- frame_ == 3
+//                             +----- start_ == 19, length_ == 26
+//
+// The buffer holds length_ == 26 bytes from file offsets 100:125.
+// Those 26 bytes "wrap around" the end of the circular buffer,
+// so file offsets 100:112 map to buffer offsets 19:31 ("A..M") and
+//    file offsets 113:125 map to buffer offsets  0:12 ("N..Z")
+// The 3-byte frame of file offsets 103:105 is contiguous in the buffer
+// at buffer offset (start_ + frame_) == 22 ("DEF").
+
+template <typename STORE, std::size_t minBuffer = 65536> class FileFrame {
 public:
   using FileOffset = std::int64_t;
 
   ~FileFrame() { FreeMemoryAndNullify(buffer_); }
 
   // The valid data in the buffer begins at buffer_[start_] and proceeds
   // with possible wrap-around for length_ bytes.  The current frame
   // is offset by frame_ bytes into that region and is guaranteed to
   // be contiguous for at least as many bytes as were requested.
 
   FileOffset FrameAt() const { return fileOffset_ + frame_; }
   char *Frame() const { return buffer_ + start_ + frame_; }
   std::size_t FrameLength() const {
     return std::min<std::size_t>(length_ - frame_, size_ - (start_ + frame_));
   }
   std::size_t BytesBufferedBeforeFrame() const { return frame_ - start_; }
 
   // Returns a short frame at a non-fatal EOF.  Can return a long frame as well.
   std::size_t ReadFrame(
       FileOffset at, std::size_t bytes, IoErrorHandler &handler) {
     Flush(handler);
     Reallocate(bytes, handler);
-    if (at < fileOffset_ || at > fileOffset_ + length_) {
+    std::int64_t newFrame{at - fileOffset_};
+    if (newFrame < 0 || newFrame > length_) {
       Reset(at);
+    } else {
+      frame_ = newFrame;
     }
-    frame_ = at - fileOffset_;
+    RUNTIME_CHECK(handler, at == fileOffset_ + frame_);
     if (static_cast<std::int64_t>(start_ + frame_ + bytes) > size_) {
       DiscardLeadingBytes(frame_, handler);
-      if (static_cast<std::int64_t>(start_ + bytes) > size_) {
-        // Frame would wrap around; shift current data (if any) to force
-        // contiguity.
-        RUNTIME_CHECK(handler, length_ < size_);
-        if (start_ + length_ <= size_) {
-          // [......abcde..] -> [abcde........]
-          std::memmove(buffer_, buffer_ + start_, length_);
-        } else {
-          // [cde........ab] -> [abcde........]
-          auto n{start_ + length_ - size_}; // 3 for cde
-          RUNTIME_CHECK(handler, length_ >= n);
-          std::memmove(buffer_ + n, buffer_ + start_, length_ - n); // cdeab
-          LeftShiftBufferCircularly(buffer_, length_, n); // abcde
-        }
-        start_ = 0;
-      }
+      MakeDataContiguous(handler, bytes);
+      RUNTIME_CHECK(handler, at == fileOffset_ + frame_);
     }
     while (FrameLength() < bytes) {
       auto next{start_ + length_};
       RUNTIME_CHECK(handler, next < size_);
       auto minBytes{bytes - FrameLength()};
       auto maxBytes{size_ - next};
       auto got{Store().Read(
           fileOffset_ + length_, buffer_ + next, minBytes, maxBytes, handler)};
       length_ += got;
-      RUNTIME_CHECK(handler, length_ < size_);
+      RUNTIME_CHECK(handler, length_ <= size_);
       if (got < minBytes) {
         break; // error or EOF & program can handle it
       }
     }
     return FrameLength();
   }
 
   void WriteFrame(FileOffset at, std::size_t bytes, IoErrorHandler &handler) {
-    if (!dirty_ || at < fileOffset_ || at > fileOffset_ + length_ ||
-        start_ + (at - fileOffset_) + static_cast<std::int64_t>(bytes) >
-            size_) {
+    Reallocate(bytes, handler);
+    std::int64_t newFrame{at - fileOffset_};
+    if (!dirty_ || newFrame < 0 || newFrame > length_) {
       Flush(handler);
       Reset(at);
-      Reallocate(bytes, handler);
+    } else if (start_ + newFrame + static_cast<std::int64_t>(bytes) > size_) {
+      // Flush leading data before "at", retain from "at" onward
+      Flush(handler, length_ - newFrame);
+      MakeDataContiguous(handler, bytes);
+    } else {
+      frame_ = newFrame;
     }
+    RUNTIME_CHECK(handler, at == fileOffset_ + frame_);
     dirty_ = true;
-    frame_ = at - fileOffset_;
     length_ = std::max<std::int64_t>(length_, frame_ + bytes);
   }
 
-  void Flush(IoErrorHandler &handler) {
+  void Flush(IoErrorHandler &handler, std::int64_t keep = 0) {
     if (dirty_) {
-      while (length_ > 0) {
-        std::size_t chunk{std::min<std::size_t>(length_, size_ - start_)};
+      while (length_ > keep) {
+        std::size_t chunk{
+            std::min<std::size_t>(length_ - keep, size_ - start_)};
         std::size_t put{
             Store().Write(fileOffset_, buffer_ + start_, chunk, handler)};
-        length_ -= put;
-        start_ += put;
-        fileOffset_ += put;
+        DiscardLeadingBytes(put, handler);
         if (put < chunk) {
           break;
         }
       }
+      if (length_ == 0) {
         Reset(fileOffset_);
       }
     }
+  }
 
 private:
   STORE &Store() { return static_cast<STORE &>(*this); }
 
   void Reallocate(std::int64_t bytes, const Terminator &terminator) {
     if (bytes > size_) {
       char *old{buffer_};
       auto oldSize{size_};
void DiscardLeadingBytes(std::int64_t n, const Terminator &terminator) {
     if (frame_ >= n) {
       frame_ -= n;
     } else {
       frame_ = 0;
     }
     fileOffset_ += n;
   }
 
-  static constexpr std::size_t minBuffer{64 << 10};
+  void MakeDataContiguous(IoErrorHandler &handler, std::size_t bytes) {
+    if (static_cast<std::int64_t>(start_ + bytes) > size_) {
+      // Frame would wrap around; shift current data (if any) to force
+      // contiguity.
+      RUNTIME_CHECK(handler, length_ < size_);
+      if (start_ + length_ <= size_) {
+        // [......abcde..] -> [abcde........]
+        std::memmove(buffer_, buffer_ + start_, length_);
+      } else {
+        // [cde........ab] -> [abcde........]
+        auto n{start_ + length_ - size_}; // 3 for cde
+        RUNTIME_CHECK(handler, length_ >= n);
+        std::memmove(buffer_ + n, buffer_ + start_, length_ - n); // cdeab
+        LeftShiftBufferCircularly(buffer_, length_, n); // abcde
+      }
+      start_ = 0;
+    }
+  }
 
   char *buffer_{nullptr};
   std::int64_t size_{0}; // current allocated buffer size
   FileOffset fileOffset_{0}; // file offset corresponding to buffer valid data
   std::int64_t start_{0}; // buffer_[] offset of valid data
   std::int64_t length_{0}; // valid data length (can wrap)
   std::int64_t frame_{0}; // offset of current frame in valid data
   bool dirty_{false};
 };
 } // namespace Fortran::runtime::io
 #endif // FORTRAN_RUNTIME_BUFFER_H_

flang/unittests/Runtime/CMakeLists.txt

add_flang_nongtest_unittest(external-io
   RuntimeTesting
   FortranRuntime
 )
 
 add_flang_nongtest_unittest(list-input
   RuntimeTesting
   FortranRuntime
 )
+
+add_flang_nongtest_unittest(buffer
+  RuntimeTesting
+  FortranRuntime
+)

flang/unittests/Runtime/buffer.cpp

+#include "../../runtime/buffer.h"
+#include "testing.h"
+#include <algorithm>
+#include <cstdint>
+#include <cstring>
+#include <memory>
+
+static constexpr std::size_t tinyBuffer{32};
+using FileOffset = std::int64_t;
+using namespace Fortran::runtime;
+using namespace Fortran::runtime::io;
+
+class Store : public FileFrame<Store, tinyBuffer> {
+public:
+  explicit Store(std::size_t bytes = 65536) : bytes_{bytes} {
+    data_.reset(new char[bytes]);
+    std::memset(&data_[0], 0, bytes);
+  }
+  std::size_t bytes() const { return bytes_; }
+  void set_enforceSequence(bool yes = true) { enforceSequence_ = yes; }
+  void set_expect(FileOffset to) { expect_ = to; }
+
+  std::size_t Read(FileOffset at, char *to, std::size_t minBytes,
+      std::size_t maxBytes, IoErrorHandler &handler) {
+    if (enforceSequence_ && at != expect_) {
+      handler.SignalError("Read(%d,%d,%d) not at expected %d",
+          static_cast<int>(at), static_cast<int>(minBytes),
+          static_cast<int>(maxBytes), static_cast<int>(expect_));
+    } else if (at < 0 || at + minBytes > bytes_) {
+      handler.SignalError("Read(%d,%d,%d) is out of bounds",
+          static_cast<int>(at), static_cast<int>(minBytes),
+          static_cast<int>(maxBytes));
+    }
+    auto result{std::min(maxBytes, bytes_ - at)};
+    std::memcpy(to, &data_[at], result);
+    expect_ = at + result;
+    return result;
+  }
+  std::size_t Write(FileOffset at, const char *from, std::size_t bytes,
+      IoErrorHandler &handler) {
+    if (enforceSequence_ && at != expect_) {
+      handler.SignalError("Write(%d,%d) not at expected %d",
+          static_cast<int>(at), static_cast<int>(bytes),
+          static_cast<int>(expect_));
+    } else if (at < 0 || at + bytes > bytes_) {
+      handler.SignalError("Write(%d,%d) is out of bounds", static_cast<int>(at),
+          static_cast<int>(bytes));
+    }
+    std::memcpy(&data_[at], from, bytes);
+    expect_ = at + bytes;
+    return bytes;
+  }
+
+private:
+  std::size_t bytes_;
+  std::unique_ptr<char[]> data_;
+  bool enforceSequence_{false};
+  FileOffset expect_{0};
+};
+
+inline int ChunkSize(int j, int most) {
+  // 31, 1, 29, 3, 27, ...
+  j %= tinyBuffer;
+  auto chunk{
+      static_cast<int>(((j % 2) ? j : (tinyBuffer - 1 - j)) % tinyBuffer)};
+  return std::min(chunk, most);
+}
+
+inline int ValueFor(int at) { return (at ^ (at >> 8)) & 0xff; }
+
+int main() {
+  StartTests();
+  Terminator terminator{__FILE__, __LINE__};
+  IoErrorHandler handler{terminator};
+  Store store;
+  store.set_enforceSequence(true);
+  const auto bytes{static_cast<FileOffset>(store.bytes())};
+  // Fill with an assortment of chunks
+  int at{0}, j{0};
+  while (at < bytes) {
+    auto chunk{ChunkSize(j, static_cast<int>(bytes - at))};
+    store.WriteFrame(at, chunk, handler);
+    char *to{store.Frame()};
+    for (int k{0}; k < chunk; ++k) {
+      to[k] = ValueFor(at + k);
+    }
+    at += chunk;
+    ++j;
+  }
+  store.Flush(handler);
+  // Validate
+  store.set_expect(0);
+  at = 0;
+  while (at < bytes) {
+    auto chunk{ChunkSize(j, static_cast<int>(bytes - at))};
+    std::size_t frame{store.ReadFrame(at, chunk, handler)};
+    if (frame < static_cast<std::size_t>(chunk)) {
+      Fail() << "Badly-sized ReadFrame at " << at << ", chunk=" << chunk
+             << ", got " << frame << '\n';
+      break;
+    }
+    const char *from{store.Frame()};
+    for (int k{0}; k < chunk; ++k) {
+      auto expect{static_cast<char>(ValueFor(at + k))};
+      if (from[k] != expect) {
+        Fail() << "At " << at << '+' << k << '(' << (at + k) << "), read "
+               << (from[k] & 0xff) << ", expected " << static_cast<int>(expect)
+               << '\n';
+      }
+    }
+    at += chunk;
+    ++j;
+  }
+  return EndTests();
+}