Static Allocator - Aligned allocations

[emersonknapp]

• Remove unwanted main symbol
• Centralize copy-pasted code for getting static allocator
• Align the memory pool and returned memory from in the static allocator to std::max_align_t in order to make it all safe for optimized builds

Fixes #30
Unblocks ros2/rmw_fastrtps#272

Signed-off-by: Emerson Knapp [email protected]

[emersonknapp]

@thomas-moulard Can you please run CI for this PR? Unfortunately we cannot restrict the test or build packages very much.

Gist: https://gist.githubusercontent.com/emersonknapp/c3646b3d48b6ad07b1c9144eac98dff7/raw/fdc4bae8a7a945f689c6748cf6fc66187b61df5c/ros2.repos
Additional BUILD args: --packages-up-to test_rclcpp test_communication test_cli test_cli_remapping rclpy
Additional TEST args: --packages-up-to test_rclcpp test_communication test_cli test_cli_remapping rclpy
CI Job: ci_launcher

[thomas-moulard]

Code LGTM, we could clean up a few things but it's fairly minor. This PR lacks unit tests.

[thomas-moulard]

• Linux
• Linux-aarch64
• macOS
• Windows

[emersonknapp]

Any thoughts on how to unit test this? I am not able to reproduce the observed problem in a toy example - only in the context of a specific system dynamically linking against OpenSSL, using an optimized build of glibc6

[emersonknapp]

@thomas-moulard The build failed because my arguments were wrong - the test and build args should have been:

--packages-up-to test_rclcpp test_communication test_cli test_cli_remapping rclpy

[thomas-moulard]

• Linux
• Linux-aarch64
• macOS
• Windows
  Triggering a new build of ci_linux

[thomas-moulard]

What about instantiating a static allocator in the unit test and checking the addresses are actually aligned?
Obviously a repro-case would be best, but it will be hard to write. I think we can find a trade-off there between repro-case and no tests.

[wjwwood]

In general, looks good to me. It might fix aarch64 too, since we've had alignment issue on ARM in the past. Thanks for looking into it, I know the code is hairy :x

[thomas-moulard]

Thanks for considering adding unit tests. I don't think that I need to take another pass on this so I'll LGTM it now 👍

[emersonknapp]

@wjwwood I've addressed the formatting concerns and added in the statics. I'm looking at test_memory_tools.cpp and trying to figure how to add a unit test for static_allocator.hpp, which is buried in the source directory. Any ideas on that without having to restructure the project?

Other than that, it looks like the test failures on CI were test_cli.test_params_yaml timeout - which I am getting locally with or without this chage (btw do you know if there is an open issue for that? if not should I open it on system_tests? i think it's one of those things we keep seeing)

[emersonknapp]

I've just pushed a commit that is an attempt at exposing the src dir to the tests without restructuring things - lmk if this is ok or if it's a kludge - I wouldn't consider myself enough of a CMake expert to determine.

[jacobperron]

Other than that, it looks like the test failures on CI were test_cli.test_params_yaml timeout - which I am getting locally with or without this chage (btw do you know if there is an open issue for that? if not should I open it on system_tests? i think it's one of those things we keep seeing)

Here's the issue for the known flake ros2/build_farmer#166

[dirk-thomas]

Testing rclcpp and test_rclcpp:

• Linux
• Linux-aarch64
• macOS
• Windows

[emersonknapp]

Regarding time source test failures in above - I am able to reproduce those locally on master ros2.repos without this patch.

As for this patch, I believe the only packages this really affects are rcl and rcutils, because from my understanding those are the only packages whose tests LD_PRELOAD the memory_tools_interpose.so library. (Those are only packages that mention the LIBRARY_PRELOAD_ENVIRONMENT_VARIABLE property exported from here)

Locally, if I run the rcl tests without this patch on master ros2.repos, I get segfaults in the following tests

The following tests FAILED:                                                                                                                                                                                                     
          8 - test_time__rmw_fastrtps_cpp (Failed)                                                                                                                                                                              
         10 - test_context__rmw_fastrtps_cpp (Failed)                                                                                                                                                                           
         17 - test_init__rmw_fastrtps_cpp (Failed)                                                                                                                                                                              
         18 - test_node__rmw_fastrtps_cpp (Failed)                                                                                                                                                                              
         22 - test_guard_condition__rmw_fastrtps_cpp (Failed)                                                                                                                                                                   
         32 - test_time__rmw_fastrtps_dynamic_cpp (Failed)                                                                                                                                                                      
         34 - test_context__rmw_fastrtps_dynamic_cpp (Failed)                                                                                                                                                                   
         41 - test_init__rmw_fastrtps_dynamic_cpp (Failed)                                                                                                                                                                      
         42 - test_node__rmw_fastrtps_dynamic_cpp (Failed)                                                                                                                                                                      
         46 - test_guard_condition__rmw_fastrtps_dynamic_cpp (Failed)                                                                                                                                                           
Errors while running CTest                                                                                                                                                                                                      
--- stderr: rcl                                                                                                                                                                                                                 
Errors while running CTest                                                           

But with this patch, 100% of rcl tests pass.

I don't know if the buildfarm is witnessing the same segfaults - it is possible that it is a very subtle combination of variables. It would be nice to see the current state of master compared to this build for rcl and rcutils

[jacobperron]

The changes look okay to me. I'll trigger a set of builds for comparison.

[jacobperron]

In addition to the current state of master I've also triggered a build with Fast-RTPS 1.7 to see if the time source test failures are coming from something introduced in Fast-RTPS 1.8.

Linux CI

• master:
• this PR:
• this PR + Fast-RTPS 1.7

[jacobperron]

I guess after ros2/rmw_fastrtps#272, we can't expect Fast-RTPS 1.7 to build.

Edit: I've re-triggered with rmw_fastrtps HEAD~1

[dirk-thomas]

The CI results are a strict improvement over the current master. Merging...