Fix ASAN / valgrind warnings due to C / C++ allocator mismatch#3054
Open
jmachowinski wants to merge 2 commits intoros2:rollingfrom
Open
Fix ASAN / valgrind warnings due to C / C++ allocator mismatch#3054jmachowinski wants to merge 2 commits intoros2:rollingfrom
jmachowinski wants to merge 2 commits intoros2:rollingfrom
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>
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Collaborator
Author
|
Pulls: #3054
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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