[libc++] [LIBCXX-DEBUG-FIXME] Constexpr char_traits::copy mustn't compare unrelated pointers.

[• Quuxplusone]

Summary

Now that __builtin_is_constant_evaluated() is present on all supported
compilers, we can use it to skip the UB-inducing assert in cases where
the computation might be happening at constexpr time.

[• Quuxplusone]

poke buildkite

[ldionne]

We should be able to compare pointers with std::less to solve this problem. It won't work if you just start using std::less because our std::less is actually wrong, however that's another problem we need to solve. We shouldn't just remove this assertion and hide the problem. So:

1. Fix std::less so that it's not UB to use it to compare unrelated pointers (will require having some sort of special contract with the compiler that std::less can do that -- this could be done with some sort of builtin).
2. Fix the debug mode so that it allows comparing unrelated pointers with std::less (frankly I'm not sure what's the best way of doing that off the top of my head, but we can explore).
3. Use std::less to implement those pointer comparisons.

WDYT?

[• Quuxplusone]

We should be able to compare pointers with std::less to solve this problem. [...]
(will require having some sort of special contract with the compiler that std::less can do that -- this could be done with some sort of builtin)

I agree that that's what WG21 seems to want vendors to do. But that requires coordinating a new pointer-comparison builtin with both Clang and GCC, which is (definitely a good idea but) far above my pay grade. I'm happy to abandon this PR and let someone-not-me pursue the builtin angle...

Discussed offline with @ldionne and decided to sit on this for now, with the intent that someone will pursue adding a builtin at least to Clang.

[• Quuxplusone]

Now that __builtin_is_constant_evaluated() is present on all supported
compilers, we can use it to skip the UB-inducing assert in cases where
the computation might be happening at constexpr time.

@ldionne ping!

[Mordante]

Nice to see more tests working in debug mode. LGTM!

libcxx/include/__string

268–270

WDYT about adding a short comment. // comparing unrelated pointers is invalid in a constexpr function?

[• Quuxplusone]

libcxx/include/__string

268–270

Seems relatively harmless, but repetitive (do it in all 6 places, right?). And if anyone did wonder enough about why the if was there, they could always remove it and see what tests broke. So, I'm ambivalent, but can add the comment if someone else — @ldionne? — also thinks it's a good idea.
Nit: s/constexpr function/constant expression/ or maybe constexpr context (which is a phrase I use a lot but has no textual support in the standard AFAIK).

libcxx/include/__string

char_traits<_CharT>::move(char_type* __s1, const char_type* __s2, size_t __n)
     return __r;
 }
 
 template <class _CharT>
 inline _LIBCPP_CONSTEXPR_AFTER_CXX17
 _CharT*
 char_traits<_CharT>::copy(char_type* __s1, const char_type* __s2, size_t __n)
 {
+    if (!__libcpp_is_constant_evaluated()) {
         _LIBCPP_ASSERT(__s2 < __s1 || __s2 >= __s1+__n, "char_traits::copy overlapped range");
+    }
     char_type* __r = __s1;
     for (; __n; --__n, ++__s1, ++__s2)
         assign(*__s1, *__s2);
     return __r;
 }
 
 template <class _CharT>
 inline _LIBCPP_CONSTEXPR_AFTER_CXX17
