[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
GitOrigin-RevId: 5da55bfc18f64b7171be150913e2f5eac6e0f184
diff --git a/runtime/buffer.h b/runtime/buffer.h
index c5bd5ae..c601ee7 100644
--- a/runtime/buffer.h
+++ b/runtime/buffer.h
@@ -27,7 +27,24 @@
// 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 @@
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 @@
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 @@
}
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 @@
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/unittests/Runtime/CMakeLists.txt b/unittests/Runtime/CMakeLists.txt
index b80ecea..616c86f 100644
--- a/unittests/Runtime/CMakeLists.txt
+++ b/unittests/Runtime/CMakeLists.txt
@@ -46,3 +46,8 @@
RuntimeTesting
FortranRuntime
)
+
+add_flang_nongtest_unittest(buffer
+ RuntimeTesting
+ FortranRuntime
+)
diff --git a/unittests/Runtime/buffer.cpp b/unittests/Runtime/buffer.cpp
new file mode 100644
index 0000000..4f1c96b
--- /dev/null
+++ b/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();
+}
