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