Fix incorrect assertion when deallocating big block
authorJonathan Wakely <jwakely@redhat.com>
Tue, 13 Nov 2018 22:57:48 +0000 (22:57 +0000)
committerJonathan Wakely <redi@gcc.gnu.org>
Tue, 13 Nov 2018 22:57:48 +0000 (22:57 +0000)
Since a big_block rounds up the size to a multiple of big_block::min it
is wrong to assert that the supplied number of bytes equals the
big_block's size(). Add big_block::alloc_size(size_t) to calculate the
allocated size consistently, and add comments to the code.

* src/c++17/memory_resource.cc (big_block): Improve comments.
(big_block::all_ones): Remove.
(big_block::big_block(size_t, size_t)): Use alloc_size.
(big_block::size()): Add comment, replace all_ones with equivalent
expression.
(big_block::align()): Shift value of correct type.
(big_block::alloc_size(size_t)): New function to round up size.
(__pool_resource::allocate(size_t, size_t)): Add comment.
(__pool_resource::deallocate(void*, size_t, size_t)): Likewise. Fix
incorrect assertion by using big_block::alloc_size(size_t).
* testsuite/20_util/unsynchronized_pool_resource/allocate.cc: Add
more tests for unpooled allocations.

From-SVN: r266088

libstdc++-v3/ChangeLog
libstdc++-v3/src/c++17/memory_resource.cc
libstdc++-v3/testsuite/20_util/unsynchronized_pool_resource/allocate.cc

index 973470dfb76e35f16931e5ace06070663a2bfd3d..9b6a72d3104080112e0ef51b4aba05de0f998b5d 100644 (file)
@@ -1,5 +1,18 @@
 2018-11-13  Jonathan Wakely  <jwakely@redhat.com>
 
+       * src/c++17/memory_resource.cc (big_block): Improve comments.
+       (big_block::all_ones): Remove.
+       (big_block::big_block(size_t, size_t)): Use alloc_size.
+       (big_block::size()): Add comment, replace all_ones with equivalent
+       expression.
+       (big_block::align()): Shift value of correct type.
+       (big_block::alloc_size(size_t)): New function to round up size.
+       (__pool_resource::allocate(size_t, size_t)): Add comment.
+       (__pool_resource::deallocate(void*, size_t, size_t)): Likewise. Fix
+       incorrect assertion by using big_block::alloc_size(size_t).
+       * testsuite/20_util/unsynchronized_pool_resource/allocate.cc: Add
+       more tests for unpooled allocations.
+
        * src/c++17/memory_resource.cc (bitset::full()): Handle edge case
        for _M_next_word maximum value.
        (bitset::get_first_unset(), bitset::set(size_type)): Use
