Fix -Wmaybe-uninitialized warning by ahans · Pull Request #2081 · ros2/rclcpp

ahans · 2023-01-12T15:38:04Z

gcc 12 warns about this when compiling in release mode (i.e., with -O2):

In file included from /usr/include/c++/12/unordered_map:46,
                 from /usr/include/c++/12/functional:61,
                 from /home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/include/rclcpp/context.hpp:19,
                 from /home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/src/rclcpp/context.cpp:15:
In member function ‘std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::iterator std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::_M_erase(size_type, __node_base_ptr, __node_ptr) [with _Key = std::shared_ptr<std::function<void()> >; _Value = std::shared_ptr<std::function<void()> >; _Alloc = std::allocator<std::shared_ptr<std::function<void()> > >; _ExtractKey = std::__detail::_Identity; _Equal = std::equal_to<std::shared_ptr<std::function<void()> > >; _Hash = std::hash<std::shared_ptr<std::function<void()> > >; _RangeHash = std::__detail::_Mod_range_hashing; _Unused = std::__detail::_Default_ranged_hash; _RehashPolicy = std::__detail::_Prime_rehash_policy; _Traits = std::__detail::_Hashtable_traits<false, true, true>]’,
    inlined from ‘std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::size_type std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::_M_erase(std::true_type, const key_type&) [with _Key = std::shared_ptr<std::function<void()> >; _Value = std::shared_ptr<std::function<void()> >; _Alloc = std::allocator<std::shared_ptr<std::function<void()> > >; _ExtractKey = std::__detail::_Identity; _Equal = std::equal_to<std::shared_ptr<std::function<void()> > >; _Hash = std::hash<std::shared_ptr<std::function<void()> > >; _RangeHash = std::__detail::_Mod_range_hashing; _Unused = std::__detail::_Default_ranged_hash; _RehashPolicy = std::__detail::_Prime_rehash_policy; _Traits = std::__detail::_Hashtable_traits<false, true, true>]’ at /usr/include/c++/12/bits/hashtable.h:2372:15,
    inlined from ‘std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::size_type std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::erase(const key_type&) [with _Key = std::shared_ptr<std::function<void()> >; _Value = std::shared_ptr<std::function<void()> >; _Alloc = std::allocator<std::shared_ptr<std::function<void()> > >; _ExtractKey = std::__detail::_Identity; _Equal = std::equal_to<std::shared_ptr<std::function<void()> > >; _Hash = std::hash<std::shared_ptr<std::function<void()> > >; _RangeHash = std::__detail::_Mod_range_hashing; _Unused = std::__detail::_Default_ranged_hash; _RehashPolicy = std::__detail::_Prime_rehash_policy; _Traits = std::__detail::_Hashtable_traits<false, true, true>]’ at /usr/include/c++/12/bits/hashtable.h:973:24,
    inlined from ‘std::unordered_set<_Value, _Hash, _Pred, _Alloc>::size_type std::unordered_set<_Value, _Hash, _Pred, _Alloc>::erase(const key_type&) [with _Value = std::shared_ptr<std::function<void()> >; _Hash = std::hash<std::shared_ptr<std::function<void()> > >; _Pred = std::equal_to<std::shared_ptr<std::function<void()> > >; _Alloc = std::allocator<std::shared_ptr<std::function<void()> > >]’ at /usr/include/c++/12/bits/unordered_set.h:546:26,
    inlined from ‘bool rclcpp::Context::remove_shutdown_callback(ShutdownType, const rclcpp::ShutdownCallbackHandle&)’ at /home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/src/rclcpp/context.cpp:443:34:
/usr/include/c++/12/bits/hashtable.h:2330:9: warning: ‘callback_list_ptr’ may be used uninitialized [-Wmaybe-uninitialized]
 2330 |       --_M_element_count;
      |         ^~~~~~~~~~~~~~~~
