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

ctest-next-test/Cargo.toml

tgross35

(first set, more in progress)

ctest-next/src/ast/structure.rs

ctest-next/src/ast/union.rs

ctest-next/src/ffi_items.rs

ctest-next/src/lib.rs

ctest-next/templates/test.rs

ctest-next/templates/test.c

ctest-next/src/translator.rs

ctest-next-test/Cargo.toml

ctest-next/templates/test.c

ctest-next/templates/test.rs

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

ctest-next/src/ffi_items.rs

ctest-next/Cargo.lock

ctest-next/templates/test.rs

tgross35 · 2025-07-16T06:32:51Z

ctest-next/src/template.rs

+    ///
+    /// Arrays and Function types in C have different rules for placement, such as array lengths
+    /// being placed after the parameter list.
+    pub(crate) fn c_signature(


Do you happen to have a small example?

tgross35 · 2025-07-16T06:37:09Z

ctest-next/templates/test.rs

@@ -5,12 +5,13 @@
 /// are not allowed at the top-level, so we hack around this by keeping it
 /// inside of a module.
 mod generated_tests {
-    #![allow(non_snake_case)]
+    #![allow(non_snake_case, unused_imports)]


Put #[allow(unused_imports)] on the specific imports that are expected to possibly be unused, to avoid accidentally importing something within a function that won't be used.

tgross35 · 2025-07-16T06:38:46Z

ctest-next/templates/test.rs

+            check_same((all_ones < all_zeros) as u32,
+                    ctest_{{ ident }}_is_signed(), "{{ ident }} signed");


Formatting nit

Suggested change

check_same((all_ones < all_zeros) as u32,

ctest_{{ ident }}_is_signed(), "{{ ident }} signed");

check_same(

(all_ones < all_zeros) as u32,

ctest_{{ ident }}_is_signed(),

"{{ ident }} signed",

);

Bind the C call as let c_is_signed = unsafe { ctest_{{ ident }}_is_signed() };, that way the rest of this doesn't need to be unsafe

tgross35 · 2025-07-16T06:44:45Z

ctest-next/templates/test.rs

+            check_same(offset_of!({{ ident }}, {{ field.ident() }}),
+                    ctest_offset_of__{{ ident }}__{{ field.ident() }}() as usize,
+                    "field offset {{ field.ident() }} of {{ ident }}");
+            check_same(size_of_val(&val) as u64,
+                    ctest_field_size__{{ ident }}__{{ field.ident() }}(),
+                    "field size {{ field.ident() }} of {{ ident }}");


Suggested change

check_same(offset_of!({{ ident }}, {{ field.ident() }}),

ctest_offset_of__{{ ident }}__{{ field.ident() }}() as usize,

"field offset {{ field.ident() }} of {{ ident }}");

check_same(size_of_val(&val) as u64,

ctest_field_size__{{ ident }}__{{ field.ident() }}(),

"field size {{ field.ident() }} of {{ ident }}");

check_same(

offset_of!(ident, {{ field.ident() }}) as u64,

ctest_offset_of__{{ ident }}__{{ field.ident() }}(),

"field offset {{ field.ident() }} of {{ ident }}",

);

check_same(

size_of_val(&val) as u64,

ctest_field_size__{{ ident }}__{{ field.ident() }}(),

"field size {{ field.ident() }} of {{ ident }}",

);

Changing the first offset_of to cast to u64 rather than turning the C call into usize, and formatting

The read_unaligned also isn't sound because it's possible that the type doesn't allow all zeros.

I don't want to delay this PR further but this does need to get changed in a followup, so please add a // FIXME(soundness): cast to a byte array before reading

tgross35 · 2025-07-16T06:53:35Z

ctest-next/templates/test.c

+}
+
+{%- if !self::should_skip_struct_field_type(generator, structure, field) +%}
+{%- let signature = self.c_signature(field.ty, &format!("__test_field_type_{ident}_{rust_field_name}({c_type}* b)")).unwrap() +%}


I think this function could be ctest_get_pointer_to__{ident}__{rust_field_name}

tgross35 · 2025-07-16T06:58:35Z

ctest-next/templates/test.rs

+            let uninit_ty = MaybeUninit::<{{ ident }}>::zeroed();
+            let ty_ptr = uninit_ty.as_ptr();
+            let field_ptr = &raw const ((*ty_ptr).{{ field.ident() }});
+            check_same(field_ptr as *mut _,
+                    __test_field_type_{{ ident }}_{{ field.ident() }}(ty_ptr),
+                    "field type {{ field.ident() }} of {{ ident }}");


There are some things I don't like about this __test_field_type_ test but I don't want to block the PR on it. Could you just comment this test (both here and in C) with a FIXME? This applies to the later check of unions as well, and should also be able to comment out c_signature which I think should look a bit different.

tgross35 · 2025-07-16T07:12:31Z

ctest-test/src/t1.h

-extern const int16_t* T1_sref;
-
-extern const int32_t* T1_mut_opt_ref;
-extern int32_t* T1_mut_opt_mut_ref;
-extern const int32_t* T1_const_opt_const_ref;
-
-extern void (*const T1_opt_fn1)(void);


Why are these removed?

If something isn't yet working, comment it with a FIXME

This was before I learnt that Option was FFI safe in some cases, so I removed them. I'll add them back.

tgross35 · 2025-07-16T07:13:13Z

ctest-test/src/t1.rs

@@ -88,7 +89,8 @@ extern "C" {
 }

 pub fn foo() {
-    assert_eq!(1, 1);
+    let x = 1;
+    assert_eq!(x, 1);


Why is this needed?

It triggered a clippy lint about identical args used in assert_eq, it's the only lint left that triggers so I thought it would be better to fix it. Although the CI currently skips checking ctest-test I think.

tgross35 · 2025-07-16T07:13:53Z

ctest-test/src/t1.h

-/* FIXME(#4365): duplicate symbol errors when enabled
-// uint32_t (*(*T1_opt_fn2)(uint8_t))(uint16_t);
-// uint32_t (*(*T1_opt_fn3)(uint8_t(*)(uint8_t), uint16_t(*)(uint16_t)))(uint16_t);


If this is no longer accurate, just uncomment the code so we can verify the test passes

Functions and statics aren't tested right now, so for now uncommenting only confirms that they parse correctly. ctest would still fail if they're uncommented.

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

ctest-test/src/t1.rs

@@ -195,6 +179,7 @@ struct timeval {
    tv_usec: c_int,
 }

+#[expect(unused)]
 #[repr(C)]
 struct log_record_t {


Make them pub instead of expect(unused)

tgross35 · 2025-07-16T07:16:26Z

ctest-test/tests/all.rs

-    let (o, status) = output(&mut cmd("t2"));
-    assert!(!status.success(), "output: {o}");
-    let errors = [
-        "bad T2Foo signed",
-        "bad T2TypedefFoo signed",
-        "bad T2TypedefInt signed",
-        "bad T2Bar size",
-        "bad T2Bar align",
-        "bad T2Bar signed",
-        "bad T2Baz size",
-        "bad field offset a of T2Baz",
-        "bad field type a of T2Baz",
-        "bad field offset b of T2Baz",
-        "bad field type b of T2Baz",
-        "bad T2a function pointer",
-        "bad T2C value at byte 0",
-        "bad T2S string",
-        "bad T2Union size",
-        "bad field type b of T2Union",
-        "bad field offset b of T2Union",
-    ];
-    let mut errors = errors.iter().cloned().collect::<HashSet<_>>();
-
-    let mut bad = false;
-    for line in o.lines().filter(|l| l.starts_with("bad ")) {
-        let msg = &line[..line.find(":").unwrap()];
-        if !errors.remove(&msg) {
-            println!("unknown error: {msg}");
-            bad = true;
-        }
-    }
-
-    for error in errors {
-        println!("didn't find error: {error}");
-        bad = true;
-    }
-    if bad {
-        println!("output was:\n\n{o}");
-        panic!();
-    }
-}


What happened to this block?

It was duplicated across the C and C++ test, nothing has been changed here.

tgross35 · 2025-07-16T07:34:32Z

ctest-next/templates/common/test_roundtrip.c

+
+#ifdef _MSC_VER
+#  pragma warning(default:4365)
+#endif


Missing newline at end of file

Same for the other files in this directory

tgross35 · 2025-07-16T07:36:48Z

ctest-next/src/template.rs

+/// Determine whether a Rust alias/struct/union should have a round trip test.
+///
+/// By default all alias/struct/unions are roundtripped. Aliases or fields with arrays should
+/// not be part of the roundtrip.
+pub(crate) fn should_roundtrip(gen: &TestGenerator, ident: &str) -> bool {


Could you define what "round trip" means here?

tgross35 · 2025-07-16T07:37:42Z

I think that should hopefully be the last round of review with a better explanation for the changes to the tests, please rebase+squash once that is done.

tgross35 · 2025-07-16T08:32:47Z

ctest-test/build.rs

@@ -1,107 +1,62 @@
-use std::process::Command;


Could ctest_next be added to the tests without removing ctest? Or do the updates to tests make something fail?

t2 would fail because ctest would fail on more errors than ctest-next since statics and functions haven't been implemented yet.

Can't you use the skips?

Unfortunately it's not even able to parse pub const T1S: *const c_char = c"foo".as_ptr(); so a skip wouldn't work.

Ah, I guess skip_function and skip_static haven't been added yet. If you add them then you should be able to cfg.skip_function(|_f| true); cfg.skip_static(|_s| true) in the tests until it works, without blocking on everything else.

The same is true for libc-test

tgross35 · 2025-07-16T09:53:45Z

This PR is probably getting a bit too big, I think I need to look at some of these tests closer. Nothing related to what you are adding, it just seems like the existing ctest does some things that are questionable.

Would you be willing to split up this PR? It would be easier if we could land tests one at a time without delaying everything here because specific tests need more updates. Specifically, I'd suggest splitting up into something like one PR for each of the following:

(new) Move logic out of templates. I did some experimentation before suggesting this, you can basically take tgross35@0fcec9d with a few tweaks to get it to build (drop the new size+align test to keep an absolute minimum amount of work here.

Basically the goal is to avoid anything other than for loops in the templates - no let bindings or function calls - which will make it easier to work on a single test at a time. This is easier to read and work on anyway.
(from this PR) add changes in ctest-next/src/generator.rs that allow skipping tests.
(from this PR) add ctest-next to ctest-test, in parallel with ctest. Needs (2) so you can skip everything that isn't yet implemented
(from this PR) add the size+align tests to the template to all types. Needs (1)
(from this PR) add field offset tests to the template for unions and structs. Needs (1)
(from this PR) add roundtrip tests to the template. Needs (1).
(from this PR) ... other tests to the template
(new, I know you're working on it) update libc-test to run both ctest and ctest-next. This needs (2) so you can skip things that aren't implemented

This way I don't think any work is blocked after 1 and 2 are merged, the rest of the tests can be improved before they get in.

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-next-test/Cargo.toml Outdated Show resolved Hide resolved

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

mbyx force-pushed the ctest-next-test-port branch from 1bf45ce to c92e941 Compare July 16, 2025 06:28

tgross35 reviewed Jul 16, 2025

View reviewed changes

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

d2b1425

mbyx force-pushed the ctest-next-test-port branch from c92e941 to d2b1425 Compare July 16, 2025 06:46

mbyx requested a review from tgross35 July 16, 2025 06:55

tgross35 reviewed Jul 16, 2025

View reviewed changes

		check_same((all_ones < all_zeros) as u32,
		ctest_{{ ident }}_is_signed(), "{{ ident }} signed");

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!

Uh oh!

tgross35 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

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

tgross35 commented Jul 16, 2025 •

edited

Loading