Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++][test] Fixes constexpr char_traits. #90981

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.

libcxx/test/support/constexpr_char_traits.h Outdated Show resolved Hide resolved
libcxx/test/support/constexpr_char_traits.h Outdated Show resolved Hide resolved
libcxx/test/support/constexpr_char_traits.h Outdated Show resolved Hide resolved
libcxx/test/support/constexpr_char_traits.h Outdated Show resolved Hide resolved
libcxx/test/support/constexpr_char_traits.h Outdated Show resolved Hide resolved
libcxx/test/support/nasty_string.h Outdated Show resolved Hide resolved
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
52 checks passed
@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