fix!: match rosidl_buffer primitive-sequence ABI on Lyrical+ (#22)

* fix: match rosidl_buffer primitive-sequence ABI on newer distros (ros2/rosidl#942)

ros2/rosidl#942 (rosidl_buffer, from Lyrical on) adds two trailing fields to
*primitive* C sequence structs:

    <T>* data; size_t size; size_t capacity;         // 0 / 8 / 16
    bool is_rosidl_buffer; bool owns_rosidl_buffer;  // -> struct is 32 bytes

`rosidl_runtime_rs::Sequence<T>` is a single 24-byte struct for all T, so on
those distros every primitive-sequence field (float64[], uint8[], ...) is at the
wrong offset: silent corruption, or a segfault when a misread length is non-zero
(e.g. visualization_msgs/Marker::from_rmw_message, sensor_msgs/JointState,
trajectory_msgs/JointTrajectoryPoint). Message-element sequences are NOT extended
by #942, so the layout is asymmetric (primitive 32, message 24) and a plain
distro cfg on the generic struct would wrongly grow message sequences too.

Fix: a per-element `LayoutTail` on a dedicated `SequenceLayout` trait (a
supertrait of `SequenceAlloc`, so that trait stays about the C alloc functions),
carried as a trailing field on `Sequence<T>`. Primitive element types set it to
`BufferFlags` on distros that have the feature and `()` otherwise, gated on
`ros_distro` the way rclrs handles distro ABI differences; String and message
element types are always `()`.

Verified on robostack-lyrical: ROS_DISTRO=lyrical gives size_of::<Sequence<u8>>()
== 32 (matches C) and message-element Sequence == 24; ROS_DISTRO=jazzy gives 24
for both. Pairs with the rosidl_rust generator change that emits the
`SequenceLayout` impl for generated message types.

Note: `SequenceAlloc` now has `SequenceLayout` as a supertrait; hand-written
impls need a matching `SequenceLayout` impl (generated code is covered by the
generator template).

Refs ros2-rust/ros2_rust#659, ros2/rosidl#942

Assisted-by: Claude:claude-opus-5 [Claude Code]

* fix: string sequences carry the buffer flags too

rosidl_runtime_c/string.h declares String__Sequence and U16String__Sequence
through the same ROSIDL_RUNTIME_C__PRIMITIVE_SEQUENCE macro as the numeric
primitives, so from Lyrical on they carry the two trailing rosidl_buffer flags
as well. Measured on a Lyrical install: double__Sequence and String__Sequence
are both 32 bytes, while a bare String stays 24 and message element sequences
stay 24.

Leaving String, WString, BoundedString and BoundedWString on an empty
LayoutTail therefore kept every string[] field 8 bytes short, which shifts the
fields after it (for example sensor_msgs/JointState.name ahead of position,
velocity and effort). Gate their tail on the distro exactly like the primitives.

Add a layout test that compares against repr(C) mirrors of the two C shapes, so
the padding comes from the compiler rather than a hand-computed size.

Assisted-by: Claude:claude-opus-5 [Claude Code]

* test: check the layout against the installed C headers

Compile a small C file against the real headers and let it export
the sizes and offsets of the C structsas globals, then compare against
those in the tests. The expected values now come from the C compiler
reading primitives_sequence.h.

The probe sits behind the abi_check feature, which is turned on by
adding a dependency to ourselves in the dev-dependencies. This builds
cc only for test targets and a plain build doesn't pull it in.

Assisted-by: Claude:claude-opus-5 [Claude Code]

* fix: add comment that users should not use abi_check feature

* fix: pass ros_distro cfg so docs.rs shim builds can run

docs.rs has no ROS_DISTRO; build.rs must read --cfg ros_distro from rustc-args the same way rclrs does. Also document that String sequences share the primitive LayoutTail.

Assisted-by: Cursor:auto [Cursor Agent]

* fix: use env::split_paths in the ABI probe

The probe compiled under abi_check could not see Path from the cfg_if import, which broke cargo test. Match main's path-separator fix so AMENT_PREFIX_PATH works on Windows too.

Assisted-by: Cursor:auto [Cursor Agent]

* fix: give RmwMessage types an empty SequenceLayout by default

Message-element sequences stay 24 bytes on Lyrical+, so generated RMW
structs do not need a generator-emitted SequenceLayout impl. The ABI
probe now also measures a C message sequence and asserts it differs from
primitive sequences on Lyrical+.

Assisted-by: Cursor:Grok-4.6 [Cursor Agent]

---------

Co-authored-by: Mathieu David <mathieudavid@mathieudavid.org>

Author: rosterloh

rosidl_runtime_rs/Cargo.toml

@@ -22,16 +22,27 @@ default = []
 # This feature is solely for the purpose of being able to generate documentation without a ROS installation
 # The only intended usage of this feature is for docs.rs builders to work, and is not intended to be used by end users
 use_ros_shim = []
+# Adds the C compiler to build and link the C probe to test size/layout against the installed ROS 2 C headers
+# This is only intended to be used for testing, and is not intended to be used by end users
+abi_check = ["dep:cc"]
 
 [dev-dependencies]
+# Trick to enable the abi_check feature for the tests, without enabling it for the library itself
+rosidl_runtime_rs = { path = ".", features = ["abi_check"] }
 # Needed for writing property tests
 quickcheck = "1"
 # Needed for testing serde support
 serde_json = "1"
 
 [build-dependencies]
+# Compiles the probe that reports the layout of the installed C sequence structs
+cc = { version = "1", optional = true }
 # Needed for uploading documentation to docs.rs
 cfg-if = "1.0.0"
+# Reads the --cfg ros_distro flag under the ROS shim (matches rclrs)
+rustflags = "0.1"
 
 [package.metadata.docs.rs]
 features = ["use_ros_shim"]
+# build.rs reads this via rustflags under use_ros_shim (same pattern as rclrs)
+rustc-args = ["--cfg", "ros_distro=\"humble\""]

rosidl_runtime_rs/build.rs

@@ -14,17 +14,75 @@ cfg_if::cfg_if! {
                 );
             }
         }
