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 <J.Machowinski@cellumation.com>

* 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 <J.Machowinski@cellumation.com>

---------

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Co-authored-by: Janosch Machowinski <j.machowinski@cellumation.com>

Author: jmachowinski

rclcpp/include/rclcpp/allocator/allocator_common.hpp

@@ -27,10 +27,39 @@ namespace rclcpp
 namespace allocator
 {
 
+template<typename T>
+using clean_t = std::remove_cv_t<std::remove_reference_t<T>>;
+
+// Primary template: false
+template<typename, typename = std::void_t<>>
+struct has_get_rcl_allocator : std::false_type {};
+
+// Specialization: true if expression is valid
+template<typename T>
+struct has_get_rcl_allocator<T,
+  std::void_t<
+    decltype(std::declval<clean_t<T> &>().get_rcl_allocator())
+  >
+>
+  : std::bool_constant<
+    std::is_same_v<
+      decltype(std::declval<clean_t<T> &>().get_rcl_allocator()),
+      rcl_allocator_t
+    >
+  >
+{};
+
+// Helper variable template
+template<typename T>
+inline constexpr bool has_get_rcl_allocator_v =
+  has_get_rcl_allocator<T>::value;
+
 template<typename T, typename Alloc>
 using AllocRebind = typename std::allocator_traits<Alloc>::template rebind_traits<T>;
 
 template<typename Alloc>
+[[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<Alloc *>(untyped_allocator);
@@ -41,6 +70,8 @@ void * retyped_allocate(size_t size, void * untyped_allocator)
 }
 
 template<typename Alloc>
+[[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<Alloc *>(untyped_allocator);
@@ -57,6 +88,8 @@ void * retyped_zero_allocate(size_t number_of_elem, size_t size_of_elem, void *
 }
 
 template<typename T, typename Alloc>
+[[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<Alloc *>(untyped_allocator);
@@ -68,6 +101,8 @@ void retyped_deallocate(void * untyped_pointer, void * untyped_allocator)
 }
 
 template<typename T, typename Alloc>
+[[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<Alloc *>(untyped_allocator);
@@ -85,6 +120,9 @@ template<
   typename T,
   typename Alloc,
   typename std::enable_if<!std::is_same<Alloc, std::allocator<void>>::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();

rclcpp/include/rclcpp/message_memory_strategy.hpp

@@ -18,6 +18,7 @@
 #include <memory>
 #include <stdexcept>
 
+#include "rcl/allocator.h"
 #include "rcl/types.h"
 
 #include "rclcpp/allocator/allocator_common.hpp"
@@ -61,15 +62,33 @@ class MessageMemoryStrategy
     message_allocator_ = std::make_shared<MessageAlloc>();
     serialized_message_allocator_ = std::make_shared<SerializedMessageAlloc>();
     buffer_allocator_ = std::make_shared<BufferAlloc>();
-    rcutils_allocator_ = allocator::get_rcl_allocator<char, BufferAlloc>(*buffer_allocator_.get());
+    if constexpr (std::is_same_v<Alloc, std::allocator<void>>) {
+      rcutils_allocator_ = rcl_get_default_allocator();
+    } else {
+      if constexpr (rclcpp::allocator::has_get_rcl_allocator_v<Alloc>) {
+        rcutils_allocator_ = message_allocator_->get_rcl_allocator();
+      } else {
+        rcutils_allocator_ = allocator::get_rcl_allocator<char,
+            BufferAlloc>(*buffer_allocator_.get());
+      }
+    }
   }
 
   explicit MessageMemoryStrategy(std::shared_ptr<Alloc> allocator)
   {
     message_allocator_ = std::make_shared<MessageAlloc>(*allocator.get());
     serialized_message_allocator_ = std::make_shared<SerializedMessageAlloc>(*allocator.get());
     buffer_allocator_ = std::make_shared<BufferAlloc>(*allocator.get());
-    rcutils_allocator_ = allocator::get_rcl_allocator<char, BufferAlloc>(*buffer_allocator_.get());
+    if constexpr (std::is_same_v<Alloc, std::allocator<void>>) {
+      rcutils_allocator_ = rcl_get_default_allocator();
+    } else {
+      if constexpr (rclcpp::allocator::has_get_rcl_allocator_v<Alloc>) {
+        rcutils_allocator_ = allocator->get_rcl_allocator();
+      } else {
+        rcutils_allocator_ = allocator::get_rcl_allocator<char,
+            BufferAlloc>(*buffer_allocator_.get());
+      }
+    }
   }
 
   virtual ~MessageMemoryStrategy() = default;

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<PlainAllocator>(*this->get_allocator());
+    if constexpr (std::is_same_v<Allocator, std::allocator<void>>) {
+      return rcl_get_default_allocator();
+    } else {
+      if constexpr (rclcpp::allocator::has_get_rcl_allocator_v<Allocator>) {
+        return get_allocator()->get_rcl_allocator();
+      } else {
+        if (!plain_allocator_storage_) {
+          plain_allocator_storage_ =
+            std::make_shared<PlainAllocator>(*this->get_allocator());
+        }
+
+        return rclcpp::allocator::get_rcl_allocator<char>(*plain_allocator_storage_);
+      }
     }
-    return rclcpp::allocator::get_rcl_allocator<char>(*plain_allocator_storage_);
   }
 
   // This is a temporal workaround, to make sure that get_allocator()

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<void *, VoidAlloc>(*allocator_.get());
+    if constexpr (std::is_same_v<Alloc, std::allocator<void>>) {
+      return rcl_get_default_allocator();
+    } else {
+      return rclcpp::allocator::get_rcl_allocator<void *, VoidAlloc>(*allocator_.get());
+    }
   }
 
   size_t number_of_ready_subscriptions() const override

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<PlainAllocator>(*this->get_allocator());
+    if constexpr (std::is_same_v<Allocator, std::allocator<void>>) {
+      return rcl_get_default_allocator();
+    } else {
+      if constexpr (rclcpp::allocator::has_get_rcl_allocator_v<Allocator>) {
+        return get_allocator()->get_rcl_allocator();
+      } else {
+        if (!plain_allocator_storage_) {
+          plain_allocator_storage_ =
+            std::make_shared<PlainAllocator>(*this->get_allocator());
+        }
+
+        return rclcpp::allocator::get_rcl_allocator<char>(*plain_allocator_storage_);
+      }
     }
-    return rclcpp::allocator::get_rcl_allocator<char>(*plain_allocator_storage_);
   }
 
   // This is a temporal workaround, to make sure that get_allocator()

rclcpp/test/rclcpp/allocator/test_allocator_common.cpp

@@ -16,11 +16,17 @@
 
 #include <memory>
 
+#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<int> allocator;
   void * untyped_allocator = &allocator;
+  RCPPUTILS_DEPRECATION_WARNING_OFF_START
   void * allocated_mem =
     rclcpp::allocator::retyped_allocate<std::allocator<char>>(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<int> 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<int> allocator;
   void * untyped_allocator = &allocator;
   void * allocated_mem =
@@ -96,19 +107,24 @@ 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<int> allocator;
   auto rcl_allocator = rclcpp::allocator::get_rcl_allocator<int>(allocator);
   EXPECT_NE(nullptr, rcl_allocator.allocate);
   EXPECT_NE(nullptr, rcl_allocator.deallocate);
   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<void> allocator;
   auto rcl_allocator =
     rclcpp::allocator::get_rcl_allocator<void, std::allocator<void>>(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
 }

rclcpp/test/rclcpp/test_intra_process_manager_with_allocators.cpp

@@ -75,6 +75,11 @@ struct MyAllocator
   {
     typedef MyAllocator<U> other;
   };
+
+  rcl_allocator_t get_rcl_allocator()
+  {
+    return rcl_get_default_allocator();
+  }
 };
 
 // Explicit specialization for void
@@ -102,6 +107,11 @@ struct MyAllocator<void>
   {
     typedef MyAllocator<U> other;
   };
+
+  rcl_allocator_t get_rcl_allocator()
+  {
+    return rcl_get_default_allocator();
+  }
 };
 
 template<typename T, typename U>