[clang] [clang] Apply internal buffering to clang diagnostics printing (PR #113440)
Tom Honermann via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 11 09:39:12 PST 2024
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
More information about the cfe-commits
mailing list