This is an archive of the discontinued LLVM Phabricator instance.

Define NULL in its own header
AbandonedPublic

Authored by iana on Dec 16 2022, 3:02 PM.

Details

Reviewers
ributzka
vsapsai
Bigcheese
aaron.ballman
rsmith
Group Reviewers
Restricted Project
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.

Diff Detail

Event Timeline

iana created this revision.Dec 16 2022, 3:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2022, 3:02 PM
Herald added a subscriber: mstorsjo. · View Herald Transcript
iana requested review of this revision.Dec 16 2022, 3:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2022, 3:02 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
iana added inline comments.Dec 16 2022, 4:48 PM
clang/lib/Headers/__stddef_null.h
16

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

iana planned changes to this revision.Jan 3 2023, 2:13 PM

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

iana updated this revision to Diff 486100.Jan 3 2023, 2:51 PM

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

Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJan 3 2023, 2:51 PM
Herald added subscribers: llvm-commits, Restricted Project, Enna1 and 2 others. · View Herald Transcript
iana planned changes to this revision.Jan 3 2023, 2:52 PM

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

iana updated this revision to Diff 491604.Jan 23 2023, 10:08 PM

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

iana updated this revision to Diff 491607.Jan 23 2023, 10:35 PM

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

iana added inline comments.Jan 23 2023, 10:38 PM
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 updated this revision to Diff 491644.Jan 24 2023, 12:47 AM

Fix the broken tests

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.

aaron.ballman added a reviewer: Restricted Project.Jan 24 2023, 6:03 AM
iana added a comment.Jan 24 2023, 10:25 AM

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'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 added a comment.Jan 24 2023, 11:41 AM

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 updated this revision to Diff 491869.Jan 24 2023, 12:13 PM

Add an explicit header test for __stddef_null.h

iana added a comment.Jan 24 2023, 12:13 PM

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 added inline comments.Jan 24 2023, 1:12 PM
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 added inline comments.Jan 24 2023, 2:39 PM
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 added inline comments.Jan 25 2023, 6:15 AM
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 added inline comments.Jan 25 2023, 8:59 AM
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 updated this revision to Diff 492179.Jan 25 2023, 10:39 AM

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

llvm/test/DebugInfo/Generic/assignment-tracking/sroa/unspecified-var-size.ll
37 ↗(On Diff #491607)

👍

This revision is now accepted and ready to land.Jan 25 2023, 12:10 PM

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

rsmith added a subscriber: rsmith.Jan 25 2023, 12:26 PM

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.

rsmith requested changes to this revision.Jan 25 2023, 12:27 PM
This revision now requires changes to proceed.Jan 25 2023, 12:27 PM
iana added a comment.Jan 25 2023, 12:40 PM

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.

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 added a comment.EditedJan 25 2023, 1:08 PM

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 added a comment.EditedJan 25 2023, 1:33 PM

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.

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 added a comment.Jan 25 2023, 1:42 PM

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 planned changes to this revision.Jan 25 2023, 2:49 PM

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 abandoned this revision.Sep 7 2023, 10:21 PM

This was succeeded by D158709.