From b1b05c2eb9a191ea266fd34150b2ac150c9448d8 Mon Sep 17 00:00:00 2001 From: Janosch Machowinski Date: Mon, 16 Feb 2026 21:20:09 +0100 Subject: [PATCH] fix: Use default rcl allocator if allocator is std::allocator (#3058) * fix: Use default rcl allocator if allocator is std::allocator This fixes a bunch of warnings if using ASAN / valgrind on newer OS versions. It also fixed a real bug, as giving the wrong size on deallocate is undefined behavior according to the C++ standard. This version of the patch keeps the behavior for users that specified an own allocator the same and in therefore back portable. Signed-off-by: Janosch Machowinski * feat: Provide a way to suppress the deprecation warning This commit adds the feature, that the user can now specify a method rcl_allocator_t get_rcl_allocator() on the given allocator. This method will be called if present, to get the rcl allocator. If the method is not present, the code will fall back to the old (and faulty) implementation and show a deprecation warning. Signed-off-by: Janosch Machowinski --------- Signed-off-by: Janosch Machowinski Co-authored-by: Janosch Machowinski (cherry picked from commit 9f79f406219b88f9dd2e8973be5d0247e81a67c5) --- .../rclcpp/allocator/allocator_common.hpp | 38 +++++++++++++++++++ .../rclcpp/message_memory_strategy.hpp | 23 ++++++++++- rclcpp/include/rclcpp/publisher_options.hpp | 17 +++++++-- .../strategies/allocator_memory_strategy.hpp | 6 ++- .../include/rclcpp/subscription_options.hpp | 17 +++++++-- .../allocator/test_allocator_common.cpp | 19 +++++++++- ..._intra_process_manager_with_allocators.cpp | 10 +++++ 7 files changed, 118 insertions(+), 12 deletions(-) diff --git a/rclcpp/include/rclcpp/allocator/allocator_common.hpp b/rclcpp/include/rclcpp/allocator/allocator_common.hpp index 12b2f383b6..18cab9e9e1 100644 --- a/rclcpp/include/rclcpp/allocator/allocator_common.hpp +++ b/rclcpp/include/rclcpp/allocator/allocator_common.hpp @@ -27,10 +27,39 @@ namespace rclcpp namespace allocator { +template +using clean_t = std::remove_cv_t>; + +// Primary template: false +template> +struct has_get_rcl_allocator : std::false_type {}; + +// Specialization: true if expression is valid +template +struct has_get_rcl_allocator &>().get_rcl_allocator()) + > +> + : std::bool_constant< + std::is_same_v< + decltype(std::declval &>().get_rcl_allocator()), + rcl_allocator_t + > + > +{}; + +// Helper variable template +template +inline constexpr bool has_get_rcl_allocator_v = + has_get_rcl_allocator::value; + template using AllocRebind = typename std::allocator_traits::template rebind_traits; template +[[deprecated("Conversion of C++ allocators to C style is not valid, as the size on deallocate" + "can not be determined. This will be remove in future versions of ros.")]] void * retyped_allocate(size_t size, void * untyped_allocator) { auto typed_allocator = static_cast(untyped_allocator); @@ -41,6 +70,8 @@ void * retyped_allocate(size_t size, void * untyped_allocator) } template +[[deprecated("Conversion of C++ allocators to C style is not valid, as the size on deallocate" + "can not be determined. This will be remove in future versions of ros.")]] void * retyped_zero_allocate(size_t number_of_elem, size_t size_of_elem, void * untyped_allocator) { auto typed_allocator = static_cast(untyped_allocator); @@ -57,6 +88,8 @@ void * retyped_zero_allocate(size_t number_of_elem, size_t size_of_elem, void * } template +[[deprecated("Conversion of C++ allocators to C style is not valid, as the size on deallocate" + "can not be determined. This will be remove in future versions of ros.")]] void retyped_deallocate(void * untyped_pointer, void * untyped_allocator) { auto typed_allocator = static_cast(untyped_allocator); @@ -68,6 +101,8 @@ void retyped_deallocate(void * untyped_pointer, void * untyped_allocator) } template +[[deprecated("Conversion of C++ allocators to C style is not valid, as the size on deallocate" + "can not be determined. This will be remove in future versions of ros.")]] void * retyped_reallocate(void * untyped_pointer, size_t size, void * untyped_allocator) { auto typed_allocator = static_cast(untyped_allocator); @@ -85,6 +120,9 @@ template< typename T, typename Alloc, typename std::enable_if>::value>::type * = nullptr> +[[deprecated("Conversion of C++ allocators to C style is not valid, as the size on deallocate" + "can not be determined. This will be remove in future versions of ros. To suppress this warning" + "define the method 'rcl_allocator_t get_rcl_allocator()' on your allocator")]] rcl_allocator_t get_rcl_allocator(Alloc & allocator) { rcl_allocator_t rcl_allocator = rcl_get_default_allocator(); diff --git a/rclcpp/include/rclcpp/message_memory_strategy.hpp b/rclcpp/include/rclcpp/message_memory_strategy.hpp index f548d953c2..b382b15023 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,16 @@ 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()); + if constexpr (std::is_same_v>) { + rcutils_allocator_ = rcl_get_default_allocator(); + } else { + if constexpr (rclcpp::allocator::has_get_rcl_allocator_v) { + rcutils_allocator_ = message_allocator_->get_rcl_allocator(); + } else { + rcutils_allocator_ = allocator::get_rcl_allocator(*buffer_allocator_.get()); + } + } } explicit MessageMemoryStrategy(std::shared_ptr allocator) @@ -69,7 +79,16 @@ 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()); + if constexpr (std::is_same_v>) { + rcutils_allocator_ = rcl_get_default_allocator(); + } else { + if constexpr (rclcpp::allocator::has_get_rcl_allocator_v) { + rcutils_allocator_ = allocator->get_rcl_allocator(); + } else { + rcutils_allocator_ = allocator::get_rcl_allocator(*buffer_allocator_.get()); + } + } } virtual ~MessageMemoryStrategy() = default; diff --git a/rclcpp/include/rclcpp/publisher_options.hpp b/rclcpp/include/rclcpp/publisher_options.hpp index 01fd314f49..b08f500a08 100644 --- a/rclcpp/include/rclcpp/publisher_options.hpp +++ b/rclcpp/include/rclcpp/publisher_options.hpp @@ -123,11 +123,20 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase rcl_allocator_t get_rcl_allocator() const { - if (!plain_allocator_storage_) { - plain_allocator_storage_ = - std::make_shared(*this->get_allocator()); + if constexpr (std::is_same_v>) { + return rcl_get_default_allocator(); + } else { + if constexpr (rclcpp::allocator::has_get_rcl_allocator_v) { + return get_allocator()->get_rcl_allocator(); + } else { + if (!plain_allocator_storage_) { + plain_allocator_storage_ = + std::make_shared(*this->get_allocator()); + } + + return rclcpp::allocator::get_rcl_allocator(*plain_allocator_storage_); + } } - return rclcpp::allocator::get_rcl_allocator(*plain_allocator_storage_); } // This is a temporal workaround, to make sure that get_allocator() diff --git a/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp b/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp index c99645c64c..91f187a638 100644 --- a/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp +++ b/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp @@ -430,7 +430,11 @@ class AllocatorMemoryStrategy : public memory_strategy::MemoryStrategy rcl_allocator_t get_allocator() override { - return rclcpp::allocator::get_rcl_allocator(*allocator_.get()); + if constexpr (std::is_same_v>) { + return rcl_get_default_allocator(); + } else { + return rclcpp::allocator::get_rcl_allocator(*allocator_.get()); + } } 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..775ad8809d 100644 --- a/rclcpp/include/rclcpp/subscription_options.hpp +++ b/rclcpp/include/rclcpp/subscription_options.hpp @@ -167,11 +167,20 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase rcl_allocator_t get_rcl_allocator() const { - if (!plain_allocator_storage_) { - plain_allocator_storage_ = - std::make_shared(*this->get_allocator()); + if constexpr (std::is_same_v>) { + return rcl_get_default_allocator(); + } else { + if constexpr (rclcpp::allocator::has_get_rcl_allocator_v) { + return get_allocator()->get_rcl_allocator(); + } else { + if (!plain_allocator_storage_) { + plain_allocator_storage_ = + std::make_shared(*this->get_allocator()); + } + + return rclcpp::allocator::get_rcl_allocator(*plain_allocator_storage_); + } } - return rclcpp::allocator::get_rcl_allocator(*plain_allocator_storage_); } // This is a temporal workaround, to make sure that get_allocator() diff --git a/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp b/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp index 4619b7665d..0d72456713 100644 --- a/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp +++ b/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp @@ -16,11 +16,17 @@ #include +#include "rcpputils/compile_warnings.hpp" + +RCPPUTILS_DEPRECATION_WARNING_OFF_START #include "rclcpp/allocator/allocator_common.hpp" +RCPPUTILS_DEPRECATION_WARNING_OFF_STOP -TEST(TestAllocatorCommon, retyped_allocate) { +TEST(TestAllocatorCommon, retyped_allocate) +{ std::allocator allocator; void * untyped_allocator = &allocator; + RCPPUTILS_DEPRECATION_WARNING_OFF_START void * allocated_mem = rclcpp::allocator::retyped_allocate>(1u, untyped_allocator); // The more natural check here is ASSERT_NE(nullptr, ptr), but clang static @@ -49,9 +55,12 @@ TEST(TestAllocatorCommon, retyped_allocate) { reallocated_mem, untyped_allocator); }; EXPECT_NO_THROW(code2()); + + RCPPUTILS_DEPRECATION_WARNING_OFF_STOP } TEST(TestAllocatorCommon, retyped_zero_allocate_basic) { +RCPPUTILS_DEPRECATION_WARNING_OFF_START std::allocator allocator; void * untyped_allocator = &allocator; void * allocated_mem = @@ -63,9 +72,11 @@ TEST(TestAllocatorCommon, retyped_zero_allocate_basic) { allocated_mem, untyped_allocator); }; EXPECT_NO_THROW(code()); +RCPPUTILS_DEPRECATION_WARNING_OFF_STOP } TEST(TestAllocatorCommon, retyped_zero_allocate) { +RCPPUTILS_DEPRECATION_WARNING_OFF_START std::allocator allocator; void * untyped_allocator = &allocator; void * allocated_mem = @@ -96,9 +107,11 @@ TEST(TestAllocatorCommon, retyped_zero_allocate) { reallocated_mem, untyped_allocator); }; EXPECT_NO_THROW(code2()); +RCPPUTILS_DEPRECATION_WARNING_OFF_STOP } TEST(TestAllocatorCommon, get_rcl_allocator) { +RCPPUTILS_DEPRECATION_WARNING_OFF_START std::allocator allocator; auto rcl_allocator = rclcpp::allocator::get_rcl_allocator(allocator); EXPECT_NE(nullptr, rcl_allocator.allocate); @@ -106,9 +119,12 @@ TEST(TestAllocatorCommon, get_rcl_allocator) { EXPECT_NE(nullptr, rcl_allocator.reallocate); EXPECT_NE(nullptr, rcl_allocator.zero_allocate); // Not testing state as that may or may not be null depending on platform +RCPPUTILS_DEPRECATION_WARNING_OFF_STOP } TEST(TestAllocatorCommon, get_void_rcl_allocator) { +RCPPUTILS_DEPRECATION_WARNING_OFF_START + std::allocator allocator; auto rcl_allocator = rclcpp::allocator::get_rcl_allocator>(allocator); @@ -117,4 +133,5 @@ TEST(TestAllocatorCommon, get_void_rcl_allocator) { EXPECT_NE(nullptr, rcl_allocator.reallocate); EXPECT_NE(nullptr, rcl_allocator.zero_allocate); // Not testing state as that may or may not be null depending on platform +RCPPUTILS_DEPRECATION_WARNING_OFF_STOP } diff --git a/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp b/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp index 823b00f368..c55f625531 100644 --- a/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp +++ b/rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp @@ -75,6 +75,11 @@ struct MyAllocator { typedef MyAllocator other; }; + + rcl_allocator_t get_rcl_allocator() + { + return rcl_get_default_allocator(); + } }; // Explicit specialization for void @@ -102,6 +107,11 @@ struct MyAllocator { typedef MyAllocator other; }; + + rcl_allocator_t get_rcl_allocator() + { + return rcl_get_default_allocator(); + } }; template