-
Notifications
You must be signed in to change notification settings - Fork 1.1k
ctest: add tests for field size, offset, and field ptr #4561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With these two small changes, the size+offset changes look good to me. I do have some more feedback for the field pointer tests though; maybe want to split that off to another PR so the rest of this can merge now?
b3008cb
to
8996f9b
Compare
It's fine to review the field ptr checks now, splitting it would be harder. |
ctest-next/src/template.rs
Outdated
pub(crate) fn translate_signature_returning_ptr( | ||
&self, | ||
signature: &str, | ||
return_type: &syn::Type, | ||
) -> Result<String, TranslationError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be a general cdecl generator right, not specific to signatures anymore right? Maybe translate_signature_returning_ptr
-> make_cdecl
, signature
-> name
, and return_type
-> ty
.
ctest-next/src/template.rs
Outdated
if let syn::PathArguments::AngleBracketed(args) = &last.arguments { | ||
if let syn::GenericArgument::Type(inner_ty) = args.args.first().unwrap() { | ||
// Option<T> is ONLY ffi-safe if it contains a function pointer, or a reference. | ||
match inner_ty { | ||
syn::Type::Reference(_) | syn::Type::BareFn(_) => { | ||
return self.translate_signature_returning_ptr(signature, inner_ty); | ||
} | ||
_ => { | ||
return Err(TranslationError::new( | ||
TranslationErrorKind::NotFfiCompatible, | ||
&p.to_token_stream().to_string(), | ||
inner_ty.span(), | ||
)); | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some similar logic for handling Option
elsewhere isn't there? Could this reuse the valid type checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was duplicated in the previous PR, but not anymore.
ctest-next/src/template.rs
Outdated
return Ok(format!("{elem_type} (*{signature})[{len_outer}]")); | ||
} | ||
} | ||
_ => (), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should unknown types return an error?
@@ -291,4 +458,139 @@ impl<'a> TranslateHelper<'a> { | |||
|
|||
Ok(self.generator.map(item)) | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the below functions are related to translating more complex Rust types to C types, not directly related to filling out the template. Could they be in the translator
module, possibly as methods on Translator
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basic_c_type
and make_cdecl
require ffi_items
and generator
, so I think it would be better to keep them here. translate_signature_partial
depends on basic_c_type
as well.
|
||
let uninit_ty = MaybeUninit::<{{ item.id }}>::zeroed(); | ||
let ty_ptr = uninit_ty.as_ptr(); | ||
// SAFETY: We don't read `field_ptr`, only compare the pointer itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// SAFETY: We don't read `field_ptr`, only compare the pointer itself. | |
// SAFETY: We don't read `field_ptr`, only compare the pointer itself. The assumption is made that | |
// this does not wrap the address space. |
Learned there is another precondition here
9b70c53
to
10163a4
Compare
Description
Adds tests for checking if the size and offset of a field is the same in Rust and C, as well as the pointer to that field.
Sources
Checklist
libc-test/semver
have been updated*LAST
or*MAX
areincluded (see #3131)
cd libc-test && cargo test --target mytarget
);especially relevant for platforms that may not be checked in CI