Fully initialise parameter msgs #358

dhood · 2017-08-16T17:28:12Z

The first commit (bf3314c) prevents the following valgrind error on test_local_parameters__rmw_fastrtps_cpp.set_parameter_if_not_set in test_rclcpp

dhood@osrf-esteve:~/ros2_ws/src/ros2/system_tests [system_tests (master=)]$ valgrind -v ~/ros2_ws/build_isolated/test_rclcpp/gtest_local_parameters__rmw_fastrtps_cpp --gtest_filter=test_local_parameters__rmw_fastrtps_cpp.set_parameter_if_not_set
==3009== Memcheck, a memory error detector
==3009== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==3009== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==3009== Command: /home/dhood/ros2_ws/build_isolated/test_rclcpp/gtest_local_parameters__rmw_fastrtps_cpp --gtest_filter=test_local_parameters__rmw_fastrtps_cpp.set_parameter_if_not_set
==3009== 
--3009-- Valgrind options:
--3009--    -v
--3009-- Contents of /proc/version:
--3009--   Linux version 4.4.0-87-generic (buildd@lcy01-31) (gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.4) ) #110-Ubuntu SMP Tue Jul 18 12:55:35 UTC 2017
--3009-- 
--3009-- Arch and hwcaps: AMD64, LittleEndian, amd64-cx16-rdtscp-sse3-avx
--3009-- Page sizes: currently 4096, max supported 4096
--3009-- Valgrind library directory: /usr/lib/valgrind
--3009-- Reading syms from /home/dhood/ros2_ws/build_isolated/test_rclcpp/gtest_local_parameters__rmw_fastrtps_cpp
...
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from test_local_parameters__rmw_fastrtps_cpp
[ RUN      ] test_local_parameters__rmw_fastrtps_cpp.set_parameter_if_not_set
--3009-- REDIR: 0x5c941a0 (libc.so.6:__GI_strcpy) redirected to 0x4c31110 (__GI_strcpy)
--3009-- REDIR: 0x5c948c0 (libc.so.6:strnlen) redirected to 0x4c30ee0 (strnlen)
--3009-- REDIR: 0x5ca8520 (libc.so.6:__GI_strncpy) redirected to 0x4c31310 (__GI_strncpy)
--3009-- REDIR: 0x5c8daa0 (libc.so.6:memalign) redirected to 0x4c2ff20 (memalign)
--3009-- REDIR: 0x5c98890 (libc.so.6:__GI_stpcpy) redirected to 0x4c33f80 (__GI_stpcpy)
--3009-- Reading syms from /home/dhood/ros2_ws/install_isolated/rcl_interfaces/lib/librcl_interfaces__rosidl_typesupport_introspection_cpp.so
==3009== Conditional jump or move depends on uninitialised value(s)
==3009==    at 0x7B8AEEF: rmw_fastrtps_cpp::TypeSupport<rosidl_typesupport_introspection_cpp::MessageMembers>::serializeROSmessage(eprosima::fastcdr::Cdr&, rosidl_typesupport_introspection_cpp::MessageMembers const*, void const*) (TypeSupport_impl.hpp:323)
==3009==    by 0x7B8B122: rmw_fastrtps_cpp::TypeSupport<rosidl_typesupport_introspection_cpp::MessageMembers>::serializeROSmessage(eprosima::fastcdr::Cdr&, rosidl_typesupport_introspection_cpp::MessageMembers const*, void const*) (TypeSupport_impl.hpp:373)
==3009==    by 0x7B8B3F5: rmw_fastrtps_cpp::TypeSupport<rosidl_typesupport_introspection_cpp::MessageMembers>::serializeROSmessage(eprosima::fastcdr::Cdr&, rosidl_typesupport_introspection_cpp::MessageMembers const*, void const*) (TypeSupport_impl.hpp:440)
==3009==    by 0x7B89F1A: rmw_fastrtps_cpp::TypeSupport<rosidl_typesupport_introspection_cpp::MessageMembers>::serializeROSmessage(void const*, eprosima::fastcdr::Cdr&) (TypeSupport_impl.hpp:836)
==3009==    by 0x7B880D5: _serialize_ros_message(void const*, eprosima::fastcdr::Cdr&, void*, char const*) (ros_message_serialization.cpp:30)
==3009==    by 0x7B7BDB0: rmw_publish (rmw_publish.cpp:47)
==3009==    by 0x65EFC0E: rmw_publish (functions.cpp:252)
==3009==    by 0x67FDB67: rcl_publish (publisher.c:206)
==3009==    by 0x5377F7C: rclcpp::publisher::Publisher<rcl_interfaces::msg::ParameterEvent_<std::allocator<void> >, std::allocator<void> >::do_inter_process_publish(rcl_interfaces::msg::ParameterEvent_<std::allocator<void> > const*) (publisher.hpp:282)
==3009==    by 0x5375856: rclcpp::publisher::Publisher<rcl_interfaces::msg::ParameterEvent_<std::allocator<void> >, std::allocator<void> >::publish(std::shared_ptr<rcl_interfaces::msg::ParameterEvent_<std::allocator<void> > > const&) (publisher.hpp:217)
==3009==    by 0x5372629: rclcpp::node_interfaces::NodeParameters::set_parameters_atomically(std::vector<rclcpp::parameter::ParameterVariant, std::allocator<rclcpp::parameter::ParameterVariant> > const&) (node_parameters.cpp:107)
==3009==    by 0x53720EA: rclcpp::node_interfaces::NodeParameters::set_parameters(std::vector<rclcpp::parameter::ParameterVariant, std::allocator<rclcpp::parameter::ParameterVariant> > const&) (node_parameters.cpp:57)
==3009== 
[       OK ] test_local_parameters__rmw_fastrtps_cpp.set_parameter_if_not_set (2466 ms)
[----------] 1 test from test_local_parameters__rmw_fastrtps_cpp (2476 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (2521 ms total)
[  PASSED  ] 1 test.
==3009== 
==3009== HEAP SUMMARY:
==3009==     in use at exit: 94,393 bytes in 45 blocks
==3009==   total heap usage: 6,981 allocs, 6,936 frees, 31,776,851 bytes allocated
==3009== 
==3009== Searching for pointers to 45 not-freed blocks
==3009== Checked 486,560 bytes
==3009== 
==3009== LEAK SUMMARY:
==3009==    definitely lost: 8 bytes in 1 blocks
==3009==    indirectly lost: 48 bytes in 1 blocks
==3009==      possibly lost: 0 bytes in 0 blocks
==3009==    still reachable: 94,337 bytes in 43 blocks
==3009==         suppressed: 0 bytes in 0 blocks
==3009== Rerun with --leak-check=full to see details of leaked memory
==3009== 
==3009== Use --track-origins=yes to see where uninitialised values come from
==3009== ERROR SUMMARY: 2 errors from 1 contexts (suppressed: 0 from 0)
==3009==

The second commit (5b93dfe) explicitly initialises some messages in other locations that I saw but I can't say for sure if it's necessary or not.

It might be more appropriate for us to set default values in the message definitions themselves: I can adapt this PR if that's the case.

Linux
Linux-aarch64
macOS
Windows

Otherwise valgrind errors that data is uninitialised

dirk-thomas

Just from reviewing the code I don't see where the problem is coming from. I would feel better if I could see which exact change

dirk-thomas · 2017-08-16T17:45:43Z

rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp

@@ -70,6 +70,9 @@ NodeParameters::set_parameters_atomically(

  // TODO(jacquelinekay): handle parameter constraints
  rcl_interfaces::msg::SetParametersResult result;
+  result.successful = false;


I don't see why this is necessary. In the if branch the result variable is overridden anyway. In the else branch the boolean is being set.

dirk-thomas · 2017-08-16T17:46:04Z

rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp

@@ -70,6 +70,9 @@ NodeParameters::set_parameters_atomically(

  // TODO(jacquelinekay): handle parameter constraints
  rcl_interfaces::msg::SetParametersResult result;
+  result.successful = false;
+  result.reason = "";


std::string are already initialized int their constructor.

dirk-thomas · 2017-08-16T17:56:17Z

rclcpp/src/rclcpp/parameter.cpp

 {
  value_.type = rcl_interfaces::msg::ParameterType::PARAMETER_NOT_SET;
+  value_.bool_value = false;
+  value_.integer_value = 0;
+  value_.double_value = 0.0;


The idea is that each constructor only initialized the type as well as the type-specific value. All other value fields should never be accessed - therefore they are not initialized for a tiny improvement to the performance.

dirk-thomas · 2017-08-16T17:58:15Z

rclcpp/src/rclcpp/parameter.cpp

+  value_.integer_value = 0;
+  value_.double_value = 0.0;
+  value_.string_value = "";
+  value_.bytes_value = std::vector<uint8_t>();


std::strings as well as other std containers are initialized in their constructor.

dirk-thomas · 2017-08-16T18:00:36Z

rclcpp/src/rclcpp/parameter.cpp

 }

 ParameterVariant::ParameterVariant(const std::string & name, const bool bool_value)
-: name_(name)
+: ParameterVariant(name)


If we don't initialize the unused value fields this as well as the added optional argument to the default constructor wouldn't be necessary.

Same below.

dhood · 2017-08-16T19:10:55Z

the minimal change required to prevent the valgrind error for that specific test is initialising value_.bool_value when using an int ParameterVariant. It's because the unitialised bool is being used here https://github.com/ros2/rmw_fastrtps/blob/master/rmw_fastrtps_cpp/include/rmw_fastrtps_cpp/TypeSupport_impl.hpp#L323 but I see from the comment that uninitialised bools have been taken into consideration. Does this mean that we are happy to ignore that valgrind error?

As I mentioned the other changes were just to be conservative because I'm not sure it what context the fields are initialised. It seems that they're not necessary for the most part, and that even when they are left uninitialised it was on purpose for performance

dirk-thomas · 2017-08-16T19:30:04Z

Does this mean that we are happy to ignore that valgrind error?

I start to remember implementing #37 a while back. We should certainly try to fix the valgrind error. I am just not yet sure how to do it 😉 ~~Would it be cleaner if we add default values to this message definition instead:~~ As you suggested adding defaults to this message makes sense: https://github.com/ros2/rcl_interfaces/blob/master/rcl_interfaces/msg/ParameterValue.msg ?

dhood · 2017-08-16T19:35:19Z

Yeah I was also asking that in the initial post. If we're both thinking it then I'll go in that direction instead (specifying defaults in the message definition); it will be clearer to users what the defaults are then

dirk-thomas · 2017-08-16T19:36:45Z

If we're both thinking it then I'll go in that direction instead (specifying defaults in the message definition)

👍

For this message it makes sense since it is expected that the code using it will only touch some of the fields.

dhood · 2017-08-16T21:24:45Z

OK ros2/rcl_interfaces#18 adds the defaults to the message definition instead. This PR will likely be closed in favour of that one

* Refactor utility functions to api module Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>

dhood added 2 commits August 16, 2017 10:21

Fully initialise ParameterValue messages

bf3314c

Otherwise valgrind errors that data is uninitialised

Additional initialisation that may or may not be required

5b93dfe

dhood added the in review Waiting for review (Kanban column) label Aug 16, 2017

dhood self-assigned this Aug 16, 2017

dirk-thomas reviewed Aug 16, 2017

View reviewed changes

dhood mentioned this pull request Aug 16, 2017

Provide defaults for ParameterValue fields ros2/rcl_interfaces#18

Merged

dhood closed this Aug 18, 2017

dhood removed the in review Waiting for review (Kanban column) label Aug 18, 2017

dhood deleted the initialise_param_msgs branch August 18, 2017 01:14

mikaelarguedas mentioned this pull request Nov 1, 2017

Revert "Provide defaults for ParameterValue fields (#18)" ros2/rcl_interfaces#24

Merged

DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022

Refactor utility functions in ros2bag (ros2#358)

e05ea4f

* Refactor utility functions to api module Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fully initialise parameter msgs #358

Fully initialise parameter msgs #358

dhood commented Aug 16, 2017

dirk-thomas left a comment

dirk-thomas Aug 16, 2017

dirk-thomas Aug 16, 2017

dirk-thomas Aug 16, 2017

dirk-thomas Aug 16, 2017

dirk-thomas Aug 16, 2017

dhood commented Aug 16, 2017 •

edited

Loading

dirk-thomas commented Aug 16, 2017 •

edited

Loading

dhood commented Aug 16, 2017

dirk-thomas commented Aug 16, 2017 •

edited

Loading

dhood commented Aug 16, 2017

Fully initialise parameter msgs #358

Fully initialise parameter msgs #358

Conversation

dhood commented Aug 16, 2017

dirk-thomas left a comment

Choose a reason for hiding this comment

dirk-thomas Aug 16, 2017

Choose a reason for hiding this comment

dirk-thomas Aug 16, 2017

Choose a reason for hiding this comment

dirk-thomas Aug 16, 2017

Choose a reason for hiding this comment

dirk-thomas Aug 16, 2017

Choose a reason for hiding this comment

dirk-thomas Aug 16, 2017

Choose a reason for hiding this comment

dhood commented Aug 16, 2017 • edited Loading

dirk-thomas commented Aug 16, 2017 • edited Loading

dhood commented Aug 16, 2017

dirk-thomas commented Aug 16, 2017 • edited Loading

dhood commented Aug 16, 2017

dhood commented Aug 16, 2017 •

edited

Loading

dirk-thomas commented Aug 16, 2017 •

edited

Loading

dirk-thomas commented Aug 16, 2017 •

edited

Loading