ctest: add tests for aliases, structs, unions, as well as a test crate. #4543

mbyx · 2025-07-08T10:02:37Z

Description

This PR adds support for testing aliases, structs, and unions. It additionally ports ctest-test to use ctest-next.

Sources

Checklist

Relevant tests in libc-test/semver have been updated
No placeholder or unstable values like *LAST or *MAX are
included (see #3131)
Tested locally (cd libc-test && cargo test --target mytarget);
especially relevant for platforms that may not be checked in CI

tgross35 · 2025-07-14T06:59:23Z

ctest-next-test/Cargo.toml

@@ -0,0 +1,33 @@
+[package]


Is there anything different between ctest-next-test and the existing ctest-test? If it's nothing significant, I think it would be better to reuse ctest-test and just add the invocations to test ctest-next in its build.rs, rather than duplicating everything.

There's a bunch of removals, deleting the cpp side of the tests and modifying all.rs, other than that the actual test files have minor changes.

Feel free to remove the c++ tests and make whatever changes are needed, the current ctest won't be getting any updates so we don't really have anything to keep testing there.

I'd prefer modifying the existing ctest-test anyway to see what the actual diff is.

tgross35

(first set, more in progress)

tgross35 · 2025-07-14T07:00:19Z

ctest-next/src/ast/structure.rs

@@ -15,4 +13,9 @@ impl Struct {
    pub fn ident(&self) -> &str {
        &self.ident
    }
+
+    /// Return the public fields of the struct.
+    pub fn public_fields(&self) -> Vec<&Field> {


Suggested change

pub fn public_fields(&self) -> Vec<&Field> {

pub fn public_fields(&self) -> impl Iterator<Item = &Field> {

That way you don't need to .collect()

tgross35 · 2025-07-14T07:01:05Z

ctest-next/src/ast/union.rs

@@ -15,4 +14,9 @@ impl Union {
    pub fn ident(&self) -> &str {
        &self.ident
    }
+
+    /// Return the public fields of the union.
+    pub(crate) fn public_fields(&self) -> Vec<&Field> {


Use impl Iterator<Item = &Field> here too

tgross35 · 2025-07-14T07:03:22Z

ctest-next/src/ffi_items.rs

+    let link_name = i
+        .attrs
+        .iter()
+        .find(|attr| attr.path().is_ident("link_name"))


We should probably error here if there are >1 link_name attrs

The Visit trait doesn't return anything, so it would have to be a panic.

I think that's fine, shouldn't happen anyway

tgross35 · 2025-07-14T07:06:13Z

ctest-next/src/lib.rs

+    Field(Either<&'a Struct, &'a Union>, &'a Field),
    Alias(&'a Type),
    Const(&'a Const),
    Static(&'a Static),
    Type(&'a str),
    /// This variant is used for renaming the struct type.
    StructType(&'a str),
    UnionType(&'a str),
+    FieldType(Either<&'a Struct, &'a Union>, &'a Field),


Could these be split to StructField StructFieldType UnionField and UnionFieldType rather than using Either? Seems slightly more straightforward since you have to match somewhere anyway.

tgross35 · 2025-07-14T07:11:50Z

ctest-next/templates/test.rs

+        use std::ffi::c_int;
+
+        type U = {{ ident }};
+        extern "C" {
+            fn __test_roundtrip_{{ ident }}(
+                size: i32, x: U, e: *mut c_int, pad: *const u8
+            ) -> U;
+        }
+        let pad = roundtrip_padding_{{ ident }}();
+        unsafe {
+            use std::mem::{MaybeUninit, size_of};


(Applies multiple places) put these imports at the top of the file, since they'll be used everywhere.

Also you don't need to import size_of, it's in the prelude since 1.80

The imports that are within the test functions are there because they aren't used everywhere, moving them to the top level would cause unused import errors.

That's fine, just #[allow(unused_imports)]

tgross35 · 2025-07-14T07:18:47Z

ctest-next/templates/test.rs

+    ///
+    /// For type aliases, the padding map is all zeroes.
+    fn roundtrip_padding_{{ ident }}() -> Vec<u8> {
+        vec![0; mem::size_of::<{{ ident }}>()]


May as well use the prelude

Suggested change

vec![0; mem::size_of::<{{ ident }}>()]

vec![0; size_of::<{{ ident }}>()]

tgross35 · 2025-07-14T07:21:49Z

ctest-next/templates/test.c

+uint64_t __test_align_{{ ident }}(void) {
+    typedef struct {
+        unsigned char c;
+        {{ c_type }} v;
+    } type;
+    type t;
+    size_t t_addr = (size_t)(unsigned char*)(&t);
+    size_t v_addr = (size_t)(unsigned char*)(&t.v);
+    return t_addr >= v_addr? t_addr - v_addr : v_addr - t_addr;


This can just use _Alignof

tgross35 · 2025-07-14T07:27:42Z

ctest-next/templates/test.c

+{%- let c_type = self.c_type(*alias).unwrap() +%}
+
+// Return the size of a type.
+uint64_t __test_size_{{ ident }}(void) { return sizeof({{ c_type }}); }


I think these names could be adjusted a bi since they aren't really testing anything in these functions. Maybe ctest_get_{{ident}}_size, ctest_get_{{ident}}_align.

The __ prefix shouldn't be needed if ctest_ is used instead (I assume dunder was to avoid conflicts?)

Slightly changing my mind here; could you use ctest_size_of__{{ ident }} and ctest_align_of__{{ ident }} instead?

Using __ to separate "fields" is somewhat common. E.g. for struct val_tree, ctest_size_of__val_tree is a bit easier to tell that you're getting the ctest_size_of for val_tree, rather than ctest_size_of_val_tree which could be read as ctest_size_of_val for a struct named tree.

tgross35 · 2025-07-14T07:29:01Z

ctest-next/src/translator.rs

+    }
+
+    /// Determine whether a C type is a signed type.
+    pub(crate) fn has_sign(&self, ffi_items: &FfiItems, ty: &syn::Type) -> bool {


Could this be named is_signed?

tgross35 · 2025-07-14T07:32:12Z

ctest-next-test/Cargo.toml

@@ -0,0 +1,33 @@
+[package]


Feel free to remove the c++ tests and make whatever changes are needed, the current ctest won't be getting any updates so we don't really have anything to keep testing there.

I'd prefer modifying the existing ctest-test anyway to see what the actual diff is.

tgross35 · 2025-07-14T07:33:17Z

ctest-next/templates/test.c

+
+// Return `1` if the type is signed, otherwise return `0`. 
+uint32_t __test_signed_{{ ident }}(void) {
+    return ((({{ c_type }}) -1) < 0);


Suggested change

return ((({{ c_type }}) -1) < 0);

return (({{ c_type }}) -1) < 0;

Slightly unnest parens. Also rename this one ctest_{{ ident }}_is_signed

tgross35 · 2025-07-14T07:33:45Z

ctest-next/templates/test.c

+// Tests whether the type alias `x` when passed to C and back to Rust remains unchanged.
+// It checks if the size is the same as well as if the padding bytes are all in the correct place.
+{{ c_type }} __test_roundtrip_{{ ident }}(
+        int32_t rust_size, {{ c_type }} value, int* error, unsigned char* pad


Suggested change

int32_t rust_size, {{ c_type }} value, int* error, unsigned char* pad

int32_t rust_size, {{ c_type }} value, int* error, unsigned char* pad

Indentation nit

tgross35 · 2025-07-14T07:35:53Z

ctest-next/templates/test.rs

+                y_ptr.add(i).write_volatile(d);
+            }
+            let r: U = __test_roundtrip_{{ ident }}(sz as i32, x.assume_init(), &mut error, pad.as_ptr());
+            if error == 1 {


Doesn't really matter here but error != 0 is usually good practice (robust against added error codes)

tgross35 · 2025-07-14T07:37:08Z

ctest-next/templates/test.rs

+            let mut error: c_int = 0;
+            let mut y = MaybeUninit::<U>::uninit();
+            let mut x = MaybeUninit::<U>::uninit();
+            let x_ptr = x.as_mut_ptr().cast::<u8>();
+            let y_ptr = y.as_mut_ptr().cast::<u8>();
+            let sz = size_of::<U>();
+            for i in 0..sz {
+                let c: u8 = (i % 256) as u8;
+                let c = if c == 0 { 42 } else { c };
+                let d: u8 = 255_u8 - (i % 256) as u8;
+                let d = if d == 0 { 42 } else { d };
+                x_ptr.add(i).write_volatile(c);
+                y_ptr.add(i).write_volatile(d);
+            }


Could you comment what is going on here?

Also prefer MaybeUninit::<U>::zeroed() (applies more places)

tgross35 · 2025-07-14T07:39:40Z

ctest-next/templates/test.rs

+                x_ptr.add(i).write_volatile(c);
+                y_ptr.add(i).write_volatile(d);
+            }
+            let r: U = __test_roundtrip_{{ ident }}(sz as i32, x.assume_init(), &mut error, pad.as_ptr());


The assume_init isn't sound, it's unlikely but the compiler can do some wonky stuff here. You should replace the U in the function signature with MaybeUninit<U> and then don't assume_init.

I think this applies a few places.

tgross35 · 2025-07-14T08:24:08Z

ctest-next/templates/test.rs

+                x_ptr.add(i).write_volatile(c);
+                y_ptr.add(i).write_volatile(d);
+            }
+            let r: U = __test_roundtrip_{{ ident }}(sz as i32, x.assume_init(), &mut error, pad.as_ptr());


Same as earlier, this shouldn't assume_init

tgross35 · 2025-07-14T08:24:26Z

ctest-next/templates/test.rs

+        }
+        let pad = roundtrip_padding_{{ ident }}();
+        unsafe {
+            use std::mem::{MaybeUninit, size_of};


Use a module-level import for MaybeUninit

tgross35 · 2025-07-14T08:25:13Z

ctest-next/templates/test.rs

+    ///
+    /// It checks if the size is the same as well as if the padding bytes are all in the
+    /// correct place.
+    pub fn roundtrip_{{ ident }}() {


Could you add some comments within the function about what is happening?

tgross35 · 2025-07-14T08:25:32Z

ctest-next/templates/test.rs

+                y_ptr.add(i).write_volatile(d);
+            }
+            let r: U = __test_roundtrip_{{ ident }}(sz as i32, x.assume_init(), &mut error, pad.as_ptr());
+            if error == 1 {


Suggested change

if error == 1 {

if error != 0 {

tgross35 · 2025-07-14T08:26:17Z

ctest-next/templates/test.rs

+                FAILED.store(true, Ordering::Relaxed);
+                return;
+            }
+            for (i, elem) in pad.iter().enumerate().take(size_of::<U>()) {


Isn't pad already size_of::<U>()? Maybe this is something worth asserting, then you can remove the .take()

tgross35 · 2025-07-14T08:33:29Z

The size_align_* functions are repeated for unions, structs, and type aliases. In the library, I think it would be good to collect a Vec<(String, String)> or (rust_name, c_name) of all items requiring this test. E.g. [("foo::bar", "struct foo"), ("baz::pthread_t", "pthread_t")] etc. Then in the template you can iterate this and only need the pub fn size_align_* once.

There might be some other things that can be combined in a similar way

mbyx · 2025-07-14T10:57:03Z

Why is the style check failing for libc-test? I haven't touched that.

tgross35 · 2025-07-14T17:42:33Z

Sometimes we get new lints that that suggest changes to existing code

rustbot added ctest Issues relating to the ctest crate S-waiting-on-review labels Jul 8, 2025

mbyx force-pushed the ctest-next-test-port branch 2 times, most recently from d787bc1 to 6376d9e Compare July 14, 2025 06:06

ctest: add tests for aliases, structs, unions, as well as a test crate.

66cffca

mbyx force-pushed the ctest-next-test-port branch from 6ac55b3 to 66cffca Compare July 14, 2025 06:18

Merge branch 'main' into ctest-next-test-port

776318a

tgross35 reviewed Jul 14, 2025

View reviewed changes

ctest: move ctest-next-test to ctest-test

b474ed8

mbyx requested a review from tgross35 July 14, 2025 10:56

	pub fn public_fields(&self) -> Vec<&Field> {
	pub fn public_fields(&self) -> impl Iterator<Item = &Field> {

	vec![0; mem::size_of::<{{ ident }}>()]
	vec![0; size_of::<{{ ident }}>()]

	return ((({{ c_type }}) -1) < 0);
	return (({{ c_type }}) -1) < 0;

	int32_t rust_size, {{ c_type }} value, int* error, unsigned char* pad
	int32_t rust_size, {{ c_type }} value, int* error, unsigned char* pad

ctest: add tests for aliases, structs, unions, as well as a test crate. #4543

Are you sure you want to change the base?

ctest: add tests for aliases, structs, unions, as well as a test crate. #4543

Uh oh!

Conversation

mbyx commented Jul 8, 2025

Description

Sources

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgross35 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgross35 commented Jul 14, 2025

Uh oh!

mbyx commented Jul 14, 2025

Uh oh!

tgross35 commented Jul 14, 2025

Uh oh!

Uh oh!