[flang][msvc] Fix external-io unittest.

[Meinersbur]

Summary

Fix the external-io unittest under Windows.

In particular, fixes the following issues:

1. When creating a temporary file, open it with read+write permissions.
2. _chsize returns 0 on success (just like ftruncate).
3. To set a std::optional, use its assign-operator overload instead of getting a reference of its value an overwrite that. The latter is invalid if the std::optional has no value, and is caught by msvc's debug STL.

The non-GTest unittest is currently not executed under Windows because of the added .exe extension to the output file: external-io.text.exe. llvm-lit skips the file because .exe is not in the lists of test suffixes (.test is). D105315 is going to change that by converting it to a gtest-test.

[awarzynski]

These changes make a lot of sense to me. This is making sure that WIN32 implementation is consistent with Linux/Unix and it fixes bugs that we probably missed because the affected tests are skipped on Windows.

I don't have a Windows machine to test this on, but this looks fairly straightforward and I think that it is safe to approve it as is. I think that Phabricator's CI would highlight any potential issues with this.

Does it make sense to make D105315 depend on this and let the CI on Phabricator test it for us?

[Meinersbur]

In D106726#2905066, @awarzynski wrote:

I don't have a Windows machine to test this on, but this looks fairly straightforward and I think that it is safe to approve it as is. I think that Phabricator's CI would highlight any potential issues with this.

I just pushed the patch. That is, when https://reviews.llvm.org/D105315 is updated and the pre-merge bot runs again (maybe re-upload the same diff again?),we should see the result.

Does it make sense to make D105315 depend on this and let the CI on Phabricator test it for us?

Since I just pushed the patch, it does not matter. Otherwise it could have made sense such that the pre-merge builder would apply this patch beforehande before compiling D105315.

[Meinersbur]

I added D105315 as a child revision; maybe this also triggers a re-run of the pre-merge check of D105315.

flang/runtime/file.cpp

#ifdef _WIN32
   unsigned long nBufferLength{sizeof(tempDirName)};
   nBufferLength = ::GetTempPathA(nBufferLength, tempDirName);
   if (nBufferLength > sizeof(tempDirName) || nBufferLength == 0) {
     return -1;
   }
   if (::GetTempFileNameA(tempDirName, "Fortran", uUnique, tempFileName) == 0) {
     return -1;
   }
-  int fd{::_open(tempFileName, _O_CREAT | _O_TEMPORARY, _S_IREAD | _S_IWRITE)};
+  int fd{::_open(
+      tempFileName, _O_CREAT | _O_TEMPORARY | _O_RDWR, _S_IREAD | _S_IWRITE)};
 #else
   char path[]{"/tmp/Fortran-Scratch-XXXXXX"};
   int fd{::mkstemp(path)};
 #endif
   if (fd < 0) {
     handler.SignalErrno();
   }
 #ifndef _WIN32
std::size_t OpenFile::Write(FileOffset at, const char *buffer,
   if (knownSize_ && position_ > *knownSize_) {
     knownSize_ = position_;
   }
   return put;
 }
 
 inline static int openfile_ftruncate(int fd, OpenFile::FileOffset at) {
 #ifdef _WIN32
-  return !::_chsize(fd, at);
+  return ::_chsize(fd, at);
 #else
   return ::ftruncate(fd, at);
 #endif
 }
 
 void OpenFile::Truncate(FileOffset at, IoErrorHandler &handler) {
   CheckOpen(handler);
   if (!knownSize_ || *knownSize_ != at) {

flang/runtime/unit.cpp

if (prevNL < 0) {
     handler.SignalError(IostatBackspaceAtFirstRecord);
     return;
   }
   while (true) {
     if (frameOffsetInFile_ < prevNL) {
       if (const char *p{
               FindLastNewline(Frame(), prevNL - 1 - frameOffsetInFile_)}) {
         recordOffsetInFrame_ = p - Frame() + 1;
-        *recordLength = prevNL - (frameOffsetInFile_ + recordOffsetInFrame_);
+        recordLength = prevNL - (frameOffsetInFile_ + recordOffsetInFrame_);
         break;
       }
     }
     if (frameOffsetInFile_ == 0) {
       recordOffsetInFrame_ = 0;
-      *recordLength = prevNL;
+      recordLength = prevNL;
       break;
     }
     frameOffsetInFile_ -= std::min<std::int64_t>(frameOffsetInFile_, 1024);
     auto need{static_cast<std::size_t>(prevNL + 1 - frameOffsetInFile_)};
     auto got{ReadFrame(frameOffsetInFile_, need, handler)};
     RUNTIME_CHECK(handler, got >= need);
   }
   RUNTIME_CHECK(handler, Frame()[recordOffsetInFrame_ + *recordLength] == '\n');