[libc++] Simplify the implementation of <stddef.h> #86843
Conversation
Libc++'s own <stddef.h> is complicated by the need to handle various platform-specific macros and to support duplicate inclusion. In reality, we only need to add a declaration of nullptr_t to it, so we can simply include the underlying <stddef.h> outside of our guards to let it handle re-inclusion itself.
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesLibc++'s own <stddef.h> is complicated by the need to handle various platform-specific macros and to support duplicate inclusion. In reality, we only need to add a declaration of nullptr_t to it, so we can simply include the underlying <stddef.h> outside of our guards to let it handle re-inclusion itself. Full diff: https://github.com/llvm/llvm-project/pull/86843.diff 1 Files Affected:
diff --git a/libcxx/include/stddef.h b/libcxx/include/stddef.h
index 887776b150e49d..470b5408336c6d 100644
--- a/libcxx/include/stddef.h
+++ b/libcxx/include/stddef.h
@@ -7,18 +7,6 @@
//
//===----------------------------------------------------------------------===//
-#if defined(__need_ptrdiff_t) || defined(__need_size_t) || defined(__need_wchar_t) || defined(__need_NULL) || \
- defined(__need_wint_t)
-
-# if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-# pragma GCC system_header
-# endif
-
-# include_next <stddef.h>
-
-#elif !defined(_LIBCPP_STDDEF_H)
-# define _LIBCPP_STDDEF_H
-
/*
stddef.h synopsis
@@ -36,16 +24,19 @@
*/
-# include <__config>
+#include <__config>
+
+// Note: This include is outside of header guards because we sometimes get included multiple times
+// with different defines and the underlying <stddef.h> will know how to deal with that.
+#include_next <stddef.h>
+
+#ifndef _LIBCPP_STDDEF_H
+# define _LIBCPP_STDDEF_H
# if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
# pragma GCC system_header
# endif
-# if __has_include_next(<stddef.h>)
-# include_next <stddef.h>
-# endif
-
# ifdef __cplusplus
typedef decltype(nullptr) nullptr_t;
# endif
|
|
I am about to go OOO for a few days and I want to be around when I land this patch in case there is some unexpected fallout. So I'll merge this around Tuesday April 2nd. |
Libc++'s own <stddef.h> is complicated by the need to handle various platform-specific macros and to support duplicate inclusion. In reality, we only need to add a declaration of nullptr_t to it, so we can simply include the underlying <stddef.h> outside of our guards to let it handle re-inclusion itself. (cherry picked from commit 2950283)
|
|
||
| // Note: This include is outside of header guards because we sometimes get included multiple times | ||
| // with different defines and the underlying <stddef.h> will know how to deal with that. | ||
| #include_next <stddef.h> |
There was a problem hiding this comment.
Our builds now fail with
In file included from ../../base/allocator/partition_allocator/src\partition_alloc/partition_alloc_base/debug/alias.h:8:
In file included from ../../third_party/libc++/src/include\cstddef:42:
../../third_party/libc++/src/include\stddef.h(31,2): error: #include_next is a language extension [-Werror,-Wgnu-include-next]
31 | #include_next <stddef.h>
| ^
1 error generated.
…presumably because this is now before the #pragma GCC system_header below. Was that an intentional change?
(https://ci.chromium.org/ui/p/chromium/builders/try/win-rel/560031/overview)
There was a problem hiding this comment.
Hmm, no that's not intentional. Is -Wgnu-include-next enabled by default? Why did we not see this anywhere else?
I can fix that but it would be nice to have a test that fails if we break this again in the future.
There was a problem hiding this comment.
Thanks for the fix!
Is -Wgnu-include-next enabled by default?
No. Apparently someone over here thought it'd be good to compile some of our files with -pedantic, which enables -Wgnu. (Other users of libc++ probably do this too.)
Why did we not see this anywhere else?
I don't know how libc++ tests run. Maybe there isn't a test that includes all public headers and builds with -Wall -Wconversion -Wextra -pedantic -Werror? That would be a good test to have.
Does libc++ have any policy which warning levels it's clean at? Probably not really needed since all its headers are supposed to be system headers which disable warnings, so that test could (and probably should?) even use -Weverything with clang.
As reported in llvm#86843, we must have #pragma GCC system_header before we use #include_next, otherwise the compiler may not understand that we're in a system header and may issue a diagnostic for our usage of
Libc++'s own <stddef.h> is complicated by the need to handle various platform-specific macros and to support duplicate inclusion. In reality, we only need to add a declaration of nullptr_t to it, so we can simply include the underlying <stddef.h> outside of our guards to let it handle re-inclusion itself. (cherry picked from commit 2950283)
As reported in #86843, we must have #pragma GCC system_header before we use #include_next, otherwise the compiler may not understand that we're in a system header and may issue a diagnostic for our usage of
As reported in llvm#86843, we must have #pragma GCC system_header before we use #include_next, otherwise the compiler may not understand that we're in a system header and may issue a diagnostic for our usage of (cherry picked from commit 3c4b673)
As reported in llvm#86843, we must have #pragma GCC system_header before we use #include_next, otherwise the compiler may not understand that we're in a system header and may issue a diagnostic for our usage of (cherry picked from commit 3c4b673)
|
Here's another thing that happened to work before but breaks now (…but somehow only on Windows): (Details: https://crbug.com/333934598) That's of course something you shouldn't do, but since it used to work, there's code out there that does it. Not a ton as far as I can tell, but I thought I'd mention it. (No action needed.) |
|
Thanks for the heads up. We actually ran across exactly one instance of this issue downstream and finished root-causing it just this morning. Indeed, I don't think this is something we can or should do anything about, but it will break a very small number of projects indeed. The symptom we saw was a missing Pinging @llvm/libcxx-vendors in case someone comes across a weird issue, this may save you some time to diagnose it. |
As reported in llvm#86843, we must have #pragma GCC system_header before we use #include_next, otherwise the compiler may not understand that we're in a system header and may issue a diagnostic for our usage of (cherry picked from commit 3c4b673)
|
@ldionne Just a heads-up (already mentioned this on Discord): this caused GCC to fail to compile with Xcode 16 beta: ( Apparently what they do is redefine This was added in gcc-mirror/gcc@d59a576 to fix an issue similar to mine. This used to work because https://github.com/gcc-mirror/gcc/blob/1ae5fc24e86ecc9e7b60346d9ca2e56f83517bda/gcc/system.h#L42-L44 But I posted a patch for this to the GCC mailing list, but they're asking if this can be fixed on the libc++/macOS SDK side. (Louis, is macOS |
|
gcc's code base has more of these tricks. They also poison certain identifiers, causing problems with libc++ headers. See for example: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111632, which is a similar case, caused by C++ standard headers being included after standard identifiers are redefined. |
|
@BertalanD I haven't finished digging into this, but this looks like a Clang-builtins-header bug to me. I don't understand why the clang builtin headers would be overriding |
|
I think I have convinced myself that libc++ isn't to blame here. Before this PR, the way libc++ included Reproducer: cat <<EOF | clang++ -nostdinc++ -std=c++11 -xc++ - -v -fsyntax-only --trace-includes
#define NULL nullptr
#include <stddef.h> // NULL gets redefined inside that header
static_assert(__is_same(decltype(NULL), decltype(nullptr)), "");
EOFOutput: Furthermore, if I additionally add Hence, it looks like the Clang builtin headers today completely override any platform-provided However, I also think that users are not allowed to define |
|
CC @AaronBallman ^ |
The issue is modules (what problem can't modules solve?): Even when This seems to be a regression between trunk and Clang 18: https://godbolt.org/z/TKTzKThff |
I'm not sure how it would affect gcc, but I think this error in clang is probably caused by #90676. Working on a fix. |
I guess it affects building gcc with llvm? It's definitely #90676 that regresses the redefining |
This change has a long history. It was first attempted naively in https://reviews.llvm.org/D131425, which didn't work because we broke the ability for code to include e.g. <stdio.h> multiple times and get different definitions based on the pre-defined macros. However, in llvm#86843 we managed to simplify <stddef.h> by including the underlying system header outside of any include guards, which worked. This patch applies the same simplification we did to <stddef.h> to the other headers that currently mention __need_FOO macros explicitly.
This change has a long history. It was first attempted naively in https://reviews.llvm.org/D131425, which didn't work because we broke the ability for code to include e.g. <stdio.h> multiple times and get different definitions based on the pre-defined macros. However, in llvm#86843 we managed to simplify <stddef.h> by including the underlying system header outside of any include guards, which worked. This patch applies the same simplification we did to <stddef.h> to the other headers that currently mention __need_FOO macros explicitly.
This change has a long history. It was first attempted naively in https://reviews.llvm.org/D131425, which didn't work because we broke the ability for code to include e.g. <stdio.h> multiple times and get different definitions based on the pre-defined macros. However, in llvm#86843 we managed to simplify <stddef.h> by including the underlying system header outside of any include guards, which worked. This patch applies the same simplification we did to <stddef.h> to the other headers that currently mention __need_FOO macros explicitly.
This change has a long history. It was first attempted naively in https://reviews.llvm.org/D131425, which didn't work because we broke the ability for code to include e.g. <stdio.h> multiple times and get different definitions based on the pre-defined macros. However, in #86843 we managed to simplify <stddef.h> by including the underlying system header outside of any include guards, which worked. This patch applies the same simplification we did to <stddef.h> to the other headers that currently mention __need_FOO macros explicitly.
Libc++'s own <stddef.h> is complicated by the need to handle various platform-specific macros and to support duplicate inclusion. In reality, we only need to add a declaration of nullptr_t to it, so we can simply include the underlying <stddef.h> outside of our guards to let it handle re-inclusion itself.