This is an archive of the discontinued LLVM Phabricator instance.

[Headers] Replace __need_STDDEF_H_misc with specific __need_ macros
ClosedPublic

Authored by iana on Aug 11 2023, 2:43 PM.

Details

Summary

Apple needs __need_ macros for rsize_t and offsetof. Add those, and also ones for nullptr_t, unreachable, max_align_t.

Diff Detail

Event Timeline

iana created this revision.Aug 11 2023, 2:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2023, 2:43 PM
Herald added a subscriber: ributzka. · View Herald Transcript
iana requested review of this revision.Aug 11 2023, 2:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2023, 2:43 PM
efriedma added inline comments.Aug 11 2023, 3:04 PM
clang/lib/Headers/stddef.h
111–112

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.

iana updated this revision to Diff 549534.EditedAug 11 2023, 3:10 PM

Don't define nullptr_t in C++ if __need_NULL is set without __need_nullptr_t

iana marked an inline comment as done.Aug 11 2023, 3:12 PM
iana added inline comments.
clang/lib/Headers/stddef.h
111–112

👍

efriedma accepted this revision.Aug 11 2023, 3:13 PM
This revision is now accepted and ready to land.Aug 11 2023, 3:13 PM
iana marked an inline comment as done.Aug 11 2023, 3:14 PM
iana added inline comments.
clang/test/Headers/stddef.c
20–23

Does anyone know a trick to catch the note from stddef.h?

clang/test/Headers/stddefneeds.c
35–37

(same here)

85–87

(and the error here)

efriedma added inline comments.Aug 11 2023, 3:43 PM
clang/test/Headers/stddef.c
20–23

I think the syntax is something like expected-warning@stddef.h:10? See rGfcc699ae , There's also a wildcard syntax; see rG05eedf1f.

iana marked an inline comment as done.Aug 11 2023, 3:53 PM
iana added inline comments.
clang/test/Headers/stddef.c
20–23

Ah ha!

iana updated this revision to Diff 549547.EditedAug 11 2023, 4:41 PM
iana marked an inline comment as done.

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

iana updated this revision to Diff 549572.EditedAug 11 2023, 9:57 PM

Let the Phabricator linter update stddef.h

aaron.ballman added inline comments.
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?)

116–120
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.

iana updated this revision to Diff 550091.Aug 14 2023, 2:29 PM
iana marked 3 inline comments as done.

Review fixes

iana marked an inline comment as done.Aug 14 2023, 2:30 PM
iana added inline comments.
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.

aaron.ballman accepted this revision.Aug 15 2023, 5:16 AM
aaron.ballman added a reviewer: Restricted Project.

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).

This revision now requires review to proceed.Aug 15 2023, 5:16 AM

I think these changes would allow us to drop the stddef.h compat header from libc++, which would be really nice.

clang/lib/Headers/stddef.h
35–50
116–123
iana marked an inline comment as done.Aug 16 2023, 11:21 AM

I think these changes would allow us to drop the stddef.h compat header from libc++, which would be really nice.

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
116–120

I don't _think_ this change actually changes the way nullptr_t gets defined in C++, does it?

iana added a comment.Aug 16 2023, 11:27 AM

I think these changes would allow us to drop the stddef.h compat header from libc++, which would be really nice.

I'd really rather not change how nullptr_t gets declared in this change, can we do these in another one?

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
116–120

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).

iana added a comment.Aug 18 2023, 4:31 PM

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.

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
116–120

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.

aaron.ballman added inline comments.Aug 21 2023, 9:04 AM
clang/lib/Headers/stddef.h
116–120

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.

iana added inline comments.Aug 21 2023, 11:38 AM
clang/lib/Headers/stddef.h
116–120

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.

aaron.ballman added inline comments.Aug 21 2023, 12:06 PM
clang/lib/Headers/stddef.h
116–120

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.

iana added inline comments.Aug 21 2023, 12:35 PM
clang/lib/Headers/stddef.h
116–120

👍

ldionne added inline comments.Aug 22 2023, 8:53 AM
clang/lib/Headers/stddef.h
1

Making a thread out of this:

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.

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?

116–120

Does libc++ provide the symbols for a freestanding compilation?

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.

aaron.ballman added inline comments.Aug 22 2023, 11:04 AM
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.

116–120

Does libc++ provide the symbols for a freestanding compilation?

I don't think we do. We basically don't support -ffreestanding right now (we support embedded and funky platforms via other mechanisms).

Okay, that's what I thought as well. Thanks!

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

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?

iana added inline comments.Aug 22 2023, 11:33 AM
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.

iana updated this revision to Diff 552591.Aug 22 2023, 10:27 PM

Only define nullptr_t in C++ if _MSC_EXTENSIONS is defined, even if __need_nullptr_t is explicitly set.

clang/lib/Headers/stddef.h
116–120

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)

ldionne added inline comments.Aug 23 2023, 6:25 AM
clang/lib/Headers/stddef.h
116–120

@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:

  1. Libc++ doesn't need to have <stddef.h> anymore
  2. The Clang builtin headers are pure C
  3. std::nullptr_t and ::nullptr_t are always the same thing when both are defined

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?

aaron.ballman added inline comments.Aug 23 2023, 10:10 AM
clang/lib/Headers/stddef.h
116–120

Does this make sense to everyone?

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.

ldionne accepted this revision.Aug 23 2023, 1:08 PM

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
111–112

You could reduce the diff by not reformatting this.

This revision is now accepted and ready to land.Aug 23 2023, 1:08 PM

Also we should figure out why the Clang modules build is failing on top of this, it doesn't look like a false positive.

iana marked 13 inline comments as done.Aug 23 2023, 1:11 PM
iana added inline comments.
clang/lib/Headers/stddef.h
111–112

That's what I did initially, but the CI failed its formatting check.

iana marked an inline comment as done.Aug 23 2023, 1:21 PM

Also we should figure out why the Clang modules build is failing on top of this, it doesn't look like a false positive.

Where do you see that failure?

iana added a comment.Aug 23 2023, 2:50 PM

Also we should figure out why the Clang modules build is failing on top of this, it doesn't look like a false positive.

Where do you see that failure?

Never mind, I see it now.

iana updated this revision to Diff 553003.Aug 23 2023, 11:18 PM

Try rebasing one more time

iana added a comment.Aug 23 2023, 11:54 PM

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.

This revision was landed with ongoing or failed builds.Aug 23 2023, 11:55 PM
This revision was automatically updated to reflect the committed changes.