Page MenuHomePhabricator

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

Authored by ygao on Dec 15 2015, 6:09 PM.

Details

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.

Diff Detail

Event Timeline

ygao updated this revision to Diff 42953.Dec 15 2015, 6:09 PM
ygao retitled this revision from to PR25717: fatal IO error writing large outputs to console on Windows.
ygao updated this object.
ygao added reviewers: silvas, aaron.ballman, Bigcheese.
ygao added a subscriber: llvm-commits.
probinson added inline comments.
lib/Support/raw_ostream.cpp
591

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 added inline comments.Dec 16 2015, 6:53 AM
lib/Support/raw_ostream.cpp
588

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.

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 updated this revision to Diff 43164.Dec 17 2015, 11:41 AM

Addressed review comments and rebased to trunk.

aaron.ballman added inline comments.Dec 17 2015, 12:44 PM
lib/Support/raw_ostream.cpp
62

Please include WindowsSupport.h instead of doing this.

585

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

ygao updated this revision to Diff 43188.Dec 17 2015, 3:03 PM
ygao marked 2 inline comments as done.

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 edited edge metadata.Dec 17 2015, 3:32 PM
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().

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 updated this revision to Diff 43266.Dec 18 2015, 2:05 PM
ygao edited edge metadata.
ygao marked 2 inline comments as done.

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

aaron.ballman added inline comments.Dec 18 2015, 2:21 PM
lib/Support/raw_ostream.cpp
61

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

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.

+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.

#define NOGDI
is also useful.

ygao added inline comments.Dec 18 2015, 2:57 PM
lib/Support/raw_ostream.cpp
61

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

67

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 added inline comments.Dec 18 2015, 2:59 PM
lib/Support/raw_ostream.cpp
61

Sounds great, thank you!

67

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 updated this revision to Diff 43277.Dec 18 2015, 3:28 PM
ygao marked 6 inline comments as done.

Addressed review comments and rebased to trunk.

aaron.ballman accepted this revision.Dec 18 2015, 4:45 PM
aaron.ballman edited edge metadata.

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!

This revision is now accepted and ready to land.Dec 18 2015, 4:45 PM
This revision was automatically updated to reflect the committed changes.
ygao added a comment.Jan 5 2016, 7:21 PM

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.