From 0a21288c06f8a47c6e0595a6cb779c0d5936ee3f Mon Sep 17 00:00:00 2001 From: methylDragon Date: Tue, 4 Apr 2023 05:09:37 -0700 Subject: [PATCH] Move dynamic type support struct to rosidl and fix mem issues Signed-off-by: methylDragon --- .../dynamic_message_type_support.hpp | 6 +- .../dynamic_serialization_support.hpp | 7 +- .../dynamic_typesupport/dynamic_message.cpp | 9 +- .../dynamic_message_type.cpp | 9 +- .../dynamic_message_type_builder.cpp | 9 +- .../dynamic_message_type_support.cpp | 84 +++++++++++-------- .../dynamic_serialization_support.cpp | 3 + 7 files changed, 65 insertions(+), 62 deletions(-) diff --git a/rclcpp/include/rclcpp/dynamic_typesupport/dynamic_message_type_support.hpp b/rclcpp/include/rclcpp/dynamic_typesupport/dynamic_message_type_support.hpp index 327f30965d..2fa222e068 100644 --- a/rclcpp/include/rclcpp/dynamic_typesupport/dynamic_message_type_support.hpp +++ b/rclcpp/include/rclcpp/dynamic_typesupport/dynamic_message_type_support.hpp @@ -43,7 +43,7 @@ namespace dynamic_typesupport * support struct, instead of rcl_dynamic_message_type_support_handle_init, * because this class will manage the lifetimes for you. * - * Do NOT call rcl_dynamic_message_type_support_handle_fini!! + * Do NOT call rcl_dynamic_message_type_support_handle_destroy!! * * This class: * - Manages the lifetime of the raw pointer. @@ -178,10 +178,6 @@ class DynamicMessageTypeSupport : public std::enable_shared_from_this rosidl_serialization_support_; - -private: - RCLCPP_PUBLIC - DynamicSerializationSupport(); }; diff --git a/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message.cpp b/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message.cpp index e7955fb05a..4b2b601317 100644 --- a/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message.cpp +++ b/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message.cpp @@ -68,8 +68,7 @@ DynamicMessage::DynamicMessage(const DynamicMessageTypeBuilder::SharedPtr dynami rosidl_dynamic_data, // Custom deleter [](rosidl_dynamic_typesupport_dynamic_data_t * rosidl_dynamic_data)->void { - rosidl_dynamic_typesupport_dynamic_data_fini(rosidl_dynamic_data); - delete rosidl_dynamic_data; + rosidl_dynamic_typesupport_dynamic_data_destroy(rosidl_dynamic_data); }); } @@ -103,8 +102,7 @@ DynamicMessage::DynamicMessage(const DynamicMessageType::SharedPtr dynamic_type) rosidl_dynamic_data, // Custom deleter [](rosidl_dynamic_typesupport_dynamic_data_t * rosidl_dynamic_data)->void { - rosidl_dynamic_typesupport_dynamic_data_fini(rosidl_dynamic_data); - delete rosidl_dynamic_data; + rosidl_dynamic_typesupport_dynamic_data_destroy(rosidl_dynamic_data); }); } @@ -131,8 +129,7 @@ DynamicMessage::DynamicMessage( rosidl_dynamic_data, // Custom deleter [](rosidl_dynamic_typesupport_dynamic_data_t * rosidl_dynamic_data)->void { - rosidl_dynamic_typesupport_dynamic_data_fini(rosidl_dynamic_data); - delete rosidl_dynamic_data; + rosidl_dynamic_typesupport_dynamic_data_destroy(rosidl_dynamic_data); }); } diff --git a/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type.cpp b/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type.cpp index 12719cdf5b..8cb299f054 100644 --- a/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type.cpp +++ b/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type.cpp @@ -60,8 +60,7 @@ DynamicMessageType::DynamicMessageType(DynamicMessageTypeBuilder::SharedPtr dyna rosidl_dynamic_type, // Custom deleter [](rosidl_dynamic_typesupport_dynamic_type_t * rosidl_dynamic_type)->void { - rosidl_dynamic_typesupport_dynamic_type_fini(rosidl_dynamic_type); - delete rosidl_dynamic_type; + rosidl_dynamic_typesupport_dynamic_type_destroy(rosidl_dynamic_type); }); } @@ -85,8 +84,7 @@ DynamicMessageType::DynamicMessageType( rosidl_dynamic_type, // Custom deleter [](rosidl_dynamic_typesupport_dynamic_type_t * rosidl_dynamic_type)->void { - rosidl_dynamic_typesupport_dynamic_type_fini(rosidl_dynamic_type); - delete rosidl_dynamic_type; + rosidl_dynamic_typesupport_dynamic_type_destroy(rosidl_dynamic_type); }); } @@ -171,8 +169,7 @@ DynamicMessageType::init_from_description( rosidl_dynamic_type, // Custom deleter [](rosidl_dynamic_typesupport_dynamic_type_t * rosidl_dynamic_type)->void { - rosidl_dynamic_typesupport_dynamic_type_fini(rosidl_dynamic_type); - delete rosidl_dynamic_type; + rosidl_dynamic_typesupport_dynamic_type_destroy(rosidl_dynamic_type); }); } diff --git a/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type_builder.cpp b/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type_builder.cpp index c6ec6e4305..ecdec0b453 100644 --- a/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type_builder.cpp +++ b/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type_builder.cpp @@ -72,8 +72,7 @@ DynamicMessageTypeBuilder::DynamicMessageTypeBuilder( rosidl_dynamic_type_builder, // Custom deleter [](rosidl_dynamic_typesupport_dynamic_type_builder_t * rosidl_dynamic_type_builder)->void { - rosidl_dynamic_typesupport_dynamic_type_builder_fini(rosidl_dynamic_type_builder); - delete rosidl_dynamic_type_builder; + rosidl_dynamic_typesupport_dynamic_type_builder_destroy(rosidl_dynamic_type_builder); }); } @@ -165,8 +164,7 @@ DynamicMessageTypeBuilder::init_from_description( rosidl_dynamic_type_builder, // Custom deleter [](rosidl_dynamic_typesupport_dynamic_type_builder_t * rosidl_dynamic_type_builder)->void { - rosidl_dynamic_typesupport_dynamic_type_builder_fini(rosidl_dynamic_type_builder); - delete rosidl_dynamic_type_builder; + rosidl_dynamic_typesupport_dynamic_type_builder_destroy(rosidl_dynamic_type_builder); }); } @@ -201,8 +199,7 @@ DynamicMessageTypeBuilder::init_from_serialization_support_( rosidl_dynamic_type_builder, // Custom deleter [](rosidl_dynamic_typesupport_dynamic_type_builder_t * rosidl_dynamic_type_builder)->void { - rosidl_dynamic_typesupport_dynamic_type_builder_fini(rosidl_dynamic_type_builder); - delete rosidl_dynamic_type_builder; + rosidl_dynamic_typesupport_dynamic_type_builder_destroy(rosidl_dynamic_type_builder); }); } diff --git a/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type_support.cpp b/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type_support.cpp index de8f63875c..cc558297d8 100644 --- a/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type_support.cpp +++ b/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_message_type_support.cpp @@ -21,6 +21,7 @@ #include #include +#include "rcl/allocator.h" #include "rcl/dynamic_message_type_support.h" #include "rcl/type_hash.h" #include "rcl/types.h" @@ -76,9 +77,24 @@ DynamicMessageTypeSupport::DynamicMessageTypeSupport( // NOTE(methylDragon): Not technically const correct, but since it's a const void *, // we do it anyway... - auto ts_impl = static_cast(ts->data); + auto ts_impl = static_cast(ts->data); + + // NOTE(methylDragon): We don't destroy the rosidl_message_type_support->data since its members + // are managed by the passed in SharedPtr wrapper classes. We just delete it. + rosidl_message_type_support_.reset( + ts, + [](rosidl_message_type_support_t * ts) -> void { + auto ts_impl = static_cast(ts->data); + auto allocator = rcl_get_default_allocator(); + + // These are all C allocated + allocator.deallocate(ts_impl->type_hash, &allocator.state); + allocator.deallocate( + const_cast(ts_impl), &allocator.state); + allocator.deallocate(ts, &allocator.state); + } + ); - manage_rosidl_message_type_support_(ts); manage_description_(ts_impl->type_description); serialization_support_ = DynamicSerializationSupport::make_shared(ts_impl->serialization_support); @@ -136,9 +152,21 @@ DynamicMessageTypeSupport::DynamicMessageTypeSupport( throw std::runtime_error("rosidl message type support is of the wrong type"); } - auto ts_impl = static_cast(ts->data); + auto ts_impl = static_cast(ts->data); + + // NOTE(methylDragon): We don't finalize the rosidl_message_type_support->data since its members + // are managed by the passed in SharedPtr wrapper classes. We just delete it. + rosidl_message_type_support_.reset( + ts, + [](rosidl_message_type_support_t * ts) -> void { + auto ts_impl = static_cast(ts->data); - manage_rosidl_message_type_support_(ts); + // These are allocated with new + delete ts_impl->type_hash; // Only because we should've allocated it here + delete ts_impl; + delete ts; + } + ); manage_description_(ts_impl->type_description); dynamic_message_type_ = DynamicMessageType::make_shared( @@ -237,24 +265,6 @@ DynamicMessageTypeSupport::DynamicMessageTypeSupport( DynamicMessageTypeSupport::~DynamicMessageTypeSupport() {} -void -DynamicMessageTypeSupport::manage_rosidl_message_type_support_( - rosidl_message_type_support_t * rosidl_message_type_support) -{ - // NOTE(methylDragon): We don't finalize the rosidl_message_type_support->data since its members - // are managed by the passed in SharedPtr wrapper classes. We just delete it. - rosidl_message_type_support_.reset( - rosidl_message_type_support, - [](rosidl_message_type_support_t * ts) -> void { - auto ts_impl = static_cast(ts->data); - delete ts_impl->type_hash; // Only because we should've allocated it here - delete ts_impl; - delete ts; - } - ); -} - - void DynamicMessageTypeSupport::manage_description_( rosidl_runtime_c__type_description__TypeDescription * description) @@ -299,16 +309,18 @@ DynamicMessageTypeSupport::init_rosidl_message_type_support_( throw std::runtime_error("failed to get type hash"); } - rmw_dynamic_message_type_support_impl_t * ts_impl = new rmw_dynamic_message_type_support_impl_t{ - type_hash.get(), // type_hash - description, // type_description - nullptr, // NOTE(methylDragon): Not supported for now // type_description_sources - serialization_support->get_rosidl_serialization_support(), // serialization_support - dynamic_message_type->get_rosidl_dynamic_type(), // dynamic_message_type - dynamic_message->get_rosidl_dynamic_data() // dynamic_message + rosidl_dynamic_message_type_support_impl_t * ts_impl = + new rosidl_dynamic_message_type_support_impl_t { + type_hash.get(), // type_hash + description, // type_description + nullptr, // NOTE(methylDragon): Not supported for now // type_description_sources + serialization_support->get_rosidl_serialization_support(), // serialization_support + dynamic_message_type->get_rosidl_dynamic_type(), // dynamic_message_type + dynamic_message->get_rosidl_dynamic_data() // dynamic_message }; if (!ts_impl) { - throw std::runtime_error("Could not allocate rmw_dynamic_message_type_support_impl_t struct"); + throw std::runtime_error( + "Could not allocate rosidl_dynamic_message_type_support_impl_t struct"); return; } @@ -320,15 +332,17 @@ DynamicMessageTypeSupport::init_rosidl_message_type_support_( ts_impl, // data get_message_typesupport_handle_function, // func // get_type_hash_func - rmw_dynamic_message_type_support_get_type_hash_function, + rosidl_get_dynamic_message_type_support_type_hash_function, // get_type_description_func - rmw_dynamic_message_type_support_get_type_description_function, + rosidl_get_dynamic_message_type_support_type_description_function, // get_type_description_sources_func - rmw_dynamic_message_type_support_get_type_description_sources_function + rosidl_get_dynamic_message_type_support_type_description_sources_function }, [](rosidl_message_type_support_t * ts) -> void { - auto ts_impl = static_cast(ts->data); - delete ts_impl->type_hash; // Only because we should've allocated it here + auto ts_impl = static_cast(ts->data); + auto allocator = rcl_get_default_allocator(); + // Only because we should've allocated it here (also it's C allocated) + allocator.deallocate(ts_impl->type_hash, &allocator.state); delete ts_impl; } ); diff --git a/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_serialization_support.cpp b/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_serialization_support.cpp index e887a3a46d..72a163daa2 100644 --- a/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_serialization_support.cpp +++ b/rclcpp/src/rclcpp/dynamic_typesupport/dynamic_serialization_support.cpp @@ -28,6 +28,9 @@ using rclcpp::dynamic_typesupport::DynamicSerializationSupport; // CONSTRUCTION ==================================================================================== +DynamicSerializationSupport::DynamicSerializationSupport() +: DynamicSerializationSupport::DynamicSerializationSupport("") {}; + DynamicSerializationSupport::DynamicSerializationSupport( const std::string & serialization_library_name) : rosidl_serialization_support_(nullptr)