Make get_rcl_allocator a function family (not a template) #1324

stevewolter · 2020-09-22T11:04:33Z

As explained in #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 #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.

stevewolter · 2020-09-23T19:02:06Z

Sorry for the noise. This is ready for review now.

clalancette

I've got a few minor things to cleanup; otherwise looks pretty good to me. Pinging @wjwwood for any additional thoughts.

rclcpp/include/rclcpp/allocator/allocator_common.hpp

rclcpp/include/rclcpp/publisher_options.hpp

wjwwood

Changes lgtm, with one comment. I think this approach makes sense.

rclcpp/include/rclcpp/allocator/allocator_common.hpp

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>

Signed-off-by: Steve Wolter <swolter@google.com>

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>

Signed-off-by: Steve Wolter <swolter@google.com>

This allows us to simplify a bunch of code as well. Signed-off-by: Steve Wolter <swolter@google.com>

Co-authored-by: William Woodall <william+github@osrfoundation.org> Signed-off-by: Steve Wolter <swolter@google.com>

Signed-off-by: Steve Wolter <swolter@google.com>

wjwwood · 2020-10-26T18:00:09Z

CI:

Linux
Linux-aarch64
macOS
Windows

stevewolter · 2020-10-26T18:37:18Z

Thanks, and sorry for the delay. This will only build together with its companion changes ros2/demos#467 and ros2/realtime_support#103. Do you want to take a look at them as well, and I'll submit all three simultaneously?

wjwwood · 2020-10-26T18:52:18Z

Yeah we have to do them all at once.

wjwwood · 2020-10-27T17:03:21Z

rclcpp/include/rclcpp/allocator/allocator_common.hpp

+ * your own overload for this function.
+ */
+template<typename T>
+rcl_allocator_t get_rcl_allocator(std::allocator<T> allocator)


This needs to be in the rclcpp::allocator namespace, so that it matches the folder structure include/rclcpp/allocator/....

fujitatomoya · 2022-05-20T23:59:00Z

@stevewolter can you rebase this? this should fix #1925.

stevewolter · 2022-05-23T09:45:01Z

@fujitatomoya We've downprioritized our ROS efforts, so it will be while until I find time, sorry. Please feel free to clone the change & submit it under your name if you need it.

wjwwood · 2022-05-23T21:13:18Z

@fujitatomoya @stevewolter I think I can spend some time to revive this since it came up in one of my projects recently.

AlexisTM · 2022-06-22T12:35:02Z

I just tested this change locally with ASAN and it fixes #1948 and #1925

The error fixed is:

==12240==ERROR: AddressSanitizer: new-delete-type-mismatch on 0x625000002900 in thread T0:
  object passed to delete has wrong type:
  size of the allocated type:   8064 bytes;
  size of the deallocated type: 112 bytes.

tylerjw · 2022-10-25T04:54:50Z

I'd again like to use ASAN in CI for libraries I use that depend on rclcpp. Is there anything I can do to help finish this?

As far as I can tell, this works, and I have tested it locally, but it seems there is some confusion around the namespace this should be in. Could I get some help understanding what changes are blocking this so maybe I could try to update it?

clalancette · 2022-10-25T12:23:23Z

I'd again like to use ASAN in CI for libraries I use that depend on rclcpp. Is there anything I can do to help finish this?

Yes! If you had time to do it, it would be wonderful to rebase this onto the latest and get it compiling and working (it probably needs a few fixes before that happens).

As far as the namespace fixes, really we just need to move get_rcl_allocator back into the rclcpp::allocator namespace; that should be enough.

One other thing to note is that both ros2/realtime_support#103 and ros2/demos#467 may need to be updated along with this to make CI pass.

Thanks for taking a look, it is much appreciated!

cwecht · 2023-01-11T12:30:24Z

rclcpp/include/rclcpp/allocator/allocator_common.hpp

+/// Return the equivalent rcl_allocator_t for the C++ standard allocator.
+/**
+ * This assumes that the user intent behind both allocators is the
+ * same: Using system malloc for allocation.


AFAIK this is not exactly true: std::allocator is implemented in terms of new/delete. Although new/delete are often implemented in terms of malloc/free, it is not demanded by the C++ Standard (AFAIK, might be wrong), meaning that implementations are free to implement new/delete independently from malloc/free.

tylerjw · 2023-01-11T12:37:29Z

@clalancette I pushed my attempt at fixing this PR up here: #2046

stevewolter force-pushed the master branch 6 times, most recently from 7c015c1 to 1607da5 Compare September 23, 2020 18:57

stevewolter changed the title ~~Remove generation of RCL allocators from C++.~~ Make get_rcl_allocator a function family (not a template) Sep 23, 2020

stevewolter force-pushed the master branch from 1607da5 to 929d8f5 Compare September 23, 2020 19:01

stevewolter mentioned this pull request Sep 23, 2020

allocator_common.hpp overallocates 100x memory & invokes UB on dealloc #1254

Open

stevewolter force-pushed the master branch 4 times, most recently from d236001 to 71f3060 Compare September 24, 2020 22:01

clalancette requested changes Sep 28, 2020

View reviewed changes

rclcpp/include/rclcpp/allocator/allocator_common.hpp Outdated Show resolved Hide resolved

rclcpp/include/rclcpp/publisher_options.hpp Outdated Show resolved Hide resolved

stevewolter force-pushed the master branch 2 times, most recently from 458b80c to b4ac2cb Compare September 29, 2020 20:55

wjwwood reviewed Sep 29, 2020

View reviewed changes

rclcpp/include/rclcpp/allocator/allocator_common.hpp Outdated Show resolved Hide resolved

stevewolter force-pushed the master branch from d884f24 to efcef2f Compare September 30, 2020 18:36

stevewolter and others added 9 commits October 2, 2020 07:54

Addressed code style test failures.

b98e90f

Signed-off-by: Steve Wolter <swolter@google.com>

Update comments.

e63917f

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.

928bf4f

Signed-off-by: Steve Wolter <swolter@google.com>

Added reference to bug number.

3f97bc3

Signed-off-by: Steve Wolter <swolter@google.com>

Extend allocator lifetime in options.

fca62b2

Signed-off-by: Steve Wolter <swolter@google.com>

Drop the generic version of get_rcl_allocator.

7f0e5b8

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

af20431

Co-authored-by: William Woodall <william+github@osrfoundation.org> Signed-off-by: Steve Wolter <swolter@google.com>

Revert accidental deletion of allocator creation.

c51a72e

Signed-off-by: Steve Wolter <swolter@google.com>

stevewolter force-pushed the master branch from efcef2f to c51a72e Compare October 2, 2020 07:55

clalancette assigned wjwwood Oct 26, 2020

wjwwood approved these changes Oct 26, 2020

View reviewed changes

This was referenced Oct 26, 2020

Override get_rcl_allocator for TLSF allocators. ros2/realtime_support#103

Open

Implement get_rcl_allocator. ros2/demos#467

Open

wjwwood requested changes Oct 27, 2020

View reviewed changes

wjwwood mentioned this pull request Jan 26, 2021

[rosidl_generator_cpp] use polymorphic allocator by default ros2/rosidl#566

Open

fujitatomoya mentioned this pull request Feb 9, 2021

Undefined behavior in subscription_options.hpp #1544

Closed

clalancette mentioned this pull request Apr 23, 2021

Running the allocator_tutorial in intraprocess mode causes a segmentation fault ros2/demos#501

Closed

clalancette mentioned this pull request May 16, 2022

Empty node creates asan AddressSanitizer: new-delete-type-mismatch abort on construction #1925

Open

fujitatomoya mentioned this pull request Jun 6, 2022

AddressSanitizer: new-delete-type-mismatch in rcutils_string_map_fini ros2/rcutils#365

Closed

tylerjw mentioned this pull request Jun 6, 2022

Creating a Node in Galactic/Rolling results in ASAN errors #1948

Open

audrow changed the base branch from master to rolling June 28, 2022 14:21

fujitatomoya mentioned this pull request Nov 8, 2022

address sanitizer complains new-delete-type-mismatch over allocator_common #2045

Open

tylerjw mentioned this pull request Nov 12, 2022

Make get_rcl_allocator a function family (try 2) #2046

Open

cwecht reviewed Jan 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make get_rcl_allocator a function family (not a template) #1324

Make get_rcl_allocator a function family (not a template) #1324

stevewolter commented Sep 22, 2020 •

edited

Loading

stevewolter commented Sep 23, 2020

clalancette left a comment

wjwwood left a comment

wjwwood commented Oct 26, 2020

stevewolter commented Oct 26, 2020

wjwwood commented Oct 26, 2020

wjwwood Oct 27, 2020

fujitatomoya commented May 20, 2022

stevewolter commented May 23, 2022

wjwwood commented May 23, 2022

AlexisTM commented Jun 22, 2022

tylerjw commented Oct 25, 2022

clalancette commented Oct 25, 2022

cwecht Jan 11, 2023

tylerjw commented Jan 11, 2023

Make get_rcl_allocator a function family (not a template) #1324

Are you sure you want to change the base?

Make get_rcl_allocator a function family (not a template) #1324

Conversation

stevewolter commented Sep 22, 2020 • edited Loading

stevewolter commented Sep 23, 2020

clalancette left a comment

Choose a reason for hiding this comment

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood commented Oct 26, 2020

stevewolter commented Oct 26, 2020

wjwwood commented Oct 26, 2020

wjwwood Oct 27, 2020

Choose a reason for hiding this comment

fujitatomoya commented May 20, 2022

stevewolter commented May 23, 2022

wjwwood commented May 23, 2022

AlexisTM commented Jun 22, 2022

tylerjw commented Oct 25, 2022

clalancette commented Oct 25, 2022

cwecht Jan 11, 2023

Choose a reason for hiding this comment

tylerjw commented Jan 11, 2023

stevewolter commented Sep 22, 2020 •

edited

Loading