[libc++] Remove the ability to use the std::nullptr_t emulation in C++03 mode

[ldionne]

Summary

Back in https://reviews.llvm.org/D109459, we stopped using the C++03
emulation for std::nullptr_t by default, which was an ABI break. We
still left a knob for users to turn it back on if they were broken by
the change, with a note that we would remove that knob after one release.

The time has now come to remove the knob and clean up the std::nullptr_t
emulation.

[ldionne]

Rebase onto main.

[Mordante]

LGTM, it seems the build breaks since I recently added this header in include/__format/formatter_pointer.h.

[ldionne]

Rebase onto main.

[ldionne]

Rebase onto main and remove <__nullptr> include from format

[miyuki]

Is it expected that ::nullptr_t gets defined in headers other than <cstddef>
(e.g. <cmath>)? And also is it expected that ::nullptr_t gets defined even in C++03 mode?

[ldionne]

In D114786#3307456, @miyuki wrote:

Is it expected that ::nullptr_t gets defined in headers other than <cstddef>
(e.g. <cmath>)? And also is it expected that ::nullptr_t gets defined even in C++03 mode?

Yes, that was the state of things before and I did not intend to change that with this patch.

[miyuki]

OK, thanks.

[JamesNagurne]

Hi Louis,

Our downstream validations have begun failing when downstreaming this change.

C++11 18.2.9 states that nullptr_t is defined as:

namespace std {
  typedef decltype(nullptr) nullptr_t;
}

Our conformance test suite ensures this by having the following:

#include <cstddef>
nullptr_t ptr = nullptr;

And expects a diagnostic, specifically that nullptr_t is not defined.
This is a change in behavior for the compiler. Was that expected?

Here's an example comparing trunk to a previous version.

https://godbolt.org/z/GjbEh1hqT

[• Quuxplusone]

libcxx/include/stddef.h

48

@JamesNagurne @ldionne : Whoa, yeah. Shouldn't this be

#ifdef __cplusplus
namespace std { using nullptr_t = decltype(nullptr); }
#endif

and that's all? Also, what was this ever doing in C's <stddef.h>? It should go in <cstddef> instead.
If we want to preserve libc++'s historical behavior, it seems it would be

// in <cstddef>
namespace std { using nullptr_t = decltype(nullptr); }

// in <stddef.h>
namespace std { using nullptr_t = decltype(nullptr); }
using std::nullptr_t;

(Yes, including the same using nullptr_t = decltype(nullptr); token-sequence twice in a row is actually fine with C++ these days. I checked that at least GCC trunk's C++11 mode and MSVC latest's C++11 mode are happy with it.)
But, if we want to do the most sensible thing regardless of backward compatibility, I think it would be

// in <cstddef>
namespace std { using nullptr_t = decltype(nullptr); }

// in <stddef.h>
/* nothing at all */

I propose to leave this to @ldionne to address, unless he delegates it back to me. :)

[ldionne]

libcxx/include/cstddef

43

This is where I messed up in this patch, basically. By switching from include_next <stddef.h> to include <stddef.h>, I made it so that ::nullptr_t would be defined even when including <cstddef>. That being said, I really really dislike "clever" headers like this -- IMO all the <cfoo> headers should just include <foo.h> and put their declarations inside namespace std.

libcxx/include/stddef.h

48

@JamesNagurne @Quuxplusone

I agree it's messed up that we are defining ::nullptr_t, however that was the state of things before this patch too, wasn't it? I don't understand why https://godbolt.org/z/GjbEh1hqT shows different behavior, I must be missing something. Certainly this patch did not intend to change whether we provided ::nullptr_t or not.

Edit: OH, now I see it. We've always been defining ::nullptr_t from <stddef.h>, but now we also define it from <cstddef>, and that's the behavior change. That was not intended.

@Quuxplusone are we really allowed to not define ::nullptr_t at all from <stddef.h>?

[JamesNagurne]

libcxx/include/stddef.h

48

Not Arthur, but:

C++14 18.2 describes <cstddef>