index fdbbc914f2e21284b56cf7a90061199715093e9e..719cb9f1d29327131f96a0ab7d870b8424f534b4 100644 (file)
@@ -537,22 +537,22 @@ namespace pmr
   // An oversized allocation that doesn't fit in a pool.
   struct big_block
   {
+    // Alignment must be a power-of-two so we only need to use enough bits
+    // to store the power, not the actual value:
     static constexpr unsigned _S_alignbits
-      = std::__log2p1((unsigned)numeric_limits<size_t>::digits) - 1;
+      = std::__log2p1((unsigned)numeric_limits<size_t>::digits - 1);
+    // Use the remaining bits to store the size:
     static constexpr unsigned _S_sizebits
       = numeric_limits<size_t>::digits - _S_alignbits;
     // The maximum value that can be stored in _S_size
-    static constexpr size_t all_ones = (1ull << _S_sizebits) - 1u;
-    // The minimum size of a big block
+    static constexpr size_t all_ones = size_t(-1) >> _S_alignbits;
+    // The minimum size of a big block (smaller sizes will be rounded up).
     static constexpr size_t min = 1u << _S_alignbits;
 
     big_block(size_t bytes, size_t alignment)
-    : _M_size((bytes + min - 1u) >> _S_alignbits),
+    : _M_size(alloc_size(bytes) >> _S_alignbits),
       _M_align_exp(std::__log2p1(alignment) - 1u)
-    {
-      if (__builtin_expect(std::__countl_one(bytes) == _S_sizebits, false))
-       _M_size = all_ones;
-    }
+    { }
 
     void* pointer = nullptr;
     size_t _M_size : numeric_limits<size_t>::digits - _S_alignbits;
@@ -560,13 +560,26 @@ namespace pmr
 
     size_t size() const noexcept
     {
-      if (__builtin_expect(_M_size == all_ones, false))
+      // If all bits are set in _M_size it means the maximum possible size:
+      if (__builtin_expect(_M_size == (size_t(-1) >> _S_alignbits), false))
        return (size_t)-1;
       else
        return _M_size << _S_alignbits;
     }
 
-    size_t align() const noexcept { return 1ul << _M_align_exp; }
+    size_t align() const noexcept { return size_t(1) << _M_align_exp; }
+
+    // Calculate size to be allocated instead of requested number of bytes.
+    // The requested value will be rounded up to a multiple of big_block::min,
+    // so the low _S_alignbits bits are all zero and don't need to be stored.
+    static constexpr size_t alloc_size(size_t bytes) noexcept
+    {
+      const size_t s = bytes + min - 1u;
+      if (__builtin_expect(s < bytes, false))
+       return size_t(-1); // addition wrapped past zero, return max value
+      else
+       return s & ~(min - 1u);
+    }
 
     friend bool operator<(void* p, const big_block& b) noexcept
     { return less<void*>{}(p, b.pointer); }
@@ -915,6 +928,7 @@ namespace pmr
   {
     auto& b = _M_unpooled.emplace_back(bytes, alignment);
     __try {
+      // N.B. need to allocate b.size(), which might be larger than bytes.
       void* p = resource()->allocate(b.size(), alignment);
       b.pointer = p;
       if (_M_unpooled.size() > 1)
@@ -932,8 +946,7 @@ namespace pmr
   }
 
   void
-  __pool_resource::deallocate(void* p, size_t bytes [[maybe_unused]],
-                             size_t alignment [[maybe_unused]])
+  __pool_resource::deallocate(void* p, size_t bytes, size_t alignment)
   {
     const auto it
       = std::lower_bound(_M_unpooled.begin(), _M_unpooled.end(), p);
@@ -941,9 +954,10 @@ namespace pmr
     if (it != _M_unpooled.end() && it->pointer == p) // [[likely]]
       {
        const auto b = *it;
-       __glibcxx_assert(b.size() == bytes);
+       __glibcxx_assert(b.size() == b.alloc_size(bytes));
        __glibcxx_assert(b.align() == alignment);
        _M_unpooled.erase(it);
+       // N.B. need to deallocate b.size(), which might be larger than bytes.
        resource()->deallocate(p, b.size(), b.align());
       }
   }
index ce9be2f6c49a4cb7b8406d30c28d1af0be44baa1..749655b63c75de60f596601ef66b144065f21825 100644 (file)
@@ -128,7 +128,8 @@ test03()
 void
 test04()
 {
-  std::pmr::unsynchronized_pool_resource r({256, 256});
+  __gnu_test::memory_resource test_mr;
+  std::pmr::unsynchronized_pool_resource r({256, 256}, &test_mr);
   // Check alignment
   void* p1 = r.allocate(2, 64);
   VERIFY( (std::uintptr_t)p1 % 64 == 0 );
@@ -145,6 +146,95 @@ test04()
   r.deallocate(p4, 2 * largest_pool, 1024);
 }
 
+void
+test05()
+{
+  __gnu_test::memory_resource test_mr;
+  std::pmr::pool_options opts{};
+  opts.max_blocks_per_chunk = 1;
+  opts.largest_required_pool_block = 1;
+  std::pmr::unsynchronized_pool_resource r(opts, &test_mr);
+  opts = r.options();
+  // Test unpooled allocations
+  void** p = new void*[opts.largest_required_pool_block];
+  for (unsigned a : {64, 128, 256, 512})
+  {
+    for (unsigned i = 0; i < opts.largest_required_pool_block; ++i)
+      p[i] = r.allocate(i, a);
+    for (unsigned i = 0; i < opts.largest_required_pool_block; ++i)
+      r.deallocate(p[i], i, a);
+  }
+  delete[] p;
+}
+
+void
+test06()
+{
+  struct custom_mr : std::pmr::memory_resource
+  {
+    size_t expected_size = 0;
+    size_t expected_alignment = 0;
+
+    struct bad_size { };
+    struct bad_alignment { };
+
+    void* do_allocate(std::size_t b, std::size_t a)
+    {
+      if (expected_size != 0)
+      {
+       if (b < expected_size)
+         throw bad_size();
+       else if (a != expected_alignment)
+         throw bad_alignment();
+       // Else just throw, don't try to allocate:
+       throw std::bad_alloc();
+      }
+
+      return std::pmr::new_delete_resource()->allocate(b, a);
+    }
+
+    void do_deallocate(void* p, std::size_t b, std::size_t a)
+    { std::pmr::new_delete_resource()->deallocate(p, b, a); }
+
+    bool do_is_equal(const memory_resource& r) const noexcept
+    { return false; }
+  };
+
+  custom_mr c;
+  std::pmr::unsynchronized_pool_resource r({1, 1}, &c);
+  std::pmr::pool_options opts = r.options();
+  const std::size_t largest_pool = opts.largest_required_pool_block;
+  const std::size_t large_alignment = 1024;
+  // Ensure allocations won't fit in pools:
+  VERIFY( largest_pool < large_alignment );
+
+  // Ensure the vector of large allocations has some capacity
+  // and won't need to reallocate:
+  r.deallocate(r.allocate(largest_pool + 1, 1), largest_pool + 1, 1);
+
+  // Try allocating various very large sizes and ensure the size requested
+  // from the upstream allocator is at least as large as needed.
+  for (int i = 1; i < 64; ++i)
+  {
+    for (auto b : { -1, 0, 1, 3 })
+    {
+      std::size_t bytes = std::size_t(1) << i;
+      bytes += b;
+      c.expected_size = bytes;
+      c.expected_alignment = large_alignment;
+      try {
+       (void) r.allocate(bytes, large_alignment);
+      } catch (const std::bad_alloc&) {
+       // expect to catch bad_alloc
+      } catch (custom_mr::bad_size) {
+       VERIFY(false);
+      } catch (custom_mr::bad_alignment) {
+       VERIFY(false);
+      }
+    }
+  }
+}
+
 int
 main()
 {
@@ -152,4 +242,6 @@ main()
   test02();
   test03();
   test04();
+  test05();
+  test06();
 }