From d7d2c0413fbcd16d39e22087aef050da3489fc9e Mon Sep 17 00:00:00 2001 From: Janosch Machowinski Date: Wed, 11 Feb 2026 14:19:40 +0100 Subject: [PATCH 1/2] 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 --- .../include/rclcpp/allocator/allocator_common.hpp | 2 ++ rclcpp/include/rclcpp/message_memory_strategy.hpp | 15 +++++++++++++-- rclcpp/include/rclcpp/publisher_options.hpp | 11 +++++++++++ .../strategies/allocator_memory_strategy.hpp | 6 +++++- rclcpp/include/rclcpp/subscription_options.hpp | 12 ++++++++---- 5 files changed, 39 insertions(+), 7 deletions(-) diff --git a/rclcpp/include/rclcpp/allocator/allocator_common.hpp b/rclcpp/include/rclcpp/allocator/allocator_common.hpp index 12b2f383b6..396e2d7014 100644 --- a/rclcpp/include/rclcpp/allocator/allocator_common.hpp +++ b/rclcpp/include/rclcpp/allocator/allocator_common.hpp @@ -85,6 +85,8 @@ 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.")]] 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..a51002065f 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,12 @@ 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 { + rcutils_allocator_ = allocator::get_rcl_allocator(*buffer_allocator_.get()); + } } explicit MessageMemoryStrategy(std::shared_ptr allocator) @@ -69,7 +75,12 @@ 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 { + 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..72a69fd891 100644 --- a/rclcpp/include/rclcpp/publisher_options.hpp +++ b/rclcpp/include/rclcpp/publisher_options.hpp @@ -123,6 +123,10 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase rcl_allocator_t get_rcl_allocator() const { + if constexpr (std::is_same_v>) { + return rcl_get_default_allocator(); + } + if (!plain_allocator_storage_) { plain_allocator_storage_ = std::make_shared(*this->get_allocator()); @@ -139,6 +143,13 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase mutable std::shared_ptr plain_allocator_storage_; }; +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..e0e2d6798b 100644 --- a/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp +++ b/rclcpp/include/rclcpp/strategies/allocator_memory_strategy.hpp @@ -515,7 +515,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..12dc1b45ed 100644 --- a/rclcpp/include/rclcpp/subscription_options.hpp +++ b/rclcpp/include/rclcpp/subscription_options.hpp @@ -167,11 +167,15 @@ 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 (!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() From 62ab1f6b61f915bd5ccfbd2012ccb81f6e883d01 Mon Sep 17 00:00:00 2001 From: Janosch Machowinski Date: Fri, 13 Feb 2026 19:57:14 +0100 Subject: [PATCH 2/2] 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 --- .../rclcpp/allocator/allocator_common.hpp | 38 ++++++++++++++++++- .../rclcpp/message_memory_strategy.hpp | 16 ++++++-- rclcpp/include/rclcpp/publisher_options.hpp | 24 ++++++------ .../include/rclcpp/subscription_options.hpp | 13 +++++-- .../allocator/test_allocator_common.cpp | 13 +++++++ ..._intra_process_manager_with_allocators.cpp | 10 +++++ 6 files changed, 92 insertions(+), 22 deletions(-) diff --git a/rclcpp/include/rclcpp/allocator/allocator_common.hpp b/rclcpp/include/rclcpp/allocator/allocator_common.hpp index 396e2d7014..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); @@ -86,7 +121,8 @@ template< 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.")]] + "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 a51002065f..b382b15023 100644 --- a/rclcpp/include/rclcpp/message_memory_strategy.hpp +++ b/rclcpp/include/rclcpp/message_memory_strategy.hpp @@ -65,8 +65,12 @@ class MessageMemoryStrategy if constexpr (std::is_same_v>) { rcutils_allocator_ = rcl_get_default_allocator(); } else { - rcutils_allocator_ = allocator::get_rcl_allocator(*buffer_allocator_.get()); + 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()); + } } } @@ -78,8 +82,12 @@ class MessageMemoryStrategy if constexpr (std::is_same_v>) { rcutils_allocator_ = rcl_get_default_allocator(); } else { - rcutils_allocator_ = allocator::get_rcl_allocator(*buffer_allocator_.get()); + 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()); + } } } diff --git a/rclcpp/include/rclcpp/publisher_options.hpp b/rclcpp/include/rclcpp/publisher_options.hpp index 72a69fd891..b08f500a08 100644 --- a/rclcpp/include/rclcpp/publisher_options.hpp +++ b/rclcpp/include/rclcpp/publisher_options.hpp @@ -125,13 +125,18 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase { 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_); + } } - - if (!plain_allocator_storage_) { - plain_allocator_storage_ = - std::make_shared(*this->get_allocator()); - } - return rclcpp::allocator::get_rcl_allocator(*plain_allocator_storage_); } // This is a temporal workaround, to make sure that get_allocator() @@ -143,13 +148,6 @@ struct PublisherOptionsWithAllocator : public PublisherOptionsBase mutable std::shared_ptr plain_allocator_storage_; }; -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/subscription_options.hpp b/rclcpp/include/rclcpp/subscription_options.hpp index 12dc1b45ed..775ad8809d 100644 --- a/rclcpp/include/rclcpp/subscription_options.hpp +++ b/rclcpp/include/rclcpp/subscription_options.hpp @@ -170,11 +170,16 @@ struct SubscriptionOptionsWithAllocator : public SubscriptionOptionsBase if constexpr (std::is_same_v>) { return rcl_get_default_allocator(); } else { - if (!plain_allocator_storage_) { - plain_allocator_storage_ = - std::make_shared(*this->get_allocator()); + 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_); } } diff --git a/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp b/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp index 4619b7665d..c19338b9b9 100644 --- a/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp +++ b/rclcpp/test/rclcpp/allocator/test_allocator_common.cpp @@ -17,10 +17,12 @@ #include #include "rclcpp/allocator/allocator_common.hpp" +#include "rcpputils/compile_warnings.hpp" 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 +51,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 +68,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 +103,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 +115,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 +129,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