ctest: improve translation backend by mbyx · Pull Request #4617 · rust-lang/libc

mbyx · 2025-08-05T06:48:14Z

Description

Improves the Translator type by using the new cdecl module, as well as restructuring TranslateHelper to be more minimal and have less duplication.
Also modifies the cdecl module to allow the correct ABI of a function to be used.

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-08-05T06:58:39Z

Ah unfortunately this will have some small conflicts with #4616, which adds handling for volatile and restrict.

ctest-next/src/template.rs

tgross35 · 2025-08-05T07:11:37Z

ctest-next/src/cdecl.rs

+// Important change here:
+// Basically, `syn` always gives us the `constness` of the inner type of a pointer.
+// However `cdecl::ptr` wants the `constness` of the pointer. So we just modify
+// the way it is built so that `cdecl::ptr` takes the `constness` of the inner type.
+pub(crate) fn ptr_with_inner(inner: CTy, constness: Constness) -> CTy {
+    let mut ty = Box::new(inner);
+    ty.modify_constness(constness);
+    CTy::Ptr {
+        ty,
+        constness: Constness::Mut,
+    }
+}
+


Put this function in the translator module and just inline modify_constness, since "inherited constness" is a concept in Rust but not C

tgross35 · 2025-08-05T07:30:54Z

ctest-next/src/cdecl.rs

+            match ty.as_ref() {
+                CTy::Fn {
+                    abi: Some(abi_str), ..
+                } => {
+                    let modifier = match constness {
+                        Const => format!("({abi_str} *const "),
+                        Mut => format!("({abi_str} *"),
+                    };
+                    s.insert_str(0, &modifier);
+                    s.push(')');
+                }
+                _ => match constness {
+                    Const => s.insert_str(0, "*const "),
+                    Mut => s.insert(0, '*'),
+                },
            }


This doesn't seem like the best way to add it in because of the inconsistency between functions and function pointers, and the extra quoting. Also it seems like MSVC wants this attribute in a different place from the others https://gcc.godbolt.org/z/4Y84dWrv1.

I think this is worth revisiting on its own later. Would it be possible to ignore any tests with a different ABI for now with a todo!()?

Should be fine for now, the system abi expands to nothing on everything other than a specific windows target, and windows already skips all fn ptr checks, so it might not even affect it.

tgross35 · 2025-08-05T07:44:45Z

ctest-next/src/tests.rs

 /// Translate a Rust type into a c typedef declaration.
