From 3a721c32cd42f507bfc34e5b5492e375b137b13a Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Sun, 25 Sep 2016 17:45:02 -0700 Subject: [PATCH] * gpage.h: * narrow the result of locations() *after* the division to raise the maximum page size by a few more bits. (Verify in the constructor that total_size / min_alloc <= INT_MAX so that locations() is guaranteed to be representable.) * inuse and starts both need to hold only one bit per location, not one bit per byte. * In the loop in allocate() that is searching for an available space, ensure that we do not misalign i when stepping over an allocated location at j. * In contains(), use std::less<> to compare pointers into potentially differing pages. (< is not portably a total function on pointers, std::less is portably total.) Avoid the same issue in deallocate by checking contains() before calculating the difference of the page base pointer and the function argument. * In lowest_hex_digits_of_address, convert the pointer to uintptr_t instead of size_t. * In debug_print, don't end lines with " \n" when "\n" will do ;) * deferred_heap.h: * Convert the placement argument to void* to ensure we're invoking True Placement New in construct and construct_array. * In construct_array, construct the n elements in n contiguous memory locations instead of in a single memory location. If a constructor throws, cleanup the constructed elements before allowing the exception to propagate. * In collect, don't pass pg.page.locations() to pg.page.location_info() - it's out-of-range. * test.cpp: * Tell the compiler not to warn that p1, p2, p4, & p5 are unused in test_page. * In test_deferred_allocator_set, test explicitly for libstdc++ and libc++ whose implementations of std::set do not like this test. --- deferred_heap.h | 21 +++++++---- gpage.h | 96 +++++++++++++++++++++++++++++-------------------- test.cpp | 34 ++++++++++-------- util.h | 42 ++++++++++++++-------- 4 files changed, 118 insertions(+), 75 deletions(-) diff --git a/deferred_heap.h b/deferred_heap.h index 147f3c6..9fde0db 100644 --- a/deferred_heap.h +++ b/deferred_heap.h @@ -876,7 +876,7 @@ namespace gcpp { // ===================================================================== // === BEGIN REENTRANCY-SAFE: ensure no in-progress use of private state - ::new (p) T{ std::forward(args)... }; + ::new (static_cast(p.get())) T{ std::forward(args)... }; // === END REENTRANCY-SAFE: reload any stored copies of private state // ===================================================================== @@ -898,7 +898,14 @@ namespace gcpp { for (auto i = 0; i < n; ++i) { // ===================================================================== // === BEGIN REENTRANCY-SAFE: ensure no in-progress use of private state - ::new (p) T{}; + try { + ::new (static_cast(p.get() + i)) T{}; + } catch(...) { + while (i-- > 0) { + (p.get() + i)->~T(); + } + throw; + } // === END REENTRANCY-SAFE: reload any stored copies of private state // ===================================================================== } @@ -1041,7 +1048,7 @@ namespace gcpp { // find the end of the allocation auto end_i = i + 1; - auto end = pg.page.location_info(pg.page.locations()).pointer; + auto end = [](auto s) { return s.data() + s.size(); }(pg.page.extent()); for (; end_i < pg.page.locations(); ++end_i) { auto info = pg.page.location_info(end_i); if (info.is_start) { @@ -1051,7 +1058,7 @@ namespace gcpp { } // call the destructors for objects in this range - destroy_objects({ start.pointer, end - start.pointer }); + destroy_objects({ start.pointer, end }); // and then deallocate the raw storage pg.page.deallocate(start.pointer); @@ -1062,7 +1069,7 @@ namespace gcpp { // 5. finally, drop all now-unused pages // auto empty = pages.begin(); - while ((empty = std::find_if(pages.begin(), pages.end(), + while ((empty = std::find_if(pages.begin(), pages.end(), [](const auto& pg) { return pg.page.is_empty(); })) != pages.end()) { Ensures(empty->deferred_ptrs.empty() && "page with no allocations still has deferred_ptrs"); @@ -1082,8 +1089,8 @@ namespace gcpp { inline void deferred_heap::debug_print() const { - std::cout << "\n*** heap snapshot [" << (void*)this << "] *** " - << pages.size() << " page" << (pages.size() != 1 ? "s *" : " **") + std::cout << "\n*** heap snapshot [" << (void*)this << "] *** " + << pages.size() << " page" << (pages.size() != 1 ? "s *" : " **") << "***********************************\n\n"; for (auto& pg : pages) { pg.page.debug_print(); diff --git a/gpage.h b/gpage.h index fb4993a..c552069 100644 --- a/gpage.h +++ b/gpage.h @@ -1,18 +1,18 @@ -/////////////////////////////////////////////////////////////////////////////// -// -// Copyright (c) 2016 Herb Sutter. All rights reserved. -// -// This code is licensed under the MIT License (MIT). -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -// THE SOFTWARE. -// +/////////////////////////////////////////////////////////////////////////////// +// +// Copyright (c) 2016 Herb Sutter. All rights reserved. +// +// This code is licensed under the MIT License (MIT). +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +// /////////////////////////////////////////////////////////////////////////////// @@ -20,6 +20,7 @@ #define GCPP_GPAGE #include "bitflags.h" +#include "util.h" #include #include @@ -62,13 +63,18 @@ namespace gcpp { void operator=(gpage&) = delete; public: - int locations() const noexcept { return gsl::narrow_cast(total_size) / min_alloc; } + int locations() const noexcept { return gsl::narrow_cast(total_size / min_alloc); } - const void* begin() const { return storage.get(); } + gsl::span extent() const noexcept { + return { storage.get(), gsl::narrow_cast(total_size) }; + } + gsl::span extent() noexcept { + return { storage.get(), gsl::narrow_cast(total_size) }; + } - bool is_empty() const noexcept { + bool is_empty() const noexcept { auto ret = inuse.all_false(); - Ensures(!ret || starts.all_false() && "gpage with no inuse still has starts"); + Ensures((!ret || starts.all_false()) && "gpage with no inuse still has starts"); return ret; } @@ -96,7 +102,7 @@ namespace gcpp { std::size_t location; std::size_t start_location; }; - contains_info_ret + contains_info_ret contains_info(gsl::not_null p) const noexcept; // Return whether there is an allocation starting at this location. @@ -128,7 +134,7 @@ namespace gcpp { // Construct a page with a given size and chunk size // - inline + inline gpage::gpage(std::size_t total_size_, std::size_t min_alloc_) // total_size must be a multiple of min_alloc, so round up if necessary : total_size(total_size_ + @@ -137,11 +143,15 @@ namespace gcpp { : 0)) , min_alloc(min_alloc_) , storage(std::make_unique(total_size)) - , inuse(total_size, false) - , starts(total_size, false) + , inuse(locations(), false) + , starts(locations(), false) { Expects(total_size % min_alloc == 0 && "total_size must be a multiple of min_alloc"); + Expects(in_representable_range(total_size / min_alloc) && + "total_size / min_alloc must be representable by int"); + Expects(in_representable_range(total_size) && + "total_size must be representable by ptrdiff_t"); } @@ -150,6 +160,9 @@ namespace gcpp { template byte* gpage::allocate(int n) noexcept { Expects(n > 0 && "cannot request an empty allocation"); + Expects(static_cast(n) <= + std::numeric_limits::max() / sizeof(T) && + "sizeof(T)*n must be representable by std::size_t"); const auto bytes_needed = sizeof(T)*n; @@ -160,7 +173,7 @@ namespace gcpp { // check if we need to start at an offset from the beginning of the page // because of alignment requirements, and also whether the request can fit - void* aligned_start = &storage[0]; + void* aligned_start = storage.get(); auto aligned_space = total_size; if (std::align(alignof(T), bytes_needed, aligned_start, aligned_space) == nullptr) { return nullptr; // page can't have enough space for this #bytes, after alignment @@ -170,16 +183,16 @@ namespace gcpp { const auto locations_step = 1 + (alignof(T)-1) / min_alloc; // # contiguous locations needed total - // note: as a simplification, for now we just add an extra location to every + // note: as a simplification, for now we just add an extra location to every // allocation as a simple way to support one-past-the-end arithmetic const auto locations_needed = (1 + (bytes_needed - 1) / min_alloc) + 1; const auto end = locations() - locations_needed; - // intentionally omitting "+1" here in order to keep the + // intentionally omitting "+1" here in order to keep the // last location valid for one-past-the-end pointing // for each correctly aligned location candidate - std::size_t i = ((byte*)aligned_start - &storage[0]) / min_alloc; + std::size_t i = ((byte*)aligned_start - storage.get()) / min_alloc; Expects(i == 0 && "temporary debug check: the current test harness shouldn't have generated something that required a starting offset for alignment reasons"); for (; i < end; i += locations_step) { // check to see whether we have enough free locations starting here @@ -189,7 +202,7 @@ namespace gcpp { // if any location is in use, keep going if (inuse.get(i + j)) { // optimization: bump i to avoid probing the same location twice - i += j; + i += j - j % locations_step; break; } } @@ -222,16 +235,19 @@ namespace gcpp { // inline bool gpage::contains(gsl::not_null p) const noexcept { - return &storage[0] <= p && p < &storage[total_size - 1]; + // Use std::less<> to compare (possibly unrelated) pointers portably + auto const cmp = std::less<>{}; + auto const ext = extent(); + return !cmp(p, ext.data()) && cmp(p, ext.data() + ext.size()); } inline gpage::contains_info_ret gpage::contains_info(gsl::not_null p) const noexcept { - if (!(&storage[0] <= p && p < &storage[total_size - 1])) { + if (!contains(p)) { return{ not_in_range, 0, 0 }; } - auto where = (p - &storage[0]) / min_alloc; + auto where = (p - storage.get()) / min_alloc; if (!inuse.get(where)) { return{ in_range_unallocated, where, 0 }; } @@ -252,7 +268,7 @@ namespace gcpp { // Return whether there is an allocation starting at this location. // - inline + inline gpage::location_info_ret gpage::location_info(int where) const noexcept { return{ starts.get(where), &storage[where*min_alloc] }; @@ -263,11 +279,13 @@ namespace gcpp { // inline void gpage::deallocate(gsl::not_null p) noexcept { - auto here = gsl::narrow_cast((p - &storage[0]) / min_alloc); + // p had better point to our storage ... + Expects(contains(p) && "attempt to deallocate - out of range"); + + auto here = gsl::narrow_cast((p - storage.get()) / min_alloc); - // p had better point to our storage and to the start of an allocation + // ... and to the start of an allocation // (note: we could also check alignment here but that seems superfluous) - Expects(0 <= here && here < locations() && "attempt to deallocate - out of range"); Expects(starts.get(here) && "attempt to deallocate - not at start of a valid allocation"); Expects(inuse.get(here) && "attempt to deallocate - location is not in use"); @@ -307,7 +325,7 @@ namespace gcpp { static const char digits[] = "0123456789ABCDEF"; std::string ret(num, ' '); - std::size_t val = (std::size_t)p; + auto val = reinterpret_cast(p); while (num-- > 0) { ret[num] = digits[val % 16]; val >>= 4; @@ -317,7 +335,7 @@ namespace gcpp { inline void gpage::debug_print() const { - auto base = &storage[0]; + auto base = storage.get(); std::cout << "--- total_size " << total_size << " --- min_alloc " << min_alloc << " --- " << (void*)base << " ---------------------------\n "; @@ -335,8 +353,10 @@ namespace gcpp { for (auto i = 0; i < locations(); ++i) { if (i % 64 == 0) { std::cout << lowest_hex_digits_of_address(base + i*min_alloc, 4) << ' '; } std::cout << (starts.get(i) ? 'A' : inuse.get(i) ? 'a' : '.'); - if (i % 8 == 7) { std::cout << ' '; } - if (i % 64 == 63) { std::cout << '\n'; } + if (i % 8 == 7) { + if (i % 64 == 63) { std::cout << '\n'; } + else { std::cout << ' '; } + } } std::cout << '\n'; diff --git a/test.cpp b/test.cpp index 474d519..55a4876 100644 --- a/test.cpp +++ b/test.cpp @@ -1,18 +1,18 @@ -/////////////////////////////////////////////////////////////////////////////// -// -// Copyright (c) 2016 Herb Sutter. All rights reserved. -// -// This code is licensed under the MIT License (MIT). -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -// THE SOFTWARE. -// +/////////////////////////////////////////////////////////////////////////////// +// +// Copyright (c) 2016 Herb Sutter. All rights reserved. +// +// This code is licensed under the MIT License (MIT). +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +// /////////////////////////////////////////////////////////////////////////////// @@ -84,21 +84,25 @@ void test_page() { g.debug_print(); auto p1 = g.allocate(); + (void)p1; g.debug_print(); auto p2 = g.allocate(); + (void)p2; g.debug_print(); auto p3 = g.allocate(); g.debug_print(); auto p4 = g.allocate(); + (void)p4; g.debug_print(); g.deallocate(p3); g.debug_print(); auto p5 = g.allocate(); + (void)p5; g.debug_print(); } @@ -227,7 +231,7 @@ void test_deferred_allocator() { //---------------------------------------------------------------------------- void test_deferred_allocator_set() { -#ifndef __GNUC__ +#if !defined(__GLIBCXX__) && !defined(_LIBCPP_VERSION) deferred_heap heap; auto s = deferred_set(heap); diff --git a/util.h b/util.h index 3e9304d..c57f256 100644 --- a/util.h +++ b/util.h @@ -1,18 +1,18 @@ -/////////////////////////////////////////////////////////////////////////////// -// -// Copyright (c) 2016 Herb Sutter. All rights reserved. -// -// This code is licensed under the MIT License (MIT). -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -// THE SOFTWARE. -// +/////////////////////////////////////////////////////////////////////////////// +// +// Copyright (c) 2016 Herb Sutter. All rights reserved. +// +// This code is licensed under the MIT License (MIT). +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. +// /////////////////////////////////////////////////////////////////////////////// @@ -26,10 +26,12 @@ // This project requires GSL, see: https://github.com/microsoft/gsl #include +#include +#include namespace gcpp { - using byte = gsl::byte; + using gsl::byte; } @@ -43,4 +45,14 @@ bool operator<=(const Type& that) const { return compare3(that) <= 0; } \ bool operator> (const Type& that) const { return compare3(that) > 0; } \ bool operator>=(const Type& that) const { return compare3(that) >= 0; } +// Returns true iff value is within the range of values representable by (arithmetic) type Target +template +constexpr bool in_representable_range(Value const& value) { + using C = std::common_type_t; + auto const cvalue = static_cast(value); + return cvalue <= static_cast(std::numeric_limits::max()) && + (!std::is_signed::value || static_cast(std::numeric_limits::min()) <= cvalue) && + (C{} < cvalue) == (Value{} < value); +} + #endif