Skip to content

Commit 59cf77d

Browse files
committed
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]
1 parent e12b92f commit 59cf77d

5 files changed

Lines changed: 167 additions & 35 deletions

File tree

rosidl_runtime_rs/Cargo.toml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,20 @@ default = []
2222
# This feature is solely for the purpose of being able to generate documentation without a ROS installation
2323
# The only intended usage of this feature is for docs.rs builders to work, and is not intended to be used by end users
2424
use_ros_shim = []
25+
# Adds the C compiler to build and link the C probe to test size/layout against the installed ROS 2 C headers
26+
abi_check = ["dep:cc"]
2527

2628
[dev-dependencies]
29+
# Trick to enable the abi_check feature for the tests, without enabling it for the library itself
30+
rosidl_runtime_rs = { path = ".", features = ["abi_check"] }
2731
# Needed for writing property tests
2832
quickcheck = "1"
2933
# Needed for testing serde support
3034
serde_json = "1"
3135

3236
[build-dependencies]
37+
# Compiles the probe that reports the layout of the installed C sequence structs
38+
cc = { version = "1", optional = true }
3339
# Needed for uploading documentation to docs.rs
3440
cfg-if = "1.0.0"
3541
# Reads the --cfg ros_distro flag under the ROS shim (matches rclrs)

rosidl_runtime_rs/build.rs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,27 @@ cfg_if::cfg_if! {
1515
);
1616
}
1717
}
18+
19+
/// Compiles the probe that reports the layout of the installed C
20+
/// sequence structs, so that the tests can check `Sequence<T>` against
21+
/// this ROS installation instead of against a Rust recreation of those
22+
/// structs.
23+
#[cfg(feature = "abi_check")]
24+
fn compile_sequence_abi_probe(ament_prefix_path_list: &str) {
25+
let mut probe = cc::Build::new();
26+
for ament_prefix_path in ament_prefix_path_list.split(':') {
27+
// Iron and later nest the headers of a package one level
28+
// deeper, so offer both conventions and let the compiler pick.
29+
let include_path = Path::new(ament_prefix_path).join("include");
30+
probe.include(include_path.join("rosidl_runtime_c"));
31+
probe.include(include_path);
32+
}
33+
probe
34+
.file("src/sequence_abi.c")
35+
.compile("rosidl_rs_sequence_abi");
36+
37+
println!("cargo:rustc-cfg=has_c_abi_probe");
38+
}
1839
}
1940
}
2041

@@ -50,6 +71,7 @@ fn main() {
5071
);
5172
println!("cargo:rustc-cfg=ros_distro=\"{}\"", get_ros_distro());
5273
println!("cargo:rerun-if-env-changed={ROS_DISTRO}");
74+
println!("cargo:rustc-check-cfg=cfg(has_c_abi_probe)");
5375

5476
#[cfg(not(feature = "use_ros_shim"))]
5577
{
@@ -58,6 +80,9 @@ fn main() {
5880
let library_path = Path::new(ament_prefix_path).join("lib");
5981
println!("cargo:rustc-link-search=native={}", library_path.display());
6082
}
83+
84+
#[cfg(feature = "abi_check")]
85+
compile_sequence_abi_probe(&ament_prefix_path_list);
6186
}
6287

6388
// Invalidate the built crate whenever this script changes

rosidl_runtime_rs/src/sequence.rs

Lines changed: 71 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -715,46 +715,82 @@ mod tests {
715715
}
716716
}
717717

