Skip to content

Conversation

mordante
Copy link
Member

@mordante mordante commented May 3, 2024

The issue in nasty_char_traits was discovered by @StephanTLavavej who provided
the solution they use in MSVC STL. This solution is based on that example.

The same issue affects the constexpr_char_traits which was discovered in
#88389. This uses the same fix.

Fixes: #74221

@mordante mordante requested a review from a team as a code owner May 3, 2024 16:38
assign(*--s1, *--s2);
}
return r;
TEST_CONSTEXPR_CXX14 CharT* constexpr_char_traits<CharT>::move(char_type* s1, const char_type* s2, std::size_t n) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also add constexpr_char_traits to the PR title?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like short titles, so I made the title more generic and updated the commit message.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM w/ some comments.

This was discovered by @StephanTLavavej who provided the solution they use
in MSVC STL. This solution is based on that example.

The same issue affects the constexpr_char_traits which was discovered in
llvm#88389. This uses the same fix.

Fixes: llvm#74221
@mordante mordante force-pushed the review/fixes_constexpr_test_char_traits_move_operations branch from b791541 to da2f1ed Compare May 8, 2024 18:47
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label May 8, 2024
@llvmbot
Copy link
Member

llvmbot commented May 8, 2024

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

This was discovered by @StephanTLavavej who provided the solution they use in MSVC STL. This solution is based on that example.

The same issue affects the constexpr_char_traits which was discovered in #88389. This uses the same fix.

Fixes: #74221


Full diff: https://github.com/llvm/llvm-project/pull/90981.diff

2 Files Affected:

  • (modified) libcxx/test/support/constexpr_char_traits.h (+40-17)
  • (modified) libcxx/test/support/nasty_string.h (+8-4)
diff --git a/libcxx/test/support/constexpr_char_traits.h b/libcxx/test/support/constexpr_char_traits.h
index 75380d5a7ffbb..7c487c504af13 100644
--- a/libcxx/test/support/constexpr_char_traits.h
+++ b/libcxx/test/support/constexpr_char_traits.h
@@ -16,6 +16,31 @@
 
 #include "test_macros.h"
 
+// Tests whether the pointer p is in the range [first, last).
+//
+// Precondition: The range [first, last) is a valid range.
+//
+// Typically the pointers are compared with less than. This is not allowed when
+// the pointers belong to different ranges, which is UB. Typically, this is
+// benign at run-time, however since UB is not allowed during constant
+// evaluation this does not compile. This function does the validation without
+// UB.
+//
+// When p is in the range [first, last) the data can be copied from the
+// beginning to the end. Otherwise it needs to be copied from the end to the
+// beginning.
+template <class CharT>
+TEST_CONSTEXPR_CXX14 bool is_pointer_in_range(const CharT* first, const CharT* last, const CharT* p) {
+  if (first == p) // Needed when n == 0
+    return true;
+
+  for (; first != last; ++first)
+    if (first == p)
+      return true;
+
+  return false;
+}
+
 template <class CharT>
 struct constexpr_char_traits
 {
@@ -98,23 +123,21 @@ constexpr_char_traits<CharT>::find(const char_type* s, std::size_t n, const char
 }
 
 template <class CharT>
-TEST_CONSTEXPR_CXX14 CharT*
-constexpr_char_traits<CharT>::move(char_type* s1, const char_type* s2, std::size_t n)
-{
-    char_type* r = s1;
-    if (s1 < s2)
-    {
-        for (; n; --n, ++s1, ++s2)
-            assign(*s1, *s2);
-    }
-    else if (s2 < s1)
-    {
-        s1 += n;
-        s2 += n;
-        for (; n; --n)
-            assign(*--s1, *--s2);
-    }
-    return r;
+TEST_CONSTEXPR_CXX14 CharT* constexpr_char_traits<CharT>::move(char_type* s1, const char_type* s2, std::size_t n) {
+  if (s1 == s2)
+    return s1;
+
+  char_type* r = s1;
+  if (is_pointer_in_range(s1, s1 + n, s2)) {
+    for (; n; --n)
+      assign(*s1++, *s2++);
+  } else {
+    s1 += n;
+    s2 += n;
+    for (; n; --n)
+      assign(*--s1, *--s2);
+  }
+  return r;
 }
 
 template <class CharT>
diff --git a/libcxx/test/support/nasty_string.h b/libcxx/test/support/nasty_string.h
index 672c3cb4ed9ea..ea9d83ccf282a 100644
--- a/libcxx/test/support/nasty_string.h
+++ b/libcxx/test/support/nasty_string.h
@@ -16,6 +16,7 @@
 
 #include "make_string.h"
 #include "test_macros.h"
+#include "constexpr_char_traits.h" // is_pointer_in_range
 
 // This defines a nasty_string similar to nasty_containers. This string's
 // value_type does operator hijacking, which allows us to ensure that the
@@ -118,11 +119,14 @@ constexpr const nasty_char* nasty_char_traits::find(const nasty_char* s, std::si
 }
 
 constexpr nasty_char* nasty_char_traits::move(nasty_char* s1, const nasty_char* s2, std::size_t n) {
+  if (s1 == s2)
+    return s1;
+
   nasty_char* r = s1;
-  if (s1 < s2) {
-    for (; n; --n, ++s1, ++s2)
-      assign(*s1, *s2);
-  } else if (s2 < s1) {
+  if (is_pointer_in_range(s1, s1 + n, s2)) {
+    for (; n; --n)
+      assign(*s1++, *s2++);
+  } else {
     s1 += n;
     s2 += n;
     for (; n; --n)

@mordante mordante changed the title [libc++][test] Fixes constexpr nasty_char_traits. [libc++][test] Fixes constexpr char_traits. May 9, 2024
@mordante mordante merged commit 937643b into llvm:main May 9, 2024
@mordante mordante deleted the review/fixes_constexpr_test_char_traits_move_operations branch May 9, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[libc++][test] nasty_char_traits::move is incompatible with constexpr
5 participants