[llvm] Do not use Console API if the output isn't a console device

[kasper93]

This fixes the -fcolor-diagnostics to properly enable color output when not outputting to console devices, such as when using Ninja.

This affects only Windows.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-llvm-support

Author: Kacper Michajłow (kasper93)

Changes

This fixes the -fcolor-diagnostics to properly enable color output when not outputting to console devices, such as when using Ninja.

This affects only Windows.

---

Full diff: https://github.com/llvm/llvm-project/pull/90230.diff

1 Files Affected:

• (modified) llvm/lib/Support/raw_ostream.cpp (+5)

diff --git a/llvm/lib/Support/raw_ostream.cpp b/llvm/lib/Support/raw_ostream.cpp index 8cb7b5ac68ea7e..993d75536112a2 100644 --- a/llvm/lib/Support/raw_ostream.cpp +++ b/llvm/lib/Support/raw_ostream.cpp @@ -645,6 +645,11 @@ raw_fd_ostream::raw_fd_ostream(int fd, bool shouldClose, bool unbuffered, // Check if this is a console device. This is not equivalent to isatty. IsWindowsConsole = ::GetFileType((HANDLE)::_get_osfhandle(fd)) == FILE_TYPE_CHAR; + + // If this isn't a console device, don't try to use the API to set the color. + // Switch to ANSI escape codes instead. + if (!IsWindowsConsole) + llvm::sys::Process::UseANSIEscapeCodes(true); #endif // Get the starting position. 

[kasper93]

Any news about this one?

[alvinhochun]

I am not opposed to the idea of making ANSI escape codes the default on Windows when the output is redirected, and also for console output if supported by the system. But currently this patch only does the former.

Besides, in the case of Clang, it has the option -f[no-]ansi-escape-codes which currently defaults to 0. I'm not sure if this option is useful any more, but I don't think we should remove it because CMake now sets this flag. We also can't just change its default either because that would break colour output when running on older versions of Windows. We should make it such that if neither options are specified, ANSI escape code is enabled depending on whether ENABLE_VIRTUAL_TERMINAL_PROCESSING is supported. Or maybe we can just make the option no-op.

[mstorsjo]

So, I'm not very well acquainted with these APIs, so I'll try to summarize my understanding of the situation:

• If we're outputting directly to a terminal, we set colors using the console APIs - this works since a long time
• If we're invoked via ninja, the output is buffered, the output isn't a terminal, and we currently don't output any colors. But this patch changes that.
• This patch makes us output ANSI codes instead of using console APIs for color. This has traditionally not worked with older versions of windows terminal, but is supported by recent versions. This is also supported by virtually all unix-like terminals.

I'm not entirely following @alvinhochun's comment here - I presume this is about when we're outputting directly to a terminal, whether we should prefer ANSI codes or console APIs for setting color? And that it's possible to query this with ENABLE_VIRTUAL_TERMINAL_PROCESSING?

All in all, this sounds mostly reasonable to me, but I'll see if @aganea or @tru or @zmodem have anything to say.

[kasper93]

If we're outputting directly to a terminal, we set colors using the console APIs - this works since a long time

Correct.

If we're invoked via ninja, the output is buffered, the output isn't a terminal, and we currently don't output any colors. But this patch changes that.

Correct.

This patch makes us output ANSI codes instead of using console APIs for color. This has traditionally not worked with older versions of windows terminal, but is supported by recent versions. This is also supported by virtually all unix-like terminals.

To clarify. This patch makes us output ANSI codes instead of using console APIs for color, only if we are not outputting to Windows Console. So the current default behavior of using Console API is preserved. This patch changes the default behavior to output ANSI codes, if forced by -fcolor-diagnostics, just like on any other platform, when not outputting to Console.

Note that this overwrite -fno-ansi-escape-codes when not outputting to Console, but IMHO this is expected behavior. Else -fcolor-diagnostics just doesn't work.

I'm not entirely following @alvinhochun's comment here - I presume this is about when we're outputting directly to a terminal, whether we should prefer ANSI codes or console APIs for setting color? And that it's possible to query this with ENABLE_VIRTUAL_TERMINAL_PROCESSING?

Yes, this is another topic, that we briefly discussed on IRC. Generally speaking -f[no-]ansi-escape-codes is dummy option. In practice there is no need to manually control this.

I've made this patch the most basic to make it convenient from the user point of view.

Possible extension of this changes would be:

• Make ANSI codes default as recommended by Microsoft nowadays
• Automatically fallback to Console API if outputting to Console and ENABLE_VIRTUAL_TERMINAL_PROCESSING is disabled.
• Deprecate -f[no-]ansi-escape-codes option, as it is not that useful.

I realize the proposed patch is only small change and we may want to more thorough changes, but I wanted to start off discussion about it. I think this patch alone is fine and even further changes can be made in another PR.

[tru]

I think this is a good idea. But it would need a release note. Does it need a test of some kind? To make sure the fallback to console api works maybe?

[mstorsjo]

I think this is a good idea. But it would need a release note. Does it need a test of some kind? To make sure the fallback to console api works maybe?

Some sort of automated test would certainly be good, but I'm not really sure how doable that is in our test infrastructure. (I would expect that all test executions are towards something that isn't directly a console, etc.) So I think it's fine without one... But bonus points for a sequence of easy instructions for verifying that it works, that anyone can repeat both now and when looking into related things in the future.

