[libc++]Enforce `-Wzero-as-null-pointer-constant`

[fsb4000]

Summary

Fixes https://github.com/llvm/llvm-project/issues/43670

From https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0768r1.pdf :

The relational and equality operators for the comparison category types are specified with an
anonymous parameter of unspecified type. This type shall be selected by the implementation such
that these parameters can accept literal 0 as a corresponding argument. [Example: nullptr_t
satisfies this requirement. — end example] In this context, the behavior of a program that supplies
an argument other than a literal 0 is undefined

So after this patch we will accept not literal 0 but consteval zero like

#include <compare>

consteval int test() { return 0;}
auto b = 1 <=> 2 < test();

But before we accepted nullptr. So I guess it's fine and the behavior is undefined in the both cases.

Oh, I see that we rejected nullptr: https://github.com/llvm/llvm-project/blob/93a04a0591c1958d5344f3541157fbd8b8ff7370/libcxx/test/std/language.support/cmp/cmp.categories.pre/zero_type.verify.cpp

That code was accepted. Yes, the probability of it in reality is much lower than auto b = 1 <=> 2 < (1-1). But I think (1-1) is also low enough...

#include <compare>

consteval int test() { return 0;}
auto b = 1 <=> 2 < (int std::_CmpUnspecifiedParam::*)nullptr;

[philnik]

libcxx/include/__compare/ordering.h

43–48

46

This produces way better error messages on most compilers: https://godbolt.org/z/PhjaTav68

libcxx/test/std/language.support/cmp/cmp.categories.pre/zero_type.verify.cpp

1

I think this test should be inside test/libcxx, since it is very implementation-specific.

libcxx/test/std/language.support/cmp/cmp.partialord/partialord.pass.cpp

10

Is there a reason we can't enable this globally?

[fsb4000]

removed unneeded _LIBCPP_HIDE_FROM_ABI

added __literal_zero_is_expected function for a better error message.

added -Wzero-as-null-pointer-constant warning for all tests

moved cmp/cmp.categories.pre/zero_type.verify.cpp. test to libcxx tests

Also I created the same PR for MSVC STL: https://github.com/microsoft/STL/pull/3581

And GCC failed with this change: "in ‘constexpr’ expansion of ‘f.test_range()::<lambda()>()’ cc1plus: error: taking address of an immediate function ‘consteval std::__1::_CmpUnspecifiedParam::_CmpUnspecifiedParam(int)" (std/algorithms/alg_sorting/alg_min_max/ranges.min.pass.cpp")

I will try to investigate this.

[fsb4000]

libcxx/include/__compare/ordering.h

43–48

Thanks!

46

Yes, the error messages are better but if I remember correctly we can't use throw because we need to support no-exceptions.

libcxx/test/std/language.support/cmp/cmp.categories.pre/zero_type.verify.cpp

1

Ok.

libcxx/test/std/language.support/cmp/cmp.partialord/partialord.pass.cpp

10

Ok, I will try.
Let's see if that break something.

[philnik]

These are a lot more changes then expected. Let's split this up into multiple patches. I would suggest that we first enable the warning inside the clang-tidy test to check our headers and then enable the warning inside the tests. As a third patch we can then change the three way comparison.

[fsb4000]

@philnik , Okay, I'll deal with that tomorrow.

[Mordante]

Thanks for working on this!

In D146372#4204837, @philnik wrote:

These are a lot more changes then expected. Let's split this up into multiple patches. I would suggest that we first enable the warning inside the clang-tidy test to check our headers and then enable the warning inside the tests. As a third patch we can then change the three way comparison.

+1 for splitting the patch especially since the changes fail in the CI.

libcxx/test/std/input.output/iostream.format/input.streams/istream.formatted/istream.formatted.arithmetic/int.pass.cpp

50 ↗

(On Diff #506403)

Doesn't this work too?

[EricWF]

libcxx/include/__compare/ordering.h

44

Richard Smith and I have talked about creating a special type in Clang to use here.
I think we should investigate this approach more thoroughly before going down the path suggested here.

As is, your change makes the number of values and types accepted as an argument to _CmpUnspecifiedParam MUCH larger.

Before, it would only accept the _LITERAL_ zero, now it accepts anything that evaluates to zero, including (0 + 42 - 42), foo(), etc.

So this makes the library much less conforming than it was. This cannot proceed as is.

Though I grant it's very frustrating that the warning is triggered here. But we'll need Clang's help to resolve that.

libcxx/test/std/language.support/cmp/cmp.categories.pre/zero_type.verify.cpp

26

This change seems wrong. It looks like we're enforcing that an error actually occurs when zero is used.
But those test cases get removed?

Maybe we should disable the warning in this test.

libcxx/include/__compare/ordering.h

 class partial_ordering;
 class weak_ordering;
 class strong_ordering;
 
 template<class _Tp, class... _Args>
 inline constexpr bool __one_of_v = (is_same_v<_Tp, _Args> || ...);
 
 struct _CmpUnspecifiedParam {
-  _LIBCPP_HIDE_FROM_ABI constexpr
-  _CmpUnspecifiedParam(int _CmpUnspecifiedParam::*) noexcept {}
+  _LIBCPP_HIDE_FROM_ABI consteval
+  _CmpUnspecifiedParam(int __literal_zero) noexcept {
+    if (__literal_zero != 0) {
+      __builtin_abort();
+    }
+  }
 
   template<class _Tp, class = enable_if_t<!__one_of_v<_Tp, int, partial_ordering, weak_ordering, strong_ordering>>>
   _CmpUnspecifiedParam(_Tp) = delete;
 };
 
 class partial_ordering {
   using _ValueT = signed char;
 

libcxx/test/std/language.support/cmp/cmp.categories.pre/zero_type.verify.cpp

 //===----------------------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 
 // In MSVC mode, there's a slightly different number of errors printed for
 // each of these, so it doesn't add up to the exact expected count of 18.
 // XFAIL: msvc
 
 // <compare>
 
 // Ensure we reject all cases where an argument other than a literal 0 is used
 // for a comparison against a comparison category type.
 
 #include <compare>
 
 #define TEST_FAIL(v, op)                                                       \
   void(v op 0L);                                                               \
   void(0L op v);                                                               \
   void(v op nullptr);                                                          \
-  void(nullptr op v);                                                          \
-  void(v op(1 - 1));                                                           \
-  void((1 - 1) op v)
+  void(nullptr op v)
 
 #define TEST_PASS(v, op)                                                       \
   void(v op 0);                                                                \
   void(0 op v)
 
 template <typename T>
 void test_category(T v) {
   TEST_FAIL(v, ==);  // expected-error 18 {{}}

libcxx/test/std/language.support/cmp/cmp.partialord/partialord.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
+// ADDITIONAL_COMPILE_FLAGS: -Wzero-as-null-pointer-constant
 
 // <compare>
 
 // class partial_ordering
 
 
 #include <compare>
 #include <type_traits>

libcxx/test/std/language.support/cmp/cmp.strongord/strongord.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
+// ADDITIONAL_COMPILE_FLAGS: -Wzero-as-null-pointer-constant
 
 // <compare>
 
 // class strong_ordering
 
 
 #include <compare>
 #include <type_traits>

libcxx/test/std/language.support/cmp/cmp.weakord/weakord.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
+// ADDITIONAL_COMPILE_FLAGS: -Wzero-as-null-pointer-constant
 
 // <compare>
 
 // class weak_ordering
 
 
 #include <compare>
 #include <type_traits>