char_type* move(char_type* __s1, const char_type* __s2, size_t __n) _NOEXCEPT
         {
             return __libcpp_is_constant_evaluated()
                        ? _VSTD::__move_constexpr(__s1, __s2, __n)
                        : __n == 0 ? __s1 : (char_type*)_VSTD::memmove(__s1, __s2, __n);
         }
     static inline _LIBCPP_CONSTEXPR_AFTER_CXX17
     char_type* copy(char_type* __s1, const char_type* __s2, size_t __n) _NOEXCEPT
         {
+            if (!__libcpp_is_constant_evaluated()) {
                 _LIBCPP_ASSERT(__s2 < __s1 || __s2 >= __s1+__n, "char_traits::copy overlapped range");
+            }
             return __libcpp_is_constant_evaluated()
                        ? _VSTD::__copy_constexpr(__s1, __s2, __n)
                        : __n == 0 ? __s1 : (char_type*)_VSTD::memcpy(__s1, __s2, __n);
         }
     static inline _LIBCPP_CONSTEXPR_AFTER_CXX17
     char_type* assign(char_type* __s, size_t __n, char_type __a) _NOEXCEPT
         {
             return __libcpp_is_constant_evaluated()
char_type* move(char_type* __s1, const char_type* __s2, size_t __n) _NOEXCEPT
         {
             return __libcpp_is_constant_evaluated()
                        ? _VSTD::__move_constexpr(__s1, __s2, __n)
                        : __n == 0 ? __s1 : _VSTD::wmemmove(__s1, __s2, __n);
         }
     static inline _LIBCPP_CONSTEXPR_AFTER_CXX17
     char_type* copy(char_type* __s1, const char_type* __s2, size_t __n) _NOEXCEPT
         {
+            if (!__libcpp_is_constant_evaluated()) {
                 _LIBCPP_ASSERT(__s2 < __s1 || __s2 >= __s1+__n, "char_traits::copy overlapped range");
+            }
             return __libcpp_is_constant_evaluated()
                        ? _VSTD::__copy_constexpr(__s1, __s2, __n)
                        : __n == 0 ? __s1 : _VSTD::wmemcpy(__s1, __s2, __n);
         }
     static inline _LIBCPP_CONSTEXPR_AFTER_CXX17
     char_type* assign(char_type* __s, size_t __n, char_type __a) _NOEXCEPT
         {
             return __libcpp_is_constant_evaluated()
char_type*       move(char_type* __s1, const char_type* __s2, size_t __n) _NOEXCEPT
         {
             return __libcpp_is_constant_evaluated()
                        ? _VSTD::__move_constexpr(__s1, __s2, __n)
                        : __n == 0 ? __s1 : (char_type*)_VSTD::memmove(__s1, __s2, __n);
         }
 
     static _LIBCPP_CONSTEXPR_AFTER_CXX17
     char_type*       copy(char_type* __s1, const char_type* __s2, size_t __n) _NOEXCEPT
         {
+            if (!__libcpp_is_constant_evaluated()) {
                 _LIBCPP_ASSERT(__s2 < __s1 || __s2 >= __s1+__n, "char_traits::copy overlapped range");
+            }
             return __libcpp_is_constant_evaluated()
                        ? _VSTD::__copy_constexpr(__s1, __s2, __n)
                        : __n == 0 ? __s1 : (char_type*)_VSTD::memcpy(__s1, __s2, __n);
         }
 
     static _LIBCPP_CONSTEXPR_AFTER_CXX17
     char_type*       assign(char_type* __s, size_t __n, char_type __a) _NOEXCEPT
         {
char_traits<char16_t>::move(char_type* __s1, const char_type* __s2, size_t __n) _NOEXCEPT
     }
     return __r;
 }
 
 inline _LIBCPP_CONSTEXPR_AFTER_CXX17
 char16_t*
 char_traits<char16_t>::copy(char_type* __s1, const char_type* __s2, size_t __n) _NOEXCEPT
 {
+    if (!__libcpp_is_constant_evaluated()) {
         _LIBCPP_ASSERT(__s2 < __s1 || __s2 >= __s1+__n, "char_traits::copy overlapped range");
+    }
     char_type* __r = __s1;
     for (; __n; --__n, ++__s1, ++__s2)
         assign(*__s1, *__s2);
     return __r;
 }
 
 inline _LIBCPP_CONSTEXPR_AFTER_CXX17
 char16_t*
char_traits<char32_t>::move(char_type* __s1, const char_type* __s2, size_t __n) _NOEXCEPT
     }
     return __r;
 }
 
 inline _LIBCPP_CONSTEXPR_AFTER_CXX17
 char32_t*
 char_traits<char32_t>::copy(char_type* __s1, const char_type* __s2, size_t __n) _NOEXCEPT
 {
+    if (!__libcpp_is_constant_evaluated()) {
         _LIBCPP_ASSERT(__s2 < __s1 || __s2 >= __s1+__n, "char_traits::copy overlapped range");
+    }
     char_type* __r = __s1;
     for (; __n; --__n, ++__s1, ++__s2)
         assign(*__s1, *__s2);
     return __r;
 }
 
 inline _LIBCPP_CONSTEXPR_AFTER_CXX17
 char32_t*

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char/copy.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: LIBCXX-DEBUG-FIXME
-
 // <string>
 
 // template<> struct char_traits<char>
 
 // static char_type* copy(char_type* s1, const char_type* s2, size_t n);
 
 #include <string>
 #include <cassert>

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char16_t/copy.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: LIBCXX-DEBUG-FIXME
-
 // <string>
 
 // template<> struct char_traits<char16_t>
 
 // static char_type* copy(char_type* s1, const char_type* s2, size_t n);
 
 #include <string>
 #include <cassert>

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char32_t/copy.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: LIBCXX-DEBUG-FIXME
-
 // <string>
 
 // template<> struct char_traits<char32_t>
 
 // static char_type* copy(char_type* s1, const char_type* s2, size_t n);
 
 #include <string>
 #include <cassert>

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char8_t/copy.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
-// UNSUPPORTED: LIBCXX-DEBUG-FIXME
 
 // <string>
 
 // template<> struct char_traits<char8_t>
 
 // static char_type* copy(char_type* s1, const char_type* s2, size_t n);
 
 #include <string>

libcxx/test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.wchar.t/copy.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: LIBCXX-DEBUG-FIXME
-
 // <string>
 
 // template<> struct char_traits<wchar_t>
 
 // static char_type* copy(char_type* s1, const char_type* s2, size_t n);
 
 #include <string>
 #include <cassert>

libcxx/test/std/strings/string.view/string.view.ops/copy.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
 //
 //===----------------------------------------------------------------------===//
 
 // GCC's __builtin_strlen isn't constexpr yet
 // XFAIL: gcc-11 && !(c++11 || c++14 || c++17)
-// UNSUPPORTED: LIBCXX-DEBUG-FIXME
 
 // <string_view>
 
 // size_type copy(charT* s, size_type n, size_type pos = 0) const;
 
 // Throws: out_of_range if pos > size().
 // Remarks: Let rlen be the smaller of n and size() - pos.
 // Requires: [s, s+rlen) is a valid range.