[alvinhochun]

I'm not entirely following @alvinhochun's comment here - I presume this is about when we're outputting directly to a terminal, whether we should prefer ANSI codes or console APIs for setting color? And that it's possible to query this with ENABLE_VIRTUAL_TERMINAL_PROCESSING?

1. Yes
2. What I understand is that if ENABLE_VIRTUAL_TERMINAL_PROCESSING is set when calling SetConsoleMode on a system that does not support VT, it returns a non-zero value to indicate an error, which can be used to determine whether it is supported.

I think this is a good idea. But it would need a release note. Does it need a test of some kind? To make sure the fallback to console api works maybe?

Some sort of automated test would certainly be good, but I'm not really sure how doable that is in our test infrastructure. (I would expect that all test executions are towards something that isn't directly a console, etc.) So I think it's fine without one... But bonus points for a sequence of easy instructions for verifying that it works, that anyone can repeat both now and when looking into related things in the future.

Just thinking out loud, the conhost properties has an option to "use legacy console" (which should disable VT), and it is backed by the registry key HKEY_CURRENT_USER\Console\ForceV2. In theory we can use it to test the fallback, say by screen capturing the conhost window, or by intercepting SetConsoleTextAttribute to see whether it is called in the expected way. But these tests will need to be run in a separate conhost window, and should probably not run in parallel with other tests to avoid potentially affecting them. I doubt it is really worth the trouble. but I guess it may be a fun challenge for someone?

[kasper93]

What I understand is that if ENABLE_VIRTUAL_TERMINAL_PROCESSING is set when calling SetConsoleMode on a system that does not support VT, it returns a non-zero value to indicate an error, which can be used to determine whether it is supported.

That's correct, but for checking support GetConsoleMode can be used. In mpv we first try to set (this is not required for llvm) ENABLE_VIRTUAL_TERMINAL_PROCESSING if not already enabled and check it after with GetConsoleMode, if ENABLE_VIRTUAL_TERMINAL_PROCESSING bit is set, we use ANSI, else fallback to Console API, works well.

