[flang-commits] [flang] 5da55bf - [flang] Fix output buffering bug (positionability assumption)

peter klausler via flang-commits flang-commits at lists.llvm.org
Wed Mar 24 11:39:27 PDT 2021


Author: peter klausler
Date: 2021-03-24T11:39:18-07:00
New Revision: 5da55bfc18f64b7171be150913e2f5eac6e0f184

URL: https://github.com/llvm/llvm-project/commit/5da55bfc18f64b7171be150913e2f5eac6e0f184
DIFF: https://github.com/llvm/llvm-project/commit/5da55bfc18f64b7171be150913e2f5eac6e0f184.diff

LOG: [flang] Fix output buffering bug (positionability assumption)

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.

Update: added a unit test.

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

Added: 
    flang/unittests/Runtime/buffer.cpp

Modified: 
    flang/runtime/buffer.h
    flang/unittests/Runtime/CMakeLists.txt

Removed: 
    


################################################################################
diff  --git a/flang/runtime/buffer.h b/flang/runtime/buffer.h
index c5bd5aedaaee..c601ee7c4be0 100644
--- a/flang/runtime/buffer.h
+++ b/flang/runtime/buffer.h
@@ -27,7 +27,24 @@ void LeftShiftBufferCircularly(char *, std::size_t bytes, std::size_t shift);
 // 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;
 
@@ -50,28 +67,17 @@ template <typename STORE> class FileFrame {
       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_};
@@ -81,7 +87,7 @@ template <typename STORE> class FileFrame {
       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
       }
@@ -90,32 +96,38 @@ template <typename STORE> class FileFrame {
   }
 
   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;
         }
       }
-      Reset(fileOffset_);
+      if (length_ == 0) {
+        Reset(fileOffset_);
+      }
     }
   }
 
@@ -162,7 +174,24 @@ template <typename STORE> class FileFrame {
     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

diff  --git a/flang/unittests/Runtime/CMakeLists.txt b/flang/unittests/Runtime/CMakeLists.txt
index b80eceac7446..616c86f279fe 100644
--- a/flang/unittests/Runtime/CMakeLists.txt
+++ b/flang/unittests/Runtime/CMakeLists.txt
@@ -46,3 +46,8 @@ add_flang_nongtest_unittest(list-input
   RuntimeTesting
   FortranRuntime
 )
+
+add_flang_nongtest_unittest(buffer
+  RuntimeTesting
+  FortranRuntime
+)

diff  --git a/flang/unittests/Runtime/buffer.cpp b/flang/unittests/Runtime/buffer.cpp
new file mode 100644
index 000000000000..4f1c96b63350
--- /dev/null
+++ b/flang/unittests/Runtime/buffer.cpp
@@ -0,0 +1,115 @@
+#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();
+}


        


More information about the flang-commits mailing list