[C++] Expose nullptr_t from stddef.h in C++ mode #154599
Conversation
The C++ standard requires stddef.h to declare all of its contents in the global namespace. We were only doing it when trying to be compatible with Microsoft extensions. Now we expose in C++11 or later, in addition to exposing it in Microsoft extensions mode. Fixes llvm#154577
AaronBallman
added
clang
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-clang Author: Aaron Ballman (AaronBallman) ChangesThe C++ standard requires stddef.h to declare all of its contents in the global namespace. We were only doing it when trying to be compatible with Microsoft extensions. Now we expose in C++11 or later, in addition to exposing it in Microsoft extensions mode. Fixes #154577 Full diff: https://github.com/llvm/llvm-project/pull/154599.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 77629c074cdcd..bceeb63d048c8 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -223,6 +223,7 @@ Bug Fixes in This Version
cast chain. (#GH149967).
- Fixed a crash with incompatible pointer to integer conversions in designated
initializers involving string literals. (#GH154046)
+- Clang's ``<stddef.h>`` now properly declares ``nullptr_t`` in C++ mode. (#GH154577).
Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Headers/__stddef_nullptr_t.h b/clang/lib/Headers/__stddef_nullptr_t.h
index 7f3fbe6fe0d3a..c84b7bc2dc198 100644
--- a/clang/lib/Headers/__stddef_nullptr_t.h
+++ b/clang/lib/Headers/__stddef_nullptr_t.h
@@ -16,7 +16,8 @@
#define _NULLPTR_T
#ifdef __cplusplus
-#if defined(_MSC_EXTENSIONS) && defined(_NATIVE_NULLPTR_SUPPORTED)
+#if __cplusplus >= 201103L || \
+ (defined(_MSC_EXTENSIONS) && defined(_NATIVE_NULLPTR_SUPPORTED))
namespace std {
typedef decltype(nullptr) nullptr_t;
}
diff --git a/clang/test/Headers/stddefneeds.cpp b/clang/test/Headers/stddefneeds.cpp
index 0282e8afa600d..6e8829bc1be67 100644
--- a/clang/test/Headers/stddefneeds.cpp
+++ b/clang/test/Headers/stddefneeds.cpp
@@ -1,10 +1,12 @@
// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.9.0 -verify -Wsentinel -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-macosx10.9.0 -verify=old,expected -Wsentinel -std=c++98 %s
ptrdiff_t p0; // expected-error{{unknown}}
size_t s0; // expected-error{{unknown}}
void* v0 = NULL; // expected-error{{undeclared}}
wint_t w0; // expected-error{{unknown}}
max_align_t m0; // expected-error{{unknown}}
+nullptr_t n0; // expected-error {{unknown}}
#define __need_ptrdiff_t
#include <stddef.h>
@@ -14,6 +16,7 @@ size_t s1; // expected-error{{unknown}}
void* v1 = NULL; // expected-error{{undeclared}}
wint_t w1; // expected-error{{unknown}}
max_align_t m1; // expected-error{{unknown}}
+nullptr_t n1; // expected-error{{unknown}}
#define __need_size_t
#include <stddef.h>
@@ -23,6 +26,16 @@ size_t s2;
void* v2 = NULL; // expected-error{{undeclared}}
wint_t w2; // expected-error{{unknown}}
max_align_t m2; // expected-error{{unknown}}
+nullptr_t n2; // expected-error{{unknown}}
+
+#define __need_nullptr_t
+#include <stddef.h>
+ptrdiff_t p6;
+size_t s6;
+void* v6 = NULL; // expected-error{{undeclared}}
+wint_t w6; // expected-error{{unknown}}
+max_align_t m6; // expected-error{{unknown}}
+nullptr_t n6; // old-error{{unknown}}
#define __need_NULL
#include <stddef.h>
@@ -32,6 +45,16 @@ size_t s3;
void* v3 = NULL;
wint_t w3; // expected-error{{unknown}}
max_align_t m3; // expected-error{{unknown}}
+nullptr_t n3; // old-error{{unknown}}
+
+#define __need_max_align_t
+#include <stddef.h>
+ptrdiff_t p7;
+size_t s7;
+void* v7 = NULL;
+wint_t w7; // expected-error{{unknown}}
+max_align_t m7;
+nullptr_t n7; // old-error{{unknown}}
// Shouldn't bring in wint_t by default:
#include <stddef.h>
@@ -41,6 +64,7 @@ size_t s4;
void* v4 = NULL;
wint_t w4; // expected-error{{unknown}}
max_align_t m4;
+nullptr_t n4; // old-error{{unknown}}
#define __need_wint_t
#include <stddef.h>
@@ -50,7 +74,7 @@ size_t s5;
void* v5 = NULL;
wint_t w5;
max_align_t m5;
-
+nullptr_t n5; // old-error{{unknown}}
// linux/stddef.h does something like this for cpp files:
#undef NULL
|
| #if __cplusplus >= 201103L || \ | ||
| (defined(_MSC_EXTENSIONS) && defined(_NATIVE_NULLPTR_SUPPORTED)) |
There was a problem hiding this comment.
libc++ provides nullptr_t in all language modes. It would be really nice if we could remove our stddef.h header, but I'm not sure how the Clang headers would detect whether to provide it or not in C++03.
There was a problem hiding this comment.
I don't know that it's possible to not vend stddef.h; do we know that every libc actually provides a complete header that works with Clang?
There was a problem hiding this comment.
Or is your suggestion that we move stddef.h to something more like:
#if defined(__cplusplus) && __cplusplus >= 201103L /* c++11 */
#include_next <stddef.h>
#else
/* The usual stddef.h header */
#endif
Either way, I think it's orthogonal to this fix because that's a pretty substantial change that requires a fair amount of investigation. Are you okay with landing the changes as-is and doing that exploration in a follow-up if someone gets the chance? (I'm going to be out of office for a while, so I'm not able to commit to that work myself.)
There was a problem hiding this comment.
Sorry, I meant libc++'s stddef.h with "our stddef.h". We currently have a wrapping header to provide nullptr_t correctly: https://godbolt.org/z/W41dKKbd4.
There was a problem hiding this comment.
Ah! I see what you mean, thanks!
So you're okay with the current changes as-is because it heads in the right direction for libc++?
There was a problem hiding this comment.
yeah, I'm OK with it. I just wish we could go all the way.
+1, but it might be a challenge too. I'm not certain libc++ should provide nullptr_t in C++03 mode; that's taking an identifier from the user in that language mode and nullptr doesn't work in C++03 anyway: https://godbolt.org/z/96vMdbobe
There was a problem hiding this comment.
yeah, I'm OK with it. I just wish we could go all the way.
+1, but it might be a challenge too. I'm not certain libc++ should provide
nullptr_tin C++03 mode;
We claim to be a C++11 implementation, not a C++03 implementation. Just one that mostly works with -std=c++03. Whether that was the right thing to do is debatable, but it's where we are today.
that's taking an identifier from the user in that language mode and
nullptrdoesn't work in C++03 anyway: https://godbolt.org/z/96vMdbobe
Well, it does with libc++.
There was a problem hiding this comment.
We claim to be a C++11 implementation, not a C++03 implementation.
That's not how I understood the RFC, unless I missed something. I understood it as libc++ is a C++03 and later implementation but that we're not making fixes to C++03 unless there's some major security vulnerability or other extenuating circumstance.
Well, it does with libc++.
It's a non-conforming extension.
There was a problem hiding this comment.
We claim to be a C++11 implementation, not a C++03 implementation.
That's not how I understood the RFC, unless I missed something. I understood it as libc++ is a C++03 and later implementation but that we're not making fixes to C++03 unless there's some major security vulnerability or other extenuating circumstance.
This has nothing to do with the RFC. libc++ has always provided a lot of "extensions" in C++03. From the very beginning we've had C++11 features implemented in C++03. I actually think it probably was a huge mistake, but we can't change it 15 years later. This is one of the main reasons we want to split the headers, so we can leave the cruft behind and not have massive workarounds for all the stuff we provide in 03.
Well, it does with libc++.
It's a non-conforming extension.
If you look at it as a C++03 implementation. If you look at it as a C++11 implementation that works with C++03 compiler it's not.
There was a problem hiding this comment.
If you look at it as a C++03 implementation. If you look at it as a C++11 implementation that works with C++03 compiler it's not.
Yeah, I'm viewing it from the perspective of a user of Clang. This is non-conforming and has surprising behavior as a result: https://godbolt.org/z/9qPYPndKb
But, it sounds like this is moot because:
but we can't change it 15 years later.
is definitely true. :-)
|
libc++ failure appears to be unrelated: |
I agree it looks unrelated. I haven't seen that before though, so I'm not sure. |
Given the contents of the test (https://github.com/llvm/llvm-project/blob/main/libcxx/test/libcxx/utilities/tuple/no_specializations.verify.cpp) I don't see how it could be related: |
|
I'm pretty sure the libc++ failure is related. We're getting consistent failures on the premerge buildbot after this change landed: |
This reverts commit 7d167f4.
I've posted a PR for reverting, but I have no idea how to even go about debugging that failure. |
Reverts #154599 It seems to be causing staging failures: https://lab.llvm.org/staging/#/builders/192/builds/1329 https://lab.llvm.org/staging/#/builders/192/builds/1330
|
It looks like it's only the libc++ build with modules enabled that failed. It should reproduce with a libc++ runtimes build with the flags in llvm-project/.ci/monolithic-linux.sh Line 94 in 17eb05d If you can't reproduce locally, let me know and I can try and find a build configuration that reproduces it. We're planning on having the premerge system provide nice reproduction instructions, but we haven't been able to get to it yet. |
|
Thanks! I can try to investigate from my side but I'm on Windows and I don't typically build libc++ so it's possible I won't get very far. Due to timing, I may not get back to this before October, so assistance is very much appreciated. |
…(#154767) Reverts llvm/llvm-project#154599 It seems to be causing staging failures: https://lab.llvm.org/staging/#/builders/192/builds/1329 https://lab.llvm.org/staging/#/builders/192/builds/1330
|
I was unable to get to the bottom of this and have run out of time before heading out for WG14/sabbatical. If anyone wants to commandeer this PR and get it across the finish line, that's perfectly fine by me. Otherwise I'll try to get back on it in October. |
5 participants
The C++ standard requires stddef.h to declare all of its contents in the global namespace. We were only doing it when trying to be compatible with Microsoft extensions. Now we expose in C++11 or later, in addition to exposing it in Microsoft extensions mode.
Fixes #154577