-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[libc++] Guard fileno() and isatty() usage correctly. #166668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
See [1] and [2]. isatty() is part of unistd.h, but fileno() is in stdio.h, and is guarded by `_POSIX_C_SOURCE >= 1` (on my machine, `man 3 fileno` only requires `defined(_POSIX_C_SOURCE)`, though). I ran into this issue while trying to compile libc++ for a bare metal environment with Newlib as the libc. Newlib defines fileno() in stdio.h, and its unistd.h does not include stdio.h. [1]: https://linux.die.net/man/3/fileno [2]: https://linux.die.net/man/3/isatty
|
@llvm/pr-subscribers-libcxx Author: Chenguang Wang (wecing) ChangesSee 1 and 2. isatty() is part of unistd.h, but fileno() is in stdio.h, and is guarded by I ran into this issue while trying to compile libc++ for a bare metal environment with Newlib as the libc. Newlib defines fileno() in stdio.h, and its unistd.h does not include stdio.h. Full diff: https://github.com/llvm/llvm-project/pull/166668.diff 1 Files Affected:
diff --git a/libcxx/src/print.cpp b/libcxx/src/print.cpp
index 3f2baa6dcc60b..e7ff60ba58dac 100644
--- a/libcxx/src/print.cpp
+++ b/libcxx/src/print.cpp
@@ -20,8 +20,11 @@
# define NOMINMAX
# include <io.h>
# include <windows.h>
-#elif __has_include(<unistd.h>)
+#elif __has_include(<unistd.h>) && __has_include(<stdio.h>) && \
+ defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE >= 1
+# include <stdio.h>
# include <unistd.h>
+# define HAS_FILENO_AND_ISATTY
#endif
_LIBCPP_BEGIN_NAMESPACE_STD
@@ -56,7 +59,7 @@ __write_to_windows_console([[maybe_unused]] FILE* __stream, [[maybe_unused]] wst
}
# endif // _LIBCPP_HAS_WIDE_CHARACTERS
-#elif __has_include(<unistd.h>) // !_LIBCPP_WIN32API
+#elif defined(HAS_FILENO_AND_ISATTY) // !_LIBCPP_WIN32API
_LIBCPP_EXPORTED_FROM_ABI bool __is_posix_terminal(FILE* __stream) { return isatty(fileno(__stream)); }
#endif
|
|
Looks like this commit broke macos, freebsd, and android. Need to investigate how to guard properly on these platforms. |
|
Updated the implementation, and tests seem to be passing now. I added a special handling logic for Newlib only; other platforms should not be affected. The The |
__POSIX_VISIBLE is internal to Newlib [1]. We should check if _POSIX_C_SOURCE is defined instead [2]. [1]: https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/sys/features.h#l183 [2]: https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/sys/features.h#l325
…m#166668) Including unistd.h does not expose fileno() on Newlib.
|
@wecing Reviews in libc++ aren't optional. Please wait next time. If you don't get any comments within a week feel free to ping. |
I see. My apologies! Thanks for letting me know. |
See 1 and 2. isatty() is part of unistd.h, but fileno() is in stdio.h, and is guarded by
_POSIX_C_SOURCE >= 1(on my machine,man 3 filenoonly requiresdefined(_POSIX_C_SOURCE), though).I ran into this issue while trying to compile libc++ for a bare metal environment with Newlib as the libc. Newlib defines fileno() in stdio.h, and its unistd.h does not include stdio.h.