[flang][runtime] Support all non-ASCII characters in temporary path on Windows

[mmuetzel]

Summary

If the path to the TEMP folder contains (non-ASCII) characters that cannot be encoded in the current 8-bit locale of the user, openfile_mkstemp might fail on Windows.
That is an unlikely scenario. But given that the path to the default TEMP folder on Windows contains the Windows user name, it is still possible.

Use the wide character Windows API to avoid that (unlikely) issue.

[vzakhari]

Looks good to me. Thank you!
@klausler, FYI

[mmuetzel]

I received a notification from the buildbot that the tests are failing since this change:

Full details are available at:
https://lab.llvm.org/buildbot#builders/65/builds/9856

Worker for this Build: linaro-armv8-windows-msvc-04
Blamelist:
Markus Mützel <[e-mail]>,
Simon Pilgrim <[e-mail]>

BUILD FAILED: failed 37576 expected passes 90 expected failures 9 unexpected failures 34485 unsupported tests (failure)

Step 4 (build stage 1) warnings: 'ninja' (warnings)
C:/Users/tcwg/llvm-worker/clang-arm64-windows-msvc/llvm/mlir/include\mlir/IR/BuiltinAttributes.h(353,14): warning: 'complex' is deprecated: warning STL4037: The effect of instantiating the template std::complex for any type other than float, double, or long double is unspecified. You can define _SILENCE_NONFLOATING_COMPLEX_DEPRECATION_WARNING to suppress this warning. [-Wdeprecated-declarations]
[[deprecated("warning STL4037: " \
1 warning generated.

And 8 more similar warnings.
But I don't know how that could be related to this change.

Did I do something wrong when pushing?

[mmuetzel]

Oops: nBufferLength is the number of TCHARs in tempDirName. But sizeof(tempDirName) returns the number of bytes.

Should this change be reverted and a fixed version be pushed instead?
Or is a follow-up change preferred?

flang/runtime/file.cpp

35–36

Is that the preferred style? Or do you prefer sizeof(tempDirName[0]) in place of sizeof(wchar_t)?

36–43

[klausler]

flang/runtime/file.cpp

35–36

I prefer sizeof tempDirName[0]. Parentheses are required for types, not for variables.

[mmuetzel]

flang/runtime/file.cpp

35–36

Just to clarify: That means this should be initialized with sizeof tempDirName / sizeof tempDirName[0]?

I already pushed this change to the repository (too prematurely in hindsight). Should I open a new Diff on Phabricator? Or is it ok to follow up here?

[omjavaid]

Hi

This change has broken various unit tests on AArch64 Windows platform. I going to revert this change temporarily.

https://lab.llvm.org/buildbot/#/builders/65/builds/9856

Thanks

[omjavaid]

Kindly fix flang unit tests for Windows before relanding. Thanks

[mmuetzel]

@omjavaid: Thank you for reverting this change. I was unsure how to handle this situation correctly.

At least in theory, this change (if done correctly) shouldn't affect the result of the unit tests. However, the version that I pushed contained an error. (I'm sorry for that.) It was probably just a coincidence that the error only surfaced on aarch64.

I don't have the means to check with Windows on ARM locally. But I hope those unit tests will no longer fail with the updated diff.

[mmuetzel]

Ran clang-format on the diff.

[mmuetzel]

Would it be ok to give this another try now that the issue with the BufferLength is fixed?

[mstorsjo]

In D151571#4434043, @mmuetzel wrote:

Would it be ok to give this another try now that the issue with the BufferLength is fixed?

If the known issue has been fixed and it is likely that this issue is what caused the breakage before, it's definitely ok to reland the patch. If there were links given to public buildbots where it broke the last time, keep an eye out for them after relanding it.

The fact that the buffer sizes were specified as too large, would that only cause an issue if the paths actually exceed the buffer, or do some of these functions e.g. pad the buffers to the full given length, which would overwrite past the buffer end and cause issues even if actual paths were short enough?

[mmuetzel]

In D151571#4442760, @mstorsjo wrote:

The fact that the buffer sizes were specified as too large, would that only cause an issue if the paths actually exceed the buffer, or do some of these functions e.g. pad the buffers to the full given length, which would overwrite past the buffer end and cause issues even if actual paths were short enough?

My suspicion is that the issue arose because GetTempPathW was called specifying a buffer length nBufferLength that was larger than the actual buffer tempDirName (and larger than MAX_PATH).
I couldn't reproduce the error locally though (only occurs on ARM64?). So, I don't know for sure if this was actually the cause of the test errors.

flang/runtime/file.cpp

 
 void OpenFile::set_path(OwningPtr<char> &&path, std::size_t bytes) {
   path_ = std::move(path);
   pathLength_ = bytes;
 }
 
 static int openfile_mkstemp(IoErrorHandler &handler) {
 #ifdef _WIN32
   const unsigned int uUnique{0};
-  // GetTempFileNameA needs a directory name < MAX_PATH-14 characters in length.
-  // https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettempfilenamea
-  char tempDirName[MAX_PATH - 14];
-  char tempFileName[MAX_PATH];
-  unsigned long nBufferLength{sizeof(tempDirName)};
-  nBufferLength = ::GetTempPathA(nBufferLength, tempDirName);
-  if (nBufferLength > sizeof(tempDirName) || nBufferLength == 0) {
+  // GetTempFileNameW needs a directory name < MAX_PATH-14 characters in length.
+  // https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettempfilenamew
+  wchar_t tempDirName[MAX_PATH - 14];
+  wchar_t tempFileName[MAX_PATH];
+  unsigned long nBufferLength{sizeof tempDirName / sizeof tempDirName[0]};
+  nBufferLength = ::GetTempPathW(nBufferLength, tempDirName);
+  if (nBufferLength > sizeof tempDirName / sizeof tempDirName[0] ||
+      nBufferLength == 0)
     return -1;
-  }
-  if (::GetTempFileNameA(tempDirName, "Fortran", uUnique, tempFileName) == 0) {
+  if (::GetTempFileNameW(tempDirName, L"Fortran", uUnique, tempFileName) == 0)
     return -1;
-  }
-  int fd{::_open(tempFileName, _O_CREAT | _O_BINARY | _O_TEMPORARY | _O_RDWR,
+  int fd{::_wopen(tempFileName, _O_CREAT | _O_BINARY | _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();
   }