This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by fsb4000 on Mar 18 2023, 7:19 PM.

Details

Reviewers
Mordante
philnik
ldionne
EricWF
Group Reviewers
Restricted Project
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;

Diff Detail

Event Timeline

fsb4000 created this revision.Mar 18 2023, 7:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2023, 7:19 PM
fsb4000 requested review of this revision.Mar 18 2023, 7:19 PM
fsb4000 edited the summary of this revision. (Show Details)Mar 18 2023, 8:38 PM
fsb4000 updated this revision to Diff 506348.Mar 18 2023, 8:52 PM
fsb4000 edited the summary of this revision. (Show Details)Mar 18 2023, 9:06 PM
fsb4000 edited the summary of this revision. (Show Details)Mar 18 2023, 9:16 PM
fsb4000 updated this revision to Diff 506349.Mar 18 2023, 9:32 PM
philnik added inline comments.Mar 19 2023, 1:59 AM
libcxx/include/__compare/ordering.h
45–50
48

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

libcxx/test/libcxx/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 ↗(On Diff #506349)

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

fsb4000 updated this revision to Diff 506379.Mar 19 2023, 4:32 AM

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 marked 2 inline comments as done.Mar 19 2023, 4:32 AM
fsb4000 added inline comments.
libcxx/include/__compare/ordering.h
45–50

Thanks!

48

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/libcxx/language.support/cmp/cmp.categories.pre/zero_type.verify.cpp
1

Ok.

libcxx/test/std/language.support/cmp/cmp.partialord/partialord.pass.cpp
10 ↗(On Diff #506349)

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

fsb4000 marked 2 inline comments as done.Mar 19 2023, 4:35 AM
fsb4000 updated this revision to Diff 506384.Mar 19 2023, 6:26 AM
fsb4000 updated this revision to Diff 506388.Mar 19 2023, 7:00 AM
fsb4000 updated this revision to Diff 506393.Mar 19 2023, 7:28 AM
fsb4000 updated this revision to Diff 506396.Mar 19 2023, 7:55 AM
fsb4000 updated this revision to Diff 506401.Mar 19 2023, 9:22 AM
fsb4000 retitled this revision from [libc++]Don't warn when using operator<=> with 0 to [libc++]Enforce `-Wzero-as-null-pointer-constant`.Mar 19 2023, 9:27 AM

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 updated this revision to Diff 506403.Mar 19 2023, 9:50 AM

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

Thanks for working on this!

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 requested changes to this revision.Mar 20 2023, 11:45 AM
EricWF added a subscriber: EricWF.
EricWF added inline comments.
libcxx/include/__compare/ordering.h
46

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/libcxx/language.support/cmp/cmp.categories.pre/zero_type.verify.cpp
22

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.

This revision now requires changes to proceed.Mar 20 2023, 11:45 AM
fsb4000 abandoned this revision.Sep 20 2023, 8:44 AM