718-
/// The Rust `Sequence` must be the same size as the C
719-
/// `rosidl_runtime_c__<T>__Sequence` it is cast to. From Lyrical on, every
720-
/// type declared through ROSIDL_RUNTIME_C__PRIMITIVE_SEQUENCE gained two
721-
/// trailing `bool` flags, and that macro covers `String`/`U16String` as well
722-
/// as the numeric primitives. Message element types keep the 3-field layout.
718+
// The layout of the C structs, as reported by src/sequence_abi.c after the
719+
// C compiler read the headers of this ROS installation.
720+
#[cfg(has_c_abi_probe)]
721+
extern "C" {
722+
static rosidl_rs_primitive_sequence_size: usize;
723+
static rosidl_rs_string_sequence_size: usize;
724+
static rosidl_rs_u16string_sequence_size: usize;
725+
static rosidl_rs_sequence_data_offset: usize;
726+
static rosidl_rs_sequence_size_offset: usize;
727+
static rosidl_rs_sequence_capacity_offset: usize;
728+
static rosidl_rs_sequence_align: usize;
729+
}
730+
731+
/// `Sequence<T>` is handed to C as `rosidl_runtime_c__<T>__Sequence`, so it
732+
/// has to be the size of that struct in the installed headers. From Lyrical
733+
/// on, every type declared through ROSIDL_RUNTIME_C__PRIMITIVE_SEQUENCE
734+
/// carries two trailing flags, and that macro covers `String` and
735+
/// `U16String` as well as the numeric primitives.
723736
#[test]
724-
fn test_sequence_layout_matches_c() {
725-
// Mirrors of the two C shapes, so the expected size includes the same
726-
// padding the C compiler applies rather than a hand-computed number.
727-
#[repr(C)]
728-
struct CSequence {
729-
data: *mut u8,
730-
size: usize,
731-
capacity: usize,
732-
}
733-
#[repr(C)]
734-
struct CSequenceWithFlags {
735-
data: *mut u8,
736-
size: usize,
737-
capacity: usize,
738-
flags: BufferFlags,
739-
}
740-
let expected = if cfg!(any(
741-
ros_distro = "humble",
742-
ros_distro = "jazzy",
743-
ros_distro = "kilted"
744-
)) {
745-
std::mem::size_of::<CSequence>()
746-
} else {
747-
std::mem::size_of::<CSequenceWithFlags>()
737+
#[cfg(has_c_abi_probe)]
738+
fn test_sequence_size_matches_c() {
739+
// SAFETY: These are `const size_t` objects with external linkage.
740+
let (primitive, string, u16string) = unsafe {
741+
(
742+
rosidl_rs_primitive_sequence_size,
743+
rosidl_rs_string_sequence_size,
744+
rosidl_rs_u16string_sequence_size,
745+
)
748746
};
749-
assert_eq!(std::mem::size_of::<Sequence<f64>>(), expected);
750-
assert_eq!(std::mem::size_of::<Sequence<u8>>(), expected);
751-
assert_eq!(std::mem::size_of::<Sequence<crate::String>>(), expected);
752-
assert_eq!(std::mem::size_of::<Sequence<crate::WString>>(), expected);
753-
assert_eq!(std::mem::size_of::<BoundedSequence<f64, 4>>(), expected);
747+
748+
assert_eq!(std::mem::size_of::<Sequence<f64>>(), primitive);
749+
assert_eq!(std::mem::size_of::<Sequence<u8>>(), primitive);
750+
assert_eq!(std::mem::size_of::<Sequence<i16>>(), primitive);
751+
assert_eq!(std::mem::size_of::<Sequence<bool>>(), primitive);
752+
assert_eq!(std::mem::size_of::<BoundedSequence<f64, 4>>(), primitive);
753+
754+
assert_eq!(std::mem::size_of::<Sequence<crate::String>>(), string);
754755
assert_eq!(
755756
std::mem::size_of::<BoundedSequence<crate::String, 4>>(),
756-
expected
757+
string
757758
);
759+
assert_eq!(
760+
std::mem::size_of::<Sequence<crate::BoundedString<4>>>(),
761+
string
762+
);
763+
764+
assert_eq!(std::mem::size_of::<Sequence<crate::WString>>(), u16string);
765+
assert_eq!(
766+
std::mem::size_of::<BoundedSequence<crate::WString, 4>>(),
767+
u16string
768+
);
769+
assert_eq!(
770+
std::mem::size_of::<Sequence<crate::BoundedWString<4>>>(),
771+
u16string
772+
);
773+
}
774+
775+
/// A matching size is not enough on its own, because it says nothing about
776+
/// the order of the fields C reads and writes.
777+
#[test]
778+
#[cfg(has_c_abi_probe)]
779+
fn test_sequence_field_offsets_match_c() {
780+
// SAFETY: These are `const size_t` objects with external linkage.
781+
let (data, size, capacity, align) = unsafe {
782+
(
783+
rosidl_rs_sequence_data_offset,
784+
rosidl_rs_sequence_size_offset,
785+
rosidl_rs_sequence_capacity_offset,
786+
rosidl_rs_sequence_align,
787+
)
788+
};
789+
790+
assert_eq!(std::mem::offset_of!(Sequence<f64>, data), data);
791+
assert_eq!(std::mem::offset_of!(Sequence<f64>, size), size);
792+
assert_eq!(std::mem::offset_of!(Sequence<f64>, capacity), capacity);
793+
assert_eq!(std::mem::align_of::<Sequence<f64>>(), align);
758794
}
759795

760796
#[test]
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
// Reports the layout of the C structs in header files installed with the ROS 2 distribution.
2+
// rosidl_runtime_rs casts its types to those C structs so we need to make sure the sizes and
3+
// alignments match.
4+
//
5+
// Every sequence type, `String` and `U16String` included, is declared through
6+
// the ROSIDL_RUNTIME_C__PRIMITIVE_SEQUENCE macro, so one primitive sequence
7+
// stands in for the whole family, and only the string sequences need measuring
8+
// separately because their element structs have a size of their own.
9+
10+
#include <stddef.h>
11+
12+
#include <rosidl_runtime_c/primitives_sequence.h>
13+
#include <rosidl_runtime_c/string.h>
14+
#include <rosidl_runtime_c/u16string.h>
15+
16+
typedef rosidl_runtime_c__double__Sequence primitive_sequence;
17+
18+
// A member placed after a char sits at that member's alignment, which reports
19+
// the alignment without _Alignof and the C11 that it needs.
20+
struct alignment_probe
21+
{
22+
char before;
23+
primitive_sequence sequence;
24+
};
25+
26+
// The sizes of the structs a Sequence<T> is cast to.
27+
const size_t rosidl_rs_primitive_sequence_size = sizeof(primitive_sequence);
28+
const size_t rosidl_rs_string_sequence_size = sizeof(rosidl_runtime_c__String__Sequence);
29+
const size_t rosidl_rs_u16string_sequence_size = sizeof(rosidl_runtime_c__U16String__Sequence);
30+
31+
// Where C keeps the three fields both sides read and write, and how it aligns
32+
// the struct that holds them.
33+
const size_t rosidl_rs_sequence_data_offset = offsetof(primitive_sequence, data);
34+
const size_t rosidl_rs_sequence_size_offset = offsetof(primitive_sequence, size);
35+
const size_t rosidl_rs_sequence_capacity_offset = offsetof(primitive_sequence, capacity);
36+
const size_t rosidl_rs_sequence_align = offsetof(struct alignment_probe, sequence);
37+
38+
// The flags belong to the sequence structs, so the string structs themselves
39+
// keep the same size on every distro.
40+
const size_t rosidl_rs_string_size = sizeof(rosidl_runtime_c__String);
41+
const size_t rosidl_rs_u16string_size = sizeof(rosidl_runtime_c__U16String);

rosidl_runtime_rs/src/string.rs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -542,6 +542,30 @@ mod tests {
542542

543543
use super::*;
544544

545+
// The layout of the C structs, as reported by src/sequence_abi.c after the
546+
// C compiler read the headers of this ROS installation.
547+
#[cfg(has_c_abi_probe)]
548+
extern "C" {
549+
static rosidl_rs_string_size: usize;
550+
static rosidl_rs_u16string_size: usize;
551+
}
552+
553+
/// The buffer flags belong to the sequence structs, not to the string
554+
/// structs, so these sizes have to stay the same on every distro. Getting
555+
/// this wrong the other way around would shift every field of a message
556+
/// that holds a string.
557+
#[test]
558+
#[cfg(has_c_abi_probe)]
559+
fn test_string_size_matches_c() {
560+
// SAFETY: These are `const size_t` objects with external linkage.
561+
let (string, u16string) = unsafe { (rosidl_rs_string_size, rosidl_rs_u16string_size) };
562+
563+
assert_eq!(std::mem::size_of::<String>(), string);
564+
assert_eq!(std::mem::size_of::<BoundedString<4>>(), string);
565+
assert_eq!(std::mem::size_of::<WString>(), u16string);
566+
assert_eq!(std::mem::size_of::<BoundedWString<4>>(), u16string);
567+
}
568+
545569
impl Arbitrary for String {
546570
fn arbitrary(g: &mut Gen) -> Self {
547571
std::string::String::arbitrary(g).as_str().into()

0 commit comments

Comments
 (0)