[libc++] Fix std::__debug_less in c++17.

[rupprecht]

Summary

b07b5bd72716625e0976a84d23652d94d8d0165a adds a use of __comp_ref_type.h to std::min. When libc++ is built with -D_LIBCPP_DEBUG=0, this enables std::__debug_less, which is only marked constexpr after c++17.

std::min itself is marked as being constexpr as of c++14, so by extension, std::__debug_less should also be marked constexpr for the same versions so that std::min can use it. This change lowers the guard from > 17 to > 11.

Reproducer in godbolt: https://godbolt.org/z/ans3TGsj8

constexpr int x() { return std::min<int>({1, 2, 3, 4}); }

static_assert(x() == 1);

[philnik]

LGTM % nits.

libcxx/include/__algorithm/comp_ref_type.h

25

Pre-existing: This should be _LIBCPP_DEBUG_LEVEL == 1.

50–51

Please replace _LIBCPP_INLINE_VISIBILITY with _LIBCPP_HIDE_FROM_ABI as a fly-by.

libcxx/test/libcxx/algorithms/debug_less.pass.cpp

276

[rupprecht]

• Use _LIBCPP_HIDE_FROM_ABI
• Use `_LIBCPP_DEBUG_LEVEL
• Fix clang-format CI test failure
• Fix whitespace in test

[rupprecht]

libcxx/include/__algorithm/comp_ref_type.h

25

IIUC, _LIBCPP_DEBUG_LEVEL=2 should provide a superset of _LIBCPP_DEBUG_LEVEL=1: https://github.com/llvm/llvm-project/blob/c3c1c5c6953fcf9320a0cae5121ce55839169790/libcxx/include/__config#L907

Did you want #if _LIBCPP_DEBUG_LEVEL >= 1?

51

FYI, this change doesn't look very pleasant, but the clang-format test on libcxx CI was failing, and clang-format does this.

[philnik]

libcxx/include/__algorithm/comp_ref_type.h

25

Oh sorry, that is what I meant.

51

The clang-format check isn't a hard error, you can (and should) revert that part.

[• Quuxplusone]

As philnik said, please revert all the extraneous changes. (And please ignore clang-format's noise going forward. We'd disable clang-format entirely if we could. This is an ongoing pain point for libc++, over and over and over.)

IIUC, the only intentional functional change here is to change _LIBCPP_CONSTEXPR_AFTER_CXX17 into _LIBCPP_CONSTEXPR_AFTER_CXX11, is that right? That certainly seems like a good idea to me. (My mantra there is that internal details should always use the most aggressive constexpr possible. That way, if we ever drop support for C++03 and C++11 modes, the amount of stuff can be search-and-replaced to plain constexpr immediately is maximized.)

Contrariwise, your changes to _LIBCPP_DEBUG seem extraneous and/or wrong; I don't think you should touch anything in this PR that isn't related to the bug you're trying to fix. (And if we were going to touch anything in that area, I'd suggest that we remove the #ifs around __debug_less: I see no reason ever to if-out its class definition, even if it's sometimes unused.)

libcxx/include/__algorithm/comp_ref_type.h

67

This is definitely wrong, because _LIBCPP_DEBUG might not be defined at all, in which case this change will trigger -Wundef. Please leave _LIBCPP_DEBUG alone unless it's related to your patch.

[rupprecht]

• Undo clang-formatting

[rupprecht]

• Restoring original version of the patch

[philnik]

In D118940#3295112, @Quuxplusone wrote:

As philnik said, please revert all the extraneous changes. (And please ignore clang-format's noise going forward. We'd disable clang-format entirely if we could. This is an ongoing pain point for libc++, over and over and over.)

IIUC, the only intentional functional change here is to change _LIBCPP_CONSTEXPR_AFTER_CXX17 into _LIBCPP_CONSTEXPR_AFTER_CXX11, is that right? That certainly seems like a good idea to me. (My mantra there is that internal details should always use the most aggressive constexpr possible. That way, if we ever drop support for C++03 and C++11 modes, the amount of stuff can be search-and-replaced to plain constexpr immediately is maximized.)

Contrariwise, your changes to _LIBCPP_DEBUG seem extraneous and/or wrong; I don't think you should touch anything in this PR that isn't related to the bug you're trying to fix. (And if we were going to touch anything in that area, I'd suggest that we remove the #ifs around __debug_less: I see no reason ever to if-out its class definition, even if it's sometimes unused.)

I don't particularly care if we remove the _LIBCPP_DEBUGs around __debug_less, but I think they can stay in.

libcxx/include/__algorithm/comp_ref_type.h

67

If _LIBCPP_DEBUG isn't defined, _LIBCPP_DEBUG_LEVEL is 0. This should probabl just be == 0, but it's not wrong.

[rupprecht]

In D118940#3295112, @Quuxplusone wrote:

As philnik said, please revert all the extraneous changes. (And please ignore clang-format's noise going forward. We'd disable clang-format entirely if we could. This is an ongoing pain point for libc++, over and over and over.)

IIUC, the only intentional functional change here is to change _LIBCPP_CONSTEXPR_AFTER_CXX17 into _LIBCPP_CONSTEXPR_AFTER_CXX11, is that right? That certainly seems like a good idea to me. (My mantra there is that internal details should always use the most aggressive constexpr possible. That way, if we ever drop support for C++03 and C++11 modes, the amount of stuff can be search-and-replaced to plain constexpr immediately is maximized.)

That's correct -- supporting this for C++17 was the only change I care about.

Contrariwise, your changes to _LIBCPP_DEBUG seem extraneous and/or wrong; I don't think you should touch anything in this PR that isn't related to the bug you're trying to fix. (And if we were going to touch anything in that area, I'd suggest that we remove the #ifs around __debug_less: I see no reason ever to if-out its class definition, even if it's sometimes unused.)

I lean towards not making unrelated changes in patches, but I wasn't sure how libc++ reviewers felt about this, so I made those changes after review comments. They're reverted now. How about I land just the part I care about in this commit, and do the rest in a followup?

• Replacing uses of _LIBCPP_DEBUG with checking _LIBCPP_DEBUG_LEVEL levels instead seems limited to just this file
• Replacing uses of _LIBCPP_INLINE_VISIBILITY with _LIBCPP_HIDE_FROM_ABI seems a lot larger (289 files affected, if I'm grepping correctly), so I'd prefer not to do the whole cleanup (although it's not complex -- it's just a sed command), but I could do it for this file.

libcxx/include/__algorithm/comp_ref_type.h

67

Yep, https://github.com/llvm/llvm-project/blob/c3c1c5c6953fcf9320a0cae5121ce55839169790/libcxx/include/__config#L911

[• Quuxplusone]

The patch looks good to me now... but we do have a buildkite runner that runs in Debug mode, and it didn't catch the thing you originally mentioned.
Can someone explain why libcxx/test/std/algorithms/alg.sorting/alg.min.max/min_init_list_comp.pass.cpp does not detect the problem? I paste it into Godbolt and it detects the problem: https://godbolt.org/z/eKvcvbb7d yet the "Debug mode" buildkite job isn't failing.

...Ooh. It's because the "Debug mode" buildkite job uses only C++20 or C++2b, isn't it? The combinatorial explosion of config options strikes again. :( @ldionne @philnik any thoughts on how to improve our CI coverage here?

[philnik]

In D118940#3295347, @Quuxplusone wrote:

The patch looks good to me now... but we do have a buildkite runner that runs in Debug mode, and it didn't catch the thing you originally mentioned.
Can someone explain why libcxx/test/std/algorithms/alg.sorting/alg.min.max/min_init_list_comp.pass.cpp does not detect the problem? I paste it into Godbolt and it detects the problem: https://godbolt.org/z/eKvcvbb7d yet the "Debug mode" buildkite job isn't failing.

...Ooh. It's because the "Debug mode" buildkite job uses only C++20 or C++2b, isn't it? The combinatorial explosion of config options strikes again. :( @ldionne @philnik any thoughts on how to improve our CI coverage here?

Yeah, exponential growth really sucks. A few things I thought of:
(1) Have an infinite amount of money to throw at CI. That would mean we have 0 bugs, but it might not be feasible.
(2) Ignore it and hope for the best
(3) It would still be expensive, but we could see how expensive it is to only compile all the tests, and therefore effectively only testing constexpr with debug
(4) Remove some configurations

I guess option 3 would be the best option, but it might be too costly. So it's most likely going to be option 2.

[rupprecht]

In D118940#3295359, @philnik wrote:

In D118940#3295347, @Quuxplusone wrote:

The patch looks good to me now... but we do have a buildkite runner that runs in Debug mode, and it didn't catch the thing you originally mentioned.
Can someone explain why libcxx/test/std/algorithms/alg.sorting/alg.min.max/min_init_list_comp.pass.cpp does not detect the problem? I paste it into Godbolt and it detects the problem: https://godbolt.org/z/eKvcvbb7d yet the "Debug mode" buildkite job isn't failing.

...Ooh. It's because the "Debug mode" buildkite job uses only C++20 or C++2b, isn't it? The combinatorial explosion of config options strikes again. :( @ldionne @philnik any thoughts on how to improve our CI coverage here?

Yeah, exponential growth really sucks. A few things I thought of:
(1) Have an infinite amount of money to throw at CI. That would mean we have 0 bugs, but it might not be feasible.
(2) Ignore it and hope for the best

Also seems the most reasonable IMHO. Breakages seem pretty rare, I don't know if it's worth a bunch of resources for a once-a-year bug.

(3) It would still be expensive, but we could see how expensive it is to only compile all the tests, and therefore effectively only testing constexpr with debug
(4) Remove some configurations

I guess option 3 would be the best option, but it might be too costly. So it's most likely going to be option 2.

[• Quuxplusone]

Ship it!

[ldionne]

LGTM, and I agree that it's probably not worth the cost to solve this specific issue in CI.

libcxx/include/__algorithm/comp_ref_type.h

 //===----------------------------------------------------------------------===//
 //
 // 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 _LIBCPP___ALGORITHM_COMP_REF_TYPE_H
 #define _LIBCPP___ALGORITHM_COMP_REF_TYPE_H
 
 #include <__config>
 
-#ifdef _LIBCPP_DEBUG
+#if _LIBCPP_DEBUG_LEVEL >= 1
 #  include <__debug>
 #  include <__utility/declval.h>
 #endif
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
-#ifdef _LIBCPP_DEBUG
+#if _LIBCPP_DEBUG_LEVEL >= 1
 
 template <class _Compare>
 struct __debug_less
 {
     _Compare &__comp_;
-    _LIBCPP_CONSTEXPR_AFTER_CXX17
+    _LIBCPP_CONSTEXPR_AFTER_CXX11
     __debug_less(_Compare& __c) : __comp_(__c) {}
 
     template <class _Tp, class _Up>
-    _LIBCPP_CONSTEXPR_AFTER_CXX17
-    bool operator()(const _Tp& __x,  const _Up& __y)
-    {
-        bool __r = __comp_(__x, __y);
-        if (__r)
-            __do_compare_assert(0, __y, __x);
-        return __r;
+    _LIBCPP_CONSTEXPR_AFTER_CXX11 bool operator()(const _Tp& __x, const _Up& __y) {
     }
 
     template <class _Tp, class _Up>
-    _LIBCPP_CONSTEXPR_AFTER_CXX17
-    bool operator()(_Tp& __x,  _Up& __y)
-    {
-        bool __r = __comp_(__x, __y);
-        if (__r)
-            __do_compare_assert(0, __y, __x);
-        return __r;
+    _LIBCPP_CONSTEXPR_AFTER_CXX11 bool operator()(_Tp& __x, _Up& __y) {
     }
 
     template <class _LHS, class _RHS>
-    _LIBCPP_CONSTEXPR_AFTER_CXX17
-    inline _LIBCPP_INLINE_VISIBILITY
-    decltype((void)declval<_Compare&>()(
-        declval<_LHS &>(), declval<_RHS &>()))
-    __do_compare_assert(int, _LHS & __l, _RHS & __r) {
-        _LIBCPP_ASSERT(!__comp_(__l, __r),
-            "Comparator does not induce a strict weak ordering");
+    _LIBCPP_CONSTEXPR_AFTER_CXX11 inline _LIBCPP_HIDE_FROM_ABI decltype((void)declval<_Compare&>()(declval<_LHS&>(),
+                                                                                                   declval<_RHS&>()))
+    __do_compare_assert(int, _LHS& __l, _RHS& __r) {
+      _LIBCPP_ASSERT(!__comp_(__l, __r), "Comparator does not induce a strict weak ordering");
     }
 
     template <class _LHS, class _RHS>
-    _LIBCPP_CONSTEXPR_AFTER_CXX17
-    inline _LIBCPP_INLINE_VISIBILITY
-    void __do_compare_assert(long, _LHS &, _RHS &) {}
+    _LIBCPP_CONSTEXPR_AFTER_CXX11 inline _LIBCPP_HIDE_FROM_ABI void __do_compare_assert(long, _LHS&, _RHS&) {}
 };
 
-#endif // _LIBCPP_DEBUG
+#endif // _LIBCPP_DEBUG_LEVEL >= 1
 
 template <class _Comp>
 struct __comp_ref_type {
   // Pass the comparator by lvalue reference. Or in debug mode, using a
   // debugging wrapper that stores a reference.
-#ifndef _LIBCPP_DEBUG
+#if _LIBCPP_DEBUG_LEVEL < 1
   typedef _Comp& type;
 #else
   typedef __debug_less<_Comp> type;
 #endif
 };
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___ALGORITHM_COMP_REF_TYPE_H

libcxx/test/libcxx/algorithms/debug_less.pass.cpp

void test_value_categories() {
     int lvalue = 42;
     const int const_lvalue = 101;
 
     assert(dl(lvalue, const_lvalue));
     assert(dl(/*rvalue*/1, lvalue));
     assert(dl(static_cast<int&&>(1), static_cast<const int&&>(2)));
 }
 
-#if TEST_STD_VER > 17
+#if TEST_STD_VER > 11
 constexpr bool test_constexpr() {
     std::less<> cmp{};
-    __debug_less<std::less<> > dcmp(cmp);
+    __debug_less<std::less<>> dcmp(cmp);
     assert(dcmp(1, 2));
     assert(!dcmp(1, 1));
     return true;
 }
 #endif
 
 int main(int, char**) {
     test_passing();
     test_failing();
     test_upper_and_lower_bound();
     test_non_const_arg_cmp();
     test_value_iterator();
     test_value_categories();
-#if TEST_STD_VER > 17
+#if TEST_STD_VER > 11
     static_assert(test_constexpr(), "");
 #endif
     return 0;
 }