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++][string] Add regression test for sized new/delete bug #110210

Conversation

vitalybuka
Copy link
Collaborator

@vitalybuka vitalybuka commented Sep 27, 2024

This is regression test for #90292.

Allocator used in test is very similar to test_allocator.
However, reproducer requires size_type of the string
to be 64bit, but test_allocator uses 32bit.

32bit size_type makes sizeof(string::__long) to be 16,
but the alignment issue fixed with #90292 is only triggered
with default sizeof(string::__long) which is 24.

Fixes #92128.

Created using spr 1.3.4
@vitalybuka vitalybuka requested a review from a team as a code owner September 27, 2024 06:09
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 27, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 27, 2024

@llvm/pr-subscribers-libcxx

Author: Vitaly Buka (vitalybuka)

Changes

This is regression test for #90292.

Allocator used in test is very similar to
test_allocator. However reproducer requires
size_type of the string to be 64bit, and
test_allocator has 32bit.

Fixes #92128.


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

1 Files Affected:

  • (added) libcxx/test/std/strings/basic.string/string.capacity/deallocate_size.pass.cpp (+63)
diff --git a/libcxx/test/std/strings/basic.string/string.capacity/deallocate_size.pass.cpp b/libcxx/test/std/strings/basic.string/string.capacity/deallocate_size.pass.cpp
new file mode 100644
index 00000000000000..612b66195141c5
--- /dev/null
+++ b/libcxx/test/std/strings/basic.string/string.capacity/deallocate_size.pass.cpp
@@ -0,0 +1,63 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <string>
+
+// allocate/deallocate must match.
+
+#include <string>
+#include <cassert>
+#include <type_traits>
+
+#include "test_macros.h"
+
+static int allocated_;
+
+template <class T, class Sz>
+struct test_alloc {
+  typedef Sz size_type;
+  typedef std::make_signed<Sz>::type difference_type;
+  typedef T value_type;
+  typedef value_type* pointer;
+  typedef const value_type* const_pointer;
+  typedef typename std::add_lvalue_reference<value_type>::type reference;
+  typedef typename std::add_lvalue_reference<const value_type>::type const_reference;
+
+  template <class U>
+  struct rebind {
+    typedef test_alloc<U, Sz> other;
+  };
+
+  TEST_CONSTEXPR_CXX14 pointer allocate(size_type n, const void* = nullptr) {
+    allocated_ += n;
+    return std::allocator<value_type>().allocate(n);
+  }
+
+  TEST_CONSTEXPR_CXX14 void deallocate(pointer p, size_type s) {
+    allocated_ -= s;
+    std::allocator<value_type>().deallocate(p, s);
+  }
+};
+
+template <class Sz>
+void test() {
+  for (int i = 1; i < 1000; ++i) {
+    using Str = std::basic_string<char, std::char_traits<char>, test_alloc<char, Sz> >;
+    {
+      Str s(i, 't');
+      assert(allocated_ == 0 || allocated_ >= i);
+    }
+  }
+  assert(allocated_ == 0);
+}
+
+int main(int, char**) {
+  test<uint32_t>();
+  test<uint64_t>();
+  test<size_t>();
+}

Created using spr 1.3.4
Created using spr 1.3.4
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 with nitpicks, thanks!

vitalybuka and others added 2 commits September 30, 2024 10:43
…e_size.pass.cpp

Co-authored-by: Louis Dionne <ldionne.2@gmail.com>
…e_size.pass.cpp

Co-authored-by: Louis Dionne <ldionne.2@gmail.com>
Copy link

github-actions bot commented Sep 30, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@vitalybuka vitalybuka merged commit eea5e7e into main Oct 1, 2024
64 checks passed
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/libcstring-add-regression-test-for-sized-newdelete-bug branch October 1, 2024 05:29
VitaNuo pushed a commit to VitaNuo/llvm-project that referenced this pull request Oct 2, 2024
…10210)

This is regression test for llvm#90292.

Allocator used in test is very similar to test_allocator.
However, reproducer requires size_type of the string
to be 64bit, but test_allocator uses 32bit.

32bit size_type makes `sizeof(string::__long)` to be 16,
but the alignment issue fixed with llvm#90292 is only triggered
with default `sizeof(string::__long)` which is 24.

Fixes llvm#92128.

---------

Co-authored-by: Louis Dionne <ldionne.2@gmail.com>
VitaNuo pushed a commit to VitaNuo/llvm-project that referenced this pull request Oct 2, 2024
…10210)

This is regression test for llvm#90292.

Allocator used in test is very similar to test_allocator.
However, reproducer requires size_type of the string
to be 64bit, but test_allocator uses 32bit.

32bit size_type makes `sizeof(string::__long)` to be 16,
but the alignment issue fixed with llvm#90292 is only triggered
with default `sizeof(string::__long)` which is 24.

Fixes llvm#92128.

---------

Co-authored-by: Louis Dionne <ldionne.2@gmail.com>
Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Oct 3, 2024
…10210)

This is regression test for llvm#90292.

Allocator used in test is very similar to test_allocator.
However, reproducer requires size_type of the string
to be 64bit, but test_allocator uses 32bit.

32bit size_type makes `sizeof(string::__long)` to be 16,
but the alignment issue fixed with llvm#90292 is only triggered
with default `sizeof(string::__long)` which is 24.

Fixes llvm#92128.

---------

Co-authored-by: Louis Dionne <ldionne.2@gmail.com>
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
…10210)

This is regression test for llvm#90292.

Allocator used in test is very similar to test_allocator.
However, reproducer requires size_type of the string
to be 64bit, but test_allocator uses 32bit.

32bit size_type makes `sizeof(string::__long)` to be 16,
but the alignment issue fixed with llvm#90292 is only triggered
with default `sizeof(string::__long)` which is 24.

Fixes llvm#92128.

---------

Co-authored-by: Louis Dionne <ldionne.2@gmail.com>
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.

Add tests for #90292
3 participants