/home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/src/rclcpp/context.cpp: In member function ‘bool rclcpp::Context::remove_shutdown_callback(ShutdownType, const rclcpp::ShutdownCallbackHandle&)’:
/home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/src/rclcpp/context.cpp:425:70: note: ‘callback_list_ptr’ was declared here
  425 |     std::shared_ptr<ShutdownCallbackHandle::ShutdownCallbackType>> * callback_list_ptr;
      |                                                                      ^~~~~~~~~~~~~~~~~
In member function ‘std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::__node_ptr std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::_M_begin() const [with _Key = std::shared_ptr<std::function<void()> >; _Value = std::shared_ptr<std::function<void()> >; _Alloc = std::allocator<std::shared_ptr<std::function<void()> > >; _ExtractKey = std::__detail::_Identity; _Equal = std::equal_to<std::shared_ptr<std::function<void()> > >; _Hash = std::hash<std::shared_ptr<std::function<void()> > >; _RangeHash = std::__detail::_Mod_range_hashing; _Unused = std::__detail::_Default_ranged_hash; _RehashPolicy = std::__detail::_Prime_rehash_policy; _Traits = std::__detail::_Hashtable_traits<false, true, true>]’,
    inlined from ‘std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::const_iterator std::_Hashtable<_Key, _Value, _Alloc, _ExtractKey, _Equal, _Hash, _RangeHash, _Unused, _RehashPolicy, _Traits>::begin() const [with _Key = std::shared_ptr<std::function<void()> >; _Value = std::shared_ptr<std::function<void()> >; _Alloc = std::allocator<std::shared_ptr<std::function<void()> > >; _ExtractKey = std::__detail::_Identity; _Equal = std::equal_to<std::shared_ptr<std::function<void()> > >; _Hash = std::hash<std::shared_ptr<std::function<void()> > >; _RangeHash = std::__detail::_Mod_range_hashing; _Unused = std::__detail::_Default_ranged_hash; _RehashPolicy = std::__detail::_Prime_rehash_policy; _Traits = std::__detail::_Hashtable_traits<false, true, true>]’ at /usr/include/c++/12/bits/hashtable.h:629:16,
    inlined from ‘std::unordered_set<_Value, _Hash, _Pred, _Alloc>::const_iterator std::unordered_set<_Value, _Hash, _Pred, _Alloc>::begin() const [with _Value = std::shared_ptr<std::function<void()> >; _Hash = std::hash<std::shared_ptr<std::function<void()> > >; _Pred = std::equal_to<std::shared_ptr<std::function<void()> > >; _Alloc = std::allocator<std::shared_ptr<std::function<void()> > >]’ at /usr/include/c++/12/bits/unordered_set.h:325:26,
    inlined from ‘std::vector<std::function<void()> > rclcpp::Context::get_shutdown_callback(ShutdownType) const’ at /home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/src/rclcpp/context.cpp:479:25:
/usr/include/c++/12/bits/hashtable.h:466:62: warning: ‘callback_list_ptr’ may be used uninitialized [-Wmaybe-uninitialized]
  466 |       { return static_cast<__node_ptr>(_M_before_begin._M_nxt); }
      |                                                              ^
/home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/src/rclcpp/context.cpp: In member function ‘std::vector<std::function<void()> > rclcpp::Context::get_shutdown_callback(ShutdownType) const’:
/home/ahans/src/ros2_rolling/src/ros2/rclcpp/rclcpp/src/rclcpp/context.cpp:463:70: note: ‘callback_list_ptr’ was declared here
  463 |     std::shared_ptr<ShutdownCallbackHandle::ShutdownCallbackType>> * callback_list_ptr;

clalancette · 2023-01-12T16:01:07Z

rclcpp/src/rclcpp/context.cpp

+  std::unordered_set<std::shared_ptr<ShutdownCallbackHandle::ShutdownCallbackType>> *
+    callback_list_ptr = nullptr;


While I agree that this is a bug, I don't think that this is the right way to fix this. In particular, if the shutdown_type below ends up not being one of the cases, then we'll just go ahead and dereference the nullptr. I guess that is better in some ways than derferencing random memory, but it is not correct.

