[LLVM] [Support] Query the terminal width using the termios API #143514
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This essentially reverts a3eb3d3. For more information, see llvm#139499. Fixes llvm#139499.
Sirraide
requested review from
MaskRay,
hubert-reinterpretcast,
Endilll and
dwblaikie
|
@llvm/pr-subscribers-llvm-support Author: None (Sirraide) ChangesOn unix systems, we were trying to determine the terminal width using the We were previously using the termios API for this, but we stopped doing that, apparently because it wasn’t fully cross-platform. As for what systems that are, I’m not sure. I’d expect it to be available on every system that provides both This essentially reverts a3eb3d3 and parts of https://reviews.llvm.org/D61326. For more information, see #139499. Fixes #139499. Full diff: https://github.com/llvm/llvm-project/pull/143514.diff 3 Files Affected:
diff --git a/llvm/cmake/config-ix.cmake b/llvm/cmake/config-ix.cmake
index 687f5077cbfd2..319b0c0267005 100644
--- a/llvm/cmake/config-ix.cmake
+++ b/llvm/cmake/config-ix.cmake
@@ -24,6 +24,8 @@ if (ANDROID OR CYGWIN OR CMAKE_SYSTEM_NAME MATCHES "AIX|DragonFly|FreeBSD|Haiku|
set(HAVE_SYS_MMAN_H 1)
set(HAVE_SYSEXITS_H 1)
set(HAVE_UNISTD_H 1)
+ set(HAVE_SYS_IOCTL_H 1)
+ set(HAVE_TERMIOS_H 1)
elseif (APPLE)
set(HAVE_MACH_MACH_H 1)
set(HAVE_MALLOC_MALLOC_H 1)
@@ -31,6 +33,8 @@ elseif (APPLE)
set(HAVE_SYS_MMAN_H 1)
set(HAVE_SYSEXITS_H 1)
set(HAVE_UNISTD_H 1)
+ set(HAVE_SYS_IOCTL_H 1)
+ set(HAVE_TERMIOS_H 1)
elseif (PURE_WINDOWS)
set(HAVE_MACH_MACH_H 0)
set(HAVE_MALLOC_MALLOC_H 0)
@@ -38,6 +42,8 @@ elseif (PURE_WINDOWS)
set(HAVE_SYS_MMAN_H 0)
set(HAVE_SYSEXITS_H 0)
set(HAVE_UNISTD_H 0)
+ set(HAVE_SYS_IOCTL_H 0)
+ set(HAVE_TERMIOS_H 0)
elseif (ZOS)
# Confirmed in
# https://github.com/llvm/llvm-project/pull/104706#issuecomment-2297109613
@@ -47,6 +53,8 @@ elseif (ZOS)
set(HAVE_SYS_MMAN_H 1)
set(HAVE_SYSEXITS_H 0)
set(HAVE_UNISTD_H 1)
+ set(HAVE_SYS_IOCTL_H 1)
+ set(HAVE_TERMIOS_H 1)
else()
# Other platforms that we don't promise support for.
check_include_file(mach/mach.h HAVE_MACH_MACH_H)
@@ -55,6 +63,8 @@ else()
check_include_file(sys/mman.h HAVE_SYS_MMAN_H)
check_include_file(sysexits.h HAVE_SYSEXITS_H)
check_include_file(unistd.h HAVE_UNISTD_H)
+ check_include_file(sys/ioctl.h HAVE_SYS_IOCTL_H)
+ check_include_file(termios.h HAVE_TERMIOS_H)
endif()
if( UNIX AND NOT (APPLE OR BEOS OR HAIKU) )
diff --git a/llvm/include/llvm/Config/config.h.cmake b/llvm/include/llvm/Config/config.h.cmake
index 06d4756397911..3f0fa843974d2 100644
--- a/llvm/include/llvm/Config/config.h.cmake
+++ b/llvm/include/llvm/Config/config.h.cmake
@@ -164,6 +164,12 @@
/* Define to 1 if you have the <sys/mman.h> header file. */
#cmakedefine HAVE_SYS_MMAN_H ${HAVE_SYS_MMAN_H}
+/* Define to 1 if you have the <sys/ioctl.h> header file. */
+#cmakedefine HAVE_SYS_IOCTL_H ${HAVE_SYS_IOCTL_H}
+
+/* Define to 1 if you have the <termios.h> header file. */
+#cmakedefine HAVE_TERMIOS_H ${HAVE_TERMIOS_H}
+
/* Define to 1 if stat struct has st_mtimespec member .*/
#cmakedefine HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC ${HAVE_STRUCT_STAT_ST_MTIMESPEC_TV_NSEC}
diff --git a/llvm/lib/Support/Unix/Process.inc b/llvm/lib/Support/Unix/Process.inc
index b5c3719f57963..c4ffa5b2749bd 100644
--- a/llvm/lib/Support/Unix/Process.inc
+++ b/llvm/lib/Support/Unix/Process.inc
@@ -34,6 +34,12 @@
#ifdef HAVE_GETAUXVAL
#include <sys/auxv.h>
#endif
+#ifdef HAVE_SYS_IOCTL_H
+#include <sys/ioctl.h>
+#endif
+#ifdef HAVE_TERMIOS_H
+#include <termios.h>
+#endif
//===----------------------------------------------------------------------===//
//=== WARNING: Implementation here must contain only generic UNIX code that
@@ -304,7 +310,7 @@ bool Process::FileDescriptorIsDisplayed(int fd) {
#endif
}
-static unsigned getColumns() {
+static unsigned getColumns([[maybe_unused]] int FileID) {
// If COLUMNS is defined in the environment, wrap to that many columns.
if (const char *ColumnsStr = std::getenv("COLUMNS")) {
int Columns = std::atoi(ColumnsStr);
@@ -312,23 +318,33 @@ static unsigned getColumns() {
return Columns;
}
- // We used to call ioctl TIOCGWINSZ to determine the width. It is considered
- // unuseful.
- return 0;
+ // Unfortunately, COLUMNS is a shell variable, not a proper environment
+ // variable, so it is often not available; query the column count via
+ // the termios API instead if it isn't.
+ unsigned Columns = 0;
+
+#if defined(HAVE_SYS_IOCTL_H) && defined(HAVE_TERMIOS_H)
+ // Try to determine the width of the terminal.
+ struct winsize ws;
+ if (ioctl(FileID, TIOCGWINSZ, &ws) == 0)
+ Columns = ws.ws_col;
+#endif
+
+ return Columns;
}
unsigned Process::StandardOutColumns() {
if (!StandardOutIsDisplayed())
return 0;
- return getColumns();
+ return getColumns(0);
}
unsigned Process::StandardErrColumns() {
if (!StandardErrIsDisplayed())
return 0;
- return getColumns();
+ return getColumns(1);
}
static bool terminalHasColors() {
|
MaskRay
reviewed
Jun 13, 2025
MaskRay
reviewed
Jun 15, 2025
MaskRay
approved these changes
Jun 15, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/12933 Here is the relevant piece of the build log for the reference |
Looks like one of the tests is crashing w/ a weird exit code; not sure if that’s related to this but if it is we should probably just disable the |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/13/builds/7825 Here is the relevant piece of the build log for the reference |
ajaden-codes
pushed a commit
to Jaddyen/llvm-project
that referenced
this pull request
Jun 17, 2025
On unix systems, we were trying to determine the terminal width using the `COULMNS` environment variable. Unfortunately, `COLUMNS` is not exported by all shells and thus not available on some systems. We were previously using `ioctl()` for this; fall back to doing so if `COLUMNS` does not exist or does not store a positive integer. This essentially reverts a3eb3d3 and parts of https://reviews.llvm.org/D61326. For more information, see llvm#139499. Fixes llvm#139499.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/38/builds/3964 Here is the relevant piece of the build log for the reference |
This one looks like an actual failure: |
|
Is the macro for this just |
Sirraide
added a commit
that referenced
this pull request
Jun 17, 2025
…44600) #143514 broke the `clang-solaris11-sparcv9` bot; from what I can tell that’s Solaris and according to `SolarisTargetInfo::getOSDefines`, the macro `__sun__` should be defined on Solaris, so check for that and don’t try to query the terminal size if it is defined. Not sure this is the best solution but hopefully it fixes the bot.
This builder seems to be back to normal now, so that was unrelated. |
And so is this one. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On unix systems, we were trying to determine the terminal width using the
COULMNSenvironment variable. Unfortunately,COLUMNSis not a proper environment variable, but rather a shell variable and a such is not exported to actual processes (even though it is available in e.g. shell scripts) on some systems, including mine.We were previously using the termios API for this, but we stopped doing that, apparently because it wasn’t fully cross-platform. As for what systems that are, I’m not sure. I’d expect it to be available on every system that provides both
<sys/ioctl.h>and<termios.h>, but if that isn’t the case, then we can also just add e.g.&& defined(__linux__)because that’d fix this at least for some systems.This essentially reverts a3eb3d3 and parts of https://reviews.llvm.org/D61326.
For more information, see #139499.
Fixes #139499.