[libcxx] Remove the second inclusion of the system header #120946
Conversation
This was introduced in llvm#119025 and not only seems unnecessary, it broke the build with older versions of glibc.
petrhosek
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Petr Hosek (petrhosek) ChangesThis was introduced in #119025 and not only seems unnecessary, it broke the build with older versions of glibc. Full diff: https://github.com/llvm/llvm-project/pull/120946.diff 3 Files Affected:
diff --git a/libcxx/include/stdio.h b/libcxx/include/stdio.h
index d1e57b08f5da47..32ab2af5b1865f 100644
--- a/libcxx/include/stdio.h
+++ b/libcxx/include/stdio.h
@@ -106,10 +106,6 @@ void perror(const char* s);
# ifndef _LIBCPP_STDIO_H
# define _LIBCPP_STDIO_H
-# if __has_include_next(<stdio.h>)
-# include_next <stdio.h>
-# endif
-
# ifdef __cplusplus
# undef getc
diff --git a/libcxx/include/stdlib.h b/libcxx/include/stdlib.h
index e32b7b7d751b3e..a37faa1c93aa56 100644
--- a/libcxx/include/stdlib.h
+++ b/libcxx/include/stdlib.h
@@ -92,10 +92,6 @@ void *aligned_alloc(size_t alignment, size_t size); // C11
# if !defined(_LIBCPP_STDLIB_H)
# define _LIBCPP_STDLIB_H
-# if __has_include_next(<stdlib.h>)
-# include_next <stdlib.h>
-# endif
-
# ifdef __cplusplus
extern "C++" {
// abs
diff --git a/libcxx/include/wchar.h b/libcxx/include/wchar.h
index 436027b7842aeb..cc72b44f4c6419 100644
--- a/libcxx/include/wchar.h
+++ b/libcxx/include/wchar.h
@@ -122,10 +122,6 @@ size_t wcsrtombs(char* restrict dst, const wchar_t** restrict src, size_t len,
# include <__mbstate_t.h> // provide mbstate_t
# include <stddef.h> // provide size_t
-# if __has_include_next(<wchar.h>)
-# include_next <wchar.h>
-# endif
-
// Determine whether we have const-correct overloads for wcschr and friends.
# if defined(_WCHAR_H_CPLUSPLUS_98_CONFORMANCE_)
# define _LIBCPP_WCHAR_H_HAS_CONST_OVERLOADS 1
|
| @@ -106,10 +106,6 @@ void perror(const char* s); | |||
| # ifndef _LIBCPP_STDIO_H | |||
| # define _LIBCPP_STDIO_H | |||
|
|
|||
| # if __has_include_next(<stdio.h>) | |||
| # include_next <stdio.h> | |||
There was a problem hiding this comment.
It had been done that way because the first include_next <stdio.h> could potentially be the target of a __need_size_t macro (for example), which would result in the first inclusion only declaring size_t, not everything that should be in stdio.h.
After your patch, I believe that the following might not work anymore:
#define __need_size_t
#include <stdio.h>
int main() {
puts("foo");
}That's because I'd assume #include <stdio.h> to only define size_t when it gets included after defining the macro. I would expect e.g. puts to only be defined the second time around stdio.h is included, which you removed.
But if I'm wrong (and I'm happy to be), then I don't mind this patch.
There was a problem hiding this comment.
Isn't what you describe exactly the user asked for though? If we wanted to not support this we could just #undef all the __need_ macros and be done with it.
There was a problem hiding this comment.
I think some system headers do rely on __need macros working though, that's why things broke when I attempted https://reviews.llvm.org/D131425.
Take for example wchar.h. If it gets included with a __need macro, our current implementation would fail because the C library's wchar.h would not define e.g. wcsstr(...), but we use it in our implementation later in libc++'s own wchar.h. That's why I was including the header once again after the first time, with the intent that all required declarations would surely be available after including the C library's wchar.h header for the second time.
There was a problem hiding this comment.
Hm, that's indeed a problem. It doesn't seem to affect the CI though, which makes me wonder whether there's actually a __need_ macro in wchar.h or stdlib.h (I couldn't find any in my glibc). Maybe this problem solved itself somehow? stddef.h shouldn't be affected, since we don't actually use any of the libc-provided symbols in stddef.h directly.
There was a problem hiding this comment.
@petrhosek What version of glibc were you folks using when you saw the issues in #119025?
There was a problem hiding this comment.
@petrhosek What version of glibc were you folks using when you saw the issues in #119025?
2.19
There was a problem hiding this comment.
We only support glibc 2.24 and above as documented in https://libcxx.llvm.org. Since this change is technically incorrect (as explained above), I'm not sure what to do with this. When are you moving away from glibc 2.19?
There was a problem hiding this comment.
We support glibc 2.19 because some of the Linux binaries we build run in Android CI which still uses Ubuntu 14.04, so the answer is "Whenever Android CI stops using Ubuntu 14.04." According to https://wiki.ubuntu.com/Releases, Ubuntu 14.04 EOL is April 2026, so I expect it'll happen sometime before that, but I don't know if there's any date set already. @pirama-arumuga-nainar or @kongy might know more.
There was a problem hiding this comment.
I can't reproduce the issue I mentioned above. Since we're also not seeing any issues in our CI, let's leave things as-is but pay attention in case a bug report gets filed during the LLVM 20 release.
This was introduced in #119025 and not only seems unnecessary, it broke the build with older versions of glibc.