-
Notifications
You must be signed in to change notification settings - Fork 499
Fix ASAN / valgrind warnings due to C / C++ allocator mismatch #3054
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: rolling
Are you sure you want to change the base?
Conversation
As explained in ros2#1254, there's conceptually no way to implement RCL allocators in terms of C++ allocators. In order to fix this behavior, we have to delete the generic version of get_rcl_allocator. Since some ROS code depends on this, we need to allow users to write their own version of get_rcl_allocator for allocators that support the C-style interface (most do). So this CL changes get_rcl_allocator from a template function into a family of (potentially templated) functions, which allows users to add their own overloads and rely on the "most specialized" mechanism for function specialization to select the right one. See http://www.gotw.ca/publications/mill17.htm for details. This also allows us to return get_rcl_default_allocator for all specializations of std::allocator (previously, only for std::allocator), which will already fix ros2#1254 for pretty much all clients. I'll continue to work on deleting the generic version, though, to make sure that nobody is accidentally bitten by it. I've tried to test this by doing a full ROS compilation following the Dockerfile of the source Docker image, and all packages compile. Signed-off-by: Steve Wolter <swolter@google.com> Addressed code style test failures. Signed-off-by: Steve Wolter <swolter@google.com> Update comments. Incidentally, this also reruns the flaky test suite, which seems to be using the real DDS middleware and to be prone to cross-talk. Signed-off-by: Steve Wolter <swolter@google.com> Also update recently added tests. Signed-off-by: Steve Wolter <swolter@google.com> Added reference to bug number. Signed-off-by: Steve Wolter <swolter@google.com> Extend allocator lifetime in options. Signed-off-by: Steve Wolter <swolter@google.com> Drop the generic version of get_rcl_allocator. This allows us to simplify a bunch of code as well. Signed-off-by: Steve Wolter <swolter@google.com> Update rclcpp/include/rclcpp/allocator/allocator_common.hpp Co-authored-by: William Woodall <william+github@osrfoundation.org> Signed-off-by: Steve Wolter <swolter@google.com> Revert accidental deletion of allocator creation. Signed-off-by: Steve Wolter <swolter@google.com>
|
Pulls: #3054 |
1d805dd to
d36dae6
Compare
|
Pulls: #3054 |
fujitatomoya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with green CI.
really hope we can merge it this time 🤞
|
Just a note that this does break API. |
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
d36dae6 to
aab8c9e
Compare
|
Pulls: #3054 |
Description
rclcpp passes down C++ style allocators down to rcl, that then uses them.
Due to the missing / wrong size on the deallocation, all the memory checkers
complain about it in newer ubuntu versions (>= 24.04).
This PR is a rebase / rework of #2046
Did you use Generative AI?
No