+
+        /// Compiles the probe that reports the layout of the installed C
+        /// sequence structs, so that the tests can check `Sequence<T>` against
+        /// this ROS installation instead of against a Rust recreation of those
+        /// structs.
+        #[cfg(feature = "abi_check")]
+        fn compile_sequence_abi_probe(ament_prefix_path_list: &str) {
+            let mut probe = cc::Build::new();
+            for ament_prefix_path in env::split_paths(ament_prefix_path_list) {
+                // Iron and later nest the headers of a package one level
+                // deeper, so offer both conventions and let the compiler pick.
+                let include_path = ament_prefix_path.join("include");
+                probe.include(include_path.join("rosidl_runtime_c"));
+                probe.include(include_path.join("builtin_interfaces"));
+                probe.include(include_path);
+            }
+            probe
+                .file("src/sequence_abi.c")
+                .compile("rosidl_rs_sequence_abi");
+
+            println!("cargo:rustc-cfg=has_c_abi_probe");
+        }
     }
 }
 
+// Gate the primitive-sequence layout on the ROS distro, like other distro
+// differences in rclrs (starting with Lyrical, primitive sequences carry extra
+// ABI flags; see `Sequence`/`BufferFlags`).
+const ROS_DISTRO: &str = "ROS_DISTRO";
+const KNOWN_DISTROS: &[&str] = &["humble", "jazzy", "kilted", "lyrical", "rolling"];
+
+fn get_ros_distro() -> String {
+    std::env::var(ROS_DISTRO)
+        .or_else(|_| {
+            if std::env::var("CARGO_FEATURE_USE_ROS_SHIM").is_ok() {
+                rustflags::from_env()
+                    .find_map(|f| match f {
+                        rustflags::Flag::Cfg { name, value } if name.as_str() == "ros_distro" => {
+                            value
+                        }
+                        _ => None,
+                    })
+                    .ok_or_else(|| "Missing --cfg ros_distro in RUSTFLAGS".to_string())
+            } else {
+                Err(format!("Set {ROS_DISTRO} or use ROS shim"))
+            }
+        })
+        .expect("Failed to determine ROS distro")
+}
+
 fn main() {
+    println!(
+        "cargo:rustc-check-cfg=cfg(ros_distro, values(\"{}\"))",
+        KNOWN_DISTROS.join("\", \"")
+    );
+    println!("cargo:rustc-cfg=ros_distro=\"{}\"", get_ros_distro());
+    println!("cargo:rerun-if-env-changed={ROS_DISTRO}");
+    println!("cargo:rustc-check-cfg=cfg(has_c_abi_probe)");
+
     #[cfg(not(feature = "use_ros_shim"))]
     {
         let ament_prefix_path_list = get_env_var_or_abort(AMENT_PREFIX_PATH);
         for ament_prefix_path in env::split_paths(&ament_prefix_path_list) {
             let library_path = ament_prefix_path.join("lib");
             println!("cargo:rustc-link-search=native={}", library_path.display());
         }
+
+        #[cfg(feature = "abi_check")]
+        compile_sequence_abi_probe(&ament_prefix_path_list);
     }
 
     // Invalidate the built crate whenever this script changes

rosidl_runtime_rs/src/lib.rs

@@ -3,7 +3,7 @@
 
 #[macro_use]
 mod sequence;
-pub use sequence::{BoundedSequence, Sequence, SequenceExceedsBoundsError};
+pub use sequence::{BoundedSequence, BufferFlags, Sequence, SequenceExceedsBoundsError};
 
 mod string;
 pub use string::{BoundedString, BoundedWString, String, StringExceedsBoundsError, WString};

rosidl_runtime_rs/src/sequence.rs

@@ -9,7 +9,7 @@ use std::{
 #[cfg(feature = "serde")]
 mod serde;
 
-use crate::traits::SequenceAlloc;
+use crate::traits::{SequenceAlloc, SequenceLayout};
 
 /// An unbounded sequence.
 ///
@@ -41,6 +41,21 @@ pub struct Sequence<T: SequenceAlloc> {
     data: *mut T,
     size: usize,
     capacity: usize,
+    _tail: T::LayoutTail,
+}
+
+/// Mirrors the two trailing fields of `rosidl_runtime_c__<T>__Sequence` for
+/// primitive element types. Starting with Lyrical, primitive sequences have
+/// these extra flags in their ABI; they are set/read only by the C++ side, so
+/// on the CPU backend both are `false` and the sequence behaves as before. See
+/// <https://github.com/ros2/rosidl/blob/rolling/rosidl_runtime_c/include/rosidl_runtime_c/primitives_sequence.h>.
+#[repr(C)]
+#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Hash, PartialOrd, Ord)]
+pub struct BufferFlags {
+    /// `true` when `data` points to an `rosidl::Buffer<T>*` rather than a plain array.
+    _is_rosidl_buffer: bool,
+    /// `true` when the sequence's fini must destroy that buffer.
+    _owns_rosidl_buffer: bool,
 }
 
 /// A bounded sequence.
@@ -114,6 +129,7 @@ impl<T: SequenceAlloc> Default for Sequence<T> {
             data: std::ptr::null_mut(),
             size: 0,
             capacity: 0,
+            _tail: Default::default(),
         }
     }
 }
