From 17f371ddf20c5de5e77e72f080bb34666736a010 Mon Sep 17 00:00:00 2001 From: Arvid Norberg Date: Mon, 20 Mar 2017 17:41:00 -0400 Subject: [PATCH] fix alignment issues in heterogeneous_queue (#1834) fix alignment issues in heterogeneous_queue --- include/libtorrent/alert_manager.hpp | 2 +- include/libtorrent/heterogeneous_queue.hpp | 180 +++++++++++++-------- src/alert_manager.cpp | 3 +- test/test_heterogeneous_queue.cpp | 32 ++++ 4 files changed, 147 insertions(+), 70 deletions(-) diff --git a/include/libtorrent/alert_manager.hpp b/include/libtorrent/alert_manager.hpp index 809de0d24..908fb573d 100644 --- a/include/libtorrent/alert_manager.hpp +++ b/include/libtorrent/alert_manager.hpp @@ -138,7 +138,7 @@ namespace libtorrent { // the alert_manager is allowed to use right now. This is swapped when // the client calls get_all(), at which point all of the alert objects // passed to the client will be owned by libtorrent again, and reset. - int m_generation; + int m_generation = 0; // this is where all alerts are queued up. There are two heterogeneous // queues to double buffer the thread access. The std::mutex in the alert diff --git a/include/libtorrent/heterogeneous_queue.hpp b/include/libtorrent/heterogeneous_queue.hpp index 2614e06a1..030eda0ed 100644 --- a/include/libtorrent/heterogeneous_queue.hpp +++ b/include/libtorrent/heterogeneous_queue.hpp @@ -35,62 +35,96 @@ POSSIBILITY OF SUCH DAMAGE. #include #include +#include // for malloc #include +#include #include "libtorrent/assert.hpp" +#include "libtorrent/aux_/throw.hpp" namespace libtorrent { + namespace aux { + struct free_deleter + { void operator()(char* ptr) { return std::free(ptr); } }; + + inline std::size_t calculate_pad_bytes(char* inptr, std::size_t alignment) + { + std::uintptr_t const ptr = reinterpret_cast(inptr); + std::uintptr_t const offset = ptr & (alignment - 1); + return (alignment - offset) & (alignment - 1); + } + } + template struct heterogeneous_queue { - heterogeneous_queue() - : m_storage(nullptr) - , m_capacity(0) - , m_size(0) - , m_num_items(0) - {} + heterogeneous_queue() : m_storage(nullptr, aux::free_deleter()) {} + heterogeneous_queue(heterogeneous_queue const&) = delete; + heterogeneous_queue& operator=(heterogeneous_queue const&) = delete; template typename std::enable_if::value, U&>::type emplace_back(Args&&... args) { - // the size of the type rounded up to pointer alignment - const int object_size = (sizeof(U) + sizeof(*m_storage) - 1) - / sizeof(*m_storage); + // make the conservative assumption that we'll need the maximum padding + // for this object, just for purposes of growing the storage + if (std::size_t(m_size) + sizeof(header_t) + alignof(U) + sizeof(U) > std::size_t(m_capacity)) + grow_capacity(sizeof(header_t) + alignof(U) + sizeof(U)); - // +1 for the length prefix - if (m_size + object_size + header_size > m_capacity) - grow_capacity(object_size); + char* ptr = m_storage.get() + m_size; - uintptr_t* ptr = m_storage + m_size; + std::size_t const pad_bytes = aux::calculate_pad_bytes(ptr + sizeof(header_t), alignof(U)); + + // pad_bytes is only 8 bits in the header, so types that need more than + // 256 byte alignment may not be suppored + static_assert(alignof(U) <= 256 + , "heterogeneous_queue does not support types with alignment requirements > 256"); + + // if this assert triggers, the type being added to the queue has + // alignment requirements stricter than what malloc() returns. This is + // not supported + TORRENT_ASSERT((reinterpret_cast(m_storage.get()) + & (alignof(U) - 1)) == 0); + + // make sure the current position in the storage is aligned for + // creating a heder_t object + TORRENT_ASSERT((reinterpret_cast(ptr) + & (alignof(header_t) - 1)) == 0); // length prefix - header_t* hdr = reinterpret_cast(ptr); - hdr->len = object_size; + header_t* hdr = new (ptr) header_t; + hdr->pad_bytes = static_cast(pad_bytes); hdr->move = &move; - ptr += header_size; + ptr += sizeof(header_t) + pad_bytes; + hdr->len = static_cast(sizeof(U) + + aux::calculate_pad_bytes(ptr + sizeof(U), alignof(header_t))); + + // make sure ptr is correctly aligned for the object we're about to + // create there + TORRENT_ASSERT((reinterpret_cast(ptr) + & (alignof(U) - 1)) == 0); // construct in-place - new (ptr) U(std::forward(args)...); + U* const ret = new (ptr) U(std::forward(args)...); // if we constructed the object without throwing any exception // update counters to indicate the new item is in there ++m_num_items; - m_size += header_size + object_size; - return *reinterpret_cast(ptr); + m_size += int(sizeof(header_t) + pad_bytes + hdr->len); + return *ret; } void get_pointers(std::vector& out) { out.clear(); - uintptr_t* ptr = m_storage; - uintptr_t const* const end = m_storage + m_size; + char* ptr = m_storage.get(); + char const* const end = m_storage.get() + m_size; while (ptr < end) { header_t* hdr = reinterpret_cast(ptr); - ptr += header_size; + ptr += sizeof(header_t) + hdr->pad_bytes; TORRENT_ASSERT(ptr + hdr->len <= end); out.push_back(reinterpret_cast(ptr)); ptr += hdr->len; @@ -110,16 +144,17 @@ namespace libtorrent { void clear() { - uintptr_t* ptr = m_storage; - uintptr_t const* const end = m_storage + m_size; + char* ptr = m_storage.get(); + char const* const end = m_storage.get() + m_size; while (ptr < end) { header_t* hdr = reinterpret_cast(ptr); - ptr += header_size; + ptr += sizeof(header_t) + hdr->pad_bytes; TORRENT_ASSERT(ptr + hdr->len <= end); T* a = reinterpret_cast(ptr); a->~T(); ptr += hdr->len; + hdr->~header_t(); } m_size = 0; m_num_items = 0; @@ -130,81 +165,92 @@ namespace libtorrent { if (m_size == 0) return nullptr; TORRENT_ASSERT(m_size > 1); - uintptr_t* ptr = m_storage; - TORRENT_ASSERT(reinterpret_cast(ptr)->len <= m_size); - ptr += header_size; + char* ptr = m_storage.get(); + header_t* hdr = reinterpret_cast(ptr); + TORRENT_ASSERT(sizeof(header_t) + hdr->pad_bytes + hdr->len + <= std::size_t(m_size)); + ptr += sizeof(header_t) + hdr->pad_bytes; return reinterpret_cast(ptr); } - ~heterogeneous_queue() - { - clear(); - delete[] m_storage; - } + ~heterogeneous_queue() { clear(); } private: - // non-copyable - heterogeneous_queue(heterogeneous_queue const&); - heterogeneous_queue& operator=(heterogeneous_queue const&); - // this header is put in front of every element. It tells us - // how many uintptr_t's it's using for its allocation, and it + // how many bytes it's using for its allocation, and it // also tells us how to move this type if we need to grow our // allocation. struct header_t { - int len; - void (*move)(uintptr_t* dst, uintptr_t* src); - }; + // the size of the object. From the start of the object, skip this many + // bytes to get to the next header. Meaning this includes sufficient + // padding to have the next entry be appropriately aligned for header_t + std::uint16_t len; - static int const header_size = (sizeof(header_t) + sizeof(uintptr_t) - - 1) / sizeof(uintptr_t); + // the number of pad bytes between the end of this + // header and the start of the object. This supports allocating types with + // stricter alignment requirements + std::uint8_t pad_bytes; + + void (*move)(char* dst, char* src); + }; void grow_capacity(int const size) { - int const amount_to_grow = (std::max)(size + header_size + int const amount_to_grow = (std::max)(size , (std::max)(m_capacity * 3 / 2, 128)); - uintptr_t* new_storage = new uintptr_t[m_capacity + amount_to_grow]; + // we use malloc() to guarantee alignment + std::unique_ptr new_storage( + static_cast(std::malloc(std::size_t(m_capacity + amount_to_grow))) + , aux::free_deleter()); - uintptr_t* src = m_storage; - uintptr_t* dst = new_storage; - uintptr_t const* const end = m_storage + m_size; + if (new_storage.get() == nullptr) + aux::throw_ex(); + + char* src = m_storage.get(); + char* dst = new_storage.get(); + char const* const end = m_storage.get() + m_size; while (src < end) { header_t* src_hdr = reinterpret_cast(src); - header_t* dst_hdr = reinterpret_cast(dst); - *dst_hdr = *src_hdr; - src += header_size; - dst += header_size; - TORRENT_ASSERT(src + src_hdr->len <= end); - // TODO: if this throws, should we do anything? + new (dst) header_t(*src_hdr); + src += sizeof(header_t) + src_hdr->pad_bytes; + dst += sizeof(header_t) + src_hdr->pad_bytes; + int const len = src_hdr->len; + TORRENT_ASSERT(src + len <= end); + // TODO: if this throws, we should technically destruct the elements + // we've constructed so far, but maybe we should just disallow + // throwing move instead. src_hdr->move(dst, src); - src += src_hdr->len; - dst += src_hdr->len; + src_hdr->~header_t(); + src += len ; + dst += len; } - delete[] m_storage; - m_storage = new_storage; + m_storage.swap(new_storage); m_capacity += amount_to_grow; } template - static void move(uintptr_t* dst, uintptr_t* src) + static void move(char* dst, char* src) { U& rhs = *reinterpret_cast(src); - new (dst) U(static_cast(rhs)); + + TORRENT_ASSERT((reinterpret_cast(dst) & (alignof(U) - 1)) == 0); + + new (dst) U(std::move(rhs)); rhs.~U(); } - uintptr_t* m_storage; - // number of uintptr_t's allocated under m_storage - int m_capacity; - // the number of uintptr_t's used under m_storage - int m_size; - // the number of objects allocated under m_storage - int m_num_items; + std::unique_ptr m_storage; + // number of bytes of storage allocated + int m_capacity = 0; + // the number of bytes used under m_storage + int m_size = 0; + // the number of objects allocated in m_storage + int m_num_items = 0; }; } diff --git a/src/alert_manager.cpp b/src/alert_manager.cpp index 330e5b182..a5d8e1f92 100644 --- a/src/alert_manager.cpp +++ b/src/alert_manager.cpp @@ -41,10 +41,9 @@ POSSIBILITY OF SUCH DAMAGE. namespace libtorrent { - alert_manager::alert_manager(int queue_limit, std::uint32_t alert_mask) + alert_manager::alert_manager(int const queue_limit, std::uint32_t const alert_mask) : m_alert_mask(alert_mask) , m_queue_size_limit(queue_limit) - , m_generation(0) {} alert_manager::~alert_manager() = default; diff --git a/test/test_heterogeneous_queue.cpp b/test/test_heterogeneous_queue.cpp index 6eebc80b1..e6f001550 100644 --- a/test/test_heterogeneous_queue.cpp +++ b/test/test_heterogeneous_queue.cpp @@ -136,6 +136,13 @@ private: F& operator=(F const& f); }; +struct G : A +{ + G(int base, int v) : A(base), g(v) {} + int type() override { return 3; } + std::int64_t g; +}; + // test emplace_back of heterogeneous types // and retrieval of their pointers TORRENT_TEST(emplace_back) @@ -186,6 +193,31 @@ TORRENT_TEST(emplace_back) TEST_EQUAL(static_cast(ptrs[5])->c[0], 11); } +TORRENT_TEST(emplace_back_over_aligned) +{ + using namespace libtorrent; + + heterogeneous_queue q; + q.emplace_back(1, 2); + q.emplace_back(3, 4); + q.emplace_back(5, 6); + + std::vector ptrs; + q.get_pointers(ptrs); + + TEST_EQUAL(int(ptrs.size()), q.size()); + TEST_EQUAL(ptrs.size(), 3); + TEST_EQUAL(ptrs[0]->type(), 3); + TEST_EQUAL(static_cast(ptrs[0])->a, 1); + TEST_EQUAL(static_cast(ptrs[0])->g, 2); + TEST_EQUAL(ptrs[1]->type(), 3); + TEST_EQUAL(static_cast(ptrs[1])->a, 3); + TEST_EQUAL(static_cast(ptrs[1])->g, 4); + TEST_EQUAL(ptrs[2]->type(), 1); + TEST_EQUAL(static_cast(ptrs[2])->a, 5); + TEST_EQUAL(static_cast(ptrs[2])->b, 6); +} + // test swap TORRENT_TEST(swap) {