Bugfix for rosbag2_cpp serialization converter (#1814)

* Bugfix for rosbag2 serialization converter - Use rmw specific type support for rmw_serilize{deserialize} function calls. Note: It is ok for CycloneDDS to use introspection type support for rmw_serilize{deserialize} functions. However, for FastRTPS it must be rmw specific type support. e.g. rosidl_typesupport_cpp. Fix works for both CycloneDDS and FastRTPS rmw. Signed-off-by: Michael Orlov <[email protected]> * Add test coverage for default rmv serialization format converter Signed-off-by: Michael Orlov <[email protected]> * Run test_serialization_converter for each rmw implementation - Rationale: To make sure that the default serialization converter can serialize and deserialize messages with all supported rmw implementations. Since it uses rmw specific functions for serialization and deserialization inside. Signed-off-by: Michael Orlov <[email protected]> * Address uncrustify formating warnings Signed-off-by: Michael Orlov <[email protected]> * Enable sanitizer by default Signed-off-by: Michael Orlov <[email protected]> * Address Windows build warnings Signed-off-by: Michael Orlov <[email protected]> * Revert "Enable sanitizer by default" This reverts commit 7241963. Signed-off-by: Michael Orlov <[email protected]> --------- Signed-off-by: Michael Orlov <[email protected]> (cherry picked from commit 6e82f52)
ros2 · Sep 25, 2024 · 1f814cc · 1f814cc
1 parent 61dd32f
commit 1f814cc
Show file tree

Hide file tree

Showing 10 changed files with 266 additions and 16 deletions.
diff --git a/rosbag2_cpp/CMakeLists.txt b/rosbag2_cpp/CMakeLists.txt
@@ -30,7 +30,7 @@ option(DISABLE_SANITIZERS "disables the use of gcc sanitizers" ON)
 if(NOT DISABLE_SANITIZERS AND CMAKE_COMPILER_IS_GNUCXX)
   include(CheckCXXSourceRuns)
   set(OLD_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
-  set(CMAKE_REQUIRED_FLAGS "${OLD_CMAKE_REQUIRED_FLAGS} -fsanitize=leak")
+  set(CMAKE_REQUIRED_FLAGS "${OLD_CMAKE_REQUIRED_FLAGS} -fsanitize=address,leak,undefined")
   check_cxx_source_runs("int main() {}" HAVE_SANITIZERS)
   set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQUIRED_FLAGS})
   if(NOT HAVE_SANITIZERS)
@@ -139,6 +139,7 @@ if(BUILD_TESTING)
   find_package(test_msgs REQUIRED)
   find_package(std_msgs REQUIRED)
   find_package(rosbag2_test_msgdefs REQUIRED)
+  find_package(rmw_implementation_cmake REQUIRED)
   ament_lint_auto_find_test_dependencies()
 
   add_library(
@@ -254,6 +255,30 @@ if(BUILD_TESTING)
     )
   endif()
 
+  ament_add_gmock_executable(test_serialization_converter test/rosbag2_cpp/test_serialization_converter.cpp)
+  if(TARGET test_serialization_converter)
+    if(NOT DISABLE_SANITIZERS)
+      target_compile_options(test_serialization_converter PUBLIC $<$<CXX_COMPILER_ID:GNU>:-fsanitize=address,leak,undefined>)
+      target_link_libraries(test_serialization_converter $<$<CXX_COMPILER_ID:GNU>:-fsanitize=address,leak,undefined>)
+    endif()
+    target_link_libraries(test_serialization_converter
+      ${PROJECT_NAME}
+      rosbag2_storage::rosbag2_storage
+      ${std_msgs_TARGETS}
+    )
+  endif()
+
+  function(test_serialization_converter_for_rmw_implementation)
+    set(rmw_implementation_env_var RMW_IMPLEMENTATION=${rmw_implementation})
+    ament_add_gmock_test(test_serialization_converter
+      TEST_NAME test_serialization_converter${target_suffix}
+      ENV ${rmw_implementation_env_var}
+    )
+  endfunction()
+  # Run test_serialization_converter for each rmw implementation because default serialization
+  # converter uses rmw specific functions for serialization and deserialization.
+  call_for_each_rmw_implementation(test_serialization_converter_for_rmw_implementation)
+
   ament_add_gmock(test_multifile_reader
     test/rosbag2_cpp/test_multifile_reader.cpp)
   if(TARGET test_multifile_reader)

diff --git a/rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_converter.hpp b/rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_converter.hpp
@@ -20,6 +20,7 @@
 
 #include "rosbag2_cpp/converter_interfaces/serialization_format_serializer.hpp"
 #include "rosbag2_cpp/converter_interfaces/serialization_format_deserializer.hpp"