• 18.2.2: Contents are the same as stddef.h (I suspect this then implies the requirements of C11's stddef.h description), except:
• 18.2.9: nullptr_t is a namespace std typedef to decltype(nullptr)

C11 Annex B.18

• Unsurprisingly, no reference to nullptr_t

C++14 Annex D.5.2, going to quote it to avoid ambiguity:

Every C header, each of which has a name of the form name.h, behaves as if each name placed in the standard library namespace by the corresponding cname header is placed within the global namespace scope. It is unspecified whether these names are first declared or defined within namespace scope (3.3.6) of the namespace std and are then injected into the global namespace scope by explicit using-declarations (7.3.3).

• std::nullptr_t defined in cstddef
• stddef.h places that name into global namespace scope either through injection of std::nullptr_t or through a compatible redefinition

This allows for nullptr_t to be in the global namespace when including stddef.h. However, the standard is pretty clear that <cstddef> shall not define a global nullptr_t name.

[• Quuxplusone]

libcxx/include/stddef.h

48

@ldionne: Oops! I assumed that <stddef.h> was supposed to act like C's /usr/include/stddef.h and thus couldn't possibly be expected to define C++ nullptr_t; but @JamesNagurne has quoted chapter and verse that proves my assumption was wrong.
https://eel.is/c++draft/support.c.headers#other-1 (stddef.h must include all of cstddef's std stuff, but into the global namespace)
https://eel.is/c++draft/cstddef.syn#lib:nullptr_t (cstddef includes std::nullptr_t)
So yup, <stddef.h> must include nullptr_t in the global namespace.

However, the standard is pretty clear that <cstddef> shall not define a global nullptr_t name.

I think we're all on the same page that as a matter of QoI, we don't want <cstddef> to define a global ::nullptr_t. But FWIW I think the standard is very clear that <cstddef> could do that if it wanted to:
https://eel.is/c++draft/support.c.headers#other-2.sentence-2 "It may also provide these names within the global namespace." (This is a non-normative example, though, and I'm not sure there's any normative text that explains the example's reasoning any more fully.)

[JamesNagurne]

libcxx/include/stddef.h

48

Consider me 'lawyered'!

My opinion, FWIW, would be to maintain consistency with the major C++ libs (Microsoft, libstdc++), if there is consistency. But I do defer to y'all.

[ldionne]

libcxx/include/stddef.h

48

What @Quuxplusone quoted is what I had read.

Consistency with other implementations seems to be that <cstddef> does define ::nullptr_t: https://godbolt.org/z/1rnv5v43G. Given this, I think it makes sense to not jump through hoops to try to be inconsistent with other implementations -- after all, portability is a quality, not a defect. So I would be tempted to not change our status quo, except I'll add a libc++ specific test to check that we do define ::nullptr_t when including <cstddef>, since that was indeed an unintended behavior change of this patch.

[rprichard]

I'm seeing the libcxxabi/test/catch_reference_nullptr.pass.cpp test fail on Android, because the test defines a nullptr_t in main() that shadows ::nullptr_t, and that test gets compiled with -Werror -Wshadow. I think I can rename its nullptr_t to my_nullptr_t to avoid the collision. I would think the test passes on other targets, so maybe there's some subtle difference that I'm hitting.

******************** TEST 'llvm-libc++abi-android.cfg.in :: catch_reference_nullptr.pass.cpp' FAILED ********************
Script:
--
: 'COMPILED WITH';  /x/ndk/out/android-ndk-r26-canary/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++ /x/llvm-upstream/llvm-project/libcxxabi/test/catch_reference_nullptr.pass.cpp  --target=aarch64-none-linux-android32 -nostdinc++ -I /x/llvm-upstream/llvm-project/libcxxabi/include -I /x/llvm-upstream/libcxx-arm64-platform/include/c++/v1 -I /x/llvm-upstream/libcxx-arm64-platform/include/aarch64-none-linux-android32/c++/v1  -I /x/llvm-upstream/llvm-project/libcxxabi/../libcxx/test/support -I /x/llvm-upstream/llvm-project/libcxxabi/../libcxx/src -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_ASSERTIONS=1 -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings  -nostdlib++ -L /x/llvm-upstream/libcxx-arm64-platform/lib -Wl,-rpath,/data/local/tmp/libc++ -lc++ -lc++abi -o /x/llvm-upstream/libcxx-arm64-platform/Output/catch_reference_nullptr.pass.cpp.dir/t.tmp.exe
: 'EXECUTED AS';  /x/llvm-upstream/job-limiter-child.sh /x/llvm-upstream/limit.970840 /x/llvm-upstream/llvm-project/libcxx/utils/adb_run.py --env PATH=/data/local/tmp/toybox:/product/bin:/apex/com.android.runtime/bin:/apex/com.android.art/bin:/system_ext/bin:/system/bin:/system/xbin:/odm/bin:/vendor/bin:/vendor/xbin --execdir /x/llvm-upstream/libcxx-arm64-platform/Output/catch_reference_nullptr.pass.cpp.dir --  /x/llvm-upstream/libcxx-arm64-platform/Output/catch_reference_nullptr.pass.cpp.dir/t.tmp.exe
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "COMPILED WITH"
$ "/x/ndk/out/android-ndk-r26-canary/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++" "/x/llvm-upstream/llvm-project/libcxxabi/test/catch_reference_nullptr.pass.cpp" "--target=aarch64-none-linux-android32" "-nostdinc++" "-I" "/x/llvm-upstream/llvm-project/libcxxabi/include" "-I" "/x/llvm-upstream/libcxx-arm64-platform/include/c++/v1" "-I" "/x/llvm-upstream/libcxx-arm64-platform/include/aarch64-none-linux-android32/c++/v1" "-I" "/x/llvm-upstream/llvm-project/libcxxabi/../libcxx/test/support" "-I" "/x/llvm-upstream/llvm-project/libcxxabi/../libcxx/src" "-D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS" "-std=c++2b" "-Werror" "-Wall" "-Wextra" "-Wshadow" "-Wundef" "-Wno-unused-command-line-argument" "-Wno-attributes" "-Wno-pessimizing-move" "-Wno-c++11-extensions" "-Wno-noexcept-type" "-Wno-atomic-alignment" "-Wno-user-defined-literals" "-Wno-tautological-compare" "-Wsign-compare" "-Wunused-variable" "-Wunused-parameter" "-Wunreachable-code" "-Wno-unused-local-typedef" "-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER" "-D_LIBCPP_ENABLE_ASSERTIONS=1" "-D_LIBCPP_DISABLE_AVAILABILITY" "-fcoroutines-ts" "-Werror=thread-safety" "-Wuser-defined-warnings" "-nostdlib++" "-L" "/x/llvm-upstream/libcxx-arm64-platform/lib" "-Wl,-rpath,/data/local/tmp/libc++" "-lc++" "-lc++abi" "-o" "/x/llvm-upstream/libcxx-arm64-platform/Output/catch_reference_nullptr.pass.cpp.dir/t.tmp.exe"
# command stderr:
/x/llvm-upstream/llvm-project/libcxxabi/test/catch_reference_nullptr.pass.cpp:30:9: error: declaration shadows a typedef in the global namespace [-Werror,-Wshadow]
  using nullptr_t = decltype(nullptr);
        ^
/x/llvm-upstream/libcxx-arm64-platform/include/c++/v1/stddef.h:48:31: note: previous declaration is here
    typedef decltype(nullptr) nullptr_t;
                              ^
1 error generated.

error: command failed with exit status: 1

--

[rprichard]

In D114786#3778965, @rprichard wrote:

I think I can rename its nullptr_t to my_nullptr_t to avoid the collision.

The test is already unsupported for C++03, so a better fix is to just use the standard library type, either ::nullptr_t or std::nullptr_t. Maybe I'll go with the latter.

[ldionne]

In D114786#3779094, @rprichard wrote:

In D114786#3778965, @rprichard wrote:

I think I can rename its nullptr_t to my_nullptr_t to avoid the collision.

The test is already unsupported for C++03, so a better fix is to just use the standard library type, either ::nullptr_t or std::nullptr_t. Maybe I'll go with the latter.

Yeah, I'd recommend simply using std::nullptr_t.