[clang][bytecode] Fix builtin_memcmp buffer sizes for pointers

[tbaederr]

Don't use the pointer size, but the number of elements multiplied by the element size.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Don't use the pointer size, but the number of elements multiplied by the element size.

---

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

2 Files Affected:

• (modified) clang/lib/AST/ByteCode/InterpBuiltin.cpp (+6-4)
• (added) clang/test/AST/ByteCode/libcxx/memcmp-pointer.cpp (+120)

diff --git a/clang/lib/AST/ByteCode/InterpBuiltin.cpp b/clang/lib/AST/ByteCode/InterpBuiltin.cpp
index 3e8a9a77d26bf..4660c80fc90db 100644
--- a/clang/lib/AST/ByteCode/InterpBuiltin.cpp
+++ b/clang/lib/AST/ByteCode/InterpBuiltin.cpp
@@ -1892,10 +1892,12 @@ static bool interp__builtin_memcmp(InterpState &S, CodePtr OpPC,
   };
 
   const ASTContext &ASTCtx = S.getASTContext();
+  QualType ElemTypeA = getElemType(PtrA);
+  QualType ElemTypeB = getElemType(PtrB);
   // FIXME: This is an arbitrary limitation the current constant interpreter
   // had. We could remove this.
-  if (!IsWide && (!isOneByteCharacterType(getElemType(PtrA)) ||
-                  !isOneByteCharacterType(getElemType(PtrB)))) {
+  if (!IsWide && (!isOneByteCharacterType(ElemTypeA) ||
+                  !isOneByteCharacterType(ElemTypeB))) {
     S.FFDiag(S.Current->getSource(OpPC),
              diag::note_constexpr_memcmp_unsupported)
         << ASTCtx.BuiltinInfo.getQuotedName(ID) << PtrA.getType()
@@ -1908,7 +1910,7 @@ static bool interp__builtin_memcmp(InterpState &S, CodePtr OpPC,
 
   // Now, read both pointers to a buffer and compare those.
   BitcastBuffer BufferA(
-      Bits(ASTCtx.getTypeSize(PtrA.getFieldDesc()->getType())));
+      Bits(ASTCtx.getTypeSize(ElemTypeA) * PtrA.getNumElems()));
   readPointerToBuffer(S.getContext(), PtrA, BufferA, false);
   // FIXME: The swapping here is UNDOING something we do when reading the
   // data into the buffer.
@@ -1916,7 +1918,7 @@ static bool interp__builtin_memcmp(InterpState &S, CodePtr OpPC,
     swapBytes(BufferA.Data.get(), BufferA.byteSize().getQuantity());
 
   BitcastBuffer BufferB(
-      Bits(ASTCtx.getTypeSize(PtrB.getFieldDesc()->getType())));
+      Bits(ASTCtx.getTypeSize(ElemTypeB) * PtrB.getNumElems()));
   readPointerToBuffer(S.getContext(), PtrB, BufferB, false);
   // FIXME: The swapping here is UNDOING something we do when reading the
   // data into the buffer.
diff --git a/clang/test/AST/ByteCode/libcxx/memcmp-pointer.cpp b/clang/test/AST/ByteCode/libcxx/memcmp-pointer.cpp
new file mode 100644
index 0000000000000..8ffe0e077ece9
--- /dev/null
+++ b/clang/test/AST/ByteCode/libcxx/memcmp-pointer.cpp
@@ -0,0 +1,120 @@
+// RUN: %clang_cc1 -std=c++2c -fexperimental-new-constant-interpreter -verify=expected,both %s
+// RUN: %clang_cc1 -std=c++2c  -verify=ref,both %s
+
+// both-no-diagnostics
+
+namespace std {
+inline namespace {
+template <int __v> struct integral_constant {
+  static const int value = __v;
+};
+template <bool, class> using __enable_if_t = int;
+char *addressof(char &);
+struct pointer_traits {
+  template <class _Up> using rebind = _Up *;
+};
+} // namespace
+} // namespace std
+void *operator new(decltype(sizeof(int)), void *);
+namespace std {
+inline namespace {
+template <class _Tp> using __make_unsigned_t = __make_unsigned(_Tp);
+template <class _Default, template <class> class>
+using __detected_or_t = _Default;
+template <class _Tp> using __pointer_member = _Tp;
+template <class _Tp, class>
+using __pointer = __detected_or_t<_Tp *, __pointer_member>;
+template <class _Tp> using __size_type_member = _Tp;
+template <class, class _DiffType>
+using __size_type =
+    __detected_or_t<__make_unsigned_t<_DiffType>, __size_type_member>;
+struct allocation_result {
+  char *ptr;
+  unsigned long count;
+};
+template <class _Alloc> struct allocator_traits {
+  using allocator_type = _Alloc;
+  using pointer =
+      __pointer<typename allocator_type::value_type, allocator_type>;
+  using const_pointer = pointer_traits::rebind<char>;
+  using size_type =
+      __size_type<allocator_type, decltype(static_cast<int *>(nullptr) -
+                                           static_cast<int *>(nullptr))>;
+  template <class _Ap>
+  static constexpr allocation_result allocate_at_least(_Ap __alloc,
+                                                       size_type __n) {
+    return {__alloc.allocate(__n), __n};
+  }
+};
+template <class _Alloc>
+constexpr auto __allocate_at_least(_Alloc __alloc, decltype(sizeof(int)) __n) {
+  return allocator_traits<_Alloc>::allocate_at_least(__alloc, __n);
+}
+template <class> struct allocator {
+  typedef char value_type;
+  constexpr char *allocate(decltype(sizeof(int)) __n) {
+    return static_cast<char *>(operator new(__n));
+  }
+  constexpr void deallocate(char *__p) { operator delete(__p); }
+};
+struct __long {
+  allocator_traits<allocator<char>>::size_type __is_long_;
+  allocator_traits<allocator<char>>::size_type __size_;
+  allocator_traits<allocator<char>>::pointer __data_;
+};
+allocator<char> __alloc_;
+struct basic_string {
+  __long __l;
+  constexpr basic_string(basic_string &__str) {
+    allocator_traits<allocator<char>>::size_type __trans_tmp_1 =
+        __str.__get_long_size();
+    auto __allocation = __allocate_at_least(__alloc_, __trans_tmp_1);
+    for (allocator_traits<allocator<char>>::size_type __i = 0;
+         __i != __allocation.count; ++__i) {
+      char *__trans_tmp_9 = addressof(__allocation.ptr[__i]);
+      new (__trans_tmp_9) char();
+    }
+    __l.__data_ = __allocation.ptr;
+    __l.__is_long_ = __l.__size_ = __trans_tmp_1;
+  }
+  template <__enable_if_t<integral_constant<false>::value, int> = 0>
+  constexpr basic_string(const char *__s, allocator<char>) {
+    decltype(sizeof(int)) __trans_tmp_11, __i = 0;
+    for (; __s[__i]; ++__i)
+      __trans_tmp_11 = __i;
+    auto __allocation = __allocate_at_least(__alloc_, 1);
+    __l.__data_ = __allocation.ptr;
+    __l.__size_ = __trans_tmp_11;
+  }
+  constexpr ~basic_string() {
+    allocator<char> __a;
+    __a.deallocate(__l.__data_);
+  }
+  constexpr allocator_traits<allocator<char>>::size_type size() {
+    return __l.__is_long_;
+  }
+  constexpr char *data() {
+    allocator_traits<allocator<char>>::const_pointer __trans_tmp_6 =
+        __l.__is_long_ ? __l.__data_ : 0;
+    return __trans_tmp_6;
+  }
+  constexpr allocator_traits<allocator<char>>::size_type __get_long_size() {
+    return __l.__size_;
+  }
+};
+constexpr void operator==(basic_string __lhs, basic_string __rhs) {
+  decltype(sizeof(int)) __lhs_sz = __lhs.size();
+  char *__trans_tmp_10 = __rhs.data(), *__trans_tmp_15 = __lhs.data();
+  __builtin_memcmp(__trans_tmp_15, __trans_tmp_10, __lhs_sz);
+}
+} // namespace
+} // namespace std
+constexpr void test(std::basic_string s0) {
+  std::basic_string s1 = s0, s2(s0);
+  s2 == s1;
+}
+constexpr bool test() {
+  test(std::basic_string("2345678901234567890", std::allocator<char>()));
+  return true;
+}
+static_assert(test());

[philnik777]

What is this funny construct?

[tbaederr]

Just a reduced test case. Anything special about this line?

[philnik777]

I'm just wondering what the meaning of an inline anonymous namespace is.

[tbaederr]

Oh, no meaning at all, I think cvise even removed the inline and I added it back so the compiler wouldn't complain.