I think the proper thing to do here is probably to add a default label to the switch statement below, and probably throw an exception if it is not one of the expected shutdown types.

The same logic goes for below.

If only called with values that are handled by the switch, this is not a bug. But I agree that a more defensive approach is in order. The way the code is now, introducing actual bugs later is easier than it should be. TBH, I only wanted to silence the warning. I'm glad that you're in favor of a better solution.

The enum in the switch statements is only used internally from within context.cpp, and when calling {add,remove,get}_shutdown_callback(), it is always a constant. So this is actually a compile-time parameter. Adding a default label with a throw, i.e., detecting this at runtime, I would not find appropriate. Instead, I made this a compile-time parameter now, so we can static_assert that it has one of the supported values.

Let me know what you think!

gcc 12 warned about `callback_list_ptr` potentially being uninitialized when compiling in release mode (i.e., with `-O2`). Since `shutdown_type` is a compile-time parameter, we fix the warning by enforcing the decision at compile time. Signed-off-by: Alexander Hans <ahans@users.noreply.github.com>

ahans · 2023-01-12T22:31:55Z

rclcpp/src/rclcpp/context.cpp

+      const auto callback_shared_ptr = callback_handle.callback.lock();
+      if (callback_shared_ptr == nullptr) {
+        return false;
+      }


I'm not sure if this even needs to be protected by the mutex. I did not pull it out to retain the original behavior. But if it doesn't need to be protected, moving it up would be better.

yeah, right. callback_handle does not need to be protected by mutex in this function. probably move the mutex down here to protect the race condition for callback_set is better.

Yeah, we can move it to only protect callback_set. I doubt it makes much of a difference in practice, though.

ahans · 2023-01-12T22:32:27Z

rclcpp/include/rclcpp/context.hpp

    const ShutdownCallbackHandle & callback_handle);

+  template<ShutdownType shutdown_type>
+  RCLCPP_LOCAL


Not having the RCLCPP_LOCAL here seemed like an oversight.

good eye 👁️

clalancette

Overall, I like the approach here; it takes care of the original issue, and it should be a bit faster (since we aren't doing the switch statement at runtime).

That said, I'm confused by a C++ aspect of this. That is, how can you have template function declaration in the hpp file and the definition in the cpp file? In my understanding, templates generate code, so anyone who wants to call it necessarily needs the code in the header file (not the implementation). How does this work?

ahans · 2023-01-13T14:37:13Z

That said, I'm confused by a C++ aspect of this. That is, how can you have template function declaration in the hpp file and the definition in the cpp file? In my understanding, templates generate code, so anyone who wants to call it necessarily needs the code in the header file (not the implementation). How does this work?

You need the template definition when you instantiate a template. In the present case, the only instantiations are the calls from the three public functions in context.cpp (add_on_shutdown_callback, remove_on_shutdown_callback, and add_pre_shutdown_callback). Because we're in the same translation unit (same file even), the template definitions are present and the instantiations succeed. Since the helper methods that are now templates are all private, they cannot be called from elsewhere, and so we don't need the template definition in a header (directly or indirectly through an .inl file). Does that make sense?

ahans · 2023-01-13T14:47:18Z

There's also the pattern of explicit template instantiation that allows you to reduce compile times and improve encapsulation: When you know upfront the types a template will be instantiated with, you can put the template definition in the .cpp file along with explicit instantiations using those types. In the .hpp you only have the template declaration. As long as you only use the templated functions/classes with things that you have explicit instantiations for, this also works for public functions. But as soon as you try to instantiate with something you don't have an explicit instantiation in the .cpp, you will get a linker error. This pattern may have performance penalties, though, because the compiler can't inline as heavily anymore.

However, in the present case none of that matters, since the methods are private. Having the definitions in the header would yield the exact same code, but compilation of thing that #include "context.hpp" would take a tiny bit longer, since that header would have a bit more code to parse. The bigger argument IMHO is that the definition would unnecessarily clutter the header file.

fujitatomoya

lgtm with green CI

fujitatomoya · 2023-01-13T16:23:59Z

rclcpp/include/rclcpp/context.hpp

    const ShutdownCallbackHandle & callback_handle);

