Use RCUtils allocators in rosidl_generator_c #584

pablogs9 · 2021-04-26T08:10:54Z

This PR updates the usage of dynamic memory in rosidl_generator_c to rely on rcutils allocators.

As discussed in #306 that had a not priority because rosidl_generator_c was mainly used in rclpy and some rcl tests.

But now micro-ROS relies completely on the C99 infrastructure of ROS 2 and, for us, in some scenarios is not feasible to use the common dynamic memory API (malloc, free,...).

Initial Signed-off-by: Your Name <you@example.com>

Signed-off-by: Pablo Garrido <pablogs9@gmail.com>

pablogs9 · 2021-04-26T09:01:22Z

Please @clalancette @sloretz let us know your opinions.
And if everything is ok, is there any possibility of backporting this to Foxy?

CC: @Acuadros95

sloretz

I think the package.xml needs a <build_export_depend>rcutils</ so downstream projects get rcutils when they build the generated target and a <exec_depend>rcutils</ so downstream projects have librcutils.so around when their targets run.

rosidl_generator_c/CMakeLists.txt

sloretz · 2021-04-26T16:26:32Z

rosidl_generator_c/resource/msg__functions.c.em

@@ -235,14 +237,15 @@ for line in lines:
 @(message_typename) *
 @(message_typename)__create()
 {
-  @(message_typename) * msg = (@(message_typename) *)malloc(sizeof(@(message_typename)));
+  rcutils_allocator_t allocator = rcutils_get_default_allocator();


in some scenarios is not feasible to use the common dynamic memory API (malloc, free,...).

I think the default allocator uses malloc() and free(), and this appears to give no way to change which allocator is used. The __create() function would need to accept an allocator as an argument for that to be changed.

I would also recommend adding an rcutils_allocator_t to the message struct so the __destroy() function is guaranteed to use the same allocator.

is there any possibility of backporting this to Foxy?

If we modify the function signature then this can't be backported because it breaks ABI. If hard coding the default allocator is good enough for your use case (maybe it's enough to be able to LD_PRELOAD a replacement for rcutils_get_default_allocator()?) then it seems like that could be backported.

Regarding the allocation functions that are retrieved with rcutils_get_default_allocator that's true that modifying the default allocators can be tricky.
But by now, in micro-ROS, we use a slightly modified version of the rcutils (stunned for embedded purposes) that has rcutils_set_default_allocator functionality. We re planning to PR some of these changes to the rcutils mainline.

So, in that sense, in our opinion its ok to retrieve a global-wise allocator with rcutils_get_default_allocator and ABI/API is not modified so we can backport.

Ah, that makes sense. Mind opening an issue to change these APIs to accept a non-default allocator in the future?

I'm interested in being able to pass a non-default allocator. I think we can use this existing issue to track progress: #306

Signed-off-by: Pablo Garrido <pablogs9@gmail.com>

pablogs9 · 2021-04-27T09:22:41Z

@sloretz I also have added in last commit with rosidl_runtime_c modifications

Signed-off-by: Pablo Garrido <pablogs9@gmail.com>

pablogs9 · 2021-05-04T07:26:44Z

Friendly ping @sloretz

sloretz

LGTM with green CI

sloretz · 2021-05-04T17:50:04Z

CI (build: --packages-above-and-dependencies rosidl_generator_c rosidl_runtime_c test: --packages-above rosidl_generator_c rosidl_runtime_c)

Linux
Linux-aarch64
macOS
Windows

pablogs9 · 2021-05-05T06:27:59Z

Are these unstable labels a problem? What can I do for solving them?

If they are not, as soon as you merge I'm going to prepare a backport to foxy.

Thanks a lot for your reviews and time @sloretz.

sloretz · 2021-05-05T17:44:46Z

CI LGTM!

Linux CI failure appears to be ros2/ros2cli#630
Linux ARM CI failures are also in https://ci.ros2.org/view/nightly/job/nightly_linux-aarch64_repeated/1585/#showFailuresLink

If they are not, as soon as you merge I'm going to prepare a backport to foxy.

No need if it's just a cherry-pick then Mergify.io bot can handle that.

sloretz · 2021-05-05T17:47:10Z

@Mergifyio backport galactic foxy

* Initial Initial Signed-off-by: Your Name <you@example.com> * Move include Signed-off-by: Pablo Garrido <pablogs9@gmail.com> * Update package.xml Signed-off-by: Pablo Garrido <pablogs9@gmail.com> * Add rosidl_runtime_c rcutils allocators Signed-off-by: Pablo Garrido <pablogs9@gmail.com> (cherry picked from commit 44105c6)

mergify · 2021-05-05T17:47:35Z

Command backport galactic foxy: success

Backports have been created

#586 Use RCUtils allocators in rosidl_generator_c (backport #584) has been created for branch galactic

#587 Use RCUtils allocators in rosidl_generator_c (backport #584) has been created for branch foxy

sloretz · 2021-05-05T17:52:45Z

Thanks for the contribution @pablogs9 !

clalancette · 2021-05-06T13:33:32Z

@sloretz @pablogs9 This PR added a regression on all CI jobs; one example is https://ci.ros2.org/view/nightly/job/nightly_linux-aarch64_debug/1594/testReport/junit/(root)/projectroot/cpplint_rosidl_generated_c/ .

The problem I think is that we re-use the msg__functions.c.em file twice when we build services. And when we do that, this PR now unconditionally includes rcutils/allocator.h twice, which cpplint doesn't like. I think have a solution in #590; please take a look.

sloretz · 2021-05-06T16:29:03Z

