On Mon Apr 14, 2025 at 4:05 PM CEST, Alice Ryhl wrote:
On Sat, Apr 12, 2025 at 10:01:22AM +0000, Benno Lossin wrote:
On Fri Apr 11, 2025 at 4:15 PM CEST, Miguel Ojeda wrote:
On Fri, Apr 11, 2025 at 2:46 PM Benno Lossin benno.lossin@proton.me wrote:
Ah I overlooked this, you should be using `kernel::ffi` (or `crate::ffi`) instead of `core`. (for `c_char` it doesn't matter, but we shouldn't be using `core::ffi`, since we have our own mappings).
In 6.6, C `char` changed to unsigned, but `core::ffi::c_char` is signed (in x86_64 at least).
We should just never use `core::ffi` (except in `rust/ffi.rs`, of course) -- I think we should just add the C types to the prelude (which we discussed in the past) so that it is easy to avoid the mistake (something like the patch attached as the end result, but tested and across a kernel cycle or two) and mention it in the Coding Guidelines. Thoughts?
Yeah sounds like a good idea.
I tried to use Clippy's `disallowed-types` too:
disallowed-types = [ { path = "core::ffi::c_void", reason = "the `kernel::ffi`types should be used instead" }, { path = "core::ffi::c_char", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_schar", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_uchar", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_short", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_ushort", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_int", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_uint", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_long", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_ulong", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_longlong", reason = "the `kernel::ffi` types should be used instead" }, { path = "core::ffi::c_ulonglong", reason = "the `kernel::ffi` types should be used instead" }, ]
But it goes across aliases.
We could make the types in `ffi` be transparent newtypes. But not sure if that could interfere with kCFI or other stuff.
Transparent newtypes for all integers would be super inconvenient.
Yeah I noticed that too when trying... We often assign integer literals to the ffi types.
--- Cheers, Benno