diff --git a/rclcpp/include/rclcpp/allocator/allocator_common.hpp b/rclcpp/include/rclcpp/allocator/allocator_common.hpp index 12b2f383b6..f1d411d25f 100644 --- a/rclcpp/include/rclcpp/allocator/allocator_common.hpp +++ b/rclcpp/include/rclcpp/allocator/allocator_common.hpp @@ -30,14 +30,19 @@ namespace allocator template using AllocRebind = typename std::allocator_traits::template rebind_traits; -template -void * retyped_allocate(size_t size, void * untyped_allocator) +/// Return the equivalent rcl_allocator_t for the C++ standard allocator. +/** + * This assumes that the user intent behind both allocators is the + * same: Using system malloc for allocation. + * + * If you're using a custom allocator in ROS, you'll need to provide + * your own overload for this function. + */ +template +rcl_allocator_t get_rcl_allocator(std::allocator allocator) { - auto typed_allocator = static_cast(untyped_allocator); - if (!typed_allocator) { - throw std::runtime_error("Received incorrect allocator type"); - } - return std::allocator_traits::allocate(*typed_allocator, size); + (void)allocator; // Remove warning + return rcl_get_default_allocator(); } template @@ -56,61 +61,6 @@ void * retyped_zero_allocate(size_t number_of_elem, size_t size_of_elem, void * return allocated_memory; } -template -void retyped_deallocate(void * untyped_pointer, void * untyped_allocator) -{ - auto typed_allocator = static_cast(untyped_allocator); - if (!typed_allocator) { - throw std::runtime_error("Received incorrect allocator type"); - } - auto typed_ptr = static_cast(untyped_pointer); - std::allocator_traits::deallocate(*typed_allocator, typed_ptr, 1); -} - -template -void * retyped_reallocate(void * untyped_pointer, size_t size, void * untyped_allocator) -{ - auto typed_allocator = static_cast(untyped_allocator); - if (!typed_allocator) { - throw std::runtime_error("Received incorrect allocator type"); - } - auto typed_ptr = static_cast(untyped_pointer); - std::allocator_traits::deallocate(*typed_allocator, typed_ptr, 1); - return std::allocator_traits::allocate(*typed_allocator, size); -} - - -// Convert a std::allocator_traits-formatted Allocator into an rcl allocator -template< - typename T, - typename Alloc, - typename std::enable_if>::value>::type * = nullptr> -rcl_allocator_t get_rcl_allocator(Alloc & allocator) -{ - rcl_allocator_t rcl_allocator = rcl_get_default_allocator(); -#ifndef _WIN32 - rcl_allocator.allocate = &retyped_allocate; - rcl_allocator.zero_allocate = &retyped_zero_allocate; - rcl_allocator.deallocate = &retyped_deallocate; - rcl_allocator.reallocate = &retyped_reallocate; - rcl_allocator.state = &allocator; -#else - (void)allocator; // Remove warning -#endif - return rcl_allocator; -} - -// TODO(jacquelinekay) Workaround for an incomplete implementation of std::allocator -template< - typename T, - typename Alloc, - typename std::enable_if>::value>::type * = nullptr> -rcl_allocator_t get_rcl_allocator(Alloc & allocator) -{ - (void)allocator; - return rcl_get_default_allocator(); -} - } // namespace allocator } // namespace rclcpp diff --git a/rclcpp/include/rclcpp/message_memory_strategy.hpp b/rclcpp/include/rclcpp/message_memory_strategy.hpp index f548d953c2..d664239b25 100644 --- a/rclcpp/include/rclcpp/message_memory_strategy.hpp +++ b/rclcpp/include/rclcpp/message_memory_strategy.hpp @@ -18,6 +18,7 @@ #include #include +#include "rcl/allocator.h" #include "rcl/types.h" #include "rclcpp/allocator/allocator_common.hpp" @@ -61,7 +62,7 @@ class MessageMemoryStrategy message_allocator_ = std::make_shared(); serialized_message_allocator_ = std::make_shared(); buffer_allocator_ = std::make_shared(); - rcutils_allocator_ = allocator::get_rcl_allocator(*buffer_allocator_.get()); + rcutils_allocator_ = rcl_get_default_allocator(); } explicit MessageMemoryStrategy(std::shared_ptr allocator) @@ -69,7 +70,7 @@ class MessageMemoryStrategy message_allocator_ = std::make_shared(*allocator.get()); serialized_message_allocator_ = std::make_shared(*allocator.get()); buffer_allocator_ = std::make_shared(*allocator.get()); - rcutils_allocator_ = allocator::get_rcl_allocator(*buffer_allocator_.get()); + rcutils_allocator_ = rcl_get_default_allocator(); } virtual ~MessageMemoryStrategy() = default; diff --git a/rclcpp/include/rclcpp/publisher_options.hpp b/rclcpp/include/rclcpp/publisher_options.hpp index 01fd314f49..8a9bb4ea00 100644 --- a/rclcpp/include/rclcpp/publisher_options.hpp +++ b/rclcpp/include/rclcpp/publisher_options.hpp @@ -20,6 +20,7 @@ #include #include +#include "rcl/allocator.h" #include "rcl/publisher.h" #include "rclcpp/allocator/allocator_common.hpp" @@ -74,14 +75,19 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase "Publisher allocator value type must be void"); /// Optional custom allocator. - std::shared_ptr allocator = nullptr; + std::shared_ptr allocator; + std::shared_ptr rcl_allocator; - PublisherOptionsWithAllocator() {} + PublisherOptionsWithAllocator() + :allocator(std::make_shared()) + {} /// Constructor using base class as input. explicit PublisherOptionsWithAllocator(const PublisherOptionsBase & publisher_options_base) - : PublisherOptionsBase(publisher_options_base) - {} + : PublisherOptionsBase(publisher_options_base), + allocator(std::make_shared()) + { + } /// Convert this class, and a rclcpp::QoS, into an rcl_publisher_options_t. template @@ -89,7 +95,7 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase to_rcl_publisher_options(const rclcpp::QoS & qos) const { rcl_publisher_options_t result = rcl_publisher_get_default_options(); - result.allocator = this->get_rcl_allocator(); + result.allocator = get_rcl_allocator(); result.qos = qos.get_rmw_qos_profile(); result.rmw_publisher_options.require_unique_network_flow_endpoints = this->require_unique_network_flow_endpoints; @@ -102,43 +108,37 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase return result; } - /// Get the allocator, creating one if needed. std::shared_ptr get_allocator() const { - if (!this->allocator) { - if (!allocator_storage_) { - allocator_storage_ = std::make_shared(); - } - return allocator_storage_; - } return this->allocator; } private: - using PlainAllocator = - typename std::allocator_traits::template rebind_alloc; - rcl_allocator_t get_rcl_allocator() const { - if (!plain_allocator_storage_) { - plain_allocator_storage_ = - std::make_shared(*this->get_allocator()); + if(rcl_allocator) { + return *rcl_allocator; } - return rclcpp::allocator::get_rcl_allocator(*plain_allocator_storage_); - } - // This is a temporal workaround, to make sure that get_allocator() - // always returns a copy of the same allocator. - mutable std::shared_ptr allocator_storage_; + RCUTILS_LOG_ERROR_NAMED("rclcpp", + "Warning, custom C++ allocator specified, but custom rcl allocator was set. Note, " \ + "that due to a bug fix rcl allocators are not automatically generated from the C++ " \ + "allocator any more."); - // This is a temporal workaround, to keep the plain allocator that backs - // up the rcl allocator returned in rcl_publisher_options_t alive. - mutable std::shared_ptr plain_allocator_storage_; + return rcl_get_default_allocator(); + } }; +template<> +inline rcl_allocator_t +PublisherOptionsWithAllocator>::get_rcl_allocator() const +{ + return rcl_get_default_allocator(); +} + using PublisherOptions = PublisherOptionsWithAllocator>; } // namespace rclcpp diff --git a/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp b/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp index d3dcfab21c..c55d78cceb 100644 --- a/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp +++ b/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp @@ -515,7 +515,7 @@ class AllocatorMemoryStrategy : public memory_strategy::MemoryStrategy rcl_allocator_t get_allocator() override { - return rclcpp::allocator::get_rcl_allocator(*allocator_.get()); + return rcl_get_default_allocator(); } size_t number_of_ready_subscriptions() const override diff --git a/rclcpp/include/rclcpp/subscription_options.hpp b/rclcpp/include/rclcpp/subscription_options.hpp index 0dd738ee60..70701aec93 100644 --- a/rclcpp/include/rclcpp/subscription_options.hpp +++ b/rclcpp/include/rclcpp/subscription_options.hpp @@ -21,6 +21,8 @@ #include #include +#include "rcl/allocator.h" + #include "rclcpp/callback_group.hpp" #include "rclcpp/detail/rmw_implementation_specific_subscription_payload.hpp" #include "rclcpp/intra_process_buffer_type.hpp" @@ -100,15 +102,20 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase "Subscription allocator value type must be void"); /// Optional custom allocator. - std::shared_ptr allocator = nullptr; + std::shared_ptr allocator; + std::shared_ptr rcl_allocator; - SubscriptionOptionsWithAllocator() {} + SubscriptionOptionsWithAllocator() + :allocator(std::make_shared()) + {} /// Constructor using base class as input. explicit SubscriptionOptionsWithAllocator( const SubscriptionOptionsBase & subscription_options_base) - : SubscriptionOptionsBase(subscription_options_base) - {} + : SubscriptionOptionsBase(subscription_options_base), + allocator(std::make_shared()) + { + } /// Convert this class, with a rclcpp::QoS, into an rcl_subscription_options_t. /** @@ -119,7 +126,7 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase to_rcl_subscription_options(const rclcpp::QoS & qos) const { rcl_subscription_options_t result = rcl_subscription_get_default_options(); - result.allocator = this->get_rcl_allocator(); + result.allocator = rcl_get_default_allocator(); result.qos = qos.get_rmw_qos_profile(); result.rmw_subscription_options.ignore_local_publications = this->ignore_local_publications; result.rmw_subscription_options.require_unique_network_flow_endpoints = @@ -151,36 +158,18 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase std::shared_ptr get_allocator() const { - if (!this->allocator) { - if (!allocator_storage_) { - allocator_storage_ = std::make_shared(); - } - return allocator_storage_; - } return this->allocator; } private: - using PlainAllocator = - typename std::allocator_traits::template rebind_alloc; - rcl_allocator_t get_rcl_allocator() const { - if (!plain_allocator_storage_) { - plain_allocator_storage_ = - std::make_shared(*this->get_allocator()); + if(rcl_allocator) { + return *rcl_allocator; } - return rclcpp::allocator::get_rcl_allocator(*plain_allocator_storage_); + return rcl_get_default_allocator(); } - - // This is a temporal workaround, to make sure that get_allocator() - // always returns a copy of the same allocator. - mutable std::shared_ptr allocator_storage_; - - // This is a temporal workaround, to keep the plain allocator that backs - // up the rcl allocator returned in rcl_subscription_options_t alive. - mutable std::shared_ptr plain_allocator_storage_; }; using SubscriptionOptions = SubscriptionOptionsWithAllocator>; diff --git a/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp b/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp index 4619b7665d..5e1d8bc90c 100644 --- a/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp +++ b/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp @@ -18,54 +18,17 @@ #include "rclcpp/allocator/allocator_common.hpp" -TEST(TestAllocatorCommon, retyped_allocate) { - std::allocator allocator; - void * untyped_allocator = &allocator; - void * allocated_mem = - rclcpp::allocator::retyped_allocate>(1u, untyped_allocator); - // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static - // analysis throws a false-positive memory leak warning. Use ASSERT_TRUE instead. - ASSERT_TRUE(nullptr != allocated_mem); - - auto code = [&untyped_allocator, allocated_mem]() { - rclcpp::allocator::retyped_deallocate>( - allocated_mem, untyped_allocator); - }; - EXPECT_NO_THROW(code()); - - allocated_mem = allocator.allocate(1); - // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static - // analysis throws a false-positive memory leak warning. Use ASSERT_TRUE instead. - ASSERT_TRUE(nullptr != allocated_mem); - void * reallocated_mem = - rclcpp::allocator::retyped_reallocate>( - allocated_mem, 2u, untyped_allocator); - // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static - // analysis throws a false-positive memory leak warning. Use ASSERT_TRUE instead. - ASSERT_TRUE(nullptr != reallocated_mem); - - auto code2 = [&untyped_allocator, reallocated_mem]() { - rclcpp::allocator::retyped_deallocate>( - reallocated_mem, untyped_allocator); - }; - EXPECT_NO_THROW(code2()); -} - -TEST(TestAllocatorCommon, retyped_zero_allocate_basic) { +TEST(TestAllocatorCommon, retyped_zero_allocate_basic) +{ std::allocator allocator; void * untyped_allocator = &allocator; void * allocated_mem = rclcpp::allocator::retyped_zero_allocate>(20u, 1u, untyped_allocator); ASSERT_TRUE(nullptr != allocated_mem); - - auto code = [&untyped_allocator, allocated_mem]() { - rclcpp::allocator::retyped_deallocate>( - allocated_mem, untyped_allocator); - }; - EXPECT_NO_THROW(code()); } -TEST(TestAllocatorCommon, retyped_zero_allocate) { +TEST(TestAllocatorCommon, retyped_zero_allocate) +{ std::allocator allocator; void * untyped_allocator = &allocator; void * allocated_mem = @@ -73,34 +36,12 @@ TEST(TestAllocatorCommon, retyped_zero_allocate) { // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static // analysis throws a false-positive memory leak warning. Use ASSERT_TRUE instead. ASSERT_TRUE(nullptr != allocated_mem); - - auto code = [&untyped_allocator, allocated_mem]() { - rclcpp::allocator::retyped_deallocate>( - allocated_mem, untyped_allocator); - }; - EXPECT_NO_THROW(code()); - - allocated_mem = allocator.allocate(1); - // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static - // analysis throws a false-positive memory leak warning. Use ASSERT_TRUE instead. - ASSERT_TRUE(nullptr != allocated_mem); - void * reallocated_mem = - rclcpp::allocator::retyped_reallocate>( - allocated_mem, 2u, untyped_allocator); - // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static - // analysis throws a false-positive memory leak warning. Use ASSERT_TRUE instead. - ASSERT_TRUE(nullptr != reallocated_mem); - - auto code2 = [&untyped_allocator, reallocated_mem]() { - rclcpp::allocator::retyped_deallocate>( - reallocated_mem, untyped_allocator); - }; - EXPECT_NO_THROW(code2()); } -TEST(TestAllocatorCommon, get_rcl_allocator) { +TEST(TestAllocatorCommon, get_rcl_allocator) +{ std::allocator allocator; - auto rcl_allocator = rclcpp::allocator::get_rcl_allocator(allocator); + auto rcl_allocator = rclcpp::allocator::get_rcl_allocator(allocator); EXPECT_NE(nullptr, rcl_allocator.allocate); EXPECT_NE(nullptr, rcl_allocator.deallocate); EXPECT_NE(nullptr, rcl_allocator.reallocate); @@ -108,10 +49,10 @@ TEST(TestAllocatorCommon, get_rcl_allocator) { // Not testing state as that may or may not be null depending on platform } -TEST(TestAllocatorCommon, get_void_rcl_allocator) { +TEST(TestAllocatorCommon, get_void_rcl_allocator) +{ std::allocator allocator; - auto rcl_allocator = - rclcpp::allocator::get_rcl_allocator>(allocator); + auto rcl_allocator = rclcpp::allocator::get_rcl_allocator(allocator); EXPECT_NE(nullptr, rcl_allocator.allocate); EXPECT_NE(nullptr, rcl_allocator.deallocate); EXPECT_NE(nullptr, rcl_allocator.reallocate); diff --git a/rclcpp/test/rclcpp/test_node_options.cpp b/rclcpp/test/rclcpp/test_node_options.cpp index 07a522f497..14e32f2304 100644 --- a/rclcpp/test/rclcpp/test_node_options.cpp +++ b/rclcpp/test/rclcpp/test_node_options.cpp @@ -372,7 +372,7 @@ TEST(TestNodeOptions, set_get_allocator) { EXPECT_EQ(fake_allocator.state, options.allocator().state); // Check invalid allocator - EXPECT_THROW(options.get_rcl_node_options(), std::bad_alloc); + EXPECT_THROW(options.get_rcl_node_options(), rclcpp::exceptions::RCLBadAlloc); } TEST(TestNodeOptions, clock_type) {