Skip to content
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

Enable #[deny(improper_ctypes)] #466

Open
jschwe opened this issue Jun 14, 2024 · 4 comments
Open

Enable #[deny(improper_ctypes)] #466

jschwe opened this issue Jun 14, 2024 · 4 comments

Comments

@jschwe
Copy link
Member

jschwe commented Jun 14, 2024

When building moz-js locally as part of servo I'm getting some warnings about some function parameters not being FFI-safe. Is this a known issue, that can't be worked around?
In my opinion we should deny improper_ctypes by default, and perhaps allow certain cases if it really can't be avoided and people are extremely sure that in practice the types are ffi-safe.

@wusyong
Copy link
Member

wusyong commented Jun 15, 2024

I think this is fine for now because we use same target build on the same platform, or even build one for you own.
But if we want to get rid of the warning, I think we can just blacklist all those types. Many of these are not actually used in servo.
Even we need it, we could manually write one for our own.

@jschwe
Copy link
Member Author

jschwe commented Jul 30, 2024

I'm seeing an example here where an array is passed directly as a function argument:

InitSelfHostedCode(js_context, [0u64; 2], None);

The unsafe code guidelines explicit list array arguments as not FFI safe, and say a pointer to the array needs to be used instead to match the behavior on the C-side.

@jschwe
Copy link
Member Author

jschwe commented Jul 30, 2024

Here is an example on godbolt showing the different code Rust generates for pass by value vs pass by reference of an array: https://godbolt.org/z/vhqzv5fee
Note that in the by value case, the values are passed via 2 registers, while in the by reference case one register is used to pass a pointer to the array.

In C/C++ arrays are passed by pointer, so the binding needs to do the same (I guess this is also an upstream bindgen issue)

Edit: rust-lang/rust-bindgen#2071 seems to indicate we must blocklist the underlying C++ type, since bindgen is known to not work well for such types.

@sagudev
Copy link
Member

sagudev commented Jul 30, 2024

I plan to take some time and clean up mozjs (warnings, code, pull request, issues), but this will probably happen in september.

we must blocklist the underlying C++ type, since bindgen is known to not work well for such types.

This will require more glue code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants