[flang] Debugging of ACCESS='STREAM' I/O (take 2)

Corrects the runtime implementation of I/O on files with
the access mode ACCESS='STREAM'.  This is a collection
of edge-case tweaks to ensure that the distinctions between
stream and direct/sequential files, unformatted or formatted,
are respected where appropriate.

Moves NextInField() from io-stmt.h to io-stmt.cpp --
it was getting too big to keep in a header.

This patch exposed a problem with the I/O runtime
on Windows and it was reverted.  This version also
fixes that problem; files are now opened on Windows
in binary mode to prevent inadvertent insertions of
carriage returns before line feeds, and those line
endings (CR+LF) are now explicitly generated.

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

Author: klausler

flang/include/flang/Runtime/iostat.h

@@ -45,7 +45,7 @@ enum Iostat {
   IostatInternalWriteOverrun,
   IostatErrorInFormat,
   IostatErrorInKeyword,
-  IostatEndfileNonSequential,
+  IostatEndfileDirect,
   IostatEndfileUnwritable,
   IostatOpenBadRecl,
   IostatOpenUnknownSize,

flang/runtime/connection.h

@@ -22,15 +22,18 @@ class IoStatementState;
 enum class Direction { Output, Input };
 enum class Access { Sequential, Direct, Stream };
 
-inline bool IsRecordFile(Access a) { return a != Access::Stream; }
-
 // These characteristics of a connection are immutable after being
 // established in an OPEN statement.
 struct ConnectionAttributes {
   Access access{Access::Sequential}; // ACCESS='SEQUENTIAL', 'DIRECT', 'STREAM'
   std::optional<bool> isUnformatted; // FORM='UNFORMATTED' if true
   bool isUTF8{false}; // ENCODING='UTF-8'
   std::optional<std::int64_t> openRecl; // RECL= on OPEN
+
+  bool IsRecordFile() const {
+    // Formatted stream files are viewed as having records, at least on input
+    return access != Access::Stream || !isUnformatted.value_or(true);
+  }
 };
 
 struct ConnectionState : public ConnectionAttributes {

flang/runtime/edit-input.cpp

@@ -19,7 +19,7 @@ static bool EditBOZInput(IoStatementState &io, const DataEdit &edit, void *n,
   std::optional<int> remaining;
   std::optional<char32_t> next{io.PrepareInput(edit, remaining)};
   common::UnsignedInt128 value{0};
-  for (; next; next = io.NextInField(remaining)) {
+  for (; next; next = io.NextInField(remaining, edit)) {
     char32_t ch{*next};
     if (ch == ' ' || ch == '\t') {
       continue;
@@ -63,7 +63,7 @@ static bool ScanNumericPrefix(IoStatementState &io, const DataEdit &edit,
     if (negative || *next == '+') {
       io.GotChar();
       io.SkipSpaces(remaining);
-      next = io.NextInField(remaining, GetDecimalPoint(edit));
+      next = io.NextInField(remaining, edit);
     }
   }
   return negative;
@@ -101,7 +101,7 @@ bool EditIntegerInput(
   bool negate{ScanNumericPrefix(io, edit, next, remaining)};
   common::UnsignedInt128 value{0};
   bool any{negate};
-  for (; next; next = io.NextInField(remaining)) {
+  for (; next; next = io.NextInField(remaining, edit)) {
     char32_t ch{*next};
     if (ch == ' ' || ch == '\t') {
       if (edit.modes.editingFlags & blankZero) {
@@ -167,7 +167,7 @@ static int ScanRealInput(char *buffer, int bufferSize, IoStatementState &io,
     // Subtle: a blank field of digits could be followed by 'E' or 'D',
     for (; next &&
          ((*next >= 'a' && *next <= 'z') || (*next >= 'A' && *next <= 'Z'));
-         next = io.NextInField(remaining)) {
+         next = io.NextInField(remaining, edit)) {
       if (*next >= 'a' && *next <= 'z') {
         Put(*next - 'a' + 'A');
       } else {
@@ -176,7 +176,7 @@ static int ScanRealInput(char *buffer, int bufferSize, IoStatementState &io,
     }
     if (next && *next == '(') { // NaN(...)
       while (next && *next != ')') {
-        next = io.NextInField(remaining);
+        next = io.NextInField(remaining, edit);
       }
     }
     exponent = 0;
@@ -185,7 +185,7 @@ static int ScanRealInput(char *buffer, int bufferSize, IoStatementState &io,
     Put('.'); // input field is normalized to a fraction
     auto start{got};
     bool bzMode{(edit.modes.editingFlags & blankZero) != 0};
-    for (; next; next = io.NextInField(remaining, decimal)) {
+    for (; next; next = io.NextInField(remaining, edit)) {
       char32_t ch{*next};
       if (ch == ' ' || ch == '\t') {
         if (bzMode) {
@@ -214,7 +214,7 @@ static int ScanRealInput(char *buffer, int bufferSize, IoStatementState &io,
       // Optional exponent letter.  Blanks are allowed between the
       // optional exponent letter and the exponent value.
       io.SkipSpaces(remaining);
-      next = io.NextInField(remaining);
+      next = io.NextInField(remaining, edit);
     }
     // The default exponent is -kP, but the scale factor doesn't affect
     // an explicit exponent.
@@ -224,9 +224,9 @@ static int ScanRealInput(char *buffer, int bufferSize, IoStatementState &io,
             (bzMode && (*next == ' ' || *next == '\t')))) {
       bool negExpo{*next == '-'};
       if (negExpo || *next == '+') {
-        next = io.NextInField(remaining);
+        next = io.NextInField(remaining, edit);
       }
-      for (exponent = 0; next; next = io.NextInField(remaining)) {
+      for (exponent = 0; next; next = io.NextInField(remaining, edit)) {
         if (*next >= '0' && *next <= '9') {
           exponent = 10 * exponent + *next - '0';
         } else if (bzMode && (*next == ' ' || *next == '\t')) {
@@ -257,7 +257,7 @@ static int ScanRealInput(char *buffer, int bufferSize, IoStatementState &io,
   // input value.
   if (edit.descriptor == DataEdit::ListDirectedImaginaryPart) {
     if (next && (*next == ' ' || *next == '\t')) {
-      next = io.NextInField(remaining);
+      next = io.NextInField(remaining, edit);
     }
     if (!next) { // NextInField fails on separators like ')'
       next = io.GetCurrentChar();
@@ -267,7 +267,7 @@ static int ScanRealInput(char *buffer, int bufferSize, IoStatementState &io,
     }
   } else if (remaining) {
     while (next && (*next == ' ' || *next == '\t')) {
-      next = io.NextInField(remaining);
+      next = io.NextInField(remaining, edit);
     }
     if (next) {
       return 0; // error: unused nonblank character in fixed-width field
@@ -457,7 +457,7 @@ bool EditLogicalInput(IoStatementState &io, const DataEdit &edit, bool &x) {
   std::optional<int> remaining;
   std::optional<char32_t> next{io.PrepareInput(edit, remaining)};
   if (next && *next == '.') { // skip optional period
-    next = io.NextInField(remaining);
+    next = io.NextInField(remaining, edit);
   }
   if (!next) {
     io.GetIoErrorHandler().SignalError("Empty LOGICAL input field");
@@ -480,7 +480,7 @@ bool EditLogicalInput(IoStatementState &io, const DataEdit &edit, bool &x) {
   if (remaining) { // ignore the rest of the field
     io.HandleRelativePosition(*remaining);
   } else if (edit.descriptor == DataEdit::ListDirected) {
-    while (io.NextInField(remaining)) { // discard rest of field
+    while (io.NextInField(remaining, edit)) { // discard rest of field
     }
   }
   return true;
@@ -520,7 +520,7 @@ static bool EditDelimitedCharacterInput(
 }
 
 static bool EditListDirectedDefaultCharacterInput(
-    IoStatementState &io, char *x, std::size_t length) {
+    IoStatementState &io, char *x, std::size_t length, const DataEdit &edit) {
   auto ch{io.GetCurrentChar()};
   if (ch && (*ch == '\'' || *ch == '"')) {
     io.HandleRelativePosition(1);
@@ -532,8 +532,7 @@ static bool EditListDirectedDefaultCharacterInput(
   // Undelimited list-directed character input: stop at a value separator
   // or the end of the current record.
   std::optional<int> remaining{length};
-  for (std::optional<char32_t> next{io.NextInField(remaining)}; next;
-       next = io.NextInField(remaining)) {
+  while (std::optional<char32_t> next{io.NextInField(remaining, edit)}) {
     switch (*next) {
     case ' ':
     case '\t':
@@ -555,7 +554,7 @@ bool EditDefaultCharacterInput(
     IoStatementState &io, const DataEdit &edit, char *x, std::size_t length) {
   switch (edit.descriptor) {
   case DataEdit::ListDirected:
-    return EditListDirectedDefaultCharacterInput(io, x, length);
+    return EditListDirectedDefaultCharacterInput(io, x, length, edit);
   case 'A':
   case 'G':
     break;
@@ -576,8 +575,7 @@ bool EditDefaultCharacterInput(
   // characters.  When the variable is wider than the field, there's
   // trailing padding.
   std::int64_t skip{*remaining - static_cast<std::int64_t>(length)};
-  for (std::optional<char32_t> next{io.NextInField(remaining)}; next;
-       next = io.NextInField(remaining)) {
+  while (std::optional<char32_t> next{io.NextInField(remaining, edit)}) {
     if (skip > 0) {
       --skip;
       io.GotChar(-1);

flang/runtime/file.cpp

@@ -45,8 +45,8 @@ static int openfile_mkstemp(IoErrorHandler &handler) {
   if (::GetTempFileNameA(tempDirName, "Fortran", uUnique, tempFileName) == 0) {
     return -1;
   }
-  int fd{::_open(
-      tempFileName, _O_CREAT | _O_TEMPORARY | _O_RDWR, _S_IREAD | _S_IWRITE)};
+  int fd{::_open(tempFileName, _O_CREAT | _O_BINARY | _O_TEMPORARY | _O_RDWR,
+      _S_IREAD | _S_IWRITE)};
 #else
   char path[]{"/tmp/Fortran-Scratch-XXXXXX"};
   int fd{::mkstemp(path)};
@@ -82,6 +82,12 @@ void OpenFile::Open(OpenStatus status, std::optional<Action> action,
       return;
     }
     int flags{0};
+#ifdef _WIN32
+    // We emit explicit CR+LF line endings and cope with them on input
+    // for formatted files, since we can't yet always know now at OPEN
+    // time whether the file is formatted or not.
+    flags |= O_BINARY;
+#endif
     if (status != OpenStatus::Old) {
       flags |= O_CREAT;
     }
@@ -154,6 +160,9 @@ void OpenFile::Predefine(int fd) {
   mayRead_ = fd == 0;
   mayWrite_ = fd != 0;
   mayPosition_ = false;
+#ifdef _WIN32
+  isWindowsTextFile_ = true;
+#endif
 }
 
 void OpenFile::Close(CloseStatus status, IoErrorHandler &handler) {

flang/runtime/file.h

@@ -35,8 +35,8 @@ class OpenFile {
   bool mayPosition() const { return mayPosition_; }
   bool mayAsynchronous() const { return mayAsynchronous_; }
   void set_mayAsynchronous(bool yes) { mayAsynchronous_ = yes; }
-  FileOffset position() const { return position_; }
   bool isTerminal() const { return isTerminal_; }
+  bool isWindowsTextFile() const { return isWindowsTextFile_; }
   std::optional<FileOffset> knownSize() const { return knownSize_; }
 
   bool IsConnected() const { return fd_ >= 0; }
@@ -98,6 +98,7 @@ class OpenFile {
   FileOffset position_{0};
   std::optional<FileOffset> knownSize_;
   bool isTerminal_{false};
+  bool isWindowsTextFile_{false}; // expands LF to CR+LF on write
 
   int nextId_;
   OwningPtr<Pending> pending_;

flang/runtime/io-api.cpp

@@ -524,18 +524,17 @@ bool IONAME(SetPad)(Cookie cookie, const char *keyword, std::size_t length) {
 bool IONAME(SetPos)(Cookie cookie, std::int64_t pos) {
   IoStatementState &io{*cookie};
   ConnectionState &connection{io.GetConnectionState()};
+  IoErrorHandler &handler{io.GetIoErrorHandler()};
   if (connection.access != Access::Stream) {
-    io.GetIoErrorHandler().SignalError(
-        "POS= may not appear unless ACCESS='STREAM'");
+    handler.SignalError("POS= may not appear unless ACCESS='STREAM'");
     return false;
   }
-  if (pos < 1) {
-    io.GetIoErrorHandler().SignalError(
-        "POS=%zd is invalid", static_cast<std::intmax_t>(pos));
+  if (pos < 1) { // POS=1 is beginning of file (12.6.2.11)
+    handler.SignalError("POS=%zd is invalid", static_cast<std::intmax_t>(pos));
     return false;
   }
   if (auto *unit{io.GetExternalFileUnit()}) {
-    unit->SetPosition(pos);
+    unit->SetPosition(pos - 1, handler);
     return true;
   }
   io.GetIoErrorHandler().Crash("SetPos() on internal unit");
@@ -545,23 +544,22 @@ bool IONAME(SetPos)(Cookie cookie, std::int64_t pos) {
 bool IONAME(SetRec)(Cookie cookie, std::int64_t rec) {
   IoStatementState &io{*cookie};
   ConnectionState &connection{io.GetConnectionState()};
+  IoErrorHandler &handler{io.GetIoErrorHandler()};
   if (connection.access != Access::Direct) {
-    io.GetIoErrorHandler().SignalError(
-        "REC= may not appear unless ACCESS='DIRECT'");
+    handler.SignalError("REC= may not appear unless ACCESS='DIRECT'");
     return false;
   }
   if (!connection.openRecl) {
-    io.GetIoErrorHandler().SignalError("RECL= was not specified");
+    handler.SignalError("RECL= was not specified");
     return false;
   }
   if (rec < 1) {
-    io.GetIoErrorHandler().SignalError(
-        "REC=%zd is invalid", static_cast<std::intmax_t>(rec));
+    handler.SignalError("REC=%zd is invalid", static_cast<std::intmax_t>(rec));
     return false;
   }
   connection.currentRecordNumber = rec;
   if (auto *unit{io.GetExternalFileUnit()}) {
-    unit->SetPosition((rec - 1) * *connection.openRecl);
+    unit->SetPosition((rec - 1) * *connection.openRecl, handler);
   }
   return true;
 }

flang/runtime/io-stmt.cpp

@@ -268,7 +268,7 @@ ExternalIoStatementState<DIR>::ExternalIoStatementState(
     : ExternalIoStatementBase{unit, sourceFile, sourceLine}, mutableModes_{
                                                                  unit.modes} {
   if constexpr (DIR == Direction::Output) {
-    // If the last statement was a non advancing IO input statement, the unit
+    // If the last statement was a non-advancing IO input statement, the unit
     // furthestPositionInRecord was not advanced, but the positionInRecord may
     // have been advanced. Advance furthestPositionInRecord here to avoid
     // overwriting the part of the record that has been read with blanks.
@@ -505,6 +505,66 @@ bool IoStatementState::EmitField(
   }
 }
 
+std::optional<char32_t> IoStatementState::NextInField(
+    std::optional<int> &remaining, const DataEdit &edit) {
+  if (!remaining) { // Stream, list-directed, or NAMELIST
+    if (auto next{GetCurrentChar()}) {
+      if (edit.IsListDirected()) {
+        // list-directed or NAMELIST: check for separators
+        switch (*next) {
+        case ' ':
+        case '\t':
+        case ';':
+        case '/':
+        case '(':
+        case ')':
+        case '\'':
+        case '"':
+        case '*':
+        case '\n': // for stream access
+          return std::nullopt;
+        case ',':
+          if (edit.modes.editingFlags & decimalComma) {
+            break;
+          } else {
+            return std::nullopt;
+          }
+        default:
+          break;
+        }
+      }
+      HandleRelativePosition(1);
+      GotChar();
+      return next;
+    }
+  } else if (*remaining > 0) {
+    if (auto next{GetCurrentChar()}) {
+      --*remaining;
+      HandleRelativePosition(1);
+      GotChar();
+      return next;
+    }
+    const ConnectionState &connection{GetConnectionState()};
+    if (!connection.IsAtEOF()) {
+      if (auto length{connection.EffectiveRecordLength()}) {
+        if (connection.positionInRecord >= *length) {
+          IoErrorHandler &handler{GetIoErrorHandler()};
+          if (mutableModes().nonAdvancing) {
+            handler.SignalEor();
+          } else if (connection.openRecl && !connection.modes.pad) {
+            handler.SignalError(IostatRecordReadOverrun);
+          }
+          if (connection.modes.pad) { // PAD='YES'
+            --*remaining;
+            return std::optional<char32_t>{' '};
+          }
+        }
+      }
+    }
+  }
+  return std::nullopt;
+}
+
 bool IoStatementState::Inquire(
     InquiryKeywordHash inquiry, char *out, std::size_t chars) {
   return std::visit(
@@ -1060,7 +1120,7 @@ bool InquireUnitState::Inquire(
     result = unit().IsConnected() ? unit().unitNumber() : -1;
     return true;
   case HashInquiryKeyword("POS"):
-    result = unit().position();
+    result = unit().InquirePos();
     return true;
   case HashInquiryKeyword("RECL"):
     if (!unit().IsConnected()) {