@@ -312,6 +328,7 @@ impl<T: SequenceAlloc, const N: usize> Default for BoundedSequence<T, N> {
                 data: std::ptr::null_mut(),
                 size: 0,
                 capacity: 0,
+                _tail: Default::default(),
             },
         }
     }
@@ -398,6 +415,7 @@ impl<T: SequenceAlloc, const N: usize> IntoIterator for BoundedSequence<T, N> {
                 data: std::ptr::null_mut(),
                 size: 0,
                 capacity: 0,
+                _tail: Default::default(),
             },
         );
         SequenceIterator { seq, idx: 0 }
@@ -525,6 +543,16 @@ macro_rules! impl_sequence_alloc_for_primitive_type {
             ) -> bool;
         }
 
+        // Primitive sequences carry the layout flags from Lyrical on.
+        #[cfg(not(any(ros_distro = "humble", ros_distro = "jazzy", ros_distro = "kilted")))]
+        impl SequenceLayout for $rust_type {
+            type LayoutTail = crate::BufferFlags;
+        }
+        #[cfg(any(ros_distro = "humble", ros_distro = "jazzy", ros_distro = "kilted"))]
+        impl SequenceLayout for $rust_type {
+            type LayoutTail = ();
+        }
+
         impl SequenceAlloc for $rust_type {
             fn sequence_init(seq: &mut Sequence<Self>, size: usize) -> bool {
                 // SAFETY: There are no special preconditions to the sequence_init function.
@@ -687,6 +715,94 @@ mod tests {
         }
     }
 
+    // The layout of the C structs, as reported by src/sequence_abi.c after the
+    // C compiler read the headers of this ROS installation.
+    #[cfg(has_c_abi_probe)]
+    extern "C" {
+        static rosidl_rs_primitive_sequence_size: usize;
+        static rosidl_rs_string_sequence_size: usize;
+        static rosidl_rs_u16string_sequence_size: usize;
+        static rosidl_rs_message_sequence_size: usize;
+        static rosidl_rs_sequence_data_offset: usize;
+        static rosidl_rs_sequence_size_offset: usize;
+        static rosidl_rs_sequence_capacity_offset: usize;
+        static rosidl_rs_sequence_align: usize;
+    }
+
+    /// `Sequence<T>` is handed to C as `rosidl_runtime_c__<T>__Sequence`, so it
+    /// has to be the size of that struct in the installed headers. From Lyrical
+    /// on, every type declared through ROSIDL_RUNTIME_C__PRIMITIVE_SEQUENCE
+    /// carries two trailing flags, and that macro covers `String` and
+    /// `U16String` as well as the numeric primitives.
+    #[test]
+    #[cfg(has_c_abi_probe)]
+    fn test_sequence_size_matches_c() {
+        // SAFETY: These are `const size_t` objects with external linkage.
+        let (primitive, string, u16string, message) = unsafe {
+            (
+                rosidl_rs_primitive_sequence_size,
+                rosidl_rs_string_sequence_size,
+                rosidl_rs_u16string_sequence_size,
+                rosidl_rs_message_sequence_size,
+            )
+        };
+
+        #[cfg(any(ros_distro = "humble", ros_distro = "jazzy", ros_distro = "kilted"))]
+        assert_eq!(primitive, message);
+        #[cfg(not(any(ros_distro = "humble", ros_distro = "jazzy", ros_distro = "kilted")))]
+        assert_ne!(
+            primitive, message,
+            "Lyrical+ primitive sequences carry buffer flags; message sequences do not"
+        );
+
+        assert_eq!(std::mem::size_of::<Sequence<f64>>(), primitive);
+        assert_eq!(std::mem::size_of::<Sequence<u8>>(), primitive);
+        assert_eq!(std::mem::size_of::<Sequence<i16>>(), primitive);
+        assert_eq!(std::mem::size_of::<Sequence<bool>>(), primitive);
+        assert_eq!(std::mem::size_of::<BoundedSequence<f64, 4>>(), primitive);
+
+        assert_eq!(std::mem::size_of::<Sequence<crate::String>>(), string);
+        assert_eq!(
+            std::mem::size_of::<BoundedSequence<crate::String, 4>>(),
+            string
+        );
+        assert_eq!(
+            std::mem::size_of::<Sequence<crate::BoundedString<4>>>(),
+            string
+        );
+
+        assert_eq!(std::mem::size_of::<Sequence<crate::WString>>(), u16string);
+        assert_eq!(
+            std::mem::size_of::<BoundedSequence<crate::WString, 4>>(),
+            u16string
+        );
+        assert_eq!(
+            std::mem::size_of::<Sequence<crate::BoundedWString<4>>>(),
+            u16string
+        );
+    }
+
+    /// A matching size is not enough on its own, because it says nothing about
+    /// the order of the fields C reads and writes.
+    #[test]
+    #[cfg(has_c_abi_probe)]
+    fn test_sequence_field_offsets_match_c() {
+        // SAFETY: These are `const size_t` objects with external linkage.
+        let (data, size, capacity, align) = unsafe {
+            (
+                rosidl_rs_sequence_data_offset,
+                rosidl_rs_sequence_size_offset,
+                rosidl_rs_sequence_capacity_offset,
+                rosidl_rs_sequence_align,
+            )
+        };
+
+        assert_eq!(std::mem::offset_of!(Sequence<f64>, data), data);
+        assert_eq!(std::mem::offset_of!(Sequence<f64>, size), size);
+        assert_eq!(std::mem::offset_of!(Sequence<f64>, capacity), capacity);
+        assert_eq!(std::mem::align_of::<Sequence<f64>>(), align);
+    }
+
     #[test]
     fn test_empty_sequence() {
         assert!(Sequence::<i32>::default().is_empty());

rosidl_runtime_rs/src/sequence_abi.c

@@ -0,0 +1,45 @@
+// Reports the layout of the C structs in header files installed with the ROS 2 distribution.
+// rosidl_runtime_rs casts its types to those C structs so we need to make sure the sizes and
+// alignments match.
+//
+// Every sequence type, `String` and `U16String` included, is declared through
+// the ROSIDL_RUNTIME_C__PRIMITIVE_SEQUENCE macro, so one primitive sequence
+// stands in for the whole family, and only the string sequences need measuring
+// separately because their element structs have a size of their own.
+
+#include <stddef.h>
+
+#include <rosidl_runtime_c/primitives_sequence.h>
+#include <rosidl_runtime_c/string.h>
+#include <rosidl_runtime_c/u16string.h>
+#include <builtin_interfaces/msg/detail/time__struct.h>
+
+typedef rosidl_runtime_c__double__Sequence primitive_sequence;
+
+// A member placed after a char sits at that member's alignment, which reports
+// the alignment without _Alignof and the C11 that it needs.
+struct alignment_probe
+{
+  char before;
+  primitive_sequence sequence;
+};
+
+// The sizes of the structs a Sequence<T> is cast to.
+const size_t rosidl_rs_primitive_sequence_size = sizeof(primitive_sequence);
+const size_t rosidl_rs_string_sequence_size = sizeof(rosidl_runtime_c__String__Sequence);
+const size_t rosidl_rs_u16string_sequence_size = sizeof(rosidl_runtime_c__U16String__Sequence);
+// Message-element sequences were not extended in rosidl#942; they stay
+// pointer/size/capacity on every distro, including Lyrical+.
+const size_t rosidl_rs_message_sequence_size = sizeof(builtin_interfaces__msg__Time__Sequence);
+
+// Where C keeps the three fields both sides read and write, and how it aligns
+// the struct that holds them.
+const size_t rosidl_rs_sequence_data_offset = offsetof(primitive_sequence, data);
+const size_t rosidl_rs_sequence_size_offset = offsetof(primitive_sequence, size);
+const size_t rosidl_rs_sequence_capacity_offset = offsetof(primitive_sequence, capacity);
+const size_t rosidl_rs_sequence_align = offsetof(struct alignment_probe, sequence);
+
+// The flags belong to the sequence structs, so the string structs themselves
+// keep the same size on every distro.
+const size_t rosidl_rs_string_size = sizeof(rosidl_runtime_c__String);
+const size_t rosidl_rs_u16string_size = sizeof(rosidl_runtime_c__U16String);