Apple needs __need_ macros for rsize_t and offsetof. Add those, and also ones for nullptr_t, unreachable, max_align_t.
Details
- Reviewers
aaron.ballman rsmith steplong efriedma jyknight erichkeane ldionne - Group Reviewers
Restricted Project - Commits
- rG72da678d8c84: [Headers] Replace __need_STDDEF_H_misc with specific __need_ macros
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
80 ms | x64 debian > Clang.AST::ast-dump-stmt-json.cpp |
Event Timeline
clang/lib/Headers/stddef.h | ||
---|---|---|
113–114 | I think this code was pulled into the __need_NULL conditional by accident; I doubt anyone actually depends on the precise interaction here. Just move to the same place as the other __need_ defines. |
clang/lib/Headers/stddef.h | ||
---|---|---|
113–114 | 👍 |
clang/test/Headers/stddef.c | ||
---|---|---|
21–24 | I think the syntax is something like expected-warning@stddef.h:10? See rGfcc699ae , There's also a wildcard syntax; see rG05eedf1f. |
clang/test/Headers/stddef.c | ||
---|---|---|
21–24 | Ah ha! |
Update the tests to handle diagnostics from stddef.h
Fix stddef.h to support defining __STDC_WANT_LIB_EXT1__ if stddef.h has already been included
clang/lib/Headers/stddef.h | ||
---|---|---|
16 | C23 K.3.1.1p2: The functions, macros, and types declared or defined in K.3 and its subclauses are declared and defined by their respective headers if STDC_WANT_LIB_EXT1 is defined as a macro which expands to the integer constant 1 at the point in the source file where the appropriate header is first included. So I don't think this should be >= 1. Same below. (Though I notice we seem to do this in other places, so perhaps that's more of a general cleanup we should make?) | |
118–122 | ||
clang/test/Headers/stddef.c | ||
2–4 | Are the triples necessary? Better if we can remove them. | |
8 | At least one of these unknown errors should be fully spelled out (the rest can then be assumed to be the same wording as the long-form). | |
clang/test/Headers/stddefneeds.c | ||
2–3 | Do we need the triples? | |
9 | Should spell out one of these. |
clang/lib/Headers/stddef.h | ||
---|---|---|
16 | >=1 is what stddef.h was already doing, so I'd vote do this later as general cleanup. | |
clang/test/Headers/stddef.c | ||
2–4 | They shouldn't be, I just copied them from stddefneeds.cpp, let me try getting rid of them | |
8 | Let me know how this looks, I can just fully expand all of them if you prefer. I was going by stddefneeds.cpp as an example. | |
clang/test/Headers/stddefneeds.c | ||
2–3 | Looks like no. |
LGTM but I'm adding libc++ as a blocking reviewer because there are questions about how well this interacts with their stddef.h (especially the nullptr_t bits): https://github.com/llvm/llvm-project/blob/main/libcxx/include/stddef.h
clang/lib/Headers/stddef.h | ||
---|---|---|
16 | That sounds reasonable to me! | |
clang/test/Headers/stddef.c | ||
8 | This works fine -- I would have also been happy if only ptrdiff_t was fully spelled out and the rest used unknown (we can infer they're all the same diagnostic from that). |
I'd really rather not change how nullptr_t gets declared in this change, can we do these in another one?
clang/lib/Headers/stddef.h | ||
---|---|---|
118–122 | I don't _think_ this change actually changes the way nullptr_t gets defined in C++, does it? |
Also my preference would be that we take the c++ stuff _out_ of this header and put it in the libc++ one, but I'm guessing that would cause issues for libstdc++?
This is going to be really naive, but can someone explain why we need these __need_XXXXX macros? Why doesn't <stddef.h> simply always declare what it should declare? Also, does anybody understand the expected relationship between the C Standard Library headers and these Clang builtin headers? Who defines what?
Everyone I've spoken to so far about this (and myself) was extremely confused. At some point I thought these macros were only needed for compatibility with old glibcs but that wouldn't even be needed anymore, but I'm not certain.
clang/lib/Headers/stddef.h | ||
---|---|---|
118–122 | I think we absolutely don't want to touch std::nullptr_t from this header. It's libc++'s responsibility to define that, and in fact we define it in std::__1, so this is even an ABI break (or I guess it would be a compiler error, not sure). |
The __need_ macros are to support some strict mostly POSIX behaviors like <sys/types.h> is supposed to provide size_t but none of the other things in <stddef.h>. Apple has headers like <sys/_types/_rsize_t.h> and <sys/_types/_offsetof.h> that are expected to provide those types and nothing else. Right now they're redeclaring the types, and if you're using clang modules with the right flags, that's an error. So I need to switch those to doing something like this.
#define __need_rsize_t #include <stddef.h>
The relationship between clang's stddef.h and the C Standard Library stddef.h is that there is no relationship. clang's header doesn't #include_next, and it is in the search path before the OS's cstdlib.
clang/lib/Headers/stddef.h | ||
---|---|---|
118–122 | I'm really not touching it though. All I did is move it from __need_NULL to __need_nullptr_t. The old behavior is that std::nullptr_t would only be touched if (no __need_ macros were set or if __need_NULL was set), and (_MSC_EXTENSIONS and _NATIVE_NULLPTR_SUPPORTED are defined). The new behavior is that std::nullptr_t will only be touched if ((no __need_ macros are set) and (_MSC_EXTENSIONS and _NATIVE_NULLPTR_SUPPORTED are defined)) or (the new __need_nullptr_t macro is set) So the only change is that C++ code that previously set __need_NULL will no longer get std::nullptr_t. @efriedma felt like that was a fine change. |
clang/lib/Headers/stddef.h | ||
---|---|---|
118–122 | Does libc++ provide the symbols for a freestanding compilation? I was pointing out those links specifically because the C++ standard currently says that stddef.h (the C standard library header) needs to provide a definition of std::nullptr_t, but that LWG thinks that's perhaps not the right way to do that and may be removing that requirement. |
clang/lib/Headers/stddef.h | ||
---|---|---|
118–122 | It is weird the standard puts that in stddef.h and not cstddef. I think libc++ could provide that in their stddef.h anyway, but the intent in this review is to not rock the boat and only do the minimal change discussed above. |
clang/lib/Headers/stddef.h | ||
---|---|---|
118–122 | Yeah, this discussion is to figure out whether we have an existing bug we need to address and if so, where to address it (libc++, clang, or the C++ standard). I don't think your changes are exacerbating anything, more just that they've potentially pointed out something related. |
clang/lib/Headers/stddef.h | ||
---|---|---|
118–122 | 👍 |
clang/lib/Headers/stddef.h | ||
---|---|---|
1 | Making a thread out of this:
So in that case what is the purpose of the SDK/system providing a <stddef.h> header? They basically all provide one and it's never used? | |
118–122 |
I don't think we do. We basically don't support -ffreestanding right now (we support embedded and funky platforms via other mechanisms). But regardless, <stddef.h> should never define something in namespace std, that should be libc++'s responsibility IMO. What we could do here instead is just #ifdef __cplusplus typedef decltype(nullptr) nullptr_t; #else typedef typeof(nullptr) nullptr_t; #endif and then let libc++'s <cstddef> do _LIBCPP_BEGIN_NAMESPACE_STD using ::nullptr_t; _LIBCPP_END_NAMESPACE_STD If Clang's <stddef.h> did define ::nullptr_t, we could likely remove libc++'s <stddef.h> and that might simplify things. |
clang/lib/Headers/stddef.h | ||
---|---|---|
1 | The compiler provides <stddef.h> for the same reason it provides <limits.h> and others: the compiler is responsible for defining these interfaces because it's the only thing that knows the correct definitions it expects. The system might know some of it, but for example, size_t relates to the maximum size of an object, which is something only the compiler knows the answer to. | |
118–122 |
Okay, that's what I thought as well. Thanks!
Ah, so you're thinking stddef.h should provide the global nullptr_t and cstddef should provide the std::nullptr_t. I was thinking stddef.h should not define nullptr_t in C++ mode at all; it's a C header, not a C++ header. That led me to thinking about what the behavior should be in C23 given that it supports nullptr_t. Were it not for the current requirement that stddef.h provide nullptr_t, I think stddef.h should do: typedef typeof(nullptr) nullptr_t; in C23 mode and not do anything special for C++ at all. C's nullptr_t needs to be ABI compatible with C++'s nullptr_t, so a C++ user including the C header should not get any problems linking against a C++ library. However, this would mean that C++ users cannot include stddef.h to get nullptr_t; they'd need to include cstddef to be assured they'd get it. But because of the ABI compatibility, perhaps the solution is to expose the above in both C and C++ modes from stddef.h, then libc++ can do the dance to import it into namespace std? |
clang/lib/Headers/stddef.h | ||
---|---|---|
1 | I think the purpose is for the SDK/system to support compilers that don't provide <stddef.h>. In the early Apple days that was CodeWarrior, maybe gcc didn't used to provide that header? I don't know. But basically yes, they all provide one and it's practically never used. |
Only define nullptr_t in C++ if _MSC_EXTENSIONS is defined, even if __need_nullptr_t is explicitly set.
clang/lib/Headers/stddef.h | ||
---|---|---|
118–122 | Actually I think I did change it after all. If a C header does something like this // It's assumed that only C23 or later is supported, or C++ #define __need_nullptr_t #include <stddef.h> If such a header got included in a C++ program, we wouldn't want to declare std::nullptr_t. I think we need to keep the _MSC_EXTENSIONS check in there and never declare it if that isn't set, even if the includer asked for nullptr_t. That matches the behavior of wchar_t in this header, and I think for similar reasons. Otherwise clang's stddef.h would step on the nullptr_t declared by libc++'s stddef.h (modules would probably complain about a duplicate/conflicting declaration) |
clang/lib/Headers/stddef.h | ||
---|---|---|
118–122 | @aaron.ballman Ok, I follow your train of thought. So in that case what we would do is this: // <stddef.h> from the Clang builtin headers: #if we-are-in-C23 typedef typeof(nullptr) nullptr_t; #endif // never do anything special for C++ // <cstddef> from libc++: #include <stddef.h> #if we-are-in-whatever-C++-standard-synced-with-C23 _LIBCPP_BEGIN_NAMESPACE_STD using ::nullptr_t; // use the C23 ::nullptr_t as defined in <stddef.h> _LIBCPP_END_NAMESPACE_STD #else _LIBCPP_BEGIN_NAMESPACE_STD typedef decltype(nullptr) nullptr_t; // declare our own nullptr_t from scratch since C doesn't have a notion of it _LIBCPP_END_NAMESPACE_STD #endif This way:
However in this world ::nullptr_t would not be defined when including <stddef.h> in C++ prior to whatever version is synced with C23 (that's not a problem IMO, but it is a behavior change). Does this make sense to everyone? |
clang/lib/Headers/stddef.h | ||
---|---|---|
118–122 |
I think it makes sense, yes (thank you for spelling it out so clearly!). I agree that it's a behavior change, but it's one I'm hoping we can argue for in LWG when discussing https://cplusplus.github.io/LWG/issue3484 -- but that said, I'm not certain if this behavior change will cause problems for significant third-party library or system headers. |
LGTM. I think what we discussed in https://reviews.llvm.org/D157757#inline-1534166 makes sense and we should do it, but it belongs to a different patch for sure. Anybody wants to take that task?
clang/lib/Headers/stddef.h | ||
---|---|---|
113–114 | You could reduce the diff by not reformatting this. |
Also we should figure out why the Clang modules build is failing on top of this, it doesn't look like a false positive.
clang/lib/Headers/stddef.h | ||
---|---|---|
113–114 | That's what I did initially, but the CI failed its formatting check. |
Windows succeeding earlier, I'm pretty confident that the current crash isn't due to this review, so I'm going to go ahead and land.
Making a thread out of this:
So in that case what is the purpose of the SDK/system providing a <stddef.h> header? They basically all provide one and it's never used?