I would like to also note that GetConsoleMode is relatively slow, when printing lots of data (I've seen that on profile, when outputting images to terminal). Probably not critical for llvm, but we cache the mode, as we expect no runtime changes. LLVM currently already GetConsoleMode on each print to test if we are Console, so it is another topic of improvement. In the sense, adding VT check would not regress current status quo.

[alvinhochun]

So, I just don't like that this patch calls llvm::sys::Process::UseANSIEscapeCodes from the raw_fd_ostream ctor. I think making the change in llvm/lib/Support/Windows/Process.inc, specifically changing UseANSI to be initialized to true if not outputting to console would be cleaner.

And after that's done, from what I can find the test llvm/unittests/Support/WithColorTest.cpp needs to be enabled for Windows (remove #ifdef LLVM_ON_UNIX.

This won't change the behaviour of Clang however, because it calls UseANSIEscapeCodes with the option:

llvm-project/clang/lib/Frontend/CompilerInvocation.cpp

Line 2535 in 4202634

llvm::sys::Process::UseANSIEscapeCodes(Opts.UseANSIEscapeCodes);

I think the best move forward is to dummy out the option and mark it as deprecated, but that is probably best done in a separate patch.

[farzonl]

And after that's done, from what I can find the test llvm/unittests/Support/WithColorTest.cpp needs to be enabled for Windows (remove #ifdef LLVM_ON_UNIX.

@alvinhochun i'm not sure if removing LLVM_ON_UNIX is enough to make this test pass in all enviornments. ENABLE_VIRTUAL_TERMINAL_PROCESSING is only valid after 10.0.22621.1992. If there are earlier windows build agents wont they break?

My theory is If ENABLE_VIRTUAL_TERMINAL_PROCESSING is not defined or does not equal 0x0004 then windows will behave as it always did and not support ascii color encodings. So LLVM_ON_UNIX || ENABLE_VIRTUAL_TERMINAL_PROCESSING?

Also shouldn't the ifdef #if defined(ENABLE_VIRTUAL_TERMINAL_PROCESSING) check that ENABLE_VIRTUAL_TERMINAL_PROCESSING equals 0x4?
https://github.com/llvm/llvm-project/blob/efc6b50d2d93fa571572ee3ef1d4565c09ad1610/llvm/lib/Support/Windows/Process.inc#L319C2-L331C7

[alvinhochun]

And after that's done, from what I can find the test llvm/unittests/Support/WithColorTest.cpp needs to be enabled for Windows (remove #ifdef LLVM_ON_UNIX.

@alvinhochun i'm not sure if removing LLVM_ON_UNIX is enough to make this test pass in all enviornments. ENABLE_VIRTUAL_TERMINAL_PROCESSING is only valid after 10.0.22621.1992. If there are earlier windows build agents wont they break?

That's not correct, ENABLE_VIRTUAL_TERMINAL_PROCESSING has been supported since as early as Windows 10 version 1607 (10.0.14393, that's 8 years ago).

For earlier versions of Windows (or if VT support is force disabled), I guess that may depend on how the test is run. If it is being run by lit then the output should be redirected, but if not then it may output straight to console. Yes, that may be problematic if someone wants to run the unit test manually, but my hunch is that it won't cause issues when just running the full test suite normally.

In the end it should be tested with and without VT support enabled. There is a conhost/registry toggle for that.

My theory is If ENABLE_VIRTUAL_TERMINAL_PROCESSING is not defined or does not equal 0x0004 then windows will behave as it always did and not support ascii color encodings. So LLVM_ON_UNIX || ENABLE_VIRTUAL_TERMINAL_PROCESSING?

You are mixing up build-time and run-time environment. The define is really just for convenience. You can call the SetConsoleMode API with the bit flag 0x4 without the define, while if you do use the define you can still run the output binary on Windows 7 – whether it is supported can only be determined on run-time.

Also shouldn't the ifdef #if defined(ENABLE_VIRTUAL_TERMINAL_PROCESSING) check that ENABLE_VIRTUAL_TERMINAL_PROCESSING equals 0x4? https://github.com/llvm/llvm-project/blob/efc6b50d2d93fa571572ee3ef1d4565c09ad1610/llvm/lib/Support/Windows/Process.inc#L319C2-L331C7

It's one thing if it's undefined (the SDK may be too old), but if it is defined then it doesn't make much sense to doubt its value as defined by the Windows SDK (or mingw-w64 headers). (Not saying that mingw-w64 headers don't have errors, but one would only work around specific errors as they are found, not "I am skeptical of every defines and need to check all of them" – at that point you might as well not use mingw-w64.)

By the way, I do not agree with the existing code in llvm checking for the ENABLE_VIRTUAL_TERMINAL_PROCESSING define to conditionally enable the ANSI escape code support. The typical way would be to just define the value manually if it's not defined by the SDK, i.e.:

#ifndef ENABLE_VIRTUAL_TERMINAL_PROCESSING #define ENABLE_VIRTUAL_TERMINAL_PROCESSING 0x4 #endif