Define NULL in its own header

[iana]

Summary

Make a new header to define NULL so that the OS can use it independent of stddef.h, i.e. in POSIX headers that are required to define NULL but not the rest of stddef.h.

[iana]

clang/lib/Headers/__stddef_null.h

15

arc lint wanted to remove all of the indentation that was in stddef.h.

[iana]

This needs a bit more work, it looks like it's not enough to just add a header.

[iana]

Add a module for stddef_null.h and give it the same special treatment as stddef_max_align_t.h.

[iana]

I think this should be all we need, but I'm still running tests.

[iana]

Rebase, update diagnostic to include the full module name instead of just the top level.

[iana]

Fix the debuginfo-generic-assignment-tracking-sroa test

[iana]

llvm/test/DebugInfo/Generic/assignment-tracking/sroa/unspecified-var-size.ll

37 ↗

(On Diff #491607)

Adding this line is the only reason I changed this file. I'm not familiar at all with how these tests work, so I don't really know if it's necessary. The test passes with and without these changes.

[iana]

Fix the broken tests

[aaron.ballman]

I'd expect there to be a test under clang/test/Headers/ showing that this new header works as expected.

llvm/test/DebugInfo/Generic/assignment-tracking/sroa/unspecified-var-size.ll

37 ↗

(On Diff #491607)

CC @dblaikie and @echristo for questions about whether we should be updating this debug info test or not.

[iana]

In D140250#4076851, @aaron.ballman wrote:

I'd expect there to be a test under clang/test/Headers/ showing that this new header works as expected.

It's covered pretty well by stddefneeds.cpp already. I can add a new one if you want, but it'd pretty much be a copy/paste

[aaron.ballman]

In D140250#4077826, @iana wrote:

In D140250#4076851, @aaron.ballman wrote:

I'd expect there to be a test under clang/test/Headers/ showing that this new header works as expected.

It's covered pretty well by stddefneeds.cpp already. I can add a new one if you want, but it'd pretty much be a copy/paste

I was thinking of one similar to that but includes the new header and *not* stddef.h to demonstrate that this works entirely standalone. WDYT?

[iana]

In D140250#4077956, @aaron.ballman wrote:

In D140250#4077826, @iana wrote:

In D140250#4076851, @aaron.ballman wrote:

I'd expect there to be a test under clang/test/Headers/ showing that this new header works as expected.

It's covered pretty well by stddefneeds.cpp already. I can add a new one if you want, but it'd pretty much be a copy/paste

I was thinking of one similar to that but includes the new header and *not* stddef.h to demonstrate that this works entirely standalone. WDYT?

Yeah ok sure

[iana]

Add an explicit header test for __stddef_null.h

[iana]

In D140250#4077974, @iana wrote:

In D140250#4077956, @aaron.ballman wrote:

In D140250#4077826, @iana wrote:

In D140250#4076851, @aaron.ballman wrote:

I'd expect there to be a test under clang/test/Headers/ showing that this new header works as expected.

It's covered pretty well by stddefneeds.cpp already. I can add a new one if you want, but it'd pretty much be a copy/paste

I was thinking of one similar to that but includes the new header and *not* stddef.h to demonstrate that this works entirely standalone. WDYT?

Yeah ok sure

Added, let me know if it's missing anything please!

[dblaikie]

llvm/test/DebugInfo/Generic/assignment-tracking/sroa/unspecified-var-size.ll

37 ↗

(On Diff #491607)

Don't think there's any reason/need to - what motivated changing this file?

[iana]

llvm/test/DebugInfo/Generic/assignment-tracking/sroa/unspecified-var-size.ll

37 ↗

(On Diff #491607)

I found it when I was checking for places that handled __stddef_max_align_t.h specially. I'm not sure if that's in here to check _ZTS11max_align_t under it, or if it's just there because stddef.h includes it. __stddef_null.h doesn't define any types like that, so maybe it's fine to just revert this file?

[aaron.ballman]

llvm/test/DebugInfo/Generic/assignment-tracking/sroa/unspecified-var-size.ll

37 ↗

(On Diff #491607)

Yeah, I think it's reasonable to revert the changes to this file.

[dblaikie]

llvm/test/DebugInfo/Generic/assignment-tracking/sroa/unspecified-var-size.ll

37 ↗

(On Diff #491607)

Yeah, that's just some incidental debug info (because clang doesn't track use of using decls, we emit them rather unconditionally into DWARF, and it's often easier to create/manitain DWARF testing by compiling real code rather than hand crafting the IR metadata) - no need to update it for this case, I think.

[iana]

Revert the unnecessary changes to unspecified-var-size.ll

llvm/test/DebugInfo/Generic/assignment-tracking/sroa/unspecified-var-size.ll

37 ↗

(On Diff #491607)

👍

[aaron.ballman]

LGTM!

[aaron.ballman]

Please be sure to add a release note when landing the changes, btw.

[rsmith]

Wait a second... if an OS wants only NULL, we already have a supported way of achieving that, which is compatible with GCC and glibc and other POSIX compilers -- define __need_NULL before including the header.

We shouldn't be providing internal headers with __mangled_names for OSes to include, those should be implementation details.

[iana]

In D140250#4080990, @rsmith wrote:

Wait a second... if an OS wants only NULL, we already have a supported way of achieving that, which is compatible with GCC and glibc and other POSIX compilers -- define __need_NULL before including the header.

We shouldn't be providing internal headers with __mangled_names for OSes to include, those should be implementation details.

You can't do that with modules though, #define __need_NULL #include <stddef.h> doesn't work.

[aaron.ballman]

In D140250#4081009, @iana wrote:

In D140250#4080990, @rsmith wrote:

Wait a second... if an OS wants only NULL, we already have a supported way of achieving that, which is compatible with GCC and glibc and other POSIX compilers -- define __need_NULL before including the header.

We shouldn't be providing internal headers with __mangled_names for OSes to include, those should be implementation details.

You can't do that with modules though, #define __need_NULL #include <stddef.h> doesn't work.

Also, from the patch summary: i.e. in POSIX headers that are required to define NULL but not the rest of stddef.h. -- I don't think __need_NULL works for this because it gets you NULL but it also gets you the rest of the stddef.h header, right?

[iana]

In D140250#4081044, @aaron.ballman wrote:

In D140250#4081009, @iana wrote:

In D140250#4080990, @rsmith wrote:

Wait a second... if an OS wants only NULL, we already have a supported way of achieving that, which is compatible with GCC and glibc and other POSIX compilers -- define __need_NULL before including the header.

We shouldn't be providing internal headers with __mangled_names for OSes to include, those should be implementation details.

You can't do that with modules though, #define __need_NULL #include <stddef.h> doesn't work.

Also, from the patch summary: i.e. in POSIX headers that are required to define NULL but not the rest of stddef.h. -- I don't think __need_NULL works for this because it gets you NULL but it also gets you the rest of the stddef.h header, right?

Once you set one of the __need macros, then stddef.h will only give you those definitions. Or I'm interpreting the file gravely wrong... it's kind of a weird header and its customizations don't really work with modules either.

More concretely, on Apple platforms there's <sys/_types/_null.h>, <sys/_types/_ptrdiff.h>, etc. In textual mode you could have this.

// _null.h
#define __need_NULL
#include <stddef.h>

// _ptrdiff.h
#define __need_ptrdiff_t
#include <stddef.h>

But that doesn't work with modules. stddef.h is built once according to whatever macros were passed on the command line. And there's no way to set the macros on the command line such that each OS header gets just what it needs. Hence the need to split NULL, and really all of stddef.h but I'm going to do that in a future change, into constituent headers.

[rsmith]

In D140250#4081009, @iana wrote:

In D140250#4080990, @rsmith wrote:

Wait a second... if an OS wants only NULL, we already have a supported way of achieving that, which is compatible with GCC and glibc and other POSIX compilers -- define __need_NULL before including the header.

We shouldn't be providing internal headers with __mangled_names for OSes to include, those should be implementation details.

You can't do that with modules though, #define __need_NULL #include <stddef.h> doesn't work.

Our builtin header stddef.h shouldn't be built as a module. It fundamentally needs to be treated as a textual header, because it consumes macros defined by the includer. The module map that we provide with our builtin headers does not cover stddef.h for that reason.

[rsmith]

In D140250#4081102, @rsmith wrote:

Our builtin header stddef.h shouldn't be built as a module. It fundamentally needs to be treated as a textual header, because it consumes macros defined by the includer. The module map that we provide with our builtin headers does not cover stddef.h for that reason.

That said... the __has_feature(modules) checks throughout stddef.h look pretty wrong to me. They're probably working around the infelicities of the -fno-modules-local-submodule-visibility mode; maybe we can disable them when not in that mode?

[iana]

In D140250#4081119, @rsmith wrote:

In D140250#4081102, @rsmith wrote:

Our builtin header stddef.h shouldn't be built as a module. It fundamentally needs to be treated as a textual header, because it consumes macros defined by the includer. The module map that we provide with our builtin headers does not cover stddef.h for that reason.

That said... the __has_feature(modules) checks throughout stddef.h look pretty wrong to me. They're probably working around the infelicities of the -fno-modules-local-submodule-visibility mode; maybe we can disable them when not in that mode?

It ends up being covered by Darwin.C.stddef on Apple platforms (due to some builtin shenanigans elsewhere in clang). Maybe we could make stddef.h be a textual header in a _Builtin_stddef module in clang and that would do the trick instead of splitting it into a bunch of different files.

[iana]

In D140250#4081133, @iana wrote:

In D140250#4081119, @rsmith wrote:

In D140250#4081102, @rsmith wrote:

Our builtin header stddef.h shouldn't be built as a module. It fundamentally needs to be treated as a textual header, because it consumes macros defined by the includer. The module map that we provide with our builtin headers does not cover stddef.h for that reason.

That said... the __has_feature(modules) checks throughout stddef.h look pretty wrong to me. They're probably working around the infelicities of the -fno-modules-local-submodule-visibility mode; maybe we can disable them when not in that mode?

It ends up being covered by Darwin.C.stddef on Apple platforms (due to some builtin shenanigans elsewhere in clang). Maybe we could make stddef.h be a textual header in a _Builtin_stddef module in clang and that would do the trick instead of splitting it into a bunch of different files.

This seems to work in initial testing. I'll probably drop this diff and make a new one to make stddef.h be a textual header in a new _Builtin_stddef module in clang when I do some more testing then.

[iana]

This was succeeded by D158709.

clang/lib/Headers/CMakeLists.txt

 # core_files list contains the headers shared by all platforms.
 # Please consider adding new platform specific headers
 # to platform specific lists below.
 set(core_files
   builtins.h
   float.h
   inttypes.h
   iso646.h
   limits.h
   module.modulemap
   stdalign.h
   stdarg.h
   stdatomic.h
   stdbool.h
   stddef.h
   __stddef_max_align_t.h
+  __stddef_null.h
   stdint.h
   stdnoreturn.h
   tgmath.h
   unwind.h
   varargs.h
   )
 
 set(arm_common_files

clang/lib/Headers/__stddef_null.h

+/*===---- __stddef_null.h - Definition of NULL -----------------------------===
+ *
+ * Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+ * See https://llvm.org/LICENSE.txt for license information.
+ * SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+ *
+ *===-----------------------------------------------------------------------===
+ */
+
+#ifndef __STDDEF_NULL_H
+#define __STDDEF_NULL_H
+
+#undef NULL
+#ifdef __cplusplus
+#if !defined(__MINGW32__) && !defined(_MSC_VER)
+#define NULL __null
+#else
+#define NULL 0
+#endif
+#else
+#define NULL ((void *)0)
+#endif
+#ifdef __cplusplus
+#if defined(_MSC_EXTENSIONS) && defined(_NATIVE_NULLPTR_SUPPORTED)
+namespace std {
+typedef decltype(nullptr) nullptr_t;
+}
+using ::std::nullptr_t;
+#endif
+#endif
+
+#endif

clang/lib/Headers/stddef.h

 #endif
 typedef __WCHAR_TYPE__ wchar_t;
 #endif
 #endif
 #undef __need_wchar_t
 #endif /* defined(__need_wchar_t) */
 
 #if defined(__need_NULL)
-#undef NULL
-#ifdef __cplusplus
-#  if !defined(__MINGW32__) && !defined(_MSC_VER)
-#    define NULL __null
-#  else
-#    define NULL 0
-#  endif
-#else
-#  define NULL ((void*)0)
-#endif
-#ifdef __cplusplus
-#if defined(_MSC_EXTENSIONS) && defined(_NATIVE_NULLPTR_SUPPORTED)
-namespace std { typedef decltype(nullptr) nullptr_t; }
-using ::std::nullptr_t;
-#endif
-#endif
+#include <__stddef_null.h>
 #undef __need_NULL
 #endif /* defined(__need_NULL) */
 
 /* FIXME: This is using the placeholder dates Clang produces for these macros
    in C2x mode; switch to the correct values once they've been published. */
 #if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202000L
 typedef typeof(nullptr) nullptr_t;
 #endif /* defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202000L */