@sloretz @pablogs9 This PR added a regression on all CI jobs; one example is https://ci.ros2.org/view/nightly/job/nightly_linux-aarch64_debug/1594/testReport/junit/(root)/projectroot/cpplint_rosidl_generated_c/ .

Ah shoot. It looks like I put the repos file in thebranch instead of the repos_url field when I ran CI, so this change didn't actually get tested before I merged it 😶

10:51:24 II> Attempting to switch all repositories to the 'https://gist.githubusercontent.com/sloretz/f52090452ceb0a50e6fc954c8b896221/raw/15f9723dbeb8c2938f7d5be23589ac56566517b1/ros2.repos' branch

* Initial Initial Signed-off-by: Your Name <you@example.com> * Move include Signed-off-by: Pablo Garrido <pablogs9@gmail.com> * Update package.xml Signed-off-by: Pablo Garrido <pablogs9@gmail.com> * Add rosidl_runtime_c rcutils allocators Signed-off-by: Pablo Garrido <pablogs9@gmail.com> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Initial

bdeaa15

Initial Signed-off-by: Your Name <you@example.com>

pablogs9 force-pushed the feature/use_rcutils_allocators branch from 13a1ded to bdeaa15 Compare April 26, 2021 08:13

Move include

286e2ac

Signed-off-by: Pablo Garrido <pablogs9@gmail.com>

pablogs9 force-pushed the feature/use_rcutils_allocators branch from 938f2cc to 286e2ac Compare April 26, 2021 08:33

sloretz requested changes Apr 26, 2021

View reviewed changes

Update package.xml

4a09fa6

Signed-off-by: Pablo Garrido <pablogs9@gmail.com>

pablogs9 force-pushed the feature/use_rcutils_allocators branch from 5447cbd to 4a09fa6 Compare April 27, 2021 05:40

pablogs9 requested a review from sloretz April 27, 2021 05:57

pablogs9 mentioned this pull request Apr 27, 2021

Release rclc for Galactic ros2/rclc#86

Closed

4 tasks

pablogs9 force-pushed the feature/use_rcutils_allocators branch from c78c806 to f51e1b7 Compare April 27, 2021 09:21

pablogs9 force-pushed the feature/use_rcutils_allocators branch 3 times, most recently from e70849e to 897fc3f Compare April 27, 2021 10:47

Add rosidl_runtime_c rcutils allocators

80db3fb

Signed-off-by: Pablo Garrido <pablogs9@gmail.com>

pablogs9 force-pushed the feature/use_rcutils_allocators branch from 897fc3f to 80db3fb Compare April 27, 2021 11:01

This was referenced Apr 27, 2021

Add parameter server functionality ros2/rclc#78

Closed

Add parameter server [fix CI] ros2/rclc#88

Merged

sloretz approved these changes May 4, 2021

View reviewed changes

sloretz merged commit 44105c6 into ros2:master May 5, 2021

mergify bot mentioned this pull request May 5, 2021

Use RCUtils allocators in rosidl_generator_c (backport #584) #586

Closed

mergify bot mentioned this pull request May 5, 2021

Use RCUtils allocators in rosidl_generator_c (backport #584) #587

Closed

sloretz mentioned this pull request May 6, 2021

Fix a cpplint allocator regression. #590

Merged

pablogs9 deleted the feature/use_rcutils_allocators branch May 7, 2021 07:39

This was referenced Jul 15, 2021

[Galactic] backport both #584 and #590 #603

Closed

[Foxy] backport both #584 and #590 #604

Merged

jacobperron mentioned this pull request May 3, 2022

Investigate using rcutils allocators for the C generators #306

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use RCUtils allocators in rosidl_generator_c #584

Use RCUtils allocators in rosidl_generator_c #584

pablogs9 commented Apr 26, 2021 •

edited

Loading

pablogs9 commented Apr 26, 2021

sloretz left a comment

sloretz Apr 26, 2021

pablogs9 Apr 27, 2021

sloretz May 4, 2021

jacobperron May 3, 2022

pablogs9 commented Apr 27, 2021 •

edited

Loading

pablogs9 commented May 4, 2021

sloretz left a comment

sloretz commented May 4, 2021

pablogs9 commented May 5, 2021

sloretz commented May 5, 2021

sloretz commented May 5, 2021

mergify bot commented May 5, 2021

sloretz commented May 5, 2021

clalancette commented May 6, 2021

sloretz commented May 6, 2021

Use RCUtils allocators in rosidl_generator_c #584

Use RCUtils allocators in rosidl_generator_c #584

Conversation

pablogs9 commented Apr 26, 2021 • edited Loading

pablogs9 commented Apr 26, 2021

sloretz left a comment

Choose a reason for hiding this comment

sloretz Apr 26, 2021

Choose a reason for hiding this comment

pablogs9 Apr 27, 2021

Choose a reason for hiding this comment

sloretz May 4, 2021

Choose a reason for hiding this comment

jacobperron May 3, 2022

Choose a reason for hiding this comment

pablogs9 commented Apr 27, 2021 • edited Loading

pablogs9 commented May 4, 2021

sloretz left a comment

Choose a reason for hiding this comment

sloretz commented May 4, 2021

pablogs9 commented May 5, 2021

sloretz commented May 5, 2021

sloretz commented May 5, 2021

mergify bot commented May 5, 2021

sloretz commented May 5, 2021

clalancette commented May 6, 2021

sloretz commented May 6, 2021

pablogs9 commented Apr 26, 2021 •

edited

Loading

pablogs9 commented Apr 27, 2021 •

edited

Loading