tahonermann wrote:

Is it known which platforms are affected by this issue? I ask because I see 
some issues with how consoles/terminals are handled by `raw_fd_ostream`.

On Windows, `raw_fd_ostream` has a `bool` data member named `IsWindowsConsole` 
that is intended to indicate whether `FD` is a file descriptor for a Windows 
console. However, the code to detect a Windows console is incorrect.
https://github.com/llvm/llvm-project/blob/1946d32f1fdfb2c4d5e866a5c1c5c32b8cdad5b8/llvm/lib/Support/raw_ostream.cpp#L639-L641
The above code only checks that `FD` is directed to a character device, not a 
console specifically. The correct way to check for a console is already 
implemented elsewhere.
https://github.com/llvm/llvm-project/blob/1946d32f1fdfb2c4d5e866a5c1c5c32b8cdad5b8/llvm/lib/Support/Windows/Process.inc#L289-L292
Interestingly, the `llvm::Process` class is already used to detect a terminal 
for non-Windows systems via indirection through `is_displayed()`.
https://github.com/llvm/llvm-project/blob/1946d32f1fdfb2c4d5e866a5c1c5c32b8cdad5b8/llvm/lib/Support/raw_ostream.cpp#L866-L868
`is_displayed()` is also used on Windows, so there is some potential for 
inconsistent and surprising behavior across character devices.
https://github.com/llvm/llvm-project/blob/1946d32f1fdfb2c4d5e866a5c1c5c32b8cdad5b8/llvm/lib/Support/raw_ostream.cpp#L506-L520

The code for detecting a Windows console is specific to an actual Windows 
console device. It works for detecting a Windows console for processes running 
in a `cmd.exe` or Windows Terminal environment, but it won't detect the display 
used for a, e.g., Cygwin MinTTY terminal. That *might* be motivation for 
checking for a character device instead of a Windows console specifically, but 
comments should reflect that choice (and they currently suggest otherwise). 
Regardless, we should have one place where we determine whether output is 
directed to a console/terminal and the right place for that is 
`Process::FileDescriptorIsDisplayed()`.

For UNIX platforms, `isatty()` is used to detect output directed to a terminal.
https://github.com/llvm/llvm-project/blob/1946d32f1fdfb2c4d5e866a5c1c5c32b8cdad5b8/llvm/lib/Support/Unix/Process.inc#L309-L316
However, for reasons I don't understand, 
`raw_fd_ostream::preferred_buffer_size()` checks for both a character device 
and (indirectly) `isatty()` (this code used to call `isatty()` directly).
https://github.com/llvm/llvm-project/blob/1946d32f1fdfb2c4d5e866a5c1c5c32b8cdad5b8/llvm/lib/Support/raw_ostream.cpp#L851-L862

I think it is worth cleaning this up to see if that 1) fixes the reported 
problem, and 2) causes any regressions (which would then prompt improving the 
comments to better explain the intent).

https://github.com/llvm/llvm-project/pull/113440
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to