PR25717: fatal IO error writing large outputs to console on Windows

[ygao]

Summary

The fix to this bug is similar to the Python issue#11395. We need to cap the output size to
32767 on Windows to work around the size limit of WriteConsole().
Reference: https://bugs.python.org/issue11395

At least on Visual Studio 2013, the _isatty() check should be fast because it is implemented
by reading a field from the preloaded ioinfo struct.

[probinson]

lib/Support/raw_ostream.cpp

573 ↗

(On Diff #42953)

I think we don't normally cite PR numbers in source. Instead you can cite it in the commit message, and then a 'blame' will lead someone to the PR if they are that interested.

[aaron.ballman]

lib/Support/raw_ostream.cpp

570 ↗

(On Diff #42953)

Is there a noticeable regression in performance due to this chunking? If so, I would prefer if this were dynamically set based on the Windows version. Microsoft is pretty aggressively upgrading people to Windows 10, so it would be good not to punish all Windows users if the performance hit is measurable.

[• rafael]

There are a few places that have duplicated code for calling write
multiple times. I was going to suggest that we refactor this into a
helper function, but realized it is probably better to change the
other locations to use a raw_fd_ostrem.

So this is fine with me if the other concerns are addressed.

Cheers,
Rafael

[ygao]

Addressed review comments and rebased to trunk.

[aaron.ballman]

lib/Support/raw_ostream.cpp

62 ↗

(On Diff #43164)

Please include WindowsSupport.h instead of doing this.

572 ↗

(On Diff #43164)

GetVersion() is deprecated, you should be using IsWindows8OrGreater() instead (https://msdn.microsoft.com/en-us/library/windows/desktop/dn424961(v=vs.85).aspx).

[ygao]

Ok, this does make my codes simpler. I have a question: the IsWindows8OrGreater() API comes from VersionHelpers.h, which is part of Visual Studio 2013. Will this change cause problems for, say, mingw or cygwin builds (I am not sure whether we support them)?

[aaron.ballman]

In D15553#313429, @ygao wrote:

Ok, this does make my codes simpler. I have a question: the IsWindows8OrGreater() API comes from VersionHelpers.h, which is part of Visual Studio 2013. Will this change cause problems for, say, mingw or cygwin builds (I am not sure whether we support them)?

That's a good question that I don't know the answer to. I don't know if we dropped mingw support (and only support mingw-w64).

VersionHelpers.h is part of the Win32 SDK, but I have no idea how current those projects are at keeping up. If they don't keep up, then I think it would be reasonable to add a declaration to WindowsSupport.h and write our own definition based on GetVersionEx().

[yaron.keren]

We don't support building with mingw.org.
mingw-w64 has its own versionhelpers.h with IsWindows8OrGreater(), at least from gcc 4.93, probably earlier.

[ygao]

Unfortunately cygwin does not have the VersionHelpers.h header or the IsWindows8OrGreater() API, so I am implementing one using GetVersionEx().

[aaron.ballman]

lib/Support/raw_ostream.cpp

61 ↗

(On Diff #43266)

I think this should be moved into WindowsSupport.h, truth be told. I don't think *anyone* wants min or max to be macros in this project.

67 ↗

(On Diff #43266)

Would it make sense to add the VersionHelpers.h include and IsWindows8OrGreater() to WindowsSupport.h as these seem like much more generally useful than just for raw_ostream?

I don't feel strongly about this, but am curious what others think.

[yaron.keren]

+1 all general-purpose Windows stuff should move to WindowsSupport.h. I'm surprised we got away without so far.
We have NOMINMAX at lib/Driver/MSVCToolChain.cpp before Windows.h which should probably switched to use WindowsSupport.h.

[yaron.keren]

#define NOGDI
is also useful.

[ygao]

lib/Support/raw_ostream.cpp

61 ↗

(On Diff #43266)

Will do.
lib/Driver/MSVCToolChain.cpp lives in cfe so I'll look into cleaning that up in a separate patch.

67 ↗

(On Diff #43266)

It sounds like a good idea.
It may be a bit odd that for cygwin I only add a checker for Windows 8 but not for other versions, but we probably do not need checker function for every Windows version either. I suppose I can add only Windows 8 for now, and then other versions can be added later as needed?

[aaron.ballman]

lib/Support/raw_ostream.cpp

61 ↗

(On Diff #43266)

Sounds great, thank you!

67 ↗

(On Diff #43266)

I think that makes the most sense -- as we need them, we can add them in. Eventually cygwin will catch up with the Win32 APIs and we won't need the functionality anyway.

[ygao]

Addressed review comments and rebased to trunk.

[aaron.ballman]

I think a testcase for this might be challenging (especially given that it is OS dependent), but it would be good if we could devise one. If we can't, then make sure the commit message explains why. Otherwise, LGTM!

[ygao]

Ok, I had to modify the fix somewhat to make a mingw bot happy. The bot is
i686-mingw32-RA-on-linux.

Issue#1: this bot does not ship with VersionHelpers.h. I am not sure whether it is a mingw32
bot or it happens to be using a very old mingw-w64. Looking at the buildslave config page,
http://bb.pgr.jp/buildslaves/centos6, it says gcc 4.4.6.

In r256896, I tried to fix the problem by testing for mingw32; and in r256901, I tried testing
"__MINGW64_VERSION_MAJOR < 3". Neither approach seems to fix the buildbot. So for now
I am not including VersionHelpers.h on mingw (same as cygwin).

Issue#2: I had to take out the NOGDI macro from WindowsSupport.h in r256904 because
mingw defines LOGFONTW type in wingdi.h. The mingw version of shlobj.h (needed by
lib/Support/Windows/Path.inc) includes shobjidl.h and the latter uses the LOGFONTW type.

llvm/trunk/lib/Support/Windows/WindowsSupport.h

 // mingw-w64 tends to define it as 0x0502 in its headers.
 #undef _WIN32_WINNT
 #undef _WIN32_IE
 
 // Require at least Windows 7 API.
 #define _WIN32_WINNT 0x0601
 #define _WIN32_IE    0x0800 // MinGW at it again. FIXME: verify if still needed.
 #define WIN32_LEAN_AND_MEAN
+#define NOGDI
+#define NOMINMAX
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Config/config.h" // Get build system configuration settings
 #include "llvm/Support/Compiler.h"
 #include <system_error>
 #include <windows.h>
 #include <wincrypt.h>
 #include <cassert>
 #include <string>
 #include <vector>
 
+#ifndef __CYGWIN__
+#include <VersionHelpers.h>
+#else
+// Cygwin does not have the IsWindows8OrGreater() API.
+inline bool IsWindows8OrGreater() {
+  OSVERSIONINFO osvi = {};
+  osvi.dwOSVersionInfoSize = sizeof(OSVERSIONINFO);
+  if (!::GetVersionEx(&osvi))
+    return false;
+  return (osvi.dwMajorVersion > 6 ||
+          (osvi.dwMajorVersion == 6 && osvi.dwMinorVersion >= 2));
+}
+#endif // __CYGWIN__
+
 inline bool MakeErrMsg(std::string* ErrMsg, const std::string& prefix) {
   if (!ErrMsg)
     return true;
   char *buffer = NULL;
   DWORD LastError = GetLastError();
   DWORD R = FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER |
                           FORMAT_MESSAGE_FROM_SYSTEM |
                           FORMAT_MESSAGE_MAX_WIDTH_MASK,

llvm/trunk/lib/Support/raw_ostream.cpp

 #ifndef STDOUT_FILENO
 # define STDOUT_FILENO 1
 #endif
 #ifndef STDERR_FILENO
 # define STDERR_FILENO 2
 #endif
 #endif
 
+#ifdef LLVM_ON_WIN32
+#include "Windows/WindowsSupport.h"
+#endif
+
 using namespace llvm;
 
 raw_ostream::~raw_ostream() {
   // raw_ostream's subclasses should take care to flush the buffer
   // in their destructors.
   assert(OutBufCur == OutBufStart &&
          "raw_ostream destructor called with non-empty buffer!");
 
if (has_error())
     report_fatal_error("IO failure on output stream.", /*GenCrashDiag=*/false);
 }
 
 
 void raw_fd_ostream::write_impl(const char *Ptr, size_t Size) {
   assert(FD >= 0 && "File already closed.");
   pos += Size;
 
+#ifndef LLVM_ON_WIN32
+  bool ShouldWriteInChunks = false;
+#else
+  // Writing a large size of output to Windows console returns ENOMEM. It seems
+  // that, prior to Windows 8, WriteFile() is redirecting to WriteConsole(), and
+  // the latter has a size limit (66000 bytes or less, depending on heap usage).
+  bool ShouldWriteInChunks = !!::_isatty(FD) && !IsWindows8OrGreater();
+#endif
+
   do {
-    ssize_t ret = ::write(FD, Ptr, Size);
+    size_t ChunkSize = Size;
+    if (ChunkSize > 32767 && ShouldWriteInChunks)
+        ChunkSize = 32767;
+
+    ssize_t ret = ::write(FD, Ptr, ChunkSize);
 
     if (ret < 0) {
       // If it's a recoverable error, swallow it and retry the write.
       //
       // Ideally we wouldn't ever see EAGAIN or EWOULDBLOCK here, since
       // raw_ostream isn't designed to do non-blocking I/O. However, some
       // programs, such as old versions of bjam, have mistakenly used
       // O_NONBLOCK. For compatibility, emulate blocking semantics by