[rosidl_generator_cpp] use polymorphic allocator by default

[wjwwood]

Feature request

Feature description

I think we should either have a variant of the message structure that uses the newer polymorphic allocator pattern introduced in C++17, or we should altogether replace the use of the STL allocator (what we currently have with the Allocator template argument) with the new polymorphic allocator.

Root Issue

We've known for a while now that providing a custom allocator for messages and using them with rclcpp will not work, because at the rcl/rmw layer the messages are type erased and then in the middlewares they are indiscriminately cast back to the message type with the assumption that the standard allocator was used. This is a problem if the size of the allocator is different because that will change the size of things that store the allocator, like std::vector or std::string, and therefore change the size of the message itself and/or change the offest of fields in the message.

I wrote a simple example to demonstrate this:

#include <string>

template <typename AllocatorT>
struct Msg
{
  std::basic_string<char, std::char_traits<char>, AllocatorT> a;
  int c;
  std::basic_string<char, std::char_traits<char>, AllocatorT> b;
};

template <class T>
struct MyAllocator : public std::allocator<T>
{
  char padding[16];
};

int main()
{
  Msg<std::allocator<char>> foo;
  void * ptr = &foo;
  auto & bar = *static_cast<Msg<MyAllocator<char>> *>(ptr);
  // size of foo and bar are different (on macOS at least)
  // address of foo and bar should be the same
  // address of foo.c and bar.c are different (on macOS at least)
  printf("sizeof(foo) == %zu\n", sizeof(foo));
  printf("&foo == %p\n", static_cast<void *>(&foo));
  printf("&foo.c == %p\n", static_cast<void *>(&foo.c));
  printf("sizeof(bar) == %zu\n", sizeof(bar));
  printf("&bar == %p\n", static_cast<void *>(&bar));
  printf("&bar.c == %p\n", static_cast<void *>(&bar.c));
  return 0;
}

https://gist.github.com/wjwwood/6897b004a65fec1c72d6ce48813f740c

On Ubuntu 18.04 using clang7 it doesn't matter:

sizeof(foo) == 72
&foo == 0x7ffc17f55480
&foo.c == 0x7ffc17f554a0
sizeof(bar) == 72
&bar == 0x7ffc17f55480
&bar.c == 0x7ffc17f554a0

But on macOS it does matter:

sizeof(foo) == 56
&foo == 0x7ffee125b880
&foo.c == 0x7ffee125b898
sizeof(bar) == 88
&bar == 0x7ffee125b880
&bar.c == 0x7ffee125b8a8

And on Windows it also seems to not matter:

sizeof(foo) == 60
&foo == 00CFF780
&foo.c == 00CFF79C
sizeof(bar) == 60
&bar == 00CFF780
&bar.c == 00CFF79C

But in any case it's a very bad assumption to make.

This was mentioned in this review of this repository:

• Review of the rosidl_generator_cpp package #447

And our improper use of the std allocator has been noticed in these issues (though switching to the polymorphic allocator may not fix these by itself):

• allocator_common.hpp overallocates 100x memory & invokes UB on dealloc rclcpp#1254 and Make get_rcl_allocator a function family (not a template) rclcpp#1324
• UBSAN: runtime error publisher_options.hpp:67:62: runtime error: reference binding to null pointer of type 'std::__1::shared_ptr<std::__1::allocator<void> >::element_type' (aka 'std::__1::allocator<void>') rclcpp#849 (comment)

Implementation considerations

This has some important ramifications. Instead of using std::string and std::vector we would need to use the pmr variants of these. They have the same API, but they use a different default allocator type in their template argument. Also it will not be possible to move or swap them with the std:: equivalents but I haven't investigated that. Here are their references (many are documented with the normal version, having a version in the std::pmr namespace as well):

• https://en.cppreference.com/w/cpp/string/basic_string
• https://en.cppreference.com/w/cpp/container/vector
• https://en.cppreference.com/w/cpp/container/map

Just to name a few.

After reading about it to write this issue up, I think these may be the steps to take:

• change the "normal" struct alias (typedef) to use the polymorphic allocator by default, similar to how the std declares its variants
  • e.g.: using vector = std::vector<T, std::pmr::polymorphic_allocator<T>> so we'd use using MyMessage = MyMessage_<std::pmr::polymorphic_allocator<MyMessage>> or something like that...
• remove or deprecate (or deprecate and move into a different namespace) the existing message struct with a trailing _
  • I would advocate for this because changing the allocator to something other than the polymorphic allocator would never be supported by rclcpp
  • Alternatively we could leave the ability to change the type of the allocator via a template argument but assert in rclcpp that the polymorphic allocator is always used.

These steps would immediately impact rclcpp and all the rmw code generators that operate on these types because they all use (as far as I know) the non-trailing underscore "default" from this package, so we'd be changing the default and affecting them all.

Another Potential Change

While we're at it, the allocator design in our messages is a bit strange. It lets you set a general purpose allocator for the entire message, but that allocator is "rebound" to the type need for each field. For example, if you have two fields in your message, one vector of ints and another vector of floats, they will use the same allocator each. But the containers in the std don't work like this usually. They have you specify an allocator for the element in them, it's just that they usually only have one element so they just have one allocator to be specified.

It's possible that a more correct thing to do would be to have a separate allocator argument in the constructor of our message for each field that could take an allocator.

We'd still have to limit it to a polymorphic allocator, since we type erase, but it could be a different polymorphic allocator for each field that has it's own container with its own allocator.

[nightduck]

I know the C std libary doesn't fully support C++17 yet, but has there been any work done on this? I'd like to throw some development hours at it, and any existing code or notes would be great.