[llvm] [llvm] Do not use Console API if the output isn't a console device (PR #90230)

[via llvm-commits]

alvinhochun wrote:

> > 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`](https://learn.microsoft.com/en-us/windows/console/setconsolemode) 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.:

```c++
#ifndef ENABLE_VIRTUAL_TERMINAL_PROCESSING
#define ENABLE_VIRTUAL_TERMINAL_PROCESSING 0x4
#endif
```


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