+#include "rosbag2_cpp/visibility_control.hpp"
 
 /**
  * This is a convenience class for plugin developers.
@@ -31,7 +32,7 @@ namespace rosbag2_cpp
 namespace converter_interfaces
 {
 
-class SerializationFormatConverter
+class ROSBAG2_CPP_PUBLIC SerializationFormatConverter
   : public SerializationFormatSerializer, public SerializationFormatDeserializer
 {
 public:

diff --git a/rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_deserializer.hpp b/rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_deserializer.hpp
@@ -19,17 +19,16 @@
 #include <string>
 
 #include "rosbag2_cpp/types/introspection_message.hpp"
-
 #include "rosbag2_storage/serialized_bag_message.hpp"
-
 #include "rosidl_runtime_c/message_type_support_struct.h"
+#include "rosbag2_cpp/visibility_control.hpp"
 
 namespace rosbag2_cpp
 {
 namespace converter_interfaces
 {
 
-class SerializationFormatDeserializer
+class ROSBAG2_CPP_PUBLIC SerializationFormatDeserializer
 {
 public:
   virtual ~SerializationFormatDeserializer() = default;

diff --git a/rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_serializer.hpp b/rosbag2_cpp/include/rosbag2_cpp/converter_interfaces/serialization_format_serializer.hpp
@@ -19,17 +19,16 @@
 #include <string>
 
 #include "rosbag2_cpp/types/introspection_message.hpp"
-
 #include "rosbag2_storage/serialized_bag_message.hpp"
-
 #include "rosidl_runtime_c/message_type_support_struct.h"
+#include "rosbag2_cpp/visibility_control.hpp"
 
 namespace rosbag2_cpp
 {
 namespace converter_interfaces
 {
 
-class SerializationFormatSerializer
+class ROSBAG2_CPP_PUBLIC SerializationFormatSerializer
 {
 public:
   virtual ~SerializationFormatSerializer() = default;

diff --git a/...mented_serialization_format_converter.hpp → ...mented_serialization_format_converter.hpp b/...mented_serialization_format_converter.hpp → ...mented_serialization_format_converter.hpp
@@ -19,6 +19,15 @@
 #include <string>
 
 #include "rosbag2_cpp/converter_interfaces/serialization_format_converter.hpp"
+#include "rosbag2_cpp/visibility_control.hpp"
+
+// This is necessary because of using stl types here. It is completely safe, because
+// a) the member is not accessible from the outside
+// b) there are no inline functions.
+#ifdef _WIN32
+# pragma warning(push)
+# pragma warning(disable:4251)
+#endif
 
 namespace rosbag2_cpp
 {
@@ -31,7 +40,7 @@ class RMWImplementedConverterImpl;
  * searches the system for an installed RMW implementation that understands the requested format,
  * loads that library if found, and uses its implementation for serialization.
  */
-class RMWImplementedConverter
+class ROSBAG2_CPP_PUBLIC RMWImplementedConverter
   : public rosbag2_cpp::converter_interfaces::SerializationFormatConverter
 {
 public:
@@ -40,7 +49,7 @@ class RMWImplementedConverter
    * \throws std::runtime_error if no RMW implementation was found supporting the format.
    */
   explicit RMWImplementedConverter(const std::string & format);
-  virtual ~RMWImplementedConverter();
+  ~RMWImplementedConverter() override;
 
   void deserialize(
     std::shared_ptr<const rosbag2_storage::SerializedBagMessage> serialized_message,

diff --git a/rosbag2_cpp/package.xml b/rosbag2_cpp/package.xml
@@ -30,6 +30,7 @@
   <test_depend>ament_cmake_gmock</test_depend>
   <test_depend>ament_lint_auto</test_depend>
   <test_depend>ament_lint_common</test_depend>
+  <test_depend>rmw_implementation_cmake</test_depend>
   <test_depend>test_msgs</test_depend>
   <test_depend>std_msgs</test_depend>
   <test_depend>rosbag2_test_common</test_depend>

diff --git a/rosbag2_cpp/src/rosbag2_cpp/converter.cpp b/rosbag2_cpp/src/rosbag2_cpp/converter.cpp
@@ -67,23 +67,24 @@ std::shared_ptr<rosbag2_storage::SerializedBagMessage> Converter::convert(
   std::shared_ptr<const rosbag2_storage::SerializedBagMessage> message)
 {
   auto introspection_ts = topics_and_types_.at(message->topic_name).introspection_type_support;
+  auto rmw_ts = topics_and_types_.at(message->topic_name).rmw_type_support;
   auto allocator = rcutils_get_default_allocator();
   std::shared_ptr<rosbag2_introspection_message_t> allocated_ros_message =
     allocate_introspection_message(introspection_ts, &allocator);
-  auto output_message = std::make_shared<rosbag2_storage::SerializedBagMessage>();
 
   // deserialize
   rosbag2_cpp::introspection_message_set_topic_name(
     allocated_ros_message.get(), message->topic_name.c_str());
   allocated_ros_message->time_stamp = message->recv_timestamp;
-  input_converter_->deserialize(message, introspection_ts, allocated_ros_message);
+  input_converter_->deserialize(message, rmw_ts, allocated_ros_message);
 
-  // re-serialize
+  // re-serialize with the new serializer
+  auto output_message = std::make_shared<rosbag2_storage::SerializedBagMessage>();
   output_message->serialized_data = rosbag2_storage::make_empty_serialized_message(0);
   output_message->topic_name = std::string(allocated_ros_message->topic_name);
   output_message->recv_timestamp = message->recv_timestamp;
   output_message->send_timestamp = message->send_timestamp;
-  output_converter_->serialize(allocated_ros_message, introspection_ts, output_message);
+  output_converter_->serialize(allocated_ros_message, rmw_ts, output_message);
   return output_message;
 }
 

diff --git a/rosbag2_cpp/src/rosbag2_cpp/rmw_implemented_serialization_format_converter.cpp b/rosbag2_cpp/src/rosbag2_cpp/rmw_implemented_serialization_format_converter.cpp
@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#include "rmw_implemented_serialization_format_converter.hpp"
+#include "rosbag2_cpp/rmw_implemented_serialization_format_converter.hpp"
 
 #include <memory>
 #include <sstream>

diff --git a/rosbag2_cpp/src/rosbag2_cpp/serialization_format_converter_factory_impl.hpp b/rosbag2_cpp/src/rosbag2_cpp/serialization_format_converter_factory_impl.hpp
@@ -28,7 +28,7 @@
 #include "rosbag2_cpp/logging.hpp"
 #include "rosbag2_cpp/visibility_control.hpp"
 
-#include "./rmw_implemented_serialization_format_converter.hpp"
+#include "rosbag2_cpp/rmw_implemented_serialization_format_converter.hpp"
 
 namespace rosbag2_cpp
 {