+  template<ShutdownType shutdown_type>
+  RCLCPP_LOCAL


good eye 👁️

fujitatomoya · 2023-01-13T16:32:34Z

rclcpp/src/rclcpp/context.cpp

+      const auto callback_shared_ptr = callback_handle.callback.lock();
+      if (callback_shared_ptr == nullptr) {
+        return false;
+      }


yeah, right. callback_handle does not need to be protected by mutex in this function. probably move the mutex down here to protect the race condition for callback_set is better.

clalancette · 2023-01-13T18:10:00Z

You need the template definition when you instantiate a template. In the present case, the only instantiations are the calls from the three public functions in context.cpp (add_on_shutdown_callback, remove_on_shutdown_callback, and add_pre_shutdown_callback). Because we're in the same translation unit (same file even), the template definitions are present and the instantiations succeed. Since the helper methods that are now templates are all private, they cannot be called from elsewhere, and so we don't need the template definition in a header (directly or indirectly through an .inl file). Does that make sense?

Yep, that makes sense. I figured it was something like that, but I just wanted to check. Thanks for the explanation.

clalancette

This looks good to me, except for the one change below. Once we move the mutex in the remove_callback in remove_shutdown_callback, I'll approve and run CI for it.

Signed-off-by: Alexander Hans <ahans@users.noreply.github.com>

ahans · 2023-01-13T21:14:52Z

This looks good to me, except for the one change below. Once we move the mutex in the remove_callback in remove_shutdown_callback, I'll approve and run CI for it.

Alright, pulled that block out of the lambda now!

clalancette

This looks good to me. I'm going to fire up CI on it, but I'd appreciate another look from @fujitatomoya before I merge.

clalancette · 2023-01-13T21:19:46Z

CI:

Linux
Linux-aarch64
Windows

* Fix -Wmaybe-uninitialized warning gcc 12 warned about `callback_list_ptr` potentially being uninitialized when compiling in release mode (i.e., with `-O2`). Since `shutdown_type` is a compile-time parameter, we fix the warning by enforcing the decision at compile time. Signed-off-by: Alexander Hans <ahans@users.noreply.github.com>

ahans requested review from hidmic, ivanpauno and wjwwood as code owners January 12, 2023 15:38

ahans force-pushed the fix-gcc12-maybe-uninitialized-warning branch from 206a603 to e2028c4 Compare January 12, 2023 15:39

clalancette requested changes Jan 12, 2023

View reviewed changes

ahans force-pushed the fix-gcc12-maybe-uninitialized-warning branch from e2028c4 to 70e222e Compare January 12, 2023 22:27

ahans commented Jan 12, 2023

View reviewed changes

ahans requested review from clalancette and removed request for hidmic, ivanpauno and wjwwood January 12, 2023 23:00

clalancette reviewed Jan 13, 2023

View reviewed changes

fujitatomoya approved these changes Jan 13, 2023

View reviewed changes

clalancette reviewed Jan 13, 2023

View reviewed changes

Get and check callback_shared_ptr outside of lambda

e929dc3

Signed-off-by: Alexander Hans <ahans@users.noreply.github.com>

ahans force-pushed the fix-gcc12-maybe-uninitialized-warning branch from e0f172d to e929dc3 Compare January 13, 2023 21:10

ahans requested a review from clalancette January 13, 2023 21:15

clalancette approved these changes Jan 13, 2023

View reviewed changes

clalancette merged commit 37adc03 into ros2:rolling Jan 17, 2023

		std::unordered_set<std::shared_ptr<ShutdownCallbackHandle::ShutdownCallbackType>> *
		callback_list_ptr = nullptr;

Conversation

ahans commented Jan 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

ahans commented Jan 13, 2023

Uh oh!

ahans commented Jan 13, 2023

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clalancette commented Jan 13, 2023

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

ahans commented Jan 13, 2023

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

clalancette commented Jan 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants