Page MenuHomePhabricator

[clang-tidy] Use ANSI escape codes for --use-color on Windows
ClosedPublic

Authored by dsanders11 on Oct 24 2020, 5:18 PM.

Details

Summary

On Windows the --use-color option cannot be used for its originally intended purpose of forcing color when piping stdout, since Windows does not use ANSI escape codes by default. This change turns on ANSI escape codes on Windows when forcing color to a non-displayed stdout (e.g. piped).

Diff Detail

Unit TestsFailed

TimeTest
400 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

dsanders11 created this revision.Oct 24 2020, 5:18 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 24 2020, 5:18 PM
dsanders11 requested review of this revision.Oct 24 2020, 5:18 PM

Added a few inline comments for clarification.

Note: I was not able to get a debug build and unit tests working on my Windows set up, so this has only been tested manually. I'm not sure if the unit test added in D79477 runs on Windows, but in theory it should be broken on Windows since it expects ANSI escape codes in the output.

clang-tools-extra/clang-tidy/ClangTidy.cpp
113

To prevent any unintended changes this has been confined to only Windows, but the code should be safe to run on any platform. I believe it would be a no-op on Unix, which always uses ANSI escape codes and UseANSIEscapeCodes is a no-op.

114

Limited to only non-displayed standard out so that using '--use-color' from a console will be a no-op, rather than turning on ANSI escape codes and making it act differently than a default invocation which has color on by default.

llvm/lib/Support/Windows/Process.inc
330–333

Fixes a crash when calling UseANSIEscapeCodes if standard out isn't a console.

Added a few inline comments for clarification.

Note: I was not able to get a debug build and unit tests working on my Windows set up, so this has only been tested manually. I'm not sure if the unit test added in D79477 runs on Windows, but in theory it should be broken on Windows since it expects ANSI escape codes in the output.

The functionality test specifies REQUIRES: ansi-escape-sequences so I'm guessing that test is not run on Windows. The unit test is parsing configuration options, not exercising the options themselves.

What issues did you run into regarding testing, because I feel like the patch should have test coverage (esp given that color vs ANSI escape codes are a bit of an oddity to reason about)?

clang-tools-extra/clang-tidy/ClangTidy.cpp
113

I agree that the code should work on all supported platforms, I'd say you can remove the #if.

llvm/lib/Support/Windows/Process.inc
330–333

If this fails, do we still want to set UseANSI to enable below? Similar if setting the mode fails?

Added a few inline comments for clarification.

Note: I was not able to get a debug build and unit tests working on my Windows set up, so this has only been tested manually. I'm not sure if the unit test added in D79477 runs on Windows, but in theory it should be broken on Windows since it expects ANSI escape codes in the output.

The functionality test specifies REQUIRES: ansi-escape-sequences so I'm guessing that test is not run on Windows. The unit test is parsing configuration options, not exercising the options themselves.

What issues did you run into regarding testing, because I feel like the patch should have test coverage (esp given that color vs ANSI escape codes are a bit of an oddity to reason about)?

+1, I'd expect such a test could unconditionally FileCheck the output to test for the escape sequences being present there.

What issues did you run into regarding testing, because I feel like the patch should have test coverage (esp given that color vs ANSI escape codes are a bit of an oddity to reason about)?

I'm unable to create a debug build, due to issues with LLVM and Visual Studio 16.7. There's Google results on the issue, but I don't see any solution and I'm using master and the latest version of Visual Studio. It seems you can (and perhaps should?) run the tests with a Release build, so I've gone ahead and done that.

I also wasn't seeing any of the test targets being made by CMake until I included -DLLVM_INCLUDE_TESTS=On, I think maybe I set that to off when trying to determine why the debug build was failing and didn't realize that would stay in CMakeCache.txt until I set it back on.

Removing REQUIRES: ansi-escape-sequences from the clang-tidy/infrastructure/use-color.cpp test does run it on Windows and does pass as expected with this change. Is that all you're looking for with test coverage?

llvm/lib/Support/Windows/Process.inc
330–333

If we don't set UseANSI below then this patch won't do what it's intended to do, since piped stdoutwouldn't have ANSI escape codes. This code block is assuming that standard output is a console, so GetConsoleMode won't work on redirected stdout.

You could use llvm::sys::Process::StandardOutIsDisplayed() to check, but for Windows that's actually just doing the same thing, doing a GetConsoleMode call and seeing if it succeeds or not.

For this patch to do what it's intended to do, UseANSIEscapeCodes needs to turn on ANSI escape codes even if there isn't a console.

What issues did you run into regarding testing, because I feel like the patch should have test coverage (esp given that color vs ANSI escape codes are a bit of an oddity to reason about)?

I'm unable to create a debug build, due to issues with LLVM and Visual Studio 16.7. There's Google results on the issue, but I don't see any solution and I'm using master and the latest version of Visual Studio. It seems you can (and perhaps should?) run the tests with a Release build, so I've gone ahead and done that.

Oh, how neat. The bug you linked to was closed as a dup of another bug. The other bug was then closed as something that's not in scope with the general product direction, whatever that means.

I also wasn't seeing any of the test targets being made by CMake until I included -DLLVM_INCLUDE_TESTS=On, I think maybe I set that to off when trying to determine why the debug build was failing and didn't realize that would stay in CMakeCache.txt until I set it back on.

Removing REQUIRES: ansi-escape-sequences from the clang-tidy/infrastructure/use-color.cpp test does run it on Windows and does pass as expected with this change. Is that all you're looking for with test coverage?

Yeah, that would be reasonable test coverage.

llvm/lib/Support/Windows/Process.inc
330–333

Ah, thank you for the explanation -- my complaint is more that UseANSIEscapeCodes can fail but doesn't alert the caller to the failure. However, that's not a new issue, so I think it's fine as-is.

code review changes

Oh, how neat. The bug you linked to was closed as a dup of another bug. The other bug was then closed as something that's not in scope with the general product direction, whatever that means.

Yea, typical Microsoft/Windows fun there.

Yeah, that would be reasonable test coverage.

Sounds good. I've updated the revision to remove the #if guard and removed REQUIRES: ansi-escape-sequences from the test.

aaron.ballman accepted this revision.Sat, Oct 31, 5:56 AM

LGTM! Do you need someone to commit on your behalf?

This revision is now accepted and ready to land.Sat, Oct 31, 5:56 AM

LGTM! Do you need someone to commit on your behalf?

Yea, I do.

aaron.ballman closed this revision.Sat, Oct 31, 7:38 AM

LGTM! Do you need someone to commit on your behalf?

Yea, I do.

I've commit on your behalf in d915d403d7410b17e8ac52f1f435859713a7b354, thank you for the patch!