Change expected rmw GID array size to 16 bytes

[christophebedard]

The size changed from 24 to 16 bytes in ros2/rmw#345. The same size is defined in tracetools, and it was never updated.

In practice, reading 24-16=8 random extra bytes didn't change much, since nothing is currently relying on the GID values, at least not relying on getting the same GID for the same object from two different systems, because it doesn't work with rmw_cyclonedds, see ros2/rmw_cyclonedds#377.

[christophebedard]

Defining the same value as RMW_GID_STORAGE_SIZE in tracetools (TRACETOOLS_GID_STORAGE_SIZE) isn't great, but I assume it shouldn't change anymore ™️.

[clalancette]

Defining the same value as RMW_GID_STORAGE_SIZE in tracetools (TRACETOOLS_GID_STORAGE_SIZE) isn't great, but I assume it shouldn't change anymore ™️.

It probably won't, so this should be safe.

But out of curiousity, could we make tracetools here depend on rmw and just use the define directly?

[christophebedard]

But out of curiousity, could we make tracetools here depend on rmw and just use the define directly?

I think this would be technically possible, but it would mean that no packages that rmw depends on could use/depend on tracetools. In practice, that might never happen, but I don't think this duplicate definition is enough of a problem to make this change. What do you think?

[fujitatomoya]

lgtm

[clalancette]

I think this would be technically possible, but it would mean that no packages that rmw depends on could use/depend on tracetools. In practice, that might never happen, but I don't think this duplicate definition is enough of a problem to make this change. What do you think?

Yeah, that's fair. It sounds like you've already thought this through, so I'm fine with this change as-is. I'll approve, and we can run CI on it.

[clalancette]

Looks good to me with green CI.

[christophebedard]

Another option (and probably the right one) would be to make the RMW_GID_STORAGE_SIZE value a tracepoint argument:

diff --git a/tracetools/include/tracetools/tp_call.h b/tracetools/include/tracetools/tp_call.h
index c56dc1b..07edb84 100644
--- a/tracetools/include/tracetools/tp_call.h
+++ b/tracetools/include/tracetools/tp_call.h
@@ -76,11 +76,12 @@ TRACEPOINT_EVENT(
   rmw_publisher_init,
   TP_ARGS(
     const void *, rmw_publisher_handle_arg,
-    const uint8_t *, gid_arg
+    const uint8_t *, gid_arg,
+    const size_t, gid_len_arg
   ),
   TP_FIELDS(
     ctf_integer_hex(const void *, rmw_publisher_handle, rmw_publisher_handle_arg)
-    ctf_array(uint8_t, gid, gid_arg, TRACETOOLS_GID_STORAGE_SIZE)
+    ctf_array(uint8_t, gid, gid_arg, gid_len_arg)
   )
 )

Then RMW_GID_STORAGE_SIZE (or sizeof()) would be provided to the tracepoint in rmw, like this (e.g., for rmw_cyclonedds_cpp):

diff --git a/rmw_cyclonedds_cpp/src/rmw_node.cpp b/rmw_cyclonedds_cpp/src/rmw_node.cpp
index af64110..066a9d2 100644
--- a/rmw_cyclonedds_cpp/src/rmw_node.cpp
+++ b/rmw_cyclonedds_cpp/src/rmw_node.cpp
@@ -2630,7 +2630,7 @@ extern "C" rmw_publisher_t * rmw_create_publisher(
   }
 
   cleanup_publisher.cancel();
-  TRACETOOLS_TRACEPOINT(rmw_publisher_init, static_cast<const void *>(pub), cddspub->gid.data);
+  TRACETOOLS_TRACEPOINT(rmw_publisher_init, static_cast<const void *>(pub), cddspub->gid.data, RMW_GID_STORAGE_SIZE);
   return pub;
 }

This removes the duplication, except for the literals in test_tracetools, which I think is fine.

[christophebedard]

Anyway, I can do that in a separate PR (and it requires breaking tracetools ABI and modifying all rmw implementations). Here's CI:

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[christophebedard]

There's a ros2cli test failure on RHEL, but I don't think it's related.

[christophebedard]

Note: ros2/rmw#345 was merged in January 2023, so Iron and Jazzy have 16-byte rmw GIDs. However, I won't backport this PR to Iron/Jazzy because this isn't a critical issue (unless someone requests it), as I mentioned in the PR description above.