Make get_rcl_allocator a function family (not a template)

[stevewolter]

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]

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

[wjwwood]

CI:

• Linux
• Linux-aarch64
• macOS
• Windows

[stevewolter]

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

[wjwwood]

Yeah we have to do them all at once.

[fujitatomoya]

@stevewolter can you rebase this? this should fix https://github.com/ros2/rclcpp/issues/1925.

[stevewolter]

@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]

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

[AlexisTM]

I just tested this change locally with ASAN and it fixes https://github.com/ros2/rclcpp/issues/1948 and https://github.com/ros2/rclcpp/issues/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]

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]

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 https://github.com/ros2/realtime_support/pull/103 and https://github.com/ros2/demos/pull/467 may need to be updated along with this to make CI pass.

Thanks for taking a look, it is much appreciated!

[tylerjw]

@clalancette I pushed my attempt at fixing this PR up here: https://github.com/ros2/rclcpp/pull/2046