-fn cdecl(s: &str) -> Result<String, TranslationError> {
-    let ty: syn::Type = syn::parse_str(s).unwrap();
+fn cdecl(s: &str, name: &str) -> Result<String, TranslationError> {


Maybe rename this to something like r2cdecl, since the name cdecl is a bit overloaded now

tgross35 · 2025-08-05T07:50:35Z

ctest-next/src/tests.rs

 /// Translate a Rust type to C.
 fn ty(s: &str) -> Result<String, TranslationError> {
-    let translator = Translator {};
-    let ty: syn::Type = syn::parse_str(s).unwrap();
-    translator.translate_type(&ty)
+    cdecl(s, "")
 }


Probably don't need this if it just calls the below function

tgross35 · 2025-08-05T07:54:41Z

ctest-next/src/translator.rs

-                        Ok(format!("{modifier} {base_type}*").trim().to_string())
+                        let type_name = translate_primitive_type(&last_segment.ident.to_string());
+                        Ok(ptr_with_inner(
+                            cdecl::named(&type_name, cdecl::Constness::Mut),


cdecl::Constness is used everywhere, just import cdecl::Constness::{Const, Mut}

tgross35 · 2025-08-05T08:00:59Z

ctest-next/src/translator.rs

+    fn translate_mut(&self, mutability: Option<syn::Token![mut]>) -> cdecl::Constness {
+        mutability
+            .map(|_| cdecl::Constness::Mut)
+            .unwrap_or(cdecl::Constness::Const)
    }


This actually never needed &self. Could you move it to a free function?

tgross35 · 2025-08-05T08:07:38Z

ctest-next/src/translator.rs

+// Important change here:
+// Basically, `syn` always gives us the `constness` of the inner type of a pointer.
+// However `cdecl::ptr` wants the `constness` of the pointer. So we just modify
+// the way it is built so that `cdecl::ptr` takes the `constness` of the inner type.
+pub(crate) fn ptr_with_inner(inner: cdecl::CTy, constness: cdecl::Constness) -> cdecl::CTy {


Could you turn this into a doc comment?

Also, every time this is called, it is preceded by a call to self.translate_mut(reference.mutability). Instead of taking constness, could it maybe take the Option<syn::Token![mut]>? Makes it more clear that we're applying Rust const/mut rules to C (it should say that too)

tgross35 · 2025-08-05T08:11:50Z

ctest-next/src/translator.rs

-        Ok(format!(
-            "{}[{}]",
+    fn translate_array(&self, array: &syn::TypeArray) -> Result<cdecl::CTy, TranslationError> {
+        // FIXME(ctest): Maybe `array_arg` should be here?


Probably not here, we don't know whether or not the array is in a function. I think this should be handled in translate_bare_fn's arg setup.

I tried that and it works perfectly for adding the volatile keyword to the innermost type of the parameter, and in theory it should also work the same for array arg, but a simple replacement of cdecl::array to cdecl::ptr doesn't work, it needs to handle it recursively.

tgross35 · 2025-08-05T08:15:07Z

ctest-next/src/tests.rs

        ty("*const [u128; 2 + 3]").unwrap(),
-        "unsigned __int128 (*const) [2 + 3]".to_string()
+        "unsigned __int128 (*)[2 + 3]".to_string()


Is this change expected?

Yes, arrays in cdecl don't have a const qualifier.

Okay, so a non-const pointer is actually expected here? I guess that makes sense, since the data it points to is const but the pointer itself may not be (like below).

For reference, the syntax x int[const 2 + 3] is what would be needed to make an array itself const. But this is pretty limited: it's only supported in specific places (I think function definitions but not local vars or statics) and it's not supported by MSVC, so I didn't bother adding it.

It is expected for this PR, but for the extern fn tests, I did have to modify translate_ptr to handle things differently for pointers to arrays.

tgross35 · 2025-08-05T08:19:58Z

ctest-next/src/tests.rs

+    assert_eq!(ty("&u8").unwrap(), "const uint8_t *".to_string());
+    assert_eq!(ty("&&u8").unwrap(), "const uint8_t *const *".to_string());
+    assert_eq!(ty("*mut &u8").unwrap(), "const uint8_t **".to_string());
+    assert_eq!(ty("& &mut u8").unwrap(), "uint8_t *const *".to_string());


Are these changes expected?

Yes, cdecl has better formatting than the previous implementation.

Sorry, I asked the wrong thing. Meant to ask if these should have an extra const, because e.g. let x: &u8 is a const pointer to a const u8 (const uint8_t *const). I guess we don't know at this point whether or not we would want the variable to be mutable, though.

ctest-test only tests references in extern statics, and from what I can see, it chooses to use const uint8_t * for &u8. If we did change it, it would have to be after ctest is completely removed, because changing the translation leads to changing the source (t1.h), and ctest would start diverging in output from ctest-next.

tgross35

I am just merging this now because it cleans a lot up as-is. However, there are some changes needed that I have in the review comments. Could you cover this in a followup?

tgross35 · 2025-08-05T15:22:45Z

ctest-next/src/template.rs

+            )
+            .map_err(|_| {
+                TranslationError::new(
+                    crate::translator::TranslationErrorKind::InvalidReturn,


Import TranslationErrorKind so the whole path isn't needed

tgross35 · 2025-08-06T09:56:23Z

ctest-next/src/template.rs


+        let ty = cdecl::cdecl(&ty, "".to_string()).map_err(|_| {
+            TranslationError::new(
+                crate::translator::TranslationErrorKind::InvalidReturn,


tgross35 · 2025-08-06T09:56:30Z

ctest-next/src/tests.rs

+    let translated = translator.translate_type(&ty)?;
+    cdecl::cdecl(&translated, name.to_string()).map_err(|_| {
+        TranslationError::new(
+            crate::translator::TranslationErrorKind::InvalidReturn,


tgross35 · 2025-08-06T09:59:43Z

ctest-next/src/tests.rs

+        r2cdecl("*const [u128; 2 + 3]", "").unwrap(),
+        "unsigned __int128 (*)[2 + 3]".to_string()
+    );
+    assert_eq!(
+        r2cdecl("*const *mut [u8; 5]", "").unwrap(),
+        "uint8_t (*const *)[5]".to_string()


I think you might be getting at this on Zulip, but this doesn't seem right to me. On Rust it's a single indirection, but in C this is double indirection.

Yeah, I fix this in the (not yet pushed) extern function test PR.

tgross35 · 2025-08-06T10:01:29Z

ctest-next/src/tests.rs

+        r2cdecl("*const *mut i32", "").unwrap(),
+        "int32_t *const *".to_string()


May as well clean this up while you are here: the righthand side of these tests shouldn't need .to_string(), since PartialEq works between String and &str (e.g. assert_eq!(hi.to_string(), "hi")).

You could also put everything into a function so the unwrap() isn't needed everywhere:

#[track_caller] fn assert_r2cdecl(rust: &str, expected: &str) { assert_eq!(r2cdecl(rust, "").unwrap(), expected) }

(The C versions may be a bit easier to read if they always include the name e.g. the above function could always set it to "foo")

tgross35 · 2025-08-06T10:23:12Z

ctest-next/src/tests.rs

 fn test_translation_type_array() {
    assert_eq!(
-        ty("[&u8; 2 + 2]").unwrap(),
-        "const uint8_t*[2 + 2]".to_string()
+        r2cdecl("[&u8; 2 + 2]", "").unwrap(),
+        "const uint8_t *[2 + 2]".to_string()
    );
 }


I'm noting on Zulip, but how an array gets translated is actually context-dependent. For statics and struct/union fields they are inline, like Rust arrays, so this translation would be incorrect. For function arguments and for anything where they aren't the top-level item, they turn into a pointer.

So I think the entrypoint needs to be updated to take an decay_to_ptr argument that is false for statics and fields, but true otherwise.

Could you please explain a bit more about binding_constness and decay_to_ptr?

Also, I believe that most of these are actually only ever used to specify the return type of a function with a typedef, so whatever works within that context is what we would need.

Could you please explain a bit more about binding_constness and decay_to_ptr?

What I was thinking is that translate_type needs some context/config, which would be those two (maybe wrapped in a struct so others can be added easily).

For binding_constness (or maybe var_constness is a better name): the constness of a binding / arg isn't really related to its type in Rust. E.g. in both fn foo(mut a: i32) { ... } and fn foo(a: i32) { ... }, the type is only i32, there is no const/mut tied to it. You don't get that until you assign that to something in Rust.

In C though, constness of a binding/arg is tied to its type: "i32" could translate to either int or const int, those are (effectively) two distinct types in C. I'm just thinking that binding_constness: Constness would control this so you could make either as needed. If it's Const, just mark the outermost CTy item Const if it's a pointer or named ty.

For array_decays_to_ptr: in C, a T [N] in function signatures needs to map to a *const [T; N] in Rust, because it "decays" to a pointer when passed. Same thing for if it's behind indirection (another pointer or array). However, for statics and fields, that same T [N] is actually inlined, so it would be [T; N] without the pointer.

array_decays_to_ptr would just control that, basically whether or not we error on plain [T; N] rather than being an unconditional error.

Also, I believe that most of these are actually only ever used to specify the return type of a function with a typedef, so whatever works within that context is what we would need.

That's fair, we might not need the array_decays_to_ptr config. I think we do want something like binding_constness though.

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

tgross35 reviewed Aug 5, 2025

View reviewed changes

mbyx force-pushed the ctest-improve-translator branch 3 times, most recently from b837b5a to a36802f Compare August 5, 2025 07:48

mbyx requested a review from tgross35 August 5, 2025 07:48

tgross35 reviewed Aug 5, 2025

View reviewed changes

ctest: improve translation backend

46da1b8

mbyx force-pushed the ctest-improve-translator branch from a36802f to 46da1b8 Compare August 5, 2025 09:20

mbyx requested a review from tgross35 August 5, 2025 09:20

mbyx mentioned this pull request Aug 6, 2025

ctest: add foreign fn tests #4594

Merged

3 tasks

tgross35 added this pull request to the merge queue Aug 6, 2025

tgross35 reviewed Aug 6, 2025

View reviewed changes

Merged via the queue into rust-lang:main with commit 7f3d696 Aug 6, 2025
48 of 52 checks passed

rustbot removed the S-waiting-on-review label Aug 6, 2025

tgross35 added the stable-unneeded This PR is applied to main but already exists on libc-0.2 label Aug 11, 2025

		r2cdecl("const mut i32", "").unwrap(),
		"int32_t const ".to_string()

Conversation

mbyx commented Aug 5, 2025

Description

Sources

Checklist

Uh oh!

tgross35 commented